| Message ID | 20230529074302.3612294-1-carlos.song@nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | [1/2] i2c: imx-lpi2c: add bus recovery feature | expand |
Hi, On Mon, May 29, 2023 at 03:43:01PM +0800, carlos.song@nxp.com wrote: > From: Clark Wang <xiaoning.wang@nxp.com> > > Add bus recovery feature for LPI2C. > Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts. please update the commit message according to the dts changes, as well. [...] > +static void lpi2c_imx_prepare_recovery(struct i2c_adapter *adap) > +{ > + struct lpi2c_imx_struct *lpi2c_imx; > + > + lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter); > + > + pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_gpio); > +} > + > +static void lpi2c_imx_unprepare_recovery(struct i2c_adapter *adap) > +{ > + struct lpi2c_imx_struct *lpi2c_imx; > + > + lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter); > + > + pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_default); > +} > + > +/* > + * We switch SCL and SDA to their GPIO function and do some bitbanging > + * for bus recovery. These alternative pinmux settings can be > + * described in the device tree by a separate pinctrl state "gpio". If is this still true? > + * this is missing this is not a big problem, the only implication is > + * that we can't do bus recovery. > + */ > +static int lpi2c_imx_init_recovery_info(struct lpi2c_imx_struct *lpi2c_imx, > + struct platform_device *pdev) > +{ > + struct i2c_bus_recovery_info *rinfo = &lpi2c_imx->rinfo; > + > + lpi2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev); > + if (!lpi2c_imx->pinctrl || IS_ERR(lpi2c_imx->pinctrl)) { > + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n"); > + return PTR_ERR(lpi2c_imx->pinctrl); > + } > + > + lpi2c_imx->pinctrl_pins_default = pinctrl_lookup_state(lpi2c_imx->pinctrl, > + PINCTRL_STATE_DEFAULT); > + lpi2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(lpi2c_imx->pinctrl, > + "gpio"); > + rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN); > + rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN); > + > + if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER || > + PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) { > + return -EPROBE_DEFER; > + } else if (IS_ERR(rinfo->sda_gpiod) || > + IS_ERR(rinfo->scl_gpiod) || > + IS_ERR(lpi2c_imx->pinctrl_pins_default) || > + IS_ERR(lpi2c_imx->pinctrl_pins_gpio)) { > + dev_dbg(&pdev->dev, "recovery information incomplete\n"); > + return 0; > + } Why not use these assignement from the default i2c_init_recovery()? Is there anything you are doing I am not seeing? > + > + dev_info(&pdev->dev, "using scl%s for recovery\n", > + rinfo->sda_gpiod ? ",sda" : ""); is there any case when sda_gpiod is NULL? > + > + rinfo->prepare_recovery = lpi2c_imx_prepare_recovery; > + rinfo->unprepare_recovery = lpi2c_imx_unprepare_recovery; > + rinfo->recover_bus = i2c_generic_scl_recovery; > + lpi2c_imx->adapter.bus_recovery_info = rinfo; do you need also the set_scl() function? It should be mandatory. > + > + return 0; > +} Besides, this is a copy/paste from i2c-imx.c, any chance to put the two things together? Andi
Hi, Andi Sorry for the long time to reply. According to your advice, I found the patch is too redundant! so I will send V2 patch. I find gpio and pinctrl assignement from the default i2c_init_recovery() have been defined very well. Lpi2c have special initialization conditions for i2c recovery and I have added a comment in V2. > -----Original Message----- > From: Andi Shyti <andi.shyti@kernel.org> > Sent: Wednesday, June 14, 2023 7:20 AM > To: Carlos Song <carlos.song@nxp.com> > Cc: Aisheng Dong <aisheng.dong@nxp.com>; shawnguo@kernel.org; > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > Anson.Huang@nxp.com; Clark Wang <xiaoning.wang@nxp.com>; Bough Chen > <haibo.chen@nxp.com>; dl-linux-imx <linux-imx@nxp.com>; > linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report this > email' button > > > Hi, > > On Mon, May 29, 2023 at 03:43:01PM +0800, carlos.song@nxp.com wrote: > > From: Clark Wang <xiaoning.wang@nxp.com> > > > > Add bus recovery feature for LPI2C. > > Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts. > > please update the commit message according to the dts changes, as well. > Carlos: There is still a need to add gpio pinctrl to set i2c sda/scl pin to gpio > [...] > > > +static void lpi2c_imx_prepare_recovery(struct i2c_adapter *adap) { > > + struct lpi2c_imx_struct *lpi2c_imx; > > + > > + lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, > > + adapter); > > + > > + pinctrl_select_state(lpi2c_imx->pinctrl, > > +lpi2c_imx->pinctrl_pins_gpio); } > > + > > +static void lpi2c_imx_unprepare_recovery(struct i2c_adapter *adap) { > > + struct lpi2c_imx_struct *lpi2c_imx; > > + > > + lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, > > + adapter); > > + > > + pinctrl_select_state(lpi2c_imx->pinctrl, > > +lpi2c_imx->pinctrl_pins_default); } > > + > > +/* > > + * We switch SCL and SDA to their GPIO function and do some > > +bitbanging > > + * for bus recovery. These alternative pinmux settings can be > > + * described in the device tree by a separate pinctrl state "gpio". > > +If > > is this still true? > Carlos: Yes it is true. > > + * this is missing this is not a big problem, the only implication is > > + * that we can't do bus recovery. > > + */ > > +static int lpi2c_imx_init_recovery_info(struct lpi2c_imx_struct *lpi2c_imx, > > + struct platform_device *pdev) { > > + struct i2c_bus_recovery_info *rinfo = &lpi2c_imx->rinfo; > > + > > + lpi2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev); > > + if (!lpi2c_imx->pinctrl || IS_ERR(lpi2c_imx->pinctrl)) { > > + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not > supported\n"); > > + return PTR_ERR(lpi2c_imx->pinctrl); > > + } > > + > > + lpi2c_imx->pinctrl_pins_default = > pinctrl_lookup_state(lpi2c_imx->pinctrl, > > + PINCTRL_STATE_DEFAULT); > > + lpi2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(lpi2c_imx->pinctrl, > > + "gpio"); > > + rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN); > > + rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", > > + GPIOD_OUT_HIGH_OPEN_DRAIN); > > + > > + if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER || > > + PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) { > > + return -EPROBE_DEFER; > > + } else if (IS_ERR(rinfo->sda_gpiod) || > > + IS_ERR(rinfo->scl_gpiod) || > > + IS_ERR(lpi2c_imx->pinctrl_pins_default) || > > + IS_ERR(lpi2c_imx->pinctrl_pins_gpio)) { > > + dev_dbg(&pdev->dev, "recovery information incomplete\n"); > > + return 0; > > + } > > Why not use these assignement from the default i2c_init_recovery()? Is there > anything you are doing I am not seeing? > Carlos: these assignements are too redundant and I will fix it in V2 patch. > > + > > + dev_info(&pdev->dev, "using scl%s for recovery\n", > > + rinfo->sda_gpiod ? ",sda" : ""); > > is there any case when sda_gpiod is NULL? > Carlos: I will delete it in V2. > > + > > + rinfo->prepare_recovery = lpi2c_imx_prepare_recovery; > > + rinfo->unprepare_recovery = lpi2c_imx_unprepare_recovery; > > + rinfo->recover_bus = i2c_generic_scl_recovery; > > + lpi2c_imx->adapter.bus_recovery_info = rinfo; > > do you need also the set_scl() function? It should be mandatory. > Carlos: I will use the default setting in V2 patch. > > + > > + return 0; > > +} > > Besides, this is a copy/paste from i2c-imx.c, any chance to put the two things > together? > Carlos: I hope to apply the new recovery patch for lpi2c. > Andi
diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c index 62111fe9f207..40a4420d4c12 100644 --- a/drivers/i2c/busses/i2c-imx-lpi2c.c +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c @@ -18,6 +18,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/of_gpio.h> #include <linux/pinctrl/consumer.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> @@ -107,6 +108,11 @@ struct lpi2c_imx_struct { unsigned int txfifosize; unsigned int rxfifosize; enum lpi2c_imx_mode mode; + + struct i2c_bus_recovery_info rinfo; + struct pinctrl *pinctrl; + struct pinctrl_state *pinctrl_pins_default; + struct pinctrl_state *pinctrl_pins_gpio; }; static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx, @@ -134,6 +140,8 @@ static int lpi2c_imx_bus_busy(struct lpi2c_imx_struct *lpi2c_imx) if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) { dev_dbg(&lpi2c_imx->adapter.dev, "bus not work\n"); + if (lpi2c_imx->adapter.bus_recovery_info) + i2c_recover_bus(&lpi2c_imx->adapter); return -ETIMEDOUT; } schedule(); @@ -191,6 +199,8 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx) if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) { dev_dbg(&lpi2c_imx->adapter.dev, "stop timeout\n"); + if (lpi2c_imx->adapter.bus_recovery_info) + i2c_recover_bus(&lpi2c_imx->adapter); break; } schedule(); @@ -328,6 +338,8 @@ static int lpi2c_imx_txfifo_empty(struct lpi2c_imx_struct *lpi2c_imx) if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) { dev_dbg(&lpi2c_imx->adapter.dev, "txfifo empty timeout\n"); + if (lpi2c_imx->adapter.bus_recovery_info) + i2c_recover_bus(&lpi2c_imx->adapter); return -ETIMEDOUT; } schedule(); @@ -533,6 +545,71 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id) return IRQ_HANDLED; } +static void lpi2c_imx_prepare_recovery(struct i2c_adapter *adap) +{ + struct lpi2c_imx_struct *lpi2c_imx; + + lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter); + + pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_gpio); +} + +static void lpi2c_imx_unprepare_recovery(struct i2c_adapter *adap) +{ + struct lpi2c_imx_struct *lpi2c_imx; + + lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter); + + pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_default); +} + +/* + * We switch SCL and SDA to their GPIO function and do some bitbanging + * for bus recovery. These alternative pinmux settings can be + * described in the device tree by a separate pinctrl state "gpio". If + * this is missing this is not a big problem, the only implication is + * that we can't do bus recovery. + */ +static int lpi2c_imx_init_recovery_info(struct lpi2c_imx_struct *lpi2c_imx, + struct platform_device *pdev) +{ + struct i2c_bus_recovery_info *rinfo = &lpi2c_imx->rinfo; + + lpi2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev); + if (!lpi2c_imx->pinctrl || IS_ERR(lpi2c_imx->pinctrl)) { + dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n"); + return PTR_ERR(lpi2c_imx->pinctrl); + } + + lpi2c_imx->pinctrl_pins_default = pinctrl_lookup_state(lpi2c_imx->pinctrl, + PINCTRL_STATE_DEFAULT); + lpi2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(lpi2c_imx->pinctrl, + "gpio"); + rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN); + rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN); + + if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER || + PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) { + return -EPROBE_DEFER; + } else if (IS_ERR(rinfo->sda_gpiod) || + IS_ERR(rinfo->scl_gpiod) || + IS_ERR(lpi2c_imx->pinctrl_pins_default) || + IS_ERR(lpi2c_imx->pinctrl_pins_gpio)) { + dev_dbg(&pdev->dev, "recovery information incomplete\n"); + return 0; + } + + dev_info(&pdev->dev, "using scl%s for recovery\n", + rinfo->sda_gpiod ? ",sda" : ""); + + rinfo->prepare_recovery = lpi2c_imx_prepare_recovery; + rinfo->unprepare_recovery = lpi2c_imx_unprepare_recovery; + rinfo->recover_bus = i2c_generic_scl_recovery; + lpi2c_imx->adapter.bus_recovery_info = rinfo; + + return 0; +} + static u32 lpi2c_imx_func(struct i2c_adapter *adapter) { return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | @@ -611,6 +688,12 @@ static int lpi2c_imx_probe(struct platform_device *pdev) lpi2c_imx->txfifosize = 1 << (temp & 0x0f); lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f); + /* Init optional bus recovery function */ + ret = lpi2c_imx_init_recovery_info(lpi2c_imx, pdev); + /* Give it another chance if pinctrl used is not ready yet */ + if (ret == -EPROBE_DEFER) + goto rpm_disable; + ret = i2c_add_adapter(&lpi2c_imx->adapter); if (ret) goto rpm_disable;