2016-07-25 13:14 GMT+02:00 Thierry Reding <thierry.reding@xxxxxxxxx>:
> On Tue, Jul 19, 2016 at 03:36:37PM +0200, Mirza Krak wrote:
>> +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
>
> I think an ARCH_TEGRA dependency is enough here.
ACK.
>
>> + depends on OF
>
> You can drop this because Tegra has been OF-only for a long time now.
ACK.
>
>> + help
>> + Driver for NOR flash bus found on Tegra30/20 SOC`s.
>
> Maybe make this "Driver for GMI controller found on NVIDIA Tegra SoCs."
> or similar in light of the name change.
ACK.
>> --- /dev/null
>> +++ b/drivers/bus/tegra-nor.c
>> @@ -0,0 +1,118 @@
>> +/*
>> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI.
>
> Nit: I encourage the use of "NVIDIA" as spelling for consistency.
ACK.
>> +static int __init nor_parse_dt(struct platform_device *pdev,
>> + void __iomem *base)
>> +{
>> + struct device_node *of_node = pdev->dev.of_node;
>> + u32 config, timing[TEGRA_NOR_TIMING_REG_COUNT];
>> + int ret;
>> +
>> + ret = of_property_read_u32_array(of_node, "nvidia,cs-timing",
>> + timing, TEGRA_NOR_TIMING_REG_COUNT);
>> + if (!ret) {
>> + writel(timing[0], base + TEGRA_NOR_TIMING0);
>> + writel(timing[1], base + TEGRA_NOR_TIMING1);
>> + }
>> +
>> + ret = of_property_read_u32(of_node, "nvidia,config", &config);
>> + if (ret)
>> + return ret;
>> +
>> + config |= TEGRA_NOR_CONFIG_GO;
>> + writel(config, base + TEGRA_NOR_CONFIG);
>
> My preference would be for the tegra_gmi_parse_dt() function not to do
> any register programming. Instead I think it would be better to store
> any of the parameters inside a struct tegra_gmi and do the programming
> from within tegra_gmi_probe() (or via an other function such as
> tegra_gmi_initialize(), called from tegra_gmi_probe()).
>
> The reason is that you'll most likely want to do this programming when
> you resume from suspend, and you could reuse tegra_gmi_initialize() to
> do that.
ACK.
>
>> +
>> + if (of_get_child_count(of_node))
>> + ret = of_platform_populate(of_node,
>> + of_default_bus_match_table,
>> + NULL, &pdev->dev);
>
> Why the extra check? of_platform_populate() is almost a no-op if there
> aren't any children anyway. What it will do is set the OF_POPULATED_BUS
> flag, but I think we want that irrespective of whether or not there are
> any children.
>
> Was there any problem with calling it unconditionally that made you opt
> for the extra check?
I have not tested calling it unconditionally. Used another driver as
reference and they had that condition (drivers/bus/imx-weim.c), that
driver does not mention why the condition is there. But will test
removing the condition and see what happens.
>
> Also, I think you can call of_platform_default_populate(), which is a
> little shorter than the above because you can omit the match table.
Will look in to this.
>
>> +
>> + return ret;
>> +}
>> +
>> +static int __init nor_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> + struct clk *clk;
>> + void __iomem *base;
>> + int ret;
>> +
>> + /* get the resource */
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(base))
>> + return PTR_ERR(base);
>> +
>> + /* get the clock */
>> + clk = devm_clk_get(&pdev->dev, NULL);
>
> Let's use a consumer name here. The problem with a NULL consumer name is
> that it effectively restricts the DT binding. It means that whatever the
> clock is its name needs to be the first in a clock-names property. We'll
> likely get away with this because there's only one clock, but should we
> ever need to add another we'd need to add wording to the device tree
> bindings that the "gmi" entry needs to be first.
>
> I'm not sure if that's clear, so I'll try to explain in other words: If
> you pass a NULL consumer name to clk_get() it will simply use the first
> clock in the clocks property. If you want to later extend the DT binding
> by adding a clock in a backwards-compatible way, you'll need to add the
> restriction that the "gmi" clock (the one that was previously unnamed)
> must be the first in the clock-names and clocks properties.
>
> That's all a little confusing, so let's avoid this by giving it a proper
> consumer name to begin with.
Got it. Will add a proper consumer name.
>
>> + if (IS_ERR(clk))
>> + return PTR_ERR(clk);
>> +
>> + ret = clk_prepare_enable(clk);
>> + if (ret)
>> + return ret;
>> +
>> + /* parse the device node */
>> + ret = nor_parse_dt(pdev, base);
>> + if (ret) {
>> + dev_err(&pdev->dev, "%s fail to create devices.\n",
>> + pdev->dev.of_node->full_name);
>> + clk_disable_unprepare(clk);
>> + return ret;
>> + }
>
> The good thing if you don't do any programming in tegra_gmi_parse_dt(),
> is that you can easily move the clk_prepare_enable() call to here where
> things can't fail anymore, so you don't have to add cleanup code.
ACK.
>
>> +
>> + dev_info(&pdev->dev, "Driver registered.\n");
>
> Please avoid this kind of output. Users expect that everything will work
> so you want to let them know when something goes wrong, and be quiet
> when all goes as expected.
Will remove that.
>> +static struct platform_driver nor_driver = {
>> + .driver = {
>> + .name = "tegra-nor",
>> + .of_match_table = nor_id_table,
>> + },
>> +};
>> +module_platform_driver_probe(nor_driver, nor_probe);
>> +
>> +MODULE_AUTHOR("Mirza Krak <mirza.krak@xxxxxxxxx");
>> +MODULE_DESCRIPTION("NVIDIA Tegra NOR Bus Driver");
>> +MODULE_LICENSE("GPL");
>
> You're header comment says GPL version 2, which means that the
> MODULE_LICENSE needs to be "GPL v2". "GPL" means "version 2 or later".
Ok, I do not really mind it being GPL version 2 or later so will
change the header comment instead if that is ok.
Best regards,
Mirza
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
