On Mon, Apr 23, 2012 at 6:26 PM, Wolfram Sang <w.sang@xxxxxxxxxxxxxx> 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
--
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
