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