On 21/07/16 21:42, Mirza Krak wrote:

2016-07-21 12:15 GMT+02:00 Jon Hunter [off-list ref]:

quoted

On 19/07/16 14:36, Mirza Krak wrote:

quoted

From: Mirza Krak <redacted>
+config TEGRA_NOR
+     tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR"
+             depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC
+             depends on OF
+             help
+                     Driver for NOR flash bus found on Tegra30/20 SOC`s.

It is actually present on all Tegra's and so I would drop the 30/20.

ACK.

quoted

quoted

+++ b/drivers/bus/tegra-nor.c

@@ -0,0 +1,118 @@
+/*
+ * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI.
+ *
+ * Copyright (C) 2016 Host Mobility AB. All rights reserved.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>

Is this needed?

It is not. ACK.

quoted

quoted

+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>

Or this?

It is not. ACK.

quoted

quoted

+
+#define TEGRA_NOR_TIMING_REG_COUNT   2
+
+#define TEGRA_NOR_CONFIG                     0x00
+#define TEGRA_NOR_STATUS                     0x04
+#define TEGRA_NOR_ADDR_PTR                   0x08
+#define TEGRA_NOR_AHB_ADDR_PTR               0x0c
+#define TEGRA_NOR_TIMING0                    0x10
+#define TEGRA_NOR_TIMING1                    0x14
+#define TEGRA_NOR_MIO_CONFIG         0x18
+#define TEGRA_NOR_MIO_TIMING         0x1c
+#define TEGRA_NOR_DMA_CONFIG         0x20
+#define TEGRA_NOR_CS_MUX_CONFIG              0x24

Not all of these are used. It is good to define them and I wonder if we
should add support for MIO while are at it :-)

We can come back to this once I have all SNOR stuff in order.

quoted

quoted

+
+#define TEGRA_NOR_CONFIG_GO                          BIT(31)
+
+static const struct of_device_id nor_id_table[] = {
+     /* Tegra30 */

I don't think this comment is needed.

ACK.

quoted

quoted

+     { .compatible = "nvidia,tegra30-nor", .data = NULL, },

You don't need to set data to NULL.

ACK.

quoted

quoted

+     /* Tegra20 */
+     { .compatible = "nvidia,tegra20-nor", .data = NULL, },

Same here.

ACK

quoted

quoted

+static int __init nor_probe(struct platform_device *pdev)
+{

I would drop the __init.

ACK.

quoted

quoted

+static struct platform_driver nor_driver = {
+     .driver = {
+             .name           = "tegra-nor",
+             .of_match_table = nor_id_table,
+     },
+};

The driver should have a remove function given that we can build as a
module.

At the moment I do not know what we would do in a remove function and
hence me not adding one.

Should just be the inverse of the probe (although there is no inverse
for the parsing DT bit). If you don't wish to add a remove, that is
fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so
it cannot be configured as a module.

quoted

quoted

+module_platform_driver_probe(nor_driver, nor_probe);

I would use "tegra_nor" namespace for all the structs, functions, etc.
However, we may prefer to go with GMI and in which case tegra_gmi_probe,
etc.

ACK. Who gets the last call on what we should call the driver? It
seems that we both think GMI is a better name, do we need a third? :).

The patches would have to go via Thierry and so ultimately, Thierry.
However, I can't imagine he would object to GMI ;-)

Jon

-- 
nvpublic