| Message ID | 20231018210805.1569987-3-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: mv64xxx: reset-gpios | expand |
On 19/10/23 10:08, 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 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 | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > index efd28bbecf61..28f11d2e800b 100644 > --- a/drivers/i2c/busses/i2c-mv64xxx.c > +++ b/drivers/i2c/busses/i2c-mv64xxx.c kernel test robot points out I'm missing an include of gpio/consumer.h I'll fix that with a v4. I'll give it a couple of days before sending it out just in case there are any more comments. > @@ -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_duration; > 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-duration-us", &reset_duration); > + if (rc) > + reset_duration = 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) { > + usleep_range(reset_duration, reset_duration + 10); > + gpiod_set_value_cansleep(drv_data->reset_gpio, 0); > + } > + > rc = request_irq(drv_data->irq, mv64xxx_i2c_intr, 0, > MV64XXX_I2C_CTLR_NAME, drv_data); > if (rc) {
Hi Chris, as you are working on the v4... ... > + if (drv_data->reset_gpio) { > + usleep_range(reset_duration, reset_duration + 10); I'm not against this, but it's not optimal unless we know more or less what to expect from reset_duration. Do we have a rough idea of what reset_duration is? If we don't then you could consider using a generic "fsleep(reset_duration);" Would it work? Rest looks good. Andi
On 25/10/23 08:18, Andi Shyti wrote: > Hi Chris, > > as you are working on the v4... > > ... > >> + if (drv_data->reset_gpio) { >> + usleep_range(reset_duration, reset_duration + 10); > I'm not against this, but it's not optimal unless we know more or > less what to expect from reset_duration. > > Do we have a rough idea of what reset_duration is? If we don't > then you could consider using a generic "fsleep(reset_duration);" > Would it work? flseep() would work for me. All of the devices I'm testing with seem to be fine with a very short reset pulse, they'd probably be fine with no delay at all. > > Rest looks good. > > Andi
Hi Chris, > > as you are working on the v4... > > > > ... > > > >> + if (drv_data->reset_gpio) { > >> + usleep_range(reset_duration, reset_duration + 10); > > I'm not against this, but it's not optimal unless we know more or > > less what to expect from reset_duration. > > > > Do we have a rough idea of what reset_duration is? If we don't > > then you could consider using a generic "fsleep(reset_duration);" > > Would it work? > flseep() would work for me. All of the devices I'm testing with seem to > be fine with a very short reset pulse, they'd probably be fine with no > delay at all. you know this better than me :-) If you say that a delay is not necessary, then I'm also fine. In any case, we are in probe and I don't think it's time critical, so that a little delay wouldn't hurt and make everyone happy. Either way I'm fine as long as you use the correct sleeping function. Andi
On 25/10/23 09:37, Andi Shyti wrote: > Hi Chris, > >>> as you are working on the v4... >>> >>> ... >>> >>>> + if (drv_data->reset_gpio) { >>>> + usleep_range(reset_duration, reset_duration + 10); >>> I'm not against this, but it's not optimal unless we know more or >>> less what to expect from reset_duration. >>> >>> Do we have a rough idea of what reset_duration is? If we don't >>> then you could consider using a generic "fsleep(reset_duration);" >>> Would it work? >> flseep() would work for me. All of the devices I'm testing with seem to >> be fine with a very short reset pulse, they'd probably be fine with no >> delay at all. > you know this better than me :-) > If you say that a delay is not necessary, then I'm also fine. > > In any case, we are in probe and I don't think it's time > critical, so that a little delay wouldn't hurt and make everyone > happy. > > Either way I'm fine as long as you use the correct sleeping > function. My particular hardware doesn't need it but for this to be generally usable I think it is necessary to provide the capability for some kind of hardware specific reset-duration. I'll look at fsleep() for v4 (or say why I've stuck with usleep_range() in the changelog). > Andi
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c index efd28bbecf61..28f11d2e800b 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_duration; 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-duration-us", &reset_duration); + if (rc) + reset_duration = 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) { + usleep_range(reset_duration, reset_duration + 10); + gpiod_set_value_cansleep(drv_data->reset_gpio, 0); + } + rc = request_irq(drv_data->irq, mv64xxx_i2c_intr, 0, MV64XXX_I2C_CTLR_NAME, drv_data); if (rc) {
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 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 | 15 +++++++++++++++ 1 file changed, 15 insertions(+)