| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZYLD4WgOPFB9ccym@herburgerg-w2>
Date: Wed, 20 Dec 2023 11:37:21 +0100
From: Gregor Herburger <gregor.herburger@...tq-group.com>
To: esben@...nix.com
Cc: Oleksij Rempel <o.rempel@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Andi Shyti <andi.shyti@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Fabio Estevam <festevam@...il.com>,
NXP Linux Team <linux-imx@....com>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux@...tq-group.com" <linux@...tq-group.com>
Subject: Re: Re: [PATCH] i2c: i2c-imx: allow bus recovery on non-muxable pads
Hi Esben,
I had another look at your patch and tested it on a LX2160a SoC without
pinctrl. I agree that using the generic GPIO recovery you suggested is
the better solution.
To make your solution work I had to make a small change (see below).
On Tue, Dec 19, 2023 at 07:43:21AM +0000, esben@...nix.com wrote:
> Why not move to use the generic GPIO recovery instead? Will something
> like this be able to cover at least the same scenarios as your change?
>
> From 7e432496bae8c7ac35c21504bc1cd03f1dfef97f Mon Sep 17 00:00:00 2001
> Message-ID: <7e432496bae8c7ac35c21504bc1cd03f1dfef97f.1702971634.git.esben@...nix.com>
> From: Esben Haabendal <esben@...nix.com>
> Date: Tue, 25 May 2021 11:25:44 +0200
> Subject: [PATCH] i2c: imx: move to generic GPIO recovery
>
> Starting with
> commit 75820314de26 ("i2c: core: add generic I2C GPIO recovery")
> GPIO bus recovery is supported by the I2C core, so we can remove the
> driver implementation and use that one instead.
>
> As a nice side-effect, pinctrl becomes optional, allowing bus recovery on
> LS1021A, which does not have such luxury, but can be wired up to use extra
> fixed GPIO pins.
>
> Signed-off-by: Esben Haabendal <esben@...nix.com>
> ---
> drivers/i2c/busses/i2c-imx.c | 62 ++++--------------------------------
> 1 file changed, 7 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 1775a79aeba2..824d8bbb9be5 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -212,10 +212,6 @@ struct imx_i2c_struct {
> const struct imx_i2c_hwdata *hwdata;
> struct i2c_bus_recovery_info rinfo;
>
> - struct pinctrl *pinctrl;
> - struct pinctrl_state *pinctrl_pins_default;
> - struct pinctrl_state *pinctrl_pins_gpio;
> -
> struct imx_i2c_dma *dma;
> struct i2c_client *slave;
> enum i2c_slave_event last_slave_event;
> @@ -1357,24 +1353,6 @@ static int i2c_imx_xfer_atomic(struct i2c_adapter *adapter,
> return result;
> }
>
> -static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
> -{
> - struct imx_i2c_struct *i2c_imx;
> -
> - i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> -
> - pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_gpio);
> -}
> -
> -static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
> -{
> - struct imx_i2c_struct *i2c_imx;
> -
> - i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> -
> - pinctrl_select_state(i2c_imx->pinctrl, i2c_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
> @@ -1385,43 +1363,17 @@ static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
> static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
> struct platform_device *pdev)
> {
> - struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
> + struct i2c_bus_recovery_info *bri = &i2c_imx->rinfo;
>
> - i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> - if (!i2c_imx->pinctrl) {
> - dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n");
> + bri->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (PTR_ERR(bri->pinctrl) == -ENODEV) {
> + bri->pinctrl = NULL;
> return 0;
Should not return here to allow setting of adapter.bus_recovery_info
later.
> }
> - if (IS_ERR(i2c_imx->pinctrl)) {
> - dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
> - return PTR_ERR(i2c_imx->pinctrl);
> - }
> -
> - i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
> - PINCTRL_STATE_DEFAULT);
> - i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_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(i2c_imx->pinctrl_pins_default) ||
> - IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
> - dev_dbg(&pdev->dev, "recovery information incomplete\n");
> - return 0;
> - }
> -
> - dev_dbg(&pdev->dev, "using scl%s for recovery\n",
> - rinfo->sda_gpiod ? ",sda" : "");
> + if (IS_ERR(bri->pinctrl))
> + return PTR_ERR(bri->pinctrl);
>
> - rinfo->prepare_recovery = i2c_imx_prepare_recovery;
> - rinfo->unprepare_recovery = i2c_imx_unprepare_recovery;
> - rinfo->recover_bus = i2c_generic_scl_recovery;
> - i2c_imx->adapter.bus_recovery_info = rinfo;
> + i2c_imx->adapter.bus_recovery_info = bri;
>
> return 0;
> }
Best regards,
Gregor
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/
Powered by blists - more mailing lists