On Tue, Oct 17, 2017 at 11:46:51AM +0300, Roger Quadros wrote:
> Hi,
>
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
> On 16/10/17 02:20, Ladislav Michl wrote:
> > Generic probe function can deal with OneNAND node, remove
> > now useless gpmc_probe_onenand_child function.
> >
> > Signed-off-by: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
> > ---
> > drivers/memory/omap-gpmc.c | 111 ++++++++++++++++++++++++++++-----------------
> > 1 file changed, 69 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> > index f2aef0b87bc6..fc7c3c2a0b04 100644
> > --- a/drivers/memory/omap-gpmc.c
> > +++ b/drivers/memory/omap-gpmc.c
> > @@ -1921,52 +1921,22 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
> > of_property_read_bool(np, "gpmc,time-para-granularity");
> > }
> >
> > -#if IS_ENABLED(CONFIG_MTD_ONENAND)
> > -static int gpmc_probe_onenand_child(struct platform_device *pdev,
> > - struct device_node *child)
> > -{
> > - u32 val;
> > - struct omap_onenand_platform_data *gpmc_onenand_data;
> > -
> > - if (of_property_read_u32(child, "reg", &val) < 0) {
> > - dev_err(&pdev->dev, "%pOF has no 'reg' property\n",
> > - child);
> > - return -ENODEV;
> > - }
> > -
> > - gpmc_onenand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_onenand_data),
> > - GFP_KERNEL);
> > - if (!gpmc_onenand_data)
> > - return -ENOMEM;
> > -
> > - gpmc_onenand_data->cs = val;
> > - gpmc_onenand_data->of_node = child;
> > - gpmc_onenand_data->dma_channel = -1;
> > -
> > - if (!of_property_read_u32(child, "dma-channel", &val))
> > - gpmc_onenand_data->dma_channel = val;
> > -
> > - return gpmc_onenand_init(gpmc_onenand_data);
> > -}
> > -#else
> > -static int gpmc_probe_onenand_child(struct platform_device *pdev,
> > - struct device_node *child)
> > -{
> > - return 0;
> > -}
> > -#endif
> > +extern int
> > +gpmc_omap_onenand_init(struct omap_onenand_platform_data *_onenand_data);
> >
> > /**
> > - * gpmc_probe_generic_child - configures the gpmc for a child device
> > + * gpmc_probe_child - configures the gpmc for a child device
>
> Let's not do this name change it is not really required.
But it is nice, right? :) Will drop that change.
> > * @pdev: pointer to gpmc platform device
> > * @child: pointer to device-tree node for child device
> > *
> > * Allocates and configures a GPMC chip-select for a child device.
> > * Returns 0 on success and appropriate negative error code on failure.
> > */
> > -static int gpmc_probe_generic_child(struct platform_device *pdev,
> > +static int gpmc_probe_child(struct platform_device *pdev,
> > struct device_node *child)
> > {
> > + struct omap_onenand_platform_data *gpmc_onenand_data;
> > + struct platform_device *child_dev;
> > struct gpmc_settings gpmc_s;
> > struct gpmc_timings gpmc_t;
> > struct resource res;
> > @@ -2058,6 +2028,33 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
> > }
> > }
> >
> > + if (of_node_cmp(child->name, "onenand") == 0) {
> > + /*
> > + * Warn about older DT blobs with no compatible property
> > + * and fix them for now
> > + */
> > + if (!of_property_read_bool(child, "compatible")) {
> > + struct property *prop;
> > +
> > + dev_warn(&pdev->dev,
> > + "Incompatible OneNAND node, fixing.");
> > +
> > + prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > + if (!prop) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + prop->name = "compatible";
> > + /* See FIXME about device-tree migration bellow */
> > + prop->value = (gpmc_capability & GPMC_HAS_WR_ACCESS) ?
> > + "ti,omap3-onenand" : "ti,omap2-onenand";
> > + prop->length = 14;
> > +
> > + of_update_property(child, prop);
> > + }
>
> I don't want to fixup broken DTs like this as they will go silently without being fixed.
>
> Let's instead just error out after the dev_warn().
> This will force us to fix all DT nodes.
Is it really right thing to do? A lot of people scream loudly when you break
their boot.
> > + }
> > +
> > if (of_device_is_compatible(child, "ti,omap2-nand")) {
> > /* NAND specific setup */
> > val = 8;
> > @@ -2079,6 +2076,30 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
> > /* disable write protect */
> > gpmc_configure(GPMC_CONFIG_WP, 0);
> > gpmc_s.device_nand = true;
> > + } else if (of_device_is_compatible(child, "ti,omap2-onenand") ||
> > + of_device_is_compatible(child, "ti,omap3-onenand")) {
>
> Could just be if() instead of else if() as we don't have any dependency on nand.
It could, but as node is eithrt nand or onenand, this saves few cycles.
> > + gpmc_onenand_data = devm_kzalloc(&pdev->dev,
> > + sizeof(*gpmc_onenand_data),
> > + GFP_KERNEL);
> > + if (!gpmc_onenand_data) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + gpmc_onenand_data->cs = cs;
> > + gpmc_onenand_data->of_node = child;
> > + if (!of_property_read_u32(child, "dma-channel", &val))
> > + gpmc_onenand_data->dma_channel = val;
> > + else
> > + gpmc_onenand_data->dma_channel = -1;
> > +
> > + ret = gpmc_omap_onenand_init(gpmc_onenand_data);
> > + if (ret)
> > + goto err;
>
> this gpmc_onenand_data should go away if we squash patch 10 here.
But driver will stop working then as setup function will be NULL.
> > +
> > + /* OneNAND driver handles timings on its own */
> > + gpmc_cs_enable_mem(cs);
> > + goto no_timings;
>
> Actually ASYNC timings should be handled here just as the other children.
> Only SYNC timings will be triggered by the OneNAND driver.
It is handled here, it just cannot be done in one patch as MTD driver
needs to be modified.
> > } else {
> > ret = of_property_read_u32(child, "bank-width",
> > &gpmc_s.device_width);
> > @@ -2126,9 +2147,19 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
> > no_timings:
> >
> > /* create platform device, NULL on error or when disabled */
> > - if (!of_platform_device_create(child, NULL, &pdev->dev))
> > + child_dev = of_platform_device_create(child, NULL, &pdev->dev);
> > + if (!child_dev)
> > goto err_child_fail;
> >
> > + /* Use platform data until OneNAND driver is DT aware */
> > + if (gpmc_onenand_data) {
> > + child_dev->name = "omap2-onenand";
> > + child_dev->dev.platform_data = gpmc_onenand_data;
> > + ret = platform_device_add_resources(child_dev, &res, 1);
> > + if (ret)
> > + goto err_child_fail;
> > + }
> > +
>
> This bit will go away on squashing patch 10.
I'm not sure how to do this and merge changes using two separate trees.
> > /* is child a common bus? */
> > if (of_match_node(of_default_bus_match_table, child))
> > /* create children and other common bus children */
> > @@ -2193,11 +2224,7 @@ static void gpmc_probe_dt_children(struct platform_device *pdev)
> > if (!child->name)
> > continue;
> >
> > - if (of_node_cmp(child->name, "onenand") == 0)
> > - ret = gpmc_probe_onenand_child(pdev, child);
> > - else
> > - ret = gpmc_probe_generic_child(pdev, child);
> > -
> > + ret = gpmc_probe_child(pdev, child);
> > if (ret) {
> > dev_err(&pdev->dev, "failed to probe DT child '%s': %d\n",
> > child->name, ret);
> >
>
> --
> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
