| Message ID | 20200804095926.205643-1-codrin.ciubotariu@microchip.com |
|---|---|
| Headers | show |
| Series | i2c: core: add generic GPIO bus recovery | expand |
On Tue, Aug 04, 2020 at 12:59:24PM +0300, Codrin Ciubotariu wrote: > Multiple I2C bus drivers use similar bindings to obtain information needed > for I2C recovery. For example, for platforms using device-tree, the > properties look something like this: > > &i2c { > ... > pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c_default>; > pinctrl-1 = <&pinctrl_i2c_gpio>; > sda-gpios = <&pio 0 GPIO_ACTIVE_HIGH>; > scl-gpios = <&pio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; > ... > } > > For this reason, we can add this common initialization in the core. This > way, other I2C bus drivers will be able to support GPIO recovery just by > providing a pointer to platform's pinctrl and calling i2c_recover_bus() > when SDA is stuck low. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > --- Applied to for-next, thanks! Two minor change: > + /* for pinctrl state changes, we need all the information */ > + if (!bri->pins_default || !bri->pins_gpio) { > + bri->pinctrl = NULL; > + bri->pins_default = NULL; > + bri->pins_gpio = NULL; > + } else { > + dev_info(dev, "using pinctrl states for GPIO recovery"); > + } I inverted the logic here: 294 /* for pinctrl state changes, we need all the information */ 295 if (bri->pins_default && bri->pins_gpio) { 296 dev_info(dev, "using pinctrl states for GPIO recovery"); 297 } else { 298 bri->pinctrl = NULL; 299 bri->pins_default = NULL; 300 bri->pins_gpio = NULL; 301 } I think it is a bit easier to read if the desired path is not in the else case. > + * @pins_default: default state of SCL/SDA lines, when they are assigned to the > + * I2C bus. Optional. Populated internally for GPIO recovery, if a state with > + * the name PINCTRL_STATE_DEFAULT is found and pinctrl is valid. > + * @pins_gpio: recovery state of SCL/SDA lines, when they are used as GPIOs. > + * Optional. Populated internally for GPIO recovery, if this state is called > + * "gpio" or "recovery" and pinctrl is valid. Added "pinctrl" to "state of SCL/SDA" to make it clear.
On Tue, Aug 04, 2020 at 12:59:25PM +0300, Codrin Ciubotariu wrote: > Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return > -EPROBE_DEFER, so we should at least treat that. This ends up with > i2c_register_adapter() to be able to return -EPROBE_DEFER. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Applied to for-next, thanks!
On Tue, Aug 04, 2020 at 12:59:26PM +0300, Codrin Ciubotariu wrote: > Make the Microchip at91 driver the first to use the generic GPIO bus > recovery support from the I2C core and discard the driver implementation. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> Applied to for-next, thanks!
Codrin, everyone > This patch series was previously sent as a RFC. Significant changes > since RFC: > - "recovery" pinctrl state marked as deprecared in bindings; > - move to "gpio" pinctrl state done after the call to prepare_recovery() > callback; > - glitch protection when SDA gpio is taken at initialization; Thanks for the fast update, now all merged for inclusion into 5.9. I think it is really good, but to verify and double check, I think two things would be even better.. One thing, I'll definately be doing is to add this feature to i2c-sh_mobile.c and scope the results. The other thing would be to convert the PXA driver and see if our generic support can help their advanced use case or if we are missing something. Codrin, do you have maybe time and interest to do that? That would be awesome! Happy hacking and kind regards, Wolfram
On 05.08.2020 11:52, wsa@kernel.org wrote: > Codrin, everyone > >> This patch series was previously sent as a RFC. Significant changes >> since RFC: >> - "recovery" pinctrl state marked as deprecared in bindings; >> - move to "gpio" pinctrl state done after the call to prepare_recovery() >> callback; >> - glitch protection when SDA gpio is taken at initialization; > > Thanks for the fast update, now all merged for inclusion into 5.9. I > think it is really good, but to verify and double check, I think two > things would be even better.. Happy to help. > > One thing, I'll definately be doing is to add this feature to > i2c-sh_mobile.c and scope the results. > > The other thing would be to convert the PXA driver and see if our > generic support can help their advanced use case or if we are missing > something. Codrin, do you have maybe time and interest to do that? That > would be awesome! Naturally, these would be the next steps. I can do this, sure, but I don't have the HW. I'll look for some development kits. If you have any recommendations, please let me know. Best regards, Codrin
> > The other thing would be to convert the PXA driver and see if our > > generic support can help their advanced use case or if we are missing > > something. Codrin, do you have maybe time and interest to do that? That > > would be awesome! > > Naturally, these would be the next steps. I can do this, sure, but I > don't have the HW. I'll look for some development kits. If you have any > recommendations, please let me know. No need for HW, I think. Just do a best effort conversion, double or triple check it, and CC the patches also to Russell King. If he accepts them, we are good. Thanks for doing it!