| Message ID | 20190614214748.2389-1-fancer.lancer@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Peter Rosin |
| Headers | show |
| Series | i2c: mux-gpio: Use "mux" con_id to find channel GPIOs | expand |
On Sat, Jun 15, 2019 at 12:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > Recent patch - ("i2c: mux/i801: Switch to use descriptor passing") > altered the i2c-mux-gpio driver to use the GPIO-descriptor > based interface to find and request the GPIOs then being utilized > to select and deselect the channels of GPIO-driven i2c-muxes. Even > though the proposed modification was correct for the platform_data-based > systems, it was invalid for the OF-based ones and caused the kernel > to crash at the driver probe procedure. There were two problems with > that modification. First of all the gpiod_count() and gpiod_get_index() > were called with NULL con_id. I always thought that this means "count me all GPIO's for this device despite their names" and "get me GPIO by index despite it's name". What's went wrong? > Due to this the methods couldn't find > the "mux-gpios" OF-properties and returned the -ENOENT error. Secondly > the return value of gpiod_count() wasn't checked for being negative, > which in case of an error caused the driver to crash. This patch > is intended to fix the described problems.
On Sat, Jun 15, 2019 at 02:43:09PM +0300, Andy Shevchenko wrote: > On Sat, Jun 15, 2019 at 12:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > Recent patch - ("i2c: mux/i801: Switch to use descriptor passing") > > altered the i2c-mux-gpio driver to use the GPIO-descriptor > > based interface to find and request the GPIOs then being utilized > > to select and deselect the channels of GPIO-driven i2c-muxes. Even > > though the proposed modification was correct for the platform_data-based > > systems, it was invalid for the OF-based ones and caused the kernel > > to crash at the driver probe procedure. There were two problems with > > that modification. First of all the gpiod_count() and gpiod_get_index() > > were called with NULL con_id. > > I always thought that this means "count me all GPIO's for this device > despite their names" and "get me GPIO by index despite it's name". > What's went wrong? > No. It's wrong as far as I can see for kernels 4.4, 4.9 and the modern 5.2.0-rcX. dt_gpio_count()/of_find_gpio()will always try to count/request either "<con_id>-gpio(s)" or "gpio(s)" GPIOs in the device of-node. While platform_gpio_count()/gpiod_find() will take into account GPIOs with matching <con_id>'s even if it is NULL. Regards, -Sergey > > > Due to this the methods couldn't find > > the "mux-gpios" OF-properties and returned the -ENOENT error. Secondly > > the return value of gpiod_count() wasn't checked for being negative, > > which in case of an error caused the driver to crash. This patch > > is intended to fix the described problems. > > -- > With Best Regards, > Andy Shevchenko
On 2019-06-16 01:24, Serge Semin wrote: > On Sat, Jun 15, 2019 at 02:43:09PM +0300, Andy Shevchenko wrote: >> On Sat, Jun 15, 2019 at 12:51 AM Serge Semin <fancer.lancer@gmail.com> wrote: >>> >>> Recent patch - ("i2c: mux/i801: Switch to use descriptor passing") >>> altered the i2c-mux-gpio driver to use the GPIO-descriptor >>> based interface to find and request the GPIOs then being utilized >>> to select and deselect the channels of GPIO-driven i2c-muxes. Even >>> though the proposed modification was correct for the platform_data-based >>> systems, it was invalid for the OF-based ones and caused the kernel >>> to crash at the driver probe procedure. There were two problems with >>> that modification. First of all the gpiod_count() and gpiod_get_index() >>> were called with NULL con_id. >> >> I always thought that this means "count me all GPIO's for this device >> despite their names" and "get me GPIO by index despite it's name". >> What's went wrong? >> > > No. It's wrong as far as I can see for kernels 4.4, 4.9 and the modern > 5.2.0-rcX. dt_gpio_count()/of_find_gpio()will always try to count/request > either "<con_id>-gpio(s)" or "gpio(s)" GPIOs in the device of-node. While > platform_gpio_count()/gpiod_find() will take into account GPIOs with matching > <con_id>'s even if it is NULL. Right, this is my reading to. For the OF case, gpiod_count calls dt_gpio_count which has no way to find gpios unless they are explicitly named. And NULL simply means unnamed (which is not the case here). The function simply does not do any trawling for gpios it has not been told about. Linus, care to squash this incremental into your patch and resend with proper credit added? Cheers, Peter
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index a377d94968af..ec54b5b4f1a1 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1276,7 +1276,7 @@ static int i801_add_mux(struct i801_priv *priv) for (i = 0; i < mux_config->n_gpios; i++) { lookup->table[i].chip_label = mux_config->gpio_chip; lookup->table[i].chip_hwnum = mux_config->gpios[i]; - lookup->table[i].con_id = NULL; + lookup->table[i].con_id = "mux"; } gpiod_add_lookup_table(lookup); priv->lookup = lookup; diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c index b9578f668fb2..1ea097dc8d5d 100644 --- a/drivers/i2c/muxes/i2c-mux-gpio.c +++ b/drivers/i2c/muxes/i2c-mux-gpio.c @@ -130,10 +130,10 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev) sizeof(mux->data)); } - ngpios = gpiod_count(&pdev->dev, NULL); - if (!ngpios) { - dev_err(&pdev->dev, "no gpios provided\n"); - return -EINVAL; + ngpios = gpiod_count(&pdev->dev, "mux"); + if (ngpios <= 0) { + dev_err(&pdev->dev, "no valid gpios provided\n"); + return ngpios ?: -EINVAL; } mux->ngpios = ngpios; @@ -173,7 +173,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev) flag = GPIOD_OUT_HIGH; else flag = GPIOD_OUT_LOW; - gpiod = devm_gpiod_get_index(&pdev->dev, NULL, i, flag); + gpiod = devm_gpiod_get_index(&pdev->dev, "mux", i, flag); if (IS_ERR(gpiod)) { ret = PTR_ERR(gpiod); goto alloc_failed;
Recent patch - ("i2c: mux/i801: Switch to use descriptor passing") altered the i2c-mux-gpio driver to use the GPIO-descriptor based interface to find and request the GPIOs then being utilized to select and deselect the channels of GPIO-driven i2c-muxes. Even though the proposed modification was correct for the platform_data-based systems, it was invalid for the OF-based ones and caused the kernel to crash at the driver probe procedure. There were two problems with that modification. First of all the gpiod_count() and gpiod_get_index() were called with NULL con_id. Due to this the methods couldn't find the "mux-gpios" OF-properties and returned the -ENOENT error. Secondly the return value of gpiod_count() wasn't checked for being negative, which in case of an error caused the driver to crash. This patch is intended to fix the described problems. Fixes: - ("i2c: mux/i801: Switch to use descriptor passing") Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Peter Rosin <peda@axentia.se> Cc: Jean Delvare <jdelvare@suse.com> Cc: Linus Walleij <linus.walleij@linaro.org> Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 2 +- drivers/i2c/muxes/i2c-mux-gpio.c | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-)