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

[Chris Packham]

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@xxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> 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) {