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

[Chris Packham]

On 28/10/23 00:37, Krzysztof Kozlowski wrote:
> On 27/10/2023 13:27, Krzysztof Kozlowski wrote:
>> On 27/10/2023 05:31, 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
> Cc: Mark,
>
> Also this part is not true. If the bus is non-discoverable, then it is
> possible to have reset-gpios in each probed device. You can share GPIOs,
> so no problem with -EBUSY at all.

Last time I checked you couldn't share GPIOs. If that's no longer the 
case then I can probably make what I need to happen work. It still 
creates an issue that I have multiple PCA954x muxes connected to a 
common reset GPIO so as each mux is probed the PCA954x driver will 
toggle the reset. That's probably OK as the PCA954x is sufficiently 
stateless that the extra resets won't do any harm but if it were a more 
complicated device then there would be issues.

Having some kind of ref-counted reset controller that is implemented 
with GPIOs is probably the better solution. I was kind of surprised that 
nothing existed like that in drivers/reset.

> The problem is doing reset:
> 1. in proper moment for all devices
> 2. without affecting other devices when one unbinds/remove()
>
> The (2) above is not solveable easy in kernel and we already had nice
> talks about it just few days ago:
> 1. Apple case:
> https://scanmail.trustwave.com/?c=20988&d=6qC75SLs-9PNM1ZHpLa6reGv82R6opEUmyI62vCytQ&u=https%3a%2f%2fsocial%2etreehouse%2esystems%2f%40marcan%2f111268780311634160
>
> 2. my WSA884x:
> https://scanmail.trustwave.com/?c=20988&d=6qC75SLs-9PNM1ZHpLa6reGv82R6opEUmyJk3q3j7g&u=https%3a%2f%2flore%2ekernel%2eorg%2falsa-devel%2f84f9f1c4-0627-4986-8160-b4ab99469b81%40linaro%2eorg%2f
Apologies for the mangled links (they're more secure now at least that's 
what our IS team have been sold).
> Last,
> I would like to apologize to you Chris. I understand that bringing such
> feedback at v5 is not that good. I had plenty of time to say something
> earlier, so this is not really professional from my side. I am sorry,
> just my brain did not connect all these topics together.
>
> I apologize.

Actually I kind of expected this feedback. I figured I could start with 
the driver that is currently causing me issues and once the dt-binding 
was considered good enough it might migrate to the i2c core.

>
> Best regards,
> Krzysztof
>
>>> this kind of hardware design by associating the bus-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 v5:
>>>      - Rename reset-gpios and reset-duration-us to bus-reset-gpios and
>>>        bus-reset-duration-us as requested by Wolfram
>>>      Changes in v4:
>>>      - Add missing gpio/consumer.h
>>>      - use fsleep() for enforcing reset-duration
>>>      Changes in v3:
>>>      - Rename reset-delay to reset-duration
>>>      - Use reset-duration-us property to control the reset pulse rather than
>>>        delaying after the reset
>>>      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 | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>>> index efd28bbecf61..6e2762d22e5a 100644
>>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>>> @@ -13,6 +13,7 @@
>>>   #include <linux/slab.h>
>>>   #include <linux/module.h>
>>>   #include <linux/spinlock.h>
>>> +#include <linux/gpio/consumer.h>
>>>   #include <linux/i2c.h>
>>>   #include <linux/interrupt.h>
>>>   #include <linux/mv643xx_i2c.h>
>>> @@ -160,6 +161,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 +1038,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_duration;
>>>   	int	rc;
>>>   
>>>   	if ((!pdata && !pd->dev.of_node))
>>> @@ -1083,6 +1086,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, "bus-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, "bus-reset-duration-us", &reset_duration);
>>> +	if (rc)
>>> +		reset_duration = 1;
>> No, this should be solved by core - for entire I2C at minimum. This is
>> not specific to this device.