Re: [PATCH V3 1/2] i2c/adapter: Add bus recovery infrastructure

[viresh kumar <viresh.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Mon, Apr 23, 2012 at 6:26 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Fri, Mar 02, 2012 at 11:53:42AM +0530, Viresh Kumar wrote:
> Finally, a review \o/

:)

>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> +/* i2c bus recovery routines */
>> +static int i2c_gpio_recover_bus(struct i2c_adapter *adap)
>> +{
>> +     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>
> I think something alike 'recov_info' is more readable than 'bri'.

Sure.

>> +     unsigned long delay = 1000000;
>
> udelay?

No. It's ndelay. Will rename it.

>> +     return 0;
>> +}
>> +
>> +static int i2c_scl_recover_bus(struct i2c_adapter *adap)
>> +{
>> +     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +     int i, val = 0;
>> +     unsigned long delay = 1000000;
>> +
>> +     delay /= bri->clock_rate_khz * 2;
>> +
>> +     for (i = 0; i < bri->clock_cnt * 2; i++,
>> +                     val = !val) {
>> +             bri->set_scl(adap, val);
>> +             ndelay(delay);
>> +
>> +             /* break if sda got high, check only when scl line is high */
>> +             if (!bri->skip_sda_polling && val)
>> +                     if (bri->get_sda(adap))
>> +                             break;
>> +     }
>> +
>> +     return 0;
>> +}
>
> We have two recover bus functions here. I think it would be better to
> have only one. set_scl and get_sda can be mapped to helper functions
> doing needed gpio-calls during init. Something like this pseudo-code:
>
>        if (!bri->set_scl && we_have_a_scl_gpio)
>                set_scl = i2c_set_scl_via_gpio()

gpio calls can't be done at init. As we have to request and free them
again and again. So would be required to keep two calls.

Not sure, if i understand your point here.

>>  static int i2c_device_probe(struct device *dev)
>>  {
>>       struct i2c_client       *client = i2c_verify_client(dev);
>> @@ -861,6 +945,47 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>>                        "Failed to create compatibility class link\n");
>>  #endif
>>
>> +     /* bus recovery specific initialization */
>> +     if (adap->bus_recovery_info) {
>> +             struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +
>> +             if (bri->recover_bus) {
>> +                     dev_info(&adap->dev,
>> +                             "registered for non-generic bus recovery\n");
>> +             } else {
>> +                     /* Use generic recovery routines */
>> +                     if (!bri->clock_rate_khz) {
>> +                             dev_warn(&adap->dev,
>> +                                     "doesn't have valid recovery clock rate\n");
>> +                             goto exit_recovery;
>> +                     }
>> +
>> +                     /* Most controller need 9 clocks at max */
>> +                     if (!bri->clock_cnt)
>> +                             bri->clock_cnt = 9;
>> +
>> +                     if (bri->is_gpio_recovery) {
>> +                             bri->recover_bus = i2c_gpio_recover_bus;
>> +                             dev_info(&adap->dev,
>> +                                     "registered for gpio bus recovery\n");
>> +                     } else if (bri->set_scl) {
>> +                             if (!bri->skip_sda_polling && !bri->get_sda) {
>> +                                     dev_warn(&adap->dev,
>> +                                             "!get_sda. skip sda polling\n");
>> +                                     bri->skip_sda_polling = true;
>> +                             }
>> +
>> +                             bri->recover_bus = i2c_scl_recover_bus;
>> +                             dev_info(&adap->dev,
>> +                                     "registered for scl bus recovery\n");
>> +                     } else {
>> +                             dev_warn(&adap->dev,
>> +                                     "doesn't have valid recovery type\n");
>
> Printouts here should probably be dev_dbg (if not left out), most users
> won't care.

Hmm. Actually this is a error case. User wanted to have recovery infra,
but didn't mention the method of recovery: via gpio or sda/scl. Don't
know if we could make it dev_err() instead.

>> +                     }
>> +             }
>> +     }
>> +
>> +exit_recovery:
>>       /* create pre-declared device nodes */
>>       if (adap->nr < __i2c_first_dynamic_bus_num)
>>               i2c_scan_static_board_info(adap);
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 8e25a91..1310d1a 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -365,6 +365,55 @@ struct i2c_algorithm {
>>       u32 (*functionality) (struct i2c_adapter *);
>>  };
>>
>
> The description of the members is a good start. To be complete, I'd think
> we need some higher level description in Documentation/i2c, too, with an
> example how to set up.

Sure.

>> +/**
>> + * struct i2c_bus_recovery_info - I2c bus recovery information
>> + * @recover_bus: Recover routine. Either pass driver's recover_bus() routine, or
>> + *   pass it NULL to use generic ones, i.e. gpio or scl based.
>> + * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
>> + *   it became high or not. Only required if recover_bus == NULL.
>
> Assume this when there is no get_sda() and/or SDA GPIO  defined?

Can be done with get_sda(), but have doubt for GPIO. How to check if sda polling
is required in GPIO case. Obviously by checking its value. Zero is a
valid value.
And -1 doesn't look good.

>> + * @is_gpio_recovery: true, select gpio type else scl type. Only required if
>> + *   recover_bus == NULL.
>
> Assume this when there is no GPIO defined (and no recover_bus)?

Can be done based on value of set_scl, but not on GPIO because of earlier
mentioned reasons.

>> + * @clock_cnt: count of max clocks to be generated. Required for both gpio and
>> + *   scl type recovery.
>
> Why should that be different from 9 (as said in the docs)?

Don't know. Somebody, probably Tegra guys, asked for it during earlier
versions of this
patch. IIRC, they needed 10.

>> + * @set_scl: controller specific scl configuration routine. Only required if
>> + *   is_gpio_recovery == false
>> + * @get_sda: controller specific sda read routine. Only required if
>> + *   is_gpio_recovery == false and skip_sda_polling == false.
>> + * @get_gpio: called before recover_bus() to get padmux configured for scl line.
>> + *   as gpio. Only required if is_gpio_recovery == true.
>> + * @put_gpio: called after recover_bus() to get padmux configured for scl line
>> + *   as scl. Only required if is_gpio_recovery == true.
>
> Can't we use the pinmux/pinctrl subsystem here? Not knowing too much
> about it, CCing Linus.

Can be. But probably true only for architectures where pinctrl is
defined. Linus?

>> + * @scl_gpio: gpio number of the scl line. Only required if is_gpio_recovery ==
>> + *   true.
>> + * @sda_gpio: gpio number of the sda line. Only required if is_gpio_recovery ==
>> + *   true and skip_sda_polling == false.
>> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
>> + *   GPIOF_OUT_INIT_LOW.
>> + * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
>> + *   GPIOF_OUT_INIT_LOW.
>
> For the last 4, I'd suggest to pass a struct gpio scl_sda_gpio_array[2]
> or something.

Ok.

>> + */
>> +struct i2c_bus_recovery_info {
>> +     int (*recover_bus)(struct i2c_adapter *);
>> +     bool skip_sda_polling;
>> +     bool is_gpio_recovery;
>> +     u32 clock_rate_khz;
>> +     u8 clock_cnt;
>> +
>> +     /* scl/sda recovery */
>> +     void (*set_scl)(struct i2c_adapter *, int val);
>> +     int (*get_sda)(struct i2c_adapter *);
>> +
>> +     /* gpio recovery */
>> +     int (*get_gpio)(unsigned gpio);
>> +     int (*put_gpio)(unsigned gpio);
>> +     u32 scl_gpio;
>> +     u32 sda_gpio;
>> +     u32 scl_gpio_flags;
>> +     u32 sda_gpio_flags;
>> +};
>> +
>>  /*
>>   * i2c_adapter is the structure used to identify a physical i2c bus along
>>   * with the access algorithms necessary to access it.
>> @@ -388,6 +437,9 @@ struct i2c_adapter {
>>
>>       struct mutex userspace_clients_lock;
>>       struct list_head userspace_clients;
>> +
>> +     /* Pass valid pointer if recovery infrastructure is required */
>> +     struct i2c_bus_recovery_info *bus_recovery_info;
>>  };
>>  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>>
>
> I'd also think calling the recover function should be in the core and
> not in all the drivers.

Sounds good. Based on, if timeout is returned by controller or not.

--
Viresh