[v2,2/2] i2c: mv64xxx: add an optional reset-gpios property

  Some hardware designs have a GPIO used to control the reset of all the
devices on and I2C bus. It's not possible for every child node to
declare a reset-gpios property as only the first device probed would be
able to successfully request it (the others will get -EBUSY). Represent
this kind of hardware design by associating the reset-gpios with the
parent I2C bus. The reset line will be released prior to the child I2C
devices being probed.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - Add a property to cover the length of delay after releasing the reset
      GPIO
    - Use dev_err_probe() when requesing the GPIO fails

 drivers/i2c/busses/i2c-mv64xxx.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
  

Hi!

2023-10-16 at 04:35, Chris Packham wrote:
> Some hardware designs have a GPIO used to control the reset of all the
> devices on and I2C bus. It's not possible for every child node to
> declare a reset-gpios property as only the first device probed would be
> able to successfully request it (the others will get -EBUSY). Represent
> this kind of hardware design by associating the reset-gpios with the
> parent I2C bus. The reset line will be released prior to the child I2C
> devices being probed.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Add a property to cover the length of delay after releasing the reset
>       GPIO
>     - Use dev_err_probe() when requesing the GPIO fails
> 
>  drivers/i2c/busses/i2c-mv64xxx.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index efd28bbecf61..50c470e5c4be 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -160,6 +160,7 @@ struct mv64xxx_i2c_data {
>  	bool			clk_n_base_0;
>  	struct i2c_bus_recovery_info	rinfo;
>  	bool			atomic;
> +	struct gpio_desc	*reset_gpio;
>  };
>  
>  static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
> @@ -1036,6 +1037,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  	struct mv64xxx_i2c_data		*drv_data;
>  	struct mv64xxx_i2c_pdata	*pdata = dev_get_platdata(&pd->dev);
>  	struct resource *res;
> +	u32	reset_udelay;
>  	int	rc;
>  
>  	if ((!pdata && !pd->dev.of_node))
> @@ -1083,6 +1085,14 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  	if (drv_data->irq < 0)
>  		return drv_data->irq;
>  
> +	drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(drv_data->reset_gpio))
> +		return dev_err_probe(&pd->dev, PTR_ERR(drv_data->reset_gpio),
> +				     "Cannot get reset gpio\n");
> +	rc = device_property_read_u32(&pd->dev, "reset-delay-us", &reset_udelay);
> +	if (rc)
> +		reset_udelay = 1;
> +
>  	if (pdata) {
>  		drv_data->freq_m = pdata->freq_m;
>  		drv_data->freq_n = pdata->freq_n;
> @@ -1121,6 +1131,11 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>  			goto exit_disable_pm;
>  	}
>  
> +	if (drv_data->reset_gpio) {
> +		gpiod_set_value_cansleep(drv_data->reset_gpio, 0);

There is no limit to how short the reset pulse will be with this
implementation. What I was requesting in my comment for v1 was
a way to control the length of the reset pulse (not the delay
after the pulse). There are devices that behave very badly if
the reset pulse is too short for their liking, others might not
react at all...

Some delay after the pulse might also be needed, of course.

Cheers,
Peter

> +		usleep_range(reset_udelay, reset_udelay + 10);
> +	}
> +
>  	rc = request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
>  			 MV64XXX_I2C_CTLR_NAME, drv_data);
>  	if (rc) {
  

On 18/10/23 09:32, Peter Rosin wrote:
> Hi!
>
> 2023-10-16 at 04:35, Chris Packham wrote:
>> Some hardware designs have a GPIO used to control the reset of all the
>> devices on and I2C bus. It's not possible for every child node to
>> declare a reset-gpios property as only the first device probed would be
>> able to successfully request it (the others will get -EBUSY). Represent
>> this kind of hardware design by associating the reset-gpios with the
>> parent I2C bus. The reset line will be released prior to the child I2C
>> devices being probed.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      Changes in v2:
>>      - Add a property to cover the length of delay after releasing the reset
>>        GPIO
>>      - Use dev_err_probe() when requesing the GPIO fails
>>
>>   drivers/i2c/busses/i2c-mv64xxx.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>> index efd28bbecf61..50c470e5c4be 100644
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -160,6 +160,7 @@ struct mv64xxx_i2c_data {
>>   	bool			clk_n_base_0;
>>   	struct i2c_bus_recovery_info	rinfo;
>>   	bool			atomic;
>> +	struct gpio_desc	*reset_gpio;
>>   };
>>   
>>   static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>> @@ -1036,6 +1037,7 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>   	struct mv64xxx_i2c_data		*drv_data;
>>   	struct mv64xxx_i2c_pdata	*pdata = dev_get_platdata(&pd->dev);
>>   	struct resource *res;
>> +	u32	reset_udelay;
>>   	int	rc;
>>   
>>   	if ((!pdata && !pd->dev.of_node))
>> @@ -1083,6 +1085,14 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>   	if (drv_data->irq < 0)
>>   		return drv_data->irq;
>>   
>> +	drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(drv_data->reset_gpio))
>> +		return dev_err_probe(&pd->dev, PTR_ERR(drv_data->reset_gpio),
>> +				     "Cannot get reset gpio\n");
>> +	rc = device_property_read_u32(&pd->dev, "reset-delay-us", &reset_udelay);
>> +	if (rc)
>> +		reset_udelay = 1;
>> +
>>   	if (pdata) {
>>   		drv_data->freq_m = pdata->freq_m;
>>   		drv_data->freq_n = pdata->freq_n;
>> @@ -1121,6 +1131,11 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>   			goto exit_disable_pm;
>>   	}
>>   
>> +	if (drv_data->reset_gpio) {
>> +		gpiod_set_value_cansleep(drv_data->reset_gpio, 0);
> There is no limit to how short the reset pulse will be with this
> implementation. What I was requesting in my comment for v1 was
> a way to control the length of the reset pulse (not the delay
> after the pulse). There are devices that behave very badly if
> the reset pulse is too short for their liking, others might not
> react at all...

Yeah you're right. I originally had the same delay before and after but 
decided to remove one (and chose wrong).

I think I definitely need the delay before this to ensure a minimum 
reset pulse. I'm on the fence about the delay after, I don't think any 
of the devices I regularly deal with have a particular time they need to 
be out of reset before you can start talking to them.

>
> Some delay after the pulse might also be needed, of course.
>
> Cheers,
> Peter
>
>> +		usleep_range(reset_udelay, reset_udelay + 10);
>> +	}
>> +
>>   	rc = request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
>>   			 MV64XXX_I2C_CTLR_NAME, drv_data);
>>   	if (rc) {