| Message ID | 20171103130909.ug35yc7ovjgkmahs@lenoch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 03, 2017 at 02:09:09PM +0100, Ladislav Michl wrote: > On Fri, Nov 03, 2017 at 02:15:21PM +0200, Roger Quadros wrote: > > On 03/11/17 11:57, Ladislav Michl wrote: > > > Value of device_width is previously read from documented > > > 'gpmc,device-width' property. For now leave 'bank-width' > > > as a fallback for older DT blobs. > > > > bank-width is mandatory for NOR devices only. > > > > > > > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org> > > > --- > > > drivers/memory/omap-gpmc.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > > > index 24b584b74d84..3dc9706e1f15 100644 > > > --- a/drivers/memory/omap-gpmc.c > > > +++ b/drivers/memory/omap-gpmc.c > > > @@ -2152,10 +2152,15 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, > > > gpmc_configure(GPMC_CONFIG_WP, 0); > > > gpmc_s.device_nand = true; > > > } else { > > > + /* > > > + * Some older DT blobs are missing device-width property, > > > + * so try to read bank-width and fail if neither works. > > > + */ > > > > I'd get rid of the above comment and rather say, NOR devices use bank-width > > to specify device width. > > > > > ret = of_property_read_u32(child, "bank-width", > > > &gpmc_s.device_width); > > > - if (ret < 0) { > > > - dev_err(&pdev->dev, "%pOF has no 'bank-width' property\n", > > > + if (ret < 0 && !gpmc_s.device_width) { > > > + dev_err(&pdev->dev, > > > + "%pOF has no 'gpmc,device-width' property\n", > > > child); > > > goto err; > > > } > > > > > > > I'm ok with the approach. > > We could also update the DT binding documentation to say > > "'gpmc,device-width' is mandatory if device is not NOR or NAND type. > > For NOR, bank-width is used and for NAND nand-bus-width is used to figure > > out the device-width" > > Will update documentaton in separate patch. > > > And update the patch commit log accordingly. > > Something like the following? > > 8<----------------------- > Subject: [PATCH] memory: omap-gpmc: Make 'bank-width' NOR mandatory only > > Only NOR devices are using 'bank-width' property to configure device width. > Note that code does not do exactly that - it fails only if 'bank-width' is > missing and device width was not configured previously using > 'gpmc,device-width'. > > Signed-off-by: Ladislav Michl <ladis@linux-mips.org> > --- > drivers/memory/omap-gpmc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index ff73f2ad468c..26cc447662f0 100644 > --- a/drivers/memory/omap-gpmc.c > +++ b/drivers/memory/omap-gpmc.c > @@ -2157,9 +2157,10 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, > gpmc_configure(GPMC_CONFIG_WP, 0); > gpmc_s.device_nand = true; > } else { > + /* NOR devices use bank-width to specify device width */ Actually this comment is still wrong (and thus commit message). Documentation/devicetree/bindings/net/gpmc-eth.txt Ethernet is using this as well... > ret = of_property_read_u32(child, "bank-width", > &gpmc_s.device_width); > - if (ret < 0) { > + if (ret < 0 && !gpmc_s.device_width) { > dev_err(&pdev->dev, "%pOF has no 'bank-width' property\n", > child); > goto err; > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/11/17 15:16, Ladislav Michl wrote: > On Fri, Nov 03, 2017 at 02:09:09PM +0100, Ladislav Michl wrote: >> On Fri, Nov 03, 2017 at 02:15:21PM +0200, Roger Quadros wrote: >>> On 03/11/17 11:57, Ladislav Michl wrote: >>>> Value of device_width is previously read from documented >>>> 'gpmc,device-width' property. For now leave 'bank-width' >>>> as a fallback for older DT blobs. >>> >>> bank-width is mandatory for NOR devices only. >>> >>>> >>>> Signed-off-by: Ladislav Michl <ladis@linux-mips.org> >>>> --- >>>> drivers/memory/omap-gpmc.c | 9 +++++++-- >>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c >>>> index 24b584b74d84..3dc9706e1f15 100644 >>>> --- a/drivers/memory/omap-gpmc.c >>>> +++ b/drivers/memory/omap-gpmc.c >>>> @@ -2152,10 +2152,15 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >>>> gpmc_configure(GPMC_CONFIG_WP, 0); >>>> gpmc_s.device_nand = true; >>>> } else { >>>> + /* >>>> + * Some older DT blobs are missing device-width property, >>>> + * so try to read bank-width and fail if neither works. >>>> + */ >>> >>> I'd get rid of the above comment and rather say, NOR devices use bank-width >>> to specify device width. >>> >>>> ret = of_property_read_u32(child, "bank-width", >>>> &gpmc_s.device_width); >>>> - if (ret < 0) { >>>> - dev_err(&pdev->dev, "%pOF has no 'bank-width' property\n", >>>> + if (ret < 0 && !gpmc_s.device_width) { >>>> + dev_err(&pdev->dev, >>>> + "%pOF has no 'gpmc,device-width' property\n", >>>> child); >>>> goto err; >>>> } >>>> >>> >>> I'm ok with the approach. >>> We could also update the DT binding documentation to say >>> "'gpmc,device-width' is mandatory if device is not NOR or NAND type. >>> For NOR, bank-width is used and for NAND nand-bus-width is used to figure >>> out the device-width" >> >> Will update documentaton in separate patch. >> >>> And update the patch commit log accordingly. >> >> Something like the following? >> >> 8<----------------------- >> Subject: [PATCH] memory: omap-gpmc: Make 'bank-width' NOR mandatory only >> >> Only NOR devices are using 'bank-width' property to configure device width. >> Note that code does not do exactly that - it fails only if 'bank-width' is >> missing and device width was not configured previously using >> 'gpmc,device-width'. >> >> Signed-off-by: Ladislav Michl <ladis@linux-mips.org> >> --- >> drivers/memory/omap-gpmc.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c >> index ff73f2ad468c..26cc447662f0 100644 >> --- a/drivers/memory/omap-gpmc.c >> +++ b/drivers/memory/omap-gpmc.c >> @@ -2157,9 +2157,10 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >> gpmc_configure(GPMC_CONFIG_WP, 0); >> gpmc_s.device_nand = true; >> } else { >> + /* NOR devices use bank-width to specify device width */ > > Actually this comment is still wrong (and thus commit message). > Documentation/devicetree/bindings/net/gpmc-eth.txt > Ethernet is using this as well... Oh :(. Ideally it shouldn't have been the case. But damage is already done. I think we should still fix up the gpmc-eth binding document to use gpmc,device-width there. And continue to support in code anybody already using bank-width for ethernet. > >> ret = of_property_read_u32(child, "bank-width", >> &gpmc_s.device_width); >> - if (ret < 0) { >> + if (ret < 0 && !gpmc_s.device_width) { >> dev_err(&pdev->dev, "%pOF has no 'bank-width' property\n", >> child); >> goto err; >> -- >> 2.11.0 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-omap" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c index ff73f2ad468c..26cc447662f0 100644 --- a/drivers/memory/omap-gpmc.c +++ b/drivers/memory/omap-gpmc.c @@ -2157,9 +2157,10 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, gpmc_configure(GPMC_CONFIG_WP, 0); gpmc_s.device_nand = true; } else { + /* NOR devices use bank-width to specify device width */ ret = of_property_read_u32(child, "bank-width", &gpmc_s.device_width); - if (ret < 0) { + if (ret < 0 && !gpmc_s.device_width) { dev_err(&pdev->dev, "%pOF has no 'bank-width' property\n", child); goto err;