| Message ID | 20180518210954.29044-1-jmkrzyszt@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { So, is it optional or not at the end? If it is, why do we check for NULL? > this->dev_ready = ams_delta_nand_ready; > } else { > this->dev_ready = NULL; > pr_notice("Couldn't request gpio for Delta NAND ready.\n"); dev_notice() ? > } > +err_gpiod: > + if (err == -ENODEV || err == -ENOENT) > + err = -EPROBE_DEFER; Hmm...
On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > > <jmkrzyszt@gmail.com> wrote: > > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > So, is it optional or not at the end? > If it is, why do we check for NULL? As far as I can understand, nand_chip->dev_ready() callback is optional. That's why I decided to use the _optional variant of devm_gpiod_get(). In case of ams-delta, the dev_ready() callback depends on availability of the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and ERR in order to decide if dev_ready() will be supported. I can pretty well replace it with the standard form and check for ERR only if the purpose of the _optional form is different. > > this->dev_ready = ams_delta_nand_ready; > > > > } else { > > > > this->dev_ready = NULL; > > pr_notice("Couldn't request gpio for Delta NAND > > ready.\n"); > > dev_notice() ? Sure, but maybe in a separate patch? That's not a new code just being added but an existing one, not the merit of the change. > > } > > > > +err_gpiod: > > + if (err == -ENODEV || err == -ENOENT) > > + err = -EPROBE_DEFER; > > Hmm... Amstrad Delta uses gpio-mmio driver. Unfortunatelty that driver is not availble before device init phase, unlike other crucial GPIO drivers which are initialized earlier, e.g. during the postcore or at latetst the subsys phase. Hence, devices which depend on GPIO pins provided by gpio-mmio must either be declared late or fail softly so they get another chance of being probed succesfully. I thought of replacing the gpio-mmio platform driver with bgpio functions it exports but for now I haven't implemented it, not even shared the idea. Does it really hurt to return -EPROBE_DEFER if a GPIO pin can't be obtained? Thanks, Janusz
On Sat, May 19, 2018 at 2:15 AM, Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: >> On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik >> >> <jmkrzyszt@gmail.com> wrote: >> > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); >> > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { >> >> So, is it optional or not at the end? >> If it is, why do we check for NULL? > > As far as I can understand, nand_chip->dev_ready() callback is optional. > That's why I decided to use the _optional variant of devm_gpiod_get(). In case > of ams-delta, the dev_ready() callback depends on availability of the 'rdy' > GPIO pin. As a consequence, I'm checking for both NULL and ERR in order to > decide if dev_ready() will be supported. > > I can pretty well replace it with the standard form and check for ERR only if > the purpose of the _optional form is different. NULL check in practice discards the _optional part of gpiod_get(). So, either you use non-optional variant and decide how to handle an errors, or user _optional w/o NULL check. >> > +err_gpiod: >> > + if (err == -ENODEV || err == -ENOENT) >> > + err = -EPROBE_DEFER; >> >> Hmm... > > Amstrad Delta uses gpio-mmio driver. Unfortunatelty that driver is not > availble before device init phase, unlike other crucial GPIO drivers which are > initialized earlier, e.g. during the postcore or at latetst the subsys phase. > Hence, devices which depend on GPIO pins provided by gpio-mmio must either be > declared late or fail softly so they get another chance of being probed > succesfully. > > I thought of replacing the gpio-mmio platform driver with bgpio functions it > exports but for now I haven't implemented it, not even shared the idea. > > Does it really hurt to return -EPROBE_DEFER if a GPIO pin can't be obtained? I'm only concerned if it would be an infinite defer in the case when driver will never appear. But I don't remember the details.
On Saturday, May 19, 2018 8:00:38 PM CEST Andy Shevchenko wrote: > On Sat, May 19, 2018 at 2:15 AM, Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > > On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > >> On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > >> > >> <jmkrzyszt@gmail.com> wrote: > >> > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > >> > GPIOD_IN); > >> > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > >> > >> So, is it optional or not at the end? > >> If it is, why do we check for NULL? > > > > As far as I can understand, nand_chip->dev_ready() callback is optional. > > That's why I decided to use the _optional variant of devm_gpiod_get(). In > > case of ams-delta, the dev_ready() callback depends on availability of > > the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and ERR > > in order to decide if dev_ready() will be supported. > > > > I can pretty well replace it with the standard form and check for ERR only > > if the purpose of the _optional form is different. > > NULL check in practice discards the _optional part of gpiod_get(). So, > either you use non-optional variant and decide how to handle an > errors, or user _optional w/o NULL check. OK, I'm going to use something like the below while submitting v2: - gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); - if (!IS_ERR_OR_NULL(gpiod_rdy)) { - this->dev_ready = ams_delta_nand_ready; - } else { - this->dev_ready = NULL; - pr_notice("Couldn't request gpio for Delta NAND ready.\n"); + priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", + GPIOD_IN); + if (IS_ERR(priv->gpiod_rdy)) { + err = PTR_ERR(priv->gpiod_nwp); + dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err); + goto err_gpiod; } + if (priv->gpiod_rdy) + this->dev_ready = ams_delta_nand_ready; > > >> > +err_gpiod: > >> > + if (err == -ENODEV || err == -ENOENT) > >> > + err = -EPROBE_DEFER; > >> > >> Hmm... > > > > Amstrad Delta uses gpio-mmio driver. Unfortunatelty that driver is not > > availble before device init phase, unlike other crucial GPIO drivers which > > are initialized earlier, e.g. during the postcore or at latetst the > > subsys phase. Hence, devices which depend on GPIO pins provided by > > gpio-mmio must either be declared late or fail softly so they get another > > chance of being probed succesfully. > > > > I thought of replacing the gpio-mmio platform driver with bgpio functions > > it exports but for now I haven't implemented it, not even shared the > > idea. > > > > Does it really hurt to return -EPROBE_DEFER if a GPIO pin can't be > > obtained? > I'm only concerned if it would be an infinite defer in the case when > driver will never appear. > But I don't remember the details. Deferred probes are handled effectively during late_initcall, no risk of infinite defer, see drivers/base/dd.c for details. Thanks, Janusz
On Sun, May 20, 2018 at 12:55 AM, Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > On Saturday, May 19, 2018 8:00:38 PM CEST Andy Shevchenko wrote: >> On Sat, May 19, 2018 at 2:15 AM, Janusz Krzysztofik <jmkrzyszt@gmail.com> > wrote: >> NULL check in practice discards the _optional part of gpiod_get(). So, >> either you use non-optional variant and decide how to handle an >> errors, or user _optional w/o NULL check. > > OK, I'm going to use something like the below while submitting v2: > > - gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > - if (!IS_ERR_OR_NULL(gpiod_rdy)) { > - this->dev_ready = ams_delta_nand_ready; > - } else { > - this->dev_ready = NULL; > - pr_notice("Couldn't request gpio for Delta NAND ready.\n"); > + priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > + GPIOD_IN); > + if (IS_ERR(priv->gpiod_rdy)) { > + err = PTR_ERR(priv->gpiod_nwp); > + dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err); > + goto err_gpiod; > } > > + if (priv->gpiod_rdy) > + this->dev_ready = ams_delta_nand_ready; This makes sense. Though, I completely dislike "rdy" name of GPIO. Where is it documented? >> >> > +err_gpiod: >> >> > + if (err == -ENODEV || err == -ENOENT) >> >> > + err = -EPROBE_DEFER; >> >> >> >> Hmm... >> > >> > Amstrad Delta uses gpio-mmio driver. Unfortunatelty that driver is not >> > availble before device init phase, unlike other crucial GPIO drivers which >> > are initialized earlier, e.g. during the postcore or at latetst the >> > subsys phase. Hence, devices which depend on GPIO pins provided by >> > gpio-mmio must either be declared late or fail softly so they get another >> > chance of being probed succesfully. >> > >> > I thought of replacing the gpio-mmio platform driver with bgpio functions >> > it exports but for now I haven't implemented it, not even shared the >> > idea. >> > >> > Does it really hurt to return -EPROBE_DEFER if a GPIO pin can't be >> > obtained? >> I'm only concerned if it would be an infinite defer in the case when >> driver will never appear. >> But I don't remember the details. > > Deferred probes are handled effectively during late_initcall, no risk of > infinite defer, see drivers/base/dd.c for details. Yes, but the code you provided in patch looks somehow suspicious. OK, I let Linus decide whtat to do with that.
On Sunday, May 20, 2018 4:44:31 PM CEST Andy Shevchenko wrote: > On Sun, May 20, 2018 at 12:55 AM, Janusz Krzysztofik > > <jmkrzyszt@gmail.com> wrote: > > On Saturday, May 19, 2018 8:00:38 PM CEST Andy Shevchenko wrote: > >> On Sat, May 19, 2018 at 2:15 AM, Janusz Krzysztofik <jmkrzyszt@gmail.com> > > > > wrote: > >> NULL check in practice discards the _optional part of gpiod_get(). So, > >> either you use non-optional variant and decide how to handle an > >> errors, or user _optional w/o NULL check. > > > > OK, I'm going to use something like the below while submitting v2: > > > > - gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > - if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > - this->dev_ready = ams_delta_nand_ready; > > - } else { > > - this->dev_ready = NULL; > > - pr_notice("Couldn't request gpio for Delta NAND > > ready.\n"); > > + priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > > + GPIOD_IN); > > + if (IS_ERR(priv->gpiod_rdy)) { > > + err = PTR_ERR(priv->gpiod_nwp); > > + dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", > > err); + goto err_gpiod; > > > > } > > > > + if (priv->gpiod_rdy) > > + this->dev_ready = ams_delta_nand_ready; > > This makes sense. > > Though, I completely dislike "rdy" name of GPIO. Where is it documented? No documentation files for Amstrad Delta nor for its NAND driver specifically exist under Documentation/. However, there exist some for generic GPIO NAND driver where the pin name "rdy" is used explicitly: Documentation/driver-api/gpio/drivers-on-gpio.rst Documentation/devicetree/bindings/mtd/gpio-control-nand.txt You can find that mnemonic used across drivers/mtd/nand/, standalone or as a suffix, including the Amstrad Delta NAND driver before the change discussed. To be honest, I don't like it much either, but I'm just using it instead of inventing something new. Thanks, Janusz
On Sun, May 20, 2018 at 6:37 PM, Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > On Sunday, May 20, 2018 4:44:31 PM CEST Andy Shevchenko wrote: >> Though, I completely dislike "rdy" name of GPIO. Where is it documented? > > No documentation files for Amstrad Delta nor for its NAND driver specifically > exist under Documentation/. However, there exist some for generic GPIO NAND > driver where the pin name "rdy" is used explicitly: > Documentation/driver-api/gpio/drivers-on-gpio.rst > Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > You can find that mnemonic used across drivers/mtd/nand/, standalone or as a > suffix, including the Amstrad Delta NAND driver before the change discussed. > To be honest, I don't like it much either, but I'm just using it instead of > inventing something new. OK, that's what I was looking for. Since it's already in use and documented, then it's fine for me.
Hello, On Sun, 20 May 2018 19:17:04 +0300, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> Though, I completely dislike "rdy" name of GPIO. Where is it documented? > > > > No documentation files for Amstrad Delta nor for its NAND driver specifically > > exist under Documentation/. However, there exist some for generic GPIO NAND > > driver where the pin name "rdy" is used explicitly: > > Documentation/driver-api/gpio/drivers-on-gpio.rst > > Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > You can find that mnemonic used across drivers/mtd/nand/, standalone or as a > > suffix, including the Amstrad Delta NAND driver before the change discussed. > > > To be honest, I don't like it much either, but I'm just using it instead of > > inventing something new. > > OK, that's what I was looking for. Since it's already in use and > documented, then it's fine for me. Do we actually have the possibility to rename this gpio? I guess no since it would break DT backward compatibility. Otherwise it would have been more descriptive to call it something like 'gpio-rb'. Anyway, if you find the time, documentation for this controller would be welcome! Thanks, Miquèl
On Sat, May 19, 2018 at 11:55:51PM +0200, Janusz Krzysztofik wrote: > On Saturday, May 19, 2018 8:00:38 PM CEST Andy Shevchenko wrote: > > On Sat, May 19, 2018 at 2:15 AM, Janusz Krzysztofik <jmkrzyszt@gmail.com> > wrote: > > > On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > > >> On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > > >> > > >> <jmkrzyszt@gmail.com> wrote: > > >> > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > > >> > GPIOD_IN); > > >> > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > >> > > >> So, is it optional or not at the end? > > >> If it is, why do we check for NULL? > > > > > > As far as I can understand, nand_chip->dev_ready() callback is optional. > > > That's why I decided to use the _optional variant of devm_gpiod_get(). In > > > case of ams-delta, the dev_ready() callback depends on availability of > > > the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and ERR > > > in order to decide if dev_ready() will be supported. > > > > > > I can pretty well replace it with the standard form and check for ERR only > > > if the purpose of the _optional form is different. > > > > NULL check in practice discards the _optional part of gpiod_get(). So, > > either you use non-optional variant and decide how to handle an > > errors, or user _optional w/o NULL check. > > OK, I'm going to use something like the below while submitting v2: > > - gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > - if (!IS_ERR_OR_NULL(gpiod_rdy)) { > - this->dev_ready = ams_delta_nand_ready; > - } else { > - this->dev_ready = NULL; > - pr_notice("Couldn't request gpio for Delta NAND ready.\n"); > + priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > + GPIOD_IN); > + if (IS_ERR(priv->gpiod_rdy)) { > + err = PTR_ERR(priv->gpiod_nwp); ??? --------------------------------^^^^^^^^^ > + dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err); > + goto err_gpiod; Driver will just use worst case delay instead of RDY signal, so this is perhaps too strict. I will work with degraded performance. ladis > } > > + if (priv->gpiod_rdy) > + this->dev_ready = ams_delta_nand_ready; > > > > > >> > +err_gpiod: > > >> > + if (err == -ENODEV || err == -ENOENT) > > >> > + err = -EPROBE_DEFER; > > >> > > >> Hmm... > > > > > > Amstrad Delta uses gpio-mmio driver. Unfortunatelty that driver is not > > > availble before device init phase, unlike other crucial GPIO drivers which > > > are initialized earlier, e.g. during the postcore or at latetst the > > > subsys phase. Hence, devices which depend on GPIO pins provided by > > > gpio-mmio must either be declared late or fail softly so they get another > > > chance of being probed succesfully. > > > > > > I thought of replacing the gpio-mmio platform driver with bgpio functions > > > it exports but for now I haven't implemented it, not even shared the > > > idea. > > > > > > Does it really hurt to return -EPROBE_DEFER if a GPIO pin can't be > > > obtained? > > I'm only concerned if it would be an infinite defer in the case when > > driver will never appear. > > But I don't remember the details. > > Deferred probes are handled effectively during late_initcall, no risk of > infinite defer, see drivers/base/dd.c for details. > > Thanks, > Janusz > > > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Sun, May 20, 2018 at 09:27:05PM +0200, Ladislav Michl wrote: > On Sat, May 19, 2018 at 11:55:51PM +0200, Janusz Krzysztofik wrote: > > On Saturday, May 19, 2018 8:00:38 PM CEST Andy Shevchenko wrote: > > > On Sat, May 19, 2018 at 2:15 AM, Janusz Krzysztofik <jmkrzyszt@gmail.com> > > wrote: > > > > On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > > > >> On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > > > >> > > > >> <jmkrzyszt@gmail.com> wrote: > > > >> > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > > > >> > GPIOD_IN); > > > >> > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > > >> > > > >> So, is it optional or not at the end? > > > >> If it is, why do we check for NULL? > > > > > > > > As far as I can understand, nand_chip->dev_ready() callback is optional. > > > > That's why I decided to use the _optional variant of devm_gpiod_get(). In > > > > case of ams-delta, the dev_ready() callback depends on availability of > > > > the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and ERR > > > > in order to decide if dev_ready() will be supported. > > > > > > > > I can pretty well replace it with the standard form and check for ERR only > > > > if the purpose of the _optional form is different. > > > > > > NULL check in practice discards the _optional part of gpiod_get(). So, > > > either you use non-optional variant and decide how to handle an > > > errors, or user _optional w/o NULL check. > > > > OK, I'm going to use something like the below while submitting v2: > > > > - gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > - if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > - this->dev_ready = ams_delta_nand_ready; > > - } else { > > - this->dev_ready = NULL; > > - pr_notice("Couldn't request gpio for Delta NAND ready.\n"); > > + priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > > + GPIOD_IN); > > + if (IS_ERR(priv->gpiod_rdy)) { > > + err = PTR_ERR(priv->gpiod_nwp); > ??? --------------------------------^^^^^^^^^ > > + dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err); > > + goto err_gpiod; > > Driver will just use worst case delay instead of RDY signal, so this > is perhaps too strict. I will work with degraded performance. If RDY signal is not available then the board should not define it. Degrading performance and having users wondering because RDY is sometimes not available is not great. Especially if we get -EPROBE_DEFER here. Thanks.
On Sun, May 20, 2018 at 8:25 PM, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > On Sun, 20 May 2018 19:17:04 +0300, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > Do we actually have the possibility to rename this gpio? I guess no > since it would break DT backward compatibility. No we don't. > Otherwise it would have > been more descriptive to call it something like 'gpio-rb'. "gpio" prefix, actually "gpios" suffix is a mandatory part of the name. For sake of convenience it's not used in API calls.
Hi, * Janusz Krzysztofik <jmkrzyszt@gmail.com> [180518 14:12]: > Scope of the change is limited to GPIO pins used by board specific > device drivers which will be updated by follow-up patches of the > series. Those are some OMAP GPIO (gpio-0-15) and most of Amstrad Delta > latch2 GPIO bank pins. Remaining pins of those banks, as well as > Amstrad Delta latch1 pins, will be addressed later. > > Assign a label ("latch2") to the bank, enumerate its pins and put that > information, together with OMAP GPIO bank pins, in GPIO lookup tables. > Assign lookup tables to devices as soon as those devices are registered > and their names can be obtained. > > A step froward in: > - removal of hard-coded GPIO numbers from drivers, > - removal of board mach includes from drivers, > - switching to dynamically assigned GPIO numbers. Is this first patch safe for me to apply separately? Regards, Tony
On Monday, May 21, 2018 7:35:19 PM CEST Tony Lindgren wrote: > Hi, > > * Janusz Krzysztofik <jmkrzyszt@gmail.com> [180518 14:12]: > > Scope of the change is limited to GPIO pins used by board specific > > device drivers which will be updated by follow-up patches of the > > series. Those are some OMAP GPIO (gpio-0-15) and most of Amstrad Delta > > latch2 GPIO bank pins. Remaining pins of those banks, as well as > > Amstrad Delta latch1 pins, will be addressed later. > > > > Assign a label ("latch2") to the bank, enumerate its pins and put that > > information, together with OMAP GPIO bank pins, in GPIO lookup tables. > > Assign lookup tables to devices as soon as those devices are registered > > and their names can be obtained. > > > > A step froward in: > > - removal of hard-coded GPIO numbers from drivers, > > - removal of board mach includes from drivers, > > - switching to dynamically assigned GPIO numbers. > > Is this first patch safe for me to apply separately? Absolutely, it is. Thanks, Janusz
On Sunday, May 20, 2018 10:08:22 PM CEST Dmitry Torokhov wrote: > On Sun, May 20, 2018 at 09:27:05PM +0200, Ladislav Michl wrote: > > On Sat, May 19, 2018 at 11:55:51PM +0200, Janusz Krzysztofik wrote: > > > On Saturday, May 19, 2018 8:00:38 PM CEST Andy Shevchenko wrote: > > > > On Sat, May 19, 2018 at 2:15 AM, Janusz Krzysztofik > > > > <jmkrzyszt@gmail.com> > > > > > > wrote: > > > > > On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > > > > >> On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > > > > >> > > > > >> <jmkrzyszt@gmail.com> wrote: > > > > >> > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > > > > >> > GPIOD_IN); > > > > >> > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > > > >> > > > > >> So, is it optional or not at the end? > > > > >> If it is, why do we check for NULL? > > > > > > > > > > As far as I can understand, nand_chip->dev_ready() callback is > > > > > optional. > > > > > That's why I decided to use the _optional variant of > > > > > devm_gpiod_get(). In > > > > > case of ams-delta, the dev_ready() callback depends on availability > > > > > of > > > > > the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and > > > > > ERR > > > > > in order to decide if dev_ready() will be supported. > > > > > > > > > > I can pretty well replace it with the standard form and check for > > > > > ERR only > > > > > if the purpose of the _optional form is different. > > > > > > > > NULL check in practice discards the _optional part of gpiod_get(). So, > > > > either you use non-optional variant and decide how to handle an > > > > errors, or user _optional w/o NULL check. > > > > > > OK, I'm going to use something like the below while submitting v2: > > > > > > - gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > > - if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > > - this->dev_ready = ams_delta_nand_ready; > > > - } else { > > > - this->dev_ready = NULL; > > > - pr_notice("Couldn't request gpio for Delta NAND ready.\n"); > > > + priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > > > + GPIOD_IN); > > > + if (IS_ERR(priv->gpiod_rdy)) { > > > + err = PTR_ERR(priv->gpiod_nwp); > > > > ??? --------------------------------^^^^^^^^^ > > > > > + dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err); > > > + goto err_gpiod; > > > > Driver will just use worst case delay instead of RDY signal, so this > > is perhaps too strict. I will work with degraded performance. > > If RDY signal is not available then the board should not define it. > Degrading performance and having users wondering because RDY is > sometimes not available is not great. Especially if we get -EPROBE_DEFER > here. Hi, I'm a bit lost after your comments. As far as I can read the code of gpiod_get_optional and underlying functions, if a board doesn't define the "rdy" pin in a respective lookup table, the function returns NULL and the device gets a chance to work in degraded mode. NULL may also happen if the driver probes the device before the lookup table is added. In that case other non-optional pin requests fail with -ENOENT, the probe is deferred and the device gets a chance to probe successfully in late_init if the table is added but fails if not. If the pin is defined but GPIO device providing that pin is not available (-ENODEV), the probe is initially deferred and may succeed in late_init if the GPIO device appears but fails otherwise. Isn't that behavior acceptable, close enough to the expected even if not strictly because of that -EPROBE_DEFER? Thanks, Janusz
On Mon, May 21, 2018 at 10:21:46PM +0200, Janusz Krzysztofik wrote: > On Sunday, May 20, 2018 10:08:22 PM CEST Dmitry Torokhov wrote: > > On Sun, May 20, 2018 at 09:27:05PM +0200, Ladislav Michl wrote: > > > On Sat, May 19, 2018 at 11:55:51PM +0200, Janusz Krzysztofik wrote: > > > > On Saturday, May 19, 2018 8:00:38 PM CEST Andy Shevchenko wrote: > > > > > On Sat, May 19, 2018 at 2:15 AM, Janusz Krzysztofik > > > > > <jmkrzyszt@gmail.com> > > > > > > > > wrote: > > > > > > On Friday, May 18, 2018 11:21:14 PM CEST Andy Shevchenko wrote: > > > > > >> On Sat, May 19, 2018 at 12:09 AM, Janusz Krzysztofik > > > > > >> > > > > > >> <jmkrzyszt@gmail.com> wrote: > > > > > >> > + gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > > > > > >> > GPIOD_IN); > > > > > >> > + if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > > > > >> > > > > > >> So, is it optional or not at the end? > > > > > >> If it is, why do we check for NULL? > > > > > > > > > > > > As far as I can understand, nand_chip->dev_ready() callback is > > > > > > optional. > > > > > > That's why I decided to use the _optional variant of > > > > > > devm_gpiod_get(). In > > > > > > case of ams-delta, the dev_ready() callback depends on availability > > > > > > of > > > > > > the 'rdy' GPIO pin. As a consequence, I'm checking for both NULL and > > > > > > ERR > > > > > > in order to decide if dev_ready() will be supported. > > > > > > > > > > > > I can pretty well replace it with the standard form and check for > > > > > > ERR only > > > > > > if the purpose of the _optional form is different. > > > > > > > > > > NULL check in practice discards the _optional part of gpiod_get(). So, > > > > > either you use non-optional variant and decide how to handle an > > > > > errors, or user _optional w/o NULL check. > > > > > > > > OK, I'm going to use something like the below while submitting v2: > > > > > > > > - gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", GPIOD_IN); > > > > - if (!IS_ERR_OR_NULL(gpiod_rdy)) { > > > > - this->dev_ready = ams_delta_nand_ready; > > > > - } else { > > > > - this->dev_ready = NULL; > > > > - pr_notice("Couldn't request gpio for Delta NAND ready.\n"); > > > > + priv->gpiod_rdy = devm_gpiod_get_optional(&pdev->dev, "rdy", > > > > + GPIOD_IN); > > > > + if (IS_ERR(priv->gpiod_rdy)) { > > > > + err = PTR_ERR(priv->gpiod_nwp); > > > > > > ??? --------------------------------^^^^^^^^^ > > > > > > > + dev_warn(&pdev->dev, "RDY GPIO request failed (%d)\n", err); > > > > + goto err_gpiod; > > > > > > Driver will just use worst case delay instead of RDY signal, so this > > > is perhaps too strict. I will work with degraded performance. > > > > If RDY signal is not available then the board should not define it. > > Degrading performance and having users wondering because RDY is > > sometimes not available is not great. Especially if we get -EPROBE_DEFER > > here. > > Hi, > > I'm a bit lost after your comments. > > As far as I can read the code of gpiod_get_optional and underlying functions, > if a board doesn't define the "rdy" pin in a respective lookup table, the > function returns NULL and the device gets a chance to work in degraded mode. > > NULL may also happen if the driver probes the device before the lookup table > is added. In that case other non-optional pin requests fail with -ENOENT, the > probe is deferred and the device gets a chance to probe successfully in > late_init if the table is added but fails if not. > > If the pin is defined but GPIO device providing that pin is not available > (-ENODEV), the probe is initially deferred and may succeed in late_init if the > GPIO device appears but fails otherwise. > > Isn't that behavior acceptable, close enough to the expected even if not > strictly because of that -EPROBE_DEFER? Yes, this is correct. I was responding to the comment that erroring out in "if (IS_ERR(priv->gpiod_rdy))" branch is too strict. My assertion that it is not. If a board defines RDY pin we should use it and not try to degrade to lower performance mode. Thanks.
This is a follow up of initial submission of a series consisted of
6 changes, 3 of which have been already applied or reworkeed.
V2 changelog:
[PATCH 1/6] ARM: OMAP1: ams-delta: add GPIO lookup tables
- already in mainline, commit 68e62a15a914
[PATCH 2/6] Input: ams_delta_serio: use GPIO lookup table
- reworked and submitted as a series, already in linux-omap,
commit 68e62a15a914 ("ARM: OMAP1: ams-delta: drop GPIO lookup
table for serio device") followed by 9 more
[PATCH 3/6] ASoC: ams_delta: use GPIO lookup table
- already in mainline, commit d65777d1a2cd
[PATCH 4/6] fbdev: omapfb: lcd_ams_delta: use GPIO lookup table
- resubmitting as [PATCH v2 1/3 v2]
v2: Remove problematic error code conversion no longer
needed if used on top of commit d08605a64e67 ("ARM: OMAP1:
ams-delta: move late devices back to init_machine")
and commit 8853daf3b4ac ("gpiolib: Defer on non-DT
find_chip_by_name() failure") already in linux-next
[PATCH 5/6] mtd: rawnand: ams-delta: use GPIO lookup table
- resubmitting as [PATCH v2 2/3 v4]
v2: Fix handling of devm_gpiod_get_optional() return values -
thanks to Andy Shevchenko.
v3: Remove problematic error code conversion no longer needed
if used on top of commit d08605a64e67 ("ARM: OMAP1:
ams-delta: move late devices back to init_machine") and
commit 8853daf3b4ac ("gpiolib: Defer on non-DT
find_chip_by_name() failure") already in linux-next - thanks
to Boris Brezillon
v4: fix style issue - thanks to Boris Brezillon
[PATCH 6/6] ARM: OMAP1: ams-delta: make board header file local to
mach-omap1
- resending as [PATCH v2 3/3]
Dependency on commit 8853daf3b4ac ("gpiolib: Defer on non-DT
find_chip_by_name() failure") is not critical - it is not needed for
clean build or run, it only prevents from potential future changes to
driver initializaton order during device_initcall.
I'm submitting the three patches in series because the last one depends
on the other two.
Thanks,
Janusz
Hi Janusz, On mer., juil. 18 2018, Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote: > This is a follow up of initial submission of a series consisted of > 6 changes, 3 of which have been already applied or reworkeed. > > V2 changelog: > [PATCH 1/6] ARM: OMAP1: ams-delta: add GPIO lookup tables > - already in mainline, commit 68e62a15a914 > [PATCH 2/6] Input: ams_delta_serio: use GPIO lookup table > - reworked and submitted as a series, already in linux-omap, > commit 68e62a15a914 ("ARM: OMAP1: ams-delta: drop GPIO lookup > table for serio device") followed by 9 more > [PATCH 3/6] ASoC: ams_delta: use GPIO lookup table > - already in mainline, commit d65777d1a2cd > [PATCH 4/6] fbdev: omapfb: lcd_ams_delta: use GPIO lookup table > - resubmitting as [PATCH v2 1/3 v2] > v2: Remove problematic error code conversion no longer > needed if used on top of commit d08605a64e67 ("ARM: OMAP1: > ams-delta: move late devices back to init_machine") > and commit 8853daf3b4ac ("gpiolib: Defer on non-DT > find_chip_by_name() failure") already in linux-next > [PATCH 5/6] mtd: rawnand: ams-delta: use GPIO lookup table > - resubmitting as [PATCH v2 2/3 v4] > v2: Fix handling of devm_gpiod_get_optional() return values - > thanks to Andy Shevchenko. > v3: Remove problematic error code conversion no longer needed > if used on top of commit d08605a64e67 ("ARM: OMAP1: > ams-delta: move late devices back to init_machine") and > commit 8853daf3b4ac ("gpiolib: Defer on non-DT > find_chip_by_name() failure") already in linux-next - thanks > to Boris Brezillon > v4: fix style issue - thanks to Boris Brezillon > [PATCH 6/6] ARM: OMAP1: ams-delta: make board header file local to > mach-omap1 > - resending as [PATCH v2 3/3] > > Dependency on commit 8853daf3b4ac ("gpiolib: Defer on non-DT > find_chip_by_name() failure") is not critical - it is not needed for > clean build or run, it only prevents from potential future changes to > driver initializaton order during device_initcall. > > I'm submitting the three patches in series because the last one depends > on the other two. I think that being in CC in this series is a mistake as I don't see anything related what I have done in this series. Gregory > > Thanks, > Janusz >
diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c index 52e8e53ca154..4b78e73f8bf7 100644 --- a/arch/arm/mach-omap1/board-ams-delta.c +++ b/arch/arm/mach-omap1/board-ams-delta.c @@ -12,6 +12,7 @@ * published by the Free Software Foundation. */ #include <linux/gpio/driver.h> +#include <linux/gpio/machine.h> #include <linux/gpio.h> #include <linux/kernel.h> #include <linux/init.h> @@ -202,7 +203,10 @@ static struct resource latch2_resources[] = { }, }; +#define LATCH2_LABEL "latch2" + static struct bgpio_pdata latch2_pdata = { + .label = LATCH2_LABEL, .base = AMS_DELTA_LATCH2_GPIO_BASE, .ngpio = AMS_DELTA_LATCH2_NGPIO, }; @@ -217,6 +221,23 @@ static struct platform_device latch2_gpio_device = { }, }; +#define LATCH2_PIN_LCD_VBLEN 0 +#define LATCH2_PIN_LCD_NDISP 1 +#define LATCH2_PIN_NAND_NCE 2 +#define LATCH2_PIN_NAND_NRE 3 +#define LATCH2_PIN_NAND_NWP 4 +#define LATCH2_PIN_NAND_NWE 5 +#define LATCH2_PIN_NAND_ALE 6 +#define LATCH2_PIN_NAND_CLE 7 +#define LATCH2_PIN_KEYBRD_PWR 8 +#define LATCH2_PIN_KEYBRD_DATAOUT 9 +#define LATCH2_PIN_SCARD_RSTIN 10 +#define LATCH2_PIN_SCARD_CMDVCC 11 +#define LATCH2_PIN_MODEM_NRESET 12 +#define LATCH2_PIN_MODEM_CODEC 13 +#define LATCH2_PIN_HOOKFLASH1 14 +#define LATCH2_PIN_HOOKFLASH2 15 + static const struct gpio latch_gpios[] __initconst = { { .gpio = LATCH1_GPIO_BASE + 6, @@ -323,6 +344,22 @@ static struct platform_device ams_delta_nand_device = { .resource = ams_delta_nand_resources, }; +#define OMAP_GPIO_LABEL "gpio-0-15" + +static struct gpiod_lookup_table ams_delta_nand_gpio_table = { + .table = { + GPIO_LOOKUP(OMAP_GPIO_LABEL, AMS_DELTA_GPIO_PIN_NAND_RB, "rdy", + 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_NCE, "nce", 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_NRE, "nre", 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_NWP, "nwp", 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_NWE, "nwe", 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_ALE, "ale", 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_NAND_CLE, "cle", 0), + { }, + }, +}; + static struct resource ams_delta_kp_resources[] = { [0] = { .start = INT_KEYBOARD, @@ -358,6 +395,14 @@ static struct platform_device ams_delta_lcd_device = { .id = -1, }; +static struct gpiod_lookup_table ams_delta_lcd_gpio_table = { + .table = { + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_LCD_VBLEN, "vblen", 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_LCD_NDISP, "ndisp", 0), + { }, + }, +}; + static const struct gpio_led gpio_leds[] __initconst = { { .name = "camera", @@ -449,11 +494,35 @@ static struct platform_device ams_delta_audio_device = { .id = -1, }; +static struct gpiod_lookup_table ams_delta_audio_gpio_table = { + .table = { + GPIO_LOOKUP(OMAP_GPIO_LABEL, AMS_DELTA_GPIO_PIN_HOOK_SWITCH, + "hook_switch", 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_MODEM_CODEC, + "modem_codec", 0), + { }, + }, +}; + static struct platform_device cx20442_codec_device = { .name = "cx20442-codec", .id = -1, }; +static struct gpiod_lookup_table ams_delta_serio_gpio_table = { + .table = { + GPIO_LOOKUP(OMAP_GPIO_LABEL, AMS_DELTA_GPIO_PIN_KEYBRD_DATA, + "data", 0), + GPIO_LOOKUP(OMAP_GPIO_LABEL, AMS_DELTA_GPIO_PIN_KEYBRD_CLK, + "clock", 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_KEYBRD_PWR, + "power", 0), + GPIO_LOOKUP(LATCH2_LABEL, LATCH2_PIN_KEYBRD_DATAOUT, + "dataout", 0), + { }, + }, +}; + static struct platform_device *ams_delta_devices[] __initdata = { &latch1_gpio_device, &latch2_gpio_device, @@ -468,6 +537,16 @@ static struct platform_device *late_devices[] __initdata = { &cx20442_codec_device, }; +static struct gpiod_lookup_table *ams_delta_gpio_tables[] __initdata = { + &ams_delta_audio_gpio_table, + &ams_delta_serio_gpio_table, +}; + +static struct gpiod_lookup_table *late_gpio_tables[] __initdata = { + &ams_delta_lcd_gpio_table, + &ams_delta_nand_gpio_table, +}; + static void __init ams_delta_init(void) { /* mux pins for uarts */ @@ -500,6 +579,20 @@ static void __init ams_delta_init(void) gpio_led_register_device(-1, &leds_pdata); platform_add_devices(ams_delta_devices, ARRAY_SIZE(ams_delta_devices)); + /* + * As soon as devices have been registered, assign their dev_names + * to respective GPIO lookup tables before they are added. + */ + ams_delta_audio_gpio_table.dev_id = + dev_name(&ams_delta_audio_device.dev); + /* + * No device name is assigned to GPIO lookup table for serio device + * as long as serio driver is not converted to platform device driver. + */ + + gpiod_add_lookup_tables(ams_delta_gpio_tables, + ARRAY_SIZE(ams_delta_gpio_tables)); + ams_delta_init_fiq(); omap_writew(omap_readw(ARM_RSTCT1) | 0x0004, ARM_RSTCT1); @@ -570,6 +663,15 @@ static int __init late_init(void) platform_add_devices(late_devices, ARRAY_SIZE(late_devices)); + /* + * As soon as devices have been registered, assign their dev_names + * to respective GPIO lookup tables before they are added. + */ + ams_delta_lcd_gpio_table.dev_id = dev_name(&ams_delta_lcd_device.dev); + ams_delta_nand_gpio_table.dev_id = dev_name(&ams_delta_nand_device.dev); + + gpiod_add_lookup_tables(late_gpio_tables, ARRAY_SIZE(late_gpio_tables)); + err = platform_device_register(&modem_nreset_device); if (err) { pr_err("Couldn't register the modem regulator device\n");
Scope of the change is limited to GPIO pins used by board specific device drivers which will be updated by follow-up patches of the series. Those are some OMAP GPIO (gpio-0-15) and most of Amstrad Delta latch2 GPIO bank pins. Remaining pins of those banks, as well as Amstrad Delta latch1 pins, will be addressed later. Assign a label ("latch2") to the bank, enumerate its pins and put that information, together with OMAP GPIO bank pins, in GPIO lookup tables. Assign lookup tables to devices as soon as those devices are registered and their names can be obtained. A step froward in: - removal of hard-coded GPIO numbers from drivers, - removal of board mach includes from drivers, - switching to dynamically assigned GPIO numbers. Created and compile tested agains linux-4.17-rc3 Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> --- arch/arm/mach-omap1/board-ams-delta.c | 102 ++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+)