| Message ID | 20130228120140.127ebb91@endymion.delvare |
|---|---|
| State | Accepted |
| Headers | show |
Hi Jean, On 2/28/2013 19:01, Jean Delvare wrote: > GPIOs may not be available immediately when i2c-gpio looks for them. > Implement support for deferred probing so that probing can be > attempted again later when GPIO pins are finally available. > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > Cc: Haavard Skinnemoen <hskinnemoen@gmail.com> > --- > A little more changes were needed than I initially thought, because we > want to check the pins before we allocate memory. Otherwise we would > have a possibly large number of memory allocation and freeing cycles > until GPIO pins are finally available. > > I wrote this quite some time ago already, but I do not have any system > to test it. I would appreciate if one or more users of the i2c-gpio > driver could give it a try and confirm it works as intended, or at > least doesn't introduce any regression. Thanks. > > drivers/i2c/busses/i2c-gpio.c | 75 ++++++++++++++++++++++++++++-------------- > 1 file changed, 50 insertions(+), 25 deletions(-) Test on at91sam9g20ek_2mmc board, booting with non-dt kernel and dt-kernel base on Linux-3.8. Both are OK. Tested-by: Bo Shen <voice.shen@atmel.com> Best Regards, Bo Shen > --- linux-3.8-rc2.orig/drivers/i2c/busses/i2c-gpio.c 2013-01-09 22:26:30.031060312 +0100 > +++ linux-3.8-rc2/drivers/i2c/busses/i2c-gpio.c 2013-01-09 22:32:59.950308645 +0100 > @@ -85,23 +85,29 @@ static int i2c_gpio_getscl(void *data) > return gpio_get_value(pdata->scl_pin); > } > > -static int of_i2c_gpio_probe(struct device_node *np, > - struct i2c_gpio_platform_data *pdata) > +static int of_i2c_gpio_get_pins(struct device_node *np, > + unsigned int *sda_pin, unsigned int *scl_pin) > { > - u32 reg; > - > if (of_gpio_count(np) < 2) > return -ENODEV; > > - pdata->sda_pin = of_get_gpio(np, 0); > - pdata->scl_pin = of_get_gpio(np, 1); > + *sda_pin = of_get_gpio(np, 0); > + *scl_pin = of_get_gpio(np, 1); > > - if (!gpio_is_valid(pdata->sda_pin) || !gpio_is_valid(pdata->scl_pin)) { > + if (!gpio_is_valid(*sda_pin) || !gpio_is_valid(*scl_pin)) { > pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n", > - np->full_name, pdata->sda_pin, pdata->scl_pin); > + np->full_name, *sda_pin, *scl_pin); > return -ENODEV; > } > > + return 0; > +} > + > +static void of_i2c_gpio_get_props(struct device_node *np, > + struct i2c_gpio_platform_data *pdata) > +{ > + u32 reg; > + > of_property_read_u32(np, "i2c-gpio,delay-us", &pdata->udelay); > > if (!of_property_read_u32(np, "i2c-gpio,timeout-ms", ®)) > @@ -113,8 +119,6 @@ static int of_i2c_gpio_probe(struct devi > of_property_read_bool(np, "i2c-gpio,scl-open-drain"); > pdata->scl_is_output_only = > of_property_read_bool(np, "i2c-gpio,scl-output-only"); > - > - return 0; > } > > static int i2c_gpio_probe(struct platform_device *pdev) > @@ -123,31 +127,52 @@ static int i2c_gpio_probe(struct platfor > struct i2c_gpio_platform_data *pdata; > struct i2c_algo_bit_data *bit_data; > struct i2c_adapter *adap; > + unsigned int sda_pin, scl_pin; > int ret; > > - priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > - if (!priv) > - return -ENOMEM; > - adap = &priv->adap; > - bit_data = &priv->bit_data; > - pdata = &priv->pdata; > - > + /* First get the GPIO pins; if it fails, we'll defer the probe. */ > if (pdev->dev.of_node) { > - ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata); > + ret = of_i2c_gpio_get_pins(pdev->dev.of_node, > + &sda_pin, &scl_pin); > if (ret) > return ret; > } else { > if (!pdev->dev.platform_data) > return -ENXIO; > - memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata)); > + pdata = pdev->dev.platform_data; > + sda_pin = pdata->sda_pin; > + scl_pin = pdata->scl_pin; > } > > - ret = gpio_request(pdata->sda_pin, "sda"); > - if (ret) > + ret = gpio_request(sda_pin, "sda"); > + if (ret) { > + if (ret == -EINVAL) > + ret = -EPROBE_DEFER; /* Try again later */ > goto err_request_sda; > - ret = gpio_request(pdata->scl_pin, "scl"); > - if (ret) > + } > + ret = gpio_request(scl_pin, "scl"); > + if (ret) { > + if (ret == -EINVAL) > + ret = -EPROBE_DEFER; /* Try again later */ > goto err_request_scl; > + } > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) { > + ret = -ENOMEM; > + goto err_add_bus; > + } > + adap = &priv->adap; > + bit_data = &priv->bit_data; > + pdata = &priv->pdata; > + > + if (pdev->dev.of_node) { > + pdata->sda_pin = sda_pin; > + pdata->scl_pin = scl_pin; > + of_i2c_gpio_get_props(pdev->dev.of_node, pdata); > + } else { > + memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata)); > + } > > if (pdata->sda_is_open_drain) { > gpio_direction_output(pdata->sda_pin, 1); > @@ -211,9 +236,9 @@ static int i2c_gpio_probe(struct platfor > return 0; > > err_add_bus: > - gpio_free(pdata->scl_pin); > + gpio_free(scl_pin); > err_request_scl: > - gpio_free(pdata->sda_pin); > + gpio_free(sda_pin); > err_request_sda: > return ret; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 01 Mar 2013 11:58:29 +0800, Bo Shen wrote: > Hi Jean, > > On 2/28/2013 19:01, Jean Delvare wrote: > > GPIOs may not be available immediately when i2c-gpio looks for them. > > Implement support for deferred probing so that probing can be > > attempted again later when GPIO pins are finally available. > > > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > > Cc: Haavard Skinnemoen <hskinnemoen@gmail.com> > > --- > > A little more changes were needed than I initially thought, because we > > want to check the pins before we allocate memory. Otherwise we would > > have a possibly large number of memory allocation and freeing cycles > > until GPIO pins are finally available. > > > > I wrote this quite some time ago already, but I do not have any system > > to test it. I would appreciate if one or more users of the i2c-gpio > > driver could give it a try and confirm it works as intended, or at > > least doesn't introduce any regression. Thanks. > > > > drivers/i2c/busses/i2c-gpio.c | 75 ++++++++++++++++++++++++++++-------------- > > 1 file changed, 50 insertions(+), 25 deletions(-) > > Test on at91sam9g20ek_2mmc board, booting with non-dt kernel and > dt-kernel base on Linux-3.8. Both are OK. > > Tested-by: Bo Shen <voice.shen@atmel.com> Thanks Bo, this is very appreciated!
Hi Jean, On Thu, Feb 28, 2013 at 12:01:40PM +0100, Jean Delvare wrote: > GPIOs may not be available immediately when i2c-gpio looks for them. > Implement support for deferred probing so that probing can be > attempted again later when GPIO pins are finally available. > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > Cc: Haavard Skinnemoen <hskinnemoen@gmail.com> > --- > A little more changes were needed than I initially thought, because we > want to check the pins before we allocate memory. Otherwise we would > have a possibly large number of memory allocation and freeing cycles > until GPIO pins are finally available. > > I wrote this quite some time ago already, but I do not have any system > to test it. I would appreciate if one or more users of the i2c-gpio > driver could give it a try and confirm it works as intended, or at > least doesn't introduce any regression. Thanks. > > drivers/i2c/busses/i2c-gpio.c | 75 ++++++++++++++++++++++++++++-------------- > 1 file changed, 50 insertions(+), 25 deletions(-) > > --- linux-3.8-rc2.orig/drivers/i2c/busses/i2c-gpio.c 2013-01-09 22:26:30.031060312 +0100 > +++ linux-3.8-rc2/drivers/i2c/busses/i2c-gpio.c 2013-01-09 22:32:59.950308645 +0100 > @@ -85,23 +85,29 @@ static int i2c_gpio_getscl(void *data) > return gpio_get_value(pdata->scl_pin); > } > > -static int of_i2c_gpio_probe(struct device_node *np, > - struct i2c_gpio_platform_data *pdata) > +static int of_i2c_gpio_get_pins(struct device_node *np, > + unsigned int *sda_pin, unsigned int *scl_pin) GPIOs are expressed via int. > + ret = gpio_request(sda_pin, "sda"); > + if (ret) { > + if (ret == -EINVAL) > + ret = -EPROBE_DEFER; /* Try again later */ Would gpio_request_array() make the code simpler? Thanks, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Wolfram, Thanks for the review. On Fri, 22 Mar 2013 12:56:21 +0100, Wolfram Sang wrote: > On Thu, Feb 28, 2013 at 12:01:40PM +0100, Jean Delvare wrote: > > GPIOs may not be available immediately when i2c-gpio looks for them. > > Implement support for deferred probing so that probing can be > > attempted again later when GPIO pins are finally available. > > > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > > Cc: Haavard Skinnemoen <hskinnemoen@gmail.com> > > --- > > A little more changes were needed than I initially thought, because we > > want to check the pins before we allocate memory. Otherwise we would > > have a possibly large number of memory allocation and freeing cycles > > until GPIO pins are finally available. > > > > I wrote this quite some time ago already, but I do not have any system > > to test it. I would appreciate if one or more users of the i2c-gpio > > driver could give it a try and confirm it works as intended, or at > > least doesn't introduce any regression. Thanks. > > > > drivers/i2c/busses/i2c-gpio.c | 75 ++++++++++++++++++++++++++++-------------- > > 1 file changed, 50 insertions(+), 25 deletions(-) > > > > --- linux-3.8-rc2.orig/drivers/i2c/busses/i2c-gpio.c 2013-01-09 22:26:30.031060312 +0100 > > +++ linux-3.8-rc2/drivers/i2c/busses/i2c-gpio.c 2013-01-09 22:32:59.950308645 +0100 > > @@ -85,23 +85,29 @@ static int i2c_gpio_getscl(void *data) > > return gpio_get_value(pdata->scl_pin); > > } > > > > -static int of_i2c_gpio_probe(struct device_node *np, > > - struct i2c_gpio_platform_data *pdata) > > +static int of_i2c_gpio_get_pins(struct device_node *np, > > + unsigned int *sda_pin, unsigned int *scl_pin) > > GPIOs are expressed via int. What do you mean? In <linux/gpio.h> I see: struct gpio { unsigned gpio; (...) static inline int gpio_get_value(unsigned int gpio) { return __gpio_get_value(gpio); } And in <linux/i2c-gpio.h>: struct i2c_gpio_platform_data { unsigned int sda_pin; unsigned int scl_pin; (...) So unsigned int seems to be the right type for gpios. The OF layer indeed uses int as the return type of of_get_gpio(), presumably to be able to report errors, but the original code did not handle errors from these calls so I assumed they couldn't fail and did not bother adding error handling. If you still have a concern about the types used, please clarify and let me know what change you expect. > > + ret = gpio_request(sda_pin, "sda"); > > + if (ret) { > > + if (ret == -EINVAL) > > + ret = -EPROBE_DEFER; /* Try again later */ > > Would gpio_request_array() make the code simpler? I gave it a try, this indeed makes the code slightly simpler (-4 lines) but the resulting binary slightly larger (+40 bytes on x86-64.) I'd say it's not worth it? Note that my patch doesn't introduce the gpio_request() calls, they were there before, so this decision is actually independent from my patch, and even if we decide to switch to using gpio_request_array(), I'd rather do it in a separate patch for clarity. Thanks,
> What do you mean? In <linux/gpio.h> I see: > > struct gpio { > unsigned gpio; > (...) > > static inline int gpio_get_value(unsigned int gpio) > { > return __gpio_get_value(gpio); > } > > And in <linux/i2c-gpio.h>: > > struct i2c_gpio_platform_data { > unsigned int sda_pin; > unsigned int scl_pin; > (...) I remembered this paragraph from Documentation/gpio.txt: === If you want to initialize a structure with an invalid GPIO number, use some negative number (perhaps "-EINVAL"); that will never be valid. To test if such number from such a structure could reference a GPIO, you may use this predicate: int gpio_is_valid(int number); ... === Confusingly, I know found that the chapter starts with === GPIOs are identified by unsigned integers in the range 0..MAX_INT. That reserves "negative" numbers for other purposes like marking signals as "not available on this board", or indicating faults. === Meh. > If you still have a concern about the types used, please clarify and > let me know what change you expect. Leave it. I think the fragile part is gpio_is_valid() but this is truly outside the scope of this patch. > > > + ret = gpio_request(sda_pin, "sda"); > > > + if (ret) { > > > + if (ret == -EINVAL) > > > + ret = -EPROBE_DEFER; /* Try again later */ > > > > Would gpio_request_array() make the code simpler? > > I gave it a try, this indeed makes the code slightly simpler (-4 lines) > but the resulting binary slightly larger (+40 bytes on x86-64.) I'd say > it's not worth it? OK. Then leave it. > Note that my patch doesn't introduce the gpio_request() calls, they > were there before, so this decision is actually independent from my > patch, and even if we decide to switch to using gpio_request_array(), > I'd rather do it in a separate patch for clarity. I don't fully get it. Do you want to appl gpio_request() to this patch? Otherwise, I'd take it as is. Thanks, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Wolfram, On Tue, 26 Mar 2013 12:09:08 +0100, Wolfram Sang wrote: > > If you still have a concern about the types used, please clarify and > > let me know what change you expect. > > Leave it. I think the fragile part is gpio_is_valid() but this is truly > outside the scope of this patch. > (...) > > Note that my patch doesn't introduce the gpio_request() calls, they > > were there before, so this decision is actually independent from my > > patch, and even if we decide to switch to using gpio_request_array(), > > I'd rather do it in a separate patch for clarity. > > I don't fully get it. Do you want to appl gpio_request() to this patch? > Otherwise, I'd take it as is. As I do not understand your question, I'd say you take my patch as is :) Thanks,
On Thu, Feb 28, 2013 at 12:01:40PM +0100, Jean Delvare wrote: > GPIOs may not be available immediately when i2c-gpio looks for them. > Implement support for deferred probing so that probing can be > attempted again later when GPIO pins are finally available. > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > Cc: Haavard Skinnemoen <hskinnemoen@gmail.com> Applied to for-next, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- linux-3.8-rc2.orig/drivers/i2c/busses/i2c-gpio.c 2013-01-09 22:26:30.031060312 +0100 +++ linux-3.8-rc2/drivers/i2c/busses/i2c-gpio.c 2013-01-09 22:32:59.950308645 +0100 @@ -85,23 +85,29 @@ static int i2c_gpio_getscl(void *data) return gpio_get_value(pdata->scl_pin); } -static int of_i2c_gpio_probe(struct device_node *np, - struct i2c_gpio_platform_data *pdata) +static int of_i2c_gpio_get_pins(struct device_node *np, + unsigned int *sda_pin, unsigned int *scl_pin) { - u32 reg; - if (of_gpio_count(np) < 2) return -ENODEV; - pdata->sda_pin = of_get_gpio(np, 0); - pdata->scl_pin = of_get_gpio(np, 1); + *sda_pin = of_get_gpio(np, 0); + *scl_pin = of_get_gpio(np, 1); - if (!gpio_is_valid(pdata->sda_pin) || !gpio_is_valid(pdata->scl_pin)) { + if (!gpio_is_valid(*sda_pin) || !gpio_is_valid(*scl_pin)) { pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n", - np->full_name, pdata->sda_pin, pdata->scl_pin); + np->full_name, *sda_pin, *scl_pin); return -ENODEV; } + return 0; +} + +static void of_i2c_gpio_get_props(struct device_node *np, + struct i2c_gpio_platform_data *pdata) +{ + u32 reg; + of_property_read_u32(np, "i2c-gpio,delay-us", &pdata->udelay); if (!of_property_read_u32(np, "i2c-gpio,timeout-ms", ®)) @@ -113,8 +119,6 @@ static int of_i2c_gpio_probe(struct devi of_property_read_bool(np, "i2c-gpio,scl-open-drain"); pdata->scl_is_output_only = of_property_read_bool(np, "i2c-gpio,scl-output-only"); - - return 0; } static int i2c_gpio_probe(struct platform_device *pdev) @@ -123,31 +127,52 @@ static int i2c_gpio_probe(struct platfor struct i2c_gpio_platform_data *pdata; struct i2c_algo_bit_data *bit_data; struct i2c_adapter *adap; + unsigned int sda_pin, scl_pin; int ret; - priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); - if (!priv) - return -ENOMEM; - adap = &priv->adap; - bit_data = &priv->bit_data; - pdata = &priv->pdata; - + /* First get the GPIO pins; if it fails, we'll defer the probe. */ if (pdev->dev.of_node) { - ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata); + ret = of_i2c_gpio_get_pins(pdev->dev.of_node, + &sda_pin, &scl_pin); if (ret) return ret; } else { if (!pdev->dev.platform_data) return -ENXIO; - memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata)); + pdata = pdev->dev.platform_data; + sda_pin = pdata->sda_pin; + scl_pin = pdata->scl_pin; } - ret = gpio_request(pdata->sda_pin, "sda"); - if (ret) + ret = gpio_request(sda_pin, "sda"); + if (ret) { + if (ret == -EINVAL) + ret = -EPROBE_DEFER; /* Try again later */ goto err_request_sda; - ret = gpio_request(pdata->scl_pin, "scl"); - if (ret) + } + ret = gpio_request(scl_pin, "scl"); + if (ret) { + if (ret == -EINVAL) + ret = -EPROBE_DEFER; /* Try again later */ goto err_request_scl; + } + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) { + ret = -ENOMEM; + goto err_add_bus; + } + adap = &priv->adap; + bit_data = &priv->bit_data; + pdata = &priv->pdata; + + if (pdev->dev.of_node) { + pdata->sda_pin = sda_pin; + pdata->scl_pin = scl_pin; + of_i2c_gpio_get_props(pdev->dev.of_node, pdata); + } else { + memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata)); + } if (pdata->sda_is_open_drain) { gpio_direction_output(pdata->sda_pin, 1); @@ -211,9 +236,9 @@ static int i2c_gpio_probe(struct platfor return 0; err_add_bus: - gpio_free(pdata->scl_pin); + gpio_free(scl_pin); err_request_scl: - gpio_free(pdata->sda_pin); + gpio_free(sda_pin); err_request_sda: return ret; }
GPIOs may not be available immediately when i2c-gpio looks for them. Implement support for deferred probing so that probing can be attempted again later when GPIO pins are finally available. Signed-off-by: Jean Delvare <khali@linux-fr.org> Cc: Haavard Skinnemoen <hskinnemoen@gmail.com> --- A little more changes were needed than I initially thought, because we want to check the pins before we allocate memory. Otherwise we would have a possibly large number of memory allocation and freeing cycles until GPIO pins are finally available. I wrote this quite some time ago already, but I do not have any system to test it. I would appreciate if one or more users of the i2c-gpio driver could give it a try and confirm it works as intended, or at least doesn't introduce any regression. Thanks. drivers/i2c/busses/i2c-gpio.c | 75 ++++++++++++++++++++++++++++-------------- 1 file changed, 50 insertions(+), 25 deletions(-)