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@xxxxxxxxxxxx>
> > Cc: Haavard Skinnemoen <hskinnemoen@xxxxxxxxx>
> > ---
> > 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,
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
