[PATCH v2 0/2] i2c: mv64xxx: reset-gpios

[PATCH v2 0/2] i2c: mv64xxx: reset-gpios

[Chris Packham]

This series adds the ability to associate a gpio with an I2C bus so that
downstream devices can be brought out of reset when the host controller is
probed.

Chris Packham (2):
  dt-bindings: i2c: mv64xxx: add reset-gpios property
  i2c: mv64xxx: add an optional reset-gpios property

 .../bindings/i2c/marvell,mv64xxx-i2c.yaml         |  6 ++++++
 drivers/i2c/busses/i2c-mv64xxx.c                  | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

-- 
2.42.0

[PATCH v2 1/2] dt-bindings: i2c: mv64xxx: add reset-gpios property

[Chris Packham]

Add reset-gpios and reset-delay-us properties to the marvell,mv64xxx-i2c
binding. These can be used to describe hardware where a common reset
GPIO is connected to all downstream devices on and I2C bus. This reset
will be released before the downstream devices on the bus are probed.

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

Notes:
    Changes in v2:
    - Update commit message
    - Add reset-delay-us property

 .../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml        | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
index 461d1c9ee3f7..7223797b0572 100644
--- a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
@@ -70,6 +70,12 @@ properties:
   resets:
     maxItems: 1
 
+  reset-gpios:
+    maxItems: 1
+
+  reset-delay-us:
+    description: Delay in us to wait after reset gpio de-assertion.
+
   dmas:
     items:
       - description: RX DMA Channel
-- 
2.42.0

Re: [PATCH v2 1/2] dt-bindings: i2c: mv64xxx: add reset-gpios property

[Krzysztof Kozlowski]

On 16/10/2023 04:35, Chris Packham wrote:
> Add reset-gpios and reset-delay-us properties to the marvell,mv64xxx-i2c
> binding. These can be used to describe hardware where a common reset
> GPIO is connected to all downstream devices on and I2C bus. This reset
> will be released before the downstream devices on the bus are probed.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Update commit message
>     - Add reset-delay-us property
> 
>  .../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml        | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> index 461d1c9ee3f7..7223797b0572 100644
> --- a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> @@ -70,6 +70,12 @@ properties:
>    resets:
>      maxItems: 1
>  
> +  reset-gpios:
> +    maxItems: 1
> +
> +  reset-delay-us:
> +    description: Delay in us to wait after reset gpio de-assertion.

Add:
  default: XXXX


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: i2c: mv64xxx: add reset-gpios property

[Rob Herring]

On Mon, Oct 16, 2023 at 03:35:03PM +1300, Chris Packham wrote:
> Add reset-gpios and reset-delay-us properties to the marvell,mv64xxx-i2c
> binding. These can be used to describe hardware where a common reset
> GPIO is connected to all downstream devices on and I2C bus. This reset
> will be released before the downstream devices on the bus are probed.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Update commit message
>     - Add reset-delay-us property
> 
>  .../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml        | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> index 461d1c9ee3f7..7223797b0572 100644
> --- a/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml
> @@ -70,6 +70,12 @@ properties:
>    resets:
>      maxItems: 1
>  
> +  reset-gpios:
> +    maxItems: 1

It would be worth saying this is common for all downstream devices on 
the I2C bus here.

> +
> +  reset-delay-us:
> +    description: Delay in us to wait after reset gpio de-assertion.
> +
>    dmas:
>      items:
>        - description: RX DMA Channel
> -- 
> 2.42.0
> 

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

[Chris Packham]

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);
+		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) {
-- 
2.42.0

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

[Peter Rosin]

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

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