| Message ID | 1374582499-31823-1-git-send-email-shc_work@mail.ru |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote: > This patch provide automatically determine of bus width. If resource size, > supplied to the driver more than 1 byte, the NAND_BUSWIDTH_AUTO option > will be used in the MTD core. I presume this depends on the bugfix I sent for NAND_BUSWIDTH_AUTO / nand_set_defaults()? That's worth noting (as I am right now), for whenever someone gets around to merging this. > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > .../devicetree/bindings/mtd/gpio-control-nand.txt | 5 ++--- Considering the changes in device-tree bindings, it's good to CC the device-tree list, I believe. > drivers/mtd/nand/gpio.c | 23 +++++++++++----------- > 2 files changed, 13 insertions(+), 15 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > index 36ef07d..287b8b8 100644 > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > @@ -8,15 +8,14 @@ Required properties: > - compatible : "gpio-control-nand" > - reg : should specify localbus chip select and size used for the chip. The > resource describes the data bus connected to the NAND flash and all accesses > - are made in native endianness. > + are made in native endianness. Bus width of the device is determined > + automatically if size > 1. If size = 1, 8 bit bus width will be used. > - #address-cells, #size-cells : Must be present if the device has sub-nodes > representing partitions. > - gpios : specifies the gpio pins to control the NAND device. nwp is an > optional gpio and may be set to 0 if not present. > > Optional properties: > -- bank-width : Width (in bytes) of the device. If not present, the width > - defaults to 1 byte. Do you really want to remove this property entirely? I'm not sure what the policy is on this. And it may still be useful to leave in the documentation, since older drivers (and potentially non-Linux OS?) may still need the property. Maybe you can mark it as optional, and that it may be ignored entirely. > - chip-delay : chip dependent delay for transferring data from array to > read registers (tR). If not present then a default of 20us is used. > - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read > diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c > index 800a1cc..f144b80 100644 > --- a/drivers/mtd/nand/gpio.c > +++ b/drivers/mtd/nand/gpio.c > @@ -109,14 +109,9 @@ static int gpio_nand_get_config_of(const struct device *dev, > if (!dev->of_node) > return -ENODEV; > > - if (!of_property_read_u32(dev->of_node, "bank-width", &val)) { > - if (val == 2) { > - plat->options |= NAND_BUSWIDTH_16; > - } else if (val != 1) { > - dev_err(dev, "invalid bank-width %u\n", val); > - return -EINVAL; > - } > - } > + /* Deprecated since 3.11-rc2 */ I doubt this patch will be included in 3.11-rc2 :) This is material for the next merge window. > + if (of_find_property(dev->of_node, "bank-width", NULL)) > + dev_notice(dev, "Property \"bank-width\" is deprecated"); If you don't totally kill this property (per my comments above), then you probably don't want this message either. It's probably safe to just ignore the property, if we can reliably auto-detect it instead. > plat->gpio_rdy = of_get_gpio(dev->of_node, 0); > plat->gpio_nce = of_get_gpio(dev->of_node, 1); > @@ -223,6 +218,14 @@ static int gpio_nand_probe(struct platform_device *pdev) > if (IS_ERR(chip->IO_ADDR_R)) > return PTR_ERR(chip->IO_ADDR_R); > > + ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat); > + if (ret) > + return ret; > + > + gpiomtd->plat.options &= ~(NAND_BUSWIDTH_16 | NAND_BUSWIDTH_AUTO); > + if (resource_size(res) > 1) > + gpiomtd->plat.options |= NAND_BUSWIDTH_AUTO; > + > res = gpio_nand_get_io_sync(pdev); > if (res) { > gpiomtd->io_sync = devm_ioremap_resource(&pdev->dev, res); > @@ -230,10 +233,6 @@ static int gpio_nand_probe(struct platform_device *pdev) > return PTR_ERR(gpiomtd->io_sync); > } > > - ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat); > - if (ret) > - return ret; > - > ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE"); > if (ret) > return ret; Brian
Resending to the new DT mailing list. (Guys, this isn't advertised very well, and your MAINTAINERS patch hasn't been applied to Linus' tree yet. Why the rapid transition?) On Tue, Jul 23, 2013 at 11:42 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote: >> This patch provide automatically determine of bus width. If resource size, >> supplied to the driver more than 1 byte, the NAND_BUSWIDTH_AUTO option >> will be used in the MTD core. > > I presume this depends on the bugfix I sent for NAND_BUSWIDTH_AUTO / > nand_set_defaults()? That's worth noting (as I am right now), for > whenever someone gets around to merging this. > >> Signed-off-by: Alexander Shiyan <shc_work@mail.ru> >> --- >> .../devicetree/bindings/mtd/gpio-control-nand.txt | 5 ++--- > > Considering the changes in device-tree bindings, it's good to CC the > device-tree list, I believe. > >> drivers/mtd/nand/gpio.c | 23 +++++++++++----------- >> 2 files changed, 13 insertions(+), 15 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt >> index 36ef07d..287b8b8 100644 >> --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt >> +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt >> @@ -8,15 +8,14 @@ Required properties: >> - compatible : "gpio-control-nand" >> - reg : should specify localbus chip select and size used for the chip. The >> resource describes the data bus connected to the NAND flash and all accesses >> - are made in native endianness. >> + are made in native endianness. Bus width of the device is determined >> + automatically if size > 1. If size = 1, 8 bit bus width will be used. >> - #address-cells, #size-cells : Must be present if the device has sub-nodes >> representing partitions. >> - gpios : specifies the gpio pins to control the NAND device. nwp is an >> optional gpio and may be set to 0 if not present. >> >> Optional properties: >> -- bank-width : Width (in bytes) of the device. If not present, the width >> - defaults to 1 byte. > > Do you really want to remove this property entirely? I'm not sure what > the policy is on this. And it may still be useful to leave in the > documentation, since older drivers (and potentially non-Linux OS?) may > still need the property. Maybe you can mark it as optional, and that > it may be ignored entirely. > >> - chip-delay : chip dependent delay for transferring data from array to >> read registers (tR). If not present then a default of 20us is used. >> - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read >> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c >> index 800a1cc..f144b80 100644 >> --- a/drivers/mtd/nand/gpio.c >> +++ b/drivers/mtd/nand/gpio.c >> @@ -109,14 +109,9 @@ static int gpio_nand_get_config_of(const struct device *dev, >> if (!dev->of_node) >> return -ENODEV; >> >> - if (!of_property_read_u32(dev->of_node, "bank-width", &val)) { >> - if (val == 2) { >> - plat->options |= NAND_BUSWIDTH_16; >> - } else if (val != 1) { >> - dev_err(dev, "invalid bank-width %u\n", val); >> - return -EINVAL; >> - } >> - } >> + /* Deprecated since 3.11-rc2 */ > > I doubt this patch will be included in 3.11-rc2 :) This is material > for the next merge window. > >> + if (of_find_property(dev->of_node, "bank-width", NULL)) >> + dev_notice(dev, "Property \"bank-width\" is deprecated"); > > If you don't totally kill this property (per my comments above), then > you probably don't want this message either. It's probably safe to > just ignore the property, if we can reliably auto-detect it instead. > >> plat->gpio_rdy = of_get_gpio(dev->of_node, 0); >> plat->gpio_nce = of_get_gpio(dev->of_node, 1); >> @@ -223,6 +218,14 @@ static int gpio_nand_probe(struct platform_device *pdev) >> if (IS_ERR(chip->IO_ADDR_R)) >> return PTR_ERR(chip->IO_ADDR_R); >> >> + ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat); >> + if (ret) >> + return ret; >> + >> + gpiomtd->plat.options &= ~(NAND_BUSWIDTH_16 | NAND_BUSWIDTH_AUTO); >> + if (resource_size(res) > 1) >> + gpiomtd->plat.options |= NAND_BUSWIDTH_AUTO; >> + >> res = gpio_nand_get_io_sync(pdev); >> if (res) { >> gpiomtd->io_sync = devm_ioremap_resource(&pdev->dev, res); >> @@ -230,10 +233,6 @@ static int gpio_nand_probe(struct platform_device *pdev) >> return PTR_ERR(gpiomtd->io_sync); >> } >> >> - ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat); >> - if (ret) >> - return ret; >> - >> ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE"); >> if (ret) >> return ret; > > Brian
On Tue, Jul 23, 2013 at 11:52 PM, Brian Norris <computersforpeace@gmail.com> wrote: > and your MAINTAINERS patch hasn't been applied to Linus' > tree yet I stand corrected on this point. It was applied a few hours ago. Regards, Brian
On Tue, 23 Jul 2013 23:42:08 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote: > > This patch provide automatically determine of bus width. If resource size, > > supplied to the driver more than 1 byte, the NAND_BUSWIDTH_AUTO option > > will be used in the MTD core. > > I presume this depends on the bugfix I sent for NAND_BUSWIDTH_AUTO / > nand_set_defaults()? That's worth noting (as I am right now), for > whenever someone gets around to merging this. OK. [...] > > diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > index 36ef07d..287b8b8 100644 > > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > @@ -8,15 +8,14 @@ Required properties: > > - compatible : "gpio-control-nand" > > - reg : should specify localbus chip select and size used for the chip. The > > resource describes the data bus connected to the NAND flash and all accesses > > - are made in native endianness. > > + are made in native endianness. Bus width of the device is determined > > + automatically if size > 1. If size = 1, 8 bit bus width will be used. > > - #address-cells, #size-cells : Must be present if the device has sub-nodes > > representing partitions. > > - gpios : specifies the gpio pins to control the NAND device. nwp is an > > optional gpio and may be set to 0 if not present. > > > > Optional properties: > > -- bank-width : Width (in bytes) of the device. If not present, the width > > - defaults to 1 byte. > > Do you really want to remove this property entirely? I'm not sure what > the policy is on this. And it may still be useful to leave in the > documentation, since older drivers (and potentially non-Linux OS?) may > still need the property. Maybe you can mark it as optional, and that > it may be ignored entirely. I redid it, I'll do option "bank-width" optional. [...] > > + if (of_find_property(dev->of_node, "bank-width", NULL)) > > + dev_notice(dev, "Property \"bank-width\" is deprecated"); > > If you don't totally kill this property (per my comments above), then > you probably don't want this message either. It's probably safe to > just ignore the property, if we can reliably auto-detect it instead. What do you say about such changes in the patch 4/4? Thanks. [...]
On Wed, Jul 24, 2013 at 6:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote: > On Tue, 23 Jul 2013 23:42:08 -0700 > Brian Norris <computersforpeace@gmail.com> wrote: > >> On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote: >> > diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt >> > index 36ef07d..287b8b8 100644 >> > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt >> > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt >> > @@ -8,15 +8,14 @@ Required properties: >> > - compatible : "gpio-control-nand" >> > - reg : should specify localbus chip select and size used for the chip. The >> > resource describes the data bus connected to the NAND flash and all accesses >> > - are made in native endianness. >> > + are made in native endianness. Bus width of the device is determined >> > + automatically if size > 1. If size = 1, 8 bit bus width will be used. >> > - #address-cells, #size-cells : Must be present if the device has sub-nodes >> > representing partitions. >> > - gpios : specifies the gpio pins to control the NAND device. nwp is an >> > optional gpio and may be set to 0 if not present. >> > >> > Optional properties: >> > -- bank-width : Width (in bytes) of the device. If not present, the width >> > - defaults to 1 byte. >> >> Do you really want to remove this property entirely? I'm not sure what >> the policy is on this. And it may still be useful to leave in the >> documentation, since older drivers (and potentially non-Linux OS?) may >> still need the property. Maybe you can mark it as optional, and that >> it may be ignored entirely. > > I redid it, I'll do option "bank-width" optional. > > [...] >> > + if (of_find_property(dev->of_node, "bank-width", NULL)) >> > + dev_notice(dev, "Property \"bank-width\" is deprecated"); >> >> If you don't totally kill this property (per my comments above), then >> you probably don't want this message either. It's probably safe to >> just ignore the property, if we can reliably auto-detect it instead. > > What do you say about such changes in the patch 4/4? > Thanks. I guess you're referring to this hunk from patch 4/4? - /* Deprecated since 3.11-rc2 */ + /* Deprecated since 3.11-rc2, should be removed around 3.17 */ This hunk really seems out of place in patch 4. If you even want this change, it should go in with patch 1. I don't see how it is related to patch 4. But the key point is that I don't think you should try to detect (and print notices about) the bank-width property at all. As long as the documentation clearly states that it is optional (and that some systems may autodetect it), I think that is sufficient not to print any warnings. You should consider that some user may want to retain this DT binding for some other non-Linux OS (or for some range of new and old kernel versions) so that they need this bank-width property. Another way to look at this is that it doesn't hurt Linux at all for there to be an extra DT property for a device. But it can hurt the bootloader if it needs to support other OS's/drivers that don't autodetect bank-width property. On the other hand, if you're going to include this kind of schedule-related comment at all, then it should at least be correct. I can tell you a few things for sure: (1) This code is not going into any of the 3.11 release candidates (2) It doesn't really make sense to a user to see references to "-rc2" when reading Linux source code. That's very much a process-related detail that should not go in here. Brian
(snip conversation) BTW Alexander, please be sure to CC the appropriate list (devicetree@vger.kernel.org) for the resend of your DT-related patches (I initially used the wrong list). They should have better input on the process of adding/removing/deprecating device properties. And if they don't have any opinion, then it's probably fine to do this whatever way you/we see fit :) Brian
On Tue, Jul 23, 2013 at 11:42:08PM -0700, Brian Norris wrote: > On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote: > > This patch provide automatically determine of bus width. If resource size, > > supplied to the driver more than 1 byte, the NAND_BUSWIDTH_AUTO option > > will be used in the MTD core. > > I presume this depends on the bugfix I sent for NAND_BUSWIDTH_AUTO / > nand_set_defaults()? That's worth noting (as I am right now), for > whenever someone gets around to merging this. > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > --- > > .../devicetree/bindings/mtd/gpio-control-nand.txt | 5 ++--- > > Considering the changes in device-tree bindings, it's good to CC the > device-tree list, I believe. > Adding the new devicetree list in Cc. Keep in mind you might want to split the DT binding part of the patch into a separate patch. For small patches like this one, it's probably not a big deal.
diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt index 36ef07d..287b8b8 100644 --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt @@ -8,15 +8,14 @@ Required properties: - compatible : "gpio-control-nand" - reg : should specify localbus chip select and size used for the chip. The resource describes the data bus connected to the NAND flash and all accesses - are made in native endianness. + are made in native endianness. Bus width of the device is determined + automatically if size > 1. If size = 1, 8 bit bus width will be used. - #address-cells, #size-cells : Must be present if the device has sub-nodes representing partitions. - gpios : specifies the gpio pins to control the NAND device. nwp is an optional gpio and may be set to 0 if not present. Optional properties: -- bank-width : Width (in bytes) of the device. If not present, the width - defaults to 1 byte. - chip-delay : chip dependent delay for transferring data from array to read registers (tR). If not present then a default of 20us is used. - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c index 800a1cc..f144b80 100644 --- a/drivers/mtd/nand/gpio.c +++ b/drivers/mtd/nand/gpio.c @@ -109,14 +109,9 @@ static int gpio_nand_get_config_of(const struct device *dev, if (!dev->of_node) return -ENODEV; - if (!of_property_read_u32(dev->of_node, "bank-width", &val)) { - if (val == 2) { - plat->options |= NAND_BUSWIDTH_16; - } else if (val != 1) { - dev_err(dev, "invalid bank-width %u\n", val); - return -EINVAL; - } - } + /* Deprecated since 3.11-rc2 */ + if (of_find_property(dev->of_node, "bank-width", NULL)) + dev_notice(dev, "Property \"bank-width\" is deprecated"); plat->gpio_rdy = of_get_gpio(dev->of_node, 0); plat->gpio_nce = of_get_gpio(dev->of_node, 1); @@ -223,6 +218,14 @@ static int gpio_nand_probe(struct platform_device *pdev) if (IS_ERR(chip->IO_ADDR_R)) return PTR_ERR(chip->IO_ADDR_R); + ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat); + if (ret) + return ret; + + gpiomtd->plat.options &= ~(NAND_BUSWIDTH_16 | NAND_BUSWIDTH_AUTO); + if (resource_size(res) > 1) + gpiomtd->plat.options |= NAND_BUSWIDTH_AUTO; + res = gpio_nand_get_io_sync(pdev); if (res) { gpiomtd->io_sync = devm_ioremap_resource(&pdev->dev, res); @@ -230,10 +233,6 @@ static int gpio_nand_probe(struct platform_device *pdev) return PTR_ERR(gpiomtd->io_sync); } - ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat); - if (ret) - return ret; - ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE"); if (ret) return ret;
This patch provide automatically determine of bus width. If resource size, supplied to the driver more than 1 byte, the NAND_BUSWIDTH_AUTO option will be used in the MTD core. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- .../devicetree/bindings/mtd/gpio-control-nand.txt | 5 ++--- drivers/mtd/nand/gpio.c | 23 +++++++++++----------- 2 files changed, 13 insertions(+), 15 deletions(-)