| Message ID | 20180518210954.29044-5-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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c index 37a3cc21c7bc..c44be2f5a65c 100644 --- a/drivers/mtd/nand/raw/ams-delta.c +++ b/drivers/mtd/nand/raw/ams-delta.c @@ -20,23 +20,28 @@ #include <linux/slab.h> #include <linux/module.h> #include <linux/delay.h> +#include <linux/gpio/consumer.h> #include <linux/mtd/mtd.h> #include <linux/mtd/rawnand.h> #include <linux/mtd/partitions.h> -#include <linux/gpio.h> #include <linux/platform_data/gpio-omap.h> #include <asm/io.h> #include <asm/sizes.h> -#include <mach/board-ams-delta.h> - #include <mach/hardware.h> /* * MTD structure for E3 (Delta) */ static struct mtd_info *ams_delta_mtd = NULL; +static struct gpio_desc *gpiod_rdy; +static struct gpio_desc *gpiod_nce; +static struct gpio_desc *gpiod_nre; +static struct gpio_desc *gpiod_nwp; +static struct gpio_desc *gpiod_nwe; +static struct gpio_desc *gpiod_ale; +static struct gpio_desc *gpiod_cle; /* * Define partitions for flash devices @@ -70,9 +75,9 @@ static void ams_delta_write_byte(struct mtd_info *mtd, u_char byte) writew(0, io_base + OMAP_MPUIO_IO_CNTL); writew(byte, this->IO_ADDR_W); - gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NWE, 0); + gpiod_set_value(gpiod_nwe, 0); ndelay(40); - gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NWE, 1); + gpiod_set_value(gpiod_nwe, 1); } static u_char ams_delta_read_byte(struct mtd_info *mtd) @@ -81,11 +86,11 @@ static u_char ams_delta_read_byte(struct mtd_info *mtd) struct nand_chip *this = mtd_to_nand(mtd); void __iomem *io_base = (void __iomem *)nand_get_controller_data(this); - gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NRE, 0); + gpiod_set_value(gpiod_nre, 0); ndelay(40); writew(~0, io_base + OMAP_MPUIO_IO_CNTL); res = readw(this->IO_ADDR_R); - gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NRE, 1); + gpiod_set_value(gpiod_nre, 1); return res; } @@ -120,12 +125,9 @@ static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd, { if (ctrl & NAND_CTRL_CHANGE) { - gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_NCE, - (ctrl & NAND_NCE) == 0); - gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_CLE, - (ctrl & NAND_CLE) != 0); - gpio_set_value(AMS_DELTA_GPIO_PIN_NAND_ALE, - (ctrl & NAND_ALE) != 0); + gpiod_set_value(gpiod_nce, !(ctrl & NAND_NCE)); + gpiod_set_value(gpiod_cle, !!(ctrl & NAND_CLE)); + gpiod_set_value(gpiod_ale, !!(ctrl & NAND_ALE)); } if (cmd != NAND_CMD_NONE) @@ -134,41 +136,9 @@ static void ams_delta_hwcontrol(struct mtd_info *mtd, int cmd, static int ams_delta_nand_ready(struct mtd_info *mtd) { - return gpio_get_value(AMS_DELTA_GPIO_PIN_NAND_RB); + return gpiod_get_value(gpiod_rdy); } -static const struct gpio _mandatory_gpio[] = { - { - .gpio = AMS_DELTA_GPIO_PIN_NAND_NCE, - .flags = GPIOF_OUT_INIT_HIGH, - .label = "nand_nce", - }, - { - .gpio = AMS_DELTA_GPIO_PIN_NAND_NRE, - .flags = GPIOF_OUT_INIT_HIGH, - .label = "nand_nre", - }, - { - .gpio = AMS_DELTA_GPIO_PIN_NAND_NWP, - .flags = GPIOF_OUT_INIT_HIGH, - .label = "nand_nwp", - }, - { - .gpio = AMS_DELTA_GPIO_PIN_NAND_NWE, - .flags = GPIOF_OUT_INIT_HIGH, - .label = "nand_nwe", - }, - { - .gpio = AMS_DELTA_GPIO_PIN_NAND_ALE, - .flags = GPIOF_OUT_INIT_LOW, - .label = "nand_ale", - }, - { - .gpio = AMS_DELTA_GPIO_PIN_NAND_CLE, - .flags = GPIOF_OUT_INIT_LOW, - .label = "nand_cle", - }, -}; /* * Main initialization routine @@ -216,12 +186,15 @@ static int ams_delta_init(struct platform_device *pdev) this->write_buf = ams_delta_write_buf; this->read_buf = ams_delta_read_buf; this->cmd_ctrl = ams_delta_hwcontrol; - if (gpio_request(AMS_DELTA_GPIO_PIN_NAND_RB, "nand_rdy") == 0) { + + 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"); } + /* 25 us command delay time */ this->chip_delay = 30; this->ecc.mode = NAND_ECC_SOFT; @@ -230,7 +203,44 @@ static int ams_delta_init(struct platform_device *pdev) platform_set_drvdata(pdev, io_base); /* Set chip enabled, but */ - err = gpio_request_array(_mandatory_gpio, ARRAY_SIZE(_mandatory_gpio)); + gpiod_nwp = devm_gpiod_get(&pdev->dev, "nwp", GPIOD_OUT_HIGH); + if (IS_ERR(gpiod_nwp)) { + err = PTR_ERR(gpiod_nwp); + dev_err(&pdev->dev, "NWP GPIO request failed (%d)\n", err); + goto err_gpiod; + } + gpiod_nce = devm_gpiod_get(&pdev->dev, "nce", GPIOD_OUT_HIGH); + if (IS_ERR(gpiod_nce)) { + err = PTR_ERR(gpiod_nce); + dev_err(&pdev->dev, "NCE GPIO request failed (%d)\n", err); + goto err_gpiod; + } + gpiod_nre = devm_gpiod_get(&pdev->dev, "nre", GPIOD_OUT_HIGH); + if (IS_ERR(gpiod_nre)) { + err = PTR_ERR(gpiod_nre); + dev_err(&pdev->dev, "NRE GPIO request failed (%d)\n", err); + goto err_gpiod; + } + gpiod_nwe = devm_gpiod_get(&pdev->dev, "nwe", GPIOD_OUT_HIGH); + if (IS_ERR(gpiod_nwe)) { + err = PTR_ERR(gpiod_nwe); + dev_err(&pdev->dev, "NWE GPIO request failed (%d)\n", err); + goto err_gpiod; + } + gpiod_ale = devm_gpiod_get(&pdev->dev, "ale", GPIOD_OUT_LOW); + if (IS_ERR(gpiod_ale)) { + err = PTR_ERR(gpiod_ale); + dev_err(&pdev->dev, "ALE GPIO request failed (%d)\n", err); + goto err_gpiod; + } + gpiod_cle = devm_gpiod_get(&pdev->dev, "cle", GPIOD_OUT_LOW); + if (IS_ERR(gpiod_cle)) { + err = PTR_ERR(gpiod_cle); + dev_err(&pdev->dev, "CLE GPIO request failed (%d)\n", err); + } +err_gpiod: + if (err == -ENODEV || err == -ENOENT) + err = -EPROBE_DEFER; if (err) goto out_gpio; @@ -246,9 +256,7 @@ static int ams_delta_init(struct platform_device *pdev) goto out; out_mtd: - gpio_free_array(_mandatory_gpio, ARRAY_SIZE(_mandatory_gpio)); out_gpio: - gpio_free(AMS_DELTA_GPIO_PIN_NAND_RB); iounmap(io_base); out_free: kfree(this); @@ -266,8 +274,6 @@ static int ams_delta_cleanup(struct platform_device *pdev) /* Release resources, unregister device */ nand_release(ams_delta_mtd); - gpio_free_array(_mandatory_gpio, ARRAY_SIZE(_mandatory_gpio)); - gpio_free(AMS_DELTA_GPIO_PIN_NAND_RB); iounmap(io_base); /* Free the MTD device structure */
Now as the Amstrad Delta board provides GPIO lookup tables, switch from GPIO numbers to GPIO descriptors and use the table to locate required GPIO pins. Declare static variables for storing GPIO descriptors and replace gpio_ functions with their gpiod_ equivalents. Return -EPROBE_DEFER if the GPIO pins are not yet available so device initialization is postponed instead of aborted. Pin naming used by the driver should be followed while respective GPIO lookup table is initialized by a board init code. Created and tested against linux-4.17-rc3, on top of patch 1/6 "ARM: OMAP1: ams-delta: add GPIO lookup tables" Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> --- drivers/mtd/nand/raw/ams-delta.c | 110 +++++++++++++++++++++------------------ 1 file changed, 58 insertions(+), 52 deletions(-)