| Message ID | 20200619141904.910889-5-codrin.ciubotariu@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: core: add generic GPIO bus recovery | expand |
On Fri, Jun 19, 2020 at 05:19:04PM +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> > --- > drivers/i2c/busses/i2c-at91-master.c | 69 ++-------------------------- > drivers/i2c/busses/i2c-at91.h | 3 -- Nice diffstat! I will try using this new functionality with another controller next week.
On 02.08.2020 20:08, Wolfram Sang wrote: > On Fri, Jun 19, 2020 at 05:19:04PM +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> >> --- >> drivers/i2c/busses/i2c-at91-master.c | 69 ++-------------------------- >> drivers/i2c/busses/i2c-at91.h | 3 -- > > Nice diffstat! I will try using this new functionality with another > controller next week. > Thanks, this would be great! I tested this on a sam9x60, with the HW feature for the 9 pulses disabled, with a picky audio codec as I2C device. Please let me know of the result. Best regards, Codrin
> > Nice diffstat! I will try using this new functionality with another > > controller next week. > > > > Thanks, this would be great! I tested this on a sam9x60, with the HW > feature for the 9 pulses disabled, with a picky audio codec as I2C device. > Please let me know of the result. I'll surely let you know.
> Thanks, this would be great! I tested this on a sam9x60, with the HW > feature for the 9 pulses disabled, with a picky audio codec as I2C device. > Please let me know of the result. I can't make use of the feature on the platform I had in mind, sadly. It doesn't really support switching from/to GPIO pinctrl states. If that ever changes, I will add bus recovery for that controller, but I think this is low priority. On the good side, there are patches which make i2c-mv64xxx another user of your new mechanism, so everything is well, I think.
On 26.08.2020 09:14, Wolfram Sang wrote: > >> Thanks, this would be great! I tested this on a sam9x60, with the HW >> feature for the 9 pulses disabled, with a picky audio codec as I2C device. >> Please let me know of the result. > > I can't make use of the feature on the platform I had in mind, sadly. It > doesn't really support switching from/to GPIO pinctrl states. If that > ever changes, I will add bus recovery for that controller, but I think > this is low priority. The pinmux driver needs to have strict set to false, otherwise the switching is not available, not at this time at least. Perhaps there is room for improvement here, because the I2C bus is not using the pins while we are doing GPIO recovery. > > On the good side, there are patches which make i2c-mv64xxx another user > of your new mechanism, so everything is well, I think. > I saw them, I will try to take a look. I am not sure I'll have time the next week to work on what you asked me regarding sh_mobile and PXA, but I will look into it the week after that. Sorry about my delayed reply, I was on vacation. Best regards, Codrin
Hi Codrin, > The pinmux driver needs to have strict set to false, otherwise the > switching is not available, not at this time at least. Perhaps there is > room for improvement here, because the I2C bus is not using the pins > while we are doing GPIO recovery. Our driver doesn't use 'strict'. The thing is that I can't describe a pinctrl state for GPIO. GPIO is the default state until another function is requested. Back to GPIO currently means freeing the pin again, so it defaults back to GPIO. We are currently discussing it. Geert (CCed) isn't very happy of describing the same pins with 'function = "gpio"' because the Kernel already knows the mapping, just needs to revert it. Geert, please correct me if I am wrong. > I am not sure I'll have time the next week to work on what you asked me > regarding sh_mobile and PXA, but I will look into it the week after that. > Sorry about my delayed reply, I was on vacation. Well, no need for sh_mobile, this is my todo item :) About PXA, well, I am still happy that you volunteered to do it, so I hope you had a relaxing vacation! Happy hacking, Wolfram
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c index 363d540a8345..66864f9cf7ac 100644 --- a/drivers/i2c/busses/i2c-at91-master.c +++ b/drivers/i2c/busses/i2c-at91-master.c @@ -816,79 +816,16 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr) return ret; } -static void at91_prepare_twi_recovery(struct i2c_adapter *adap) -{ - struct at91_twi_dev *dev = i2c_get_adapdata(adap); - - pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio); -} - -static void at91_unprepare_twi_recovery(struct i2c_adapter *adap) -{ - struct at91_twi_dev *dev = i2c_get_adapdata(adap); - - pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default); -} - static int at91_init_twi_recovery_gpio(struct platform_device *pdev, struct at91_twi_dev *dev) { struct i2c_bus_recovery_info *rinfo = &dev->rinfo; - dev->pinctrl = devm_pinctrl_get(&pdev->dev); - if (!dev->pinctrl || IS_ERR(dev->pinctrl)) { + rinfo->pinctrl = devm_pinctrl_get(&pdev->dev); + if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) { dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n"); - return PTR_ERR(dev->pinctrl); + return PTR_ERR(rinfo->pinctrl); } - - dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl, - PINCTRL_STATE_DEFAULT); - dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl, - "gpio"); - if (IS_ERR(dev->pinctrl_pins_default) || - IS_ERR(dev->pinctrl_pins_gpio)) { - dev_info(&pdev->dev, "pinctrl states incomplete for recovery\n"); - return -EINVAL; - } - - /* - * pins will be taken as GPIO, so we might as well inform pinctrl about - * this and move the state to GPIO - */ - pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio); - - rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN); - if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER) - return -EPROBE_DEFER; - - rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", - GPIOD_OUT_HIGH_OPEN_DRAIN); - if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) - return -EPROBE_DEFER; - - if (IS_ERR(rinfo->sda_gpiod) || - IS_ERR(rinfo->scl_gpiod)) { - dev_info(&pdev->dev, "recovery information incomplete\n"); - if (!IS_ERR(rinfo->sda_gpiod)) { - gpiod_put(rinfo->sda_gpiod); - rinfo->sda_gpiod = NULL; - } - if (!IS_ERR(rinfo->scl_gpiod)) { - gpiod_put(rinfo->scl_gpiod); - rinfo->scl_gpiod = NULL; - } - pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default); - return -EINVAL; - } - - /* change the state of the pins back to their default state */ - pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default); - - dev_info(&pdev->dev, "using scl, sda for recovery\n"); - - rinfo->prepare_recovery = at91_prepare_twi_recovery; - rinfo->unprepare_recovery = at91_unprepare_twi_recovery; - rinfo->recover_bus = i2c_generic_scl_recovery; dev->adapter.bus_recovery_info = rinfo; return 0; diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h index 7e7b4955ca7f..eae673ae786c 100644 --- a/drivers/i2c/busses/i2c-at91.h +++ b/drivers/i2c/busses/i2c-at91.h @@ -157,9 +157,6 @@ struct at91_twi_dev { struct at91_twi_dma dma; bool slave_detected; struct i2c_bus_recovery_info rinfo; - struct pinctrl *pinctrl; - struct pinctrl_state *pinctrl_pins_default; - struct pinctrl_state *pinctrl_pins_gpio; #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL unsigned smr; struct i2c_client *slave;
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> --- drivers/i2c/busses/i2c-at91-master.c | 69 ++-------------------------- drivers/i2c/busses/i2c-at91.h | 3 -- 2 files changed, 3 insertions(+), 69 deletions(-)