| 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
| ||
|
Date: Sun, 9 Jun 2019 21:34:54 +0000
From: Peter Rosin <peda@...ntia.se>
To: Serge Semin <fancer.lancer@...il.com>,
Peter Korsgaard <peter.korsgaard@...co.com>
CC: Serge Semin <Sergey.Semin@...latforms.ru>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Jean Delvare <jdelvare@...e.com>,
Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v2 2/3] i2c-mux-gpio: Unpin the platform-specific GPIOs
request code
On 2019-04-26 01:20, Serge Semin wrote:
> The GPIOs request loop can be safely moved to a separate function.
> First of all it shall improve the code readability. Secondly the
> initialization loop at this point is used for both of- and
> platform_data-based initialization paths, but it will be changed in
> the next patch, so by isolating the code we'll simplify the future
> work.
This patch is just preparatory for patch 3/3, as I see it. And since
I'm not really fond of the end result after patch 3/3, I'm going to
sum up my issues here, instead of trying do it piecemeal in the two
patches.
Linus and Jean, for your convenience, link to this patch series [1].
While I agree with the goal (to use the more flexible gpiod functions
to get at the gpio descriptors), the cost is too high when the init
code for platform and OF is basically completely separated. I much
prefer the approach taken by Linus [2], which instead converts the
platform interface and its single user to use gpio descriptors instead
of the legacy gpio interface. The i2c-mux-gpio code then has the
potential to take a unified approach to the given gpio descriptors,
wherever they are originating from, which is much nicer than the
code-fork in this series.
I also think it is pretty pointless to first split the code into
platform and OF paths, just so that the next patch (from Linus) can
unify the two paths again. I'd like to skip the intermediate step.
So, I'm hoping for the following to happen.
1. Sergey sends a revised patch for patch 1/3.
2. I put the patch on the for-next branch.
3. Linus rebases his patch on top of that (while thinking about
the questions raised by Sergey).
4. Sergey tests the result, I and Jean review it, then possibly
go back to 3.
5. I put the patch on the for-next branch.
Is that ok? Or is someone insisting that we take a detour?
Cheers,
Peter
[1] https://patchwork.ozlabs.org/cover/1091119/ (and show related)
[2] https://patchwork.ozlabs.org/patch/1109521/
> Signed-off-by: Serge Semin <fancer.lancer@...il.com>
>
> ---
> Changelog v2
> - Create a dedicated initial_state field in the "gpiomux" structure to
> keep an initial channel selector state.
> ---
> drivers/i2c/muxes/i2c-mux-gpio.c | 113 +++++++++++++++++++------------
> 1 file changed, 68 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 54158b825acd..e10f72706b99 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -20,7 +20,8 @@
>
> struct gpiomux {
> struct i2c_mux_gpio_platform_data data;
> - unsigned gpio_base;
> + unsigned int gpio_base;
> + unsigned int initial_state;
> struct gpio_desc **gpios;
> };
>
> @@ -162,13 +163,68 @@ static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
> return 0;
> }
>
> +static int i2c_mux_gpio_request_plat(struct gpiomux *mux,
> + struct platform_device *pdev)
> +{
> + struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> + struct gpio_desc *gpio_desc;
> + struct i2c_adapter *root;
> + struct device *gpio_dev;
> + int i, ret;
> +
> + root = i2c_root_adapter(&muxc->parent->dev);
> +
> + for (i = 0; i < mux->data.n_gpios; i++) {
> + ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
> + "i2c-mux-gpio");
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to request GPIO %d\n",
> + mux->data.gpios[i]);
> + goto err_request_gpio;
> + }
> +
> + ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> + mux->initial_state & (1 << i));
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Failed to set direction of GPIO %d to output\n",
> + mux->data.gpios[i]);
> + i++; /* gpio_request above succeeded, so must free */
> + goto err_request_gpio;
> + }
> +
> + gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
> + mux->gpios[i] = gpio_desc;
> +
> + if (!muxc->mux_locked)
> + continue;
> +
> + gpio_dev = &gpio_desc->gdev->dev;
> + muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
> + }
> +
> + return 0;
> +
> +err_request_gpio:
> + for (; i > 0; i--)
> + gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
> +
> + return ret;
> +}
> +
> +static void i2c_mux_gpio_free(struct gpiomux *mux)
> +{
> + int i;
> +
> + for (i = 0; i < mux->data.n_gpios; i++)
> + gpiod_free(mux->gpios[i]);
> +}
> +
> static int i2c_mux_gpio_probe(struct platform_device *pdev)
> {
> struct i2c_mux_core *muxc;
> struct gpiomux *mux;
> struct i2c_adapter *parent;
> - struct i2c_adapter *root;
> - unsigned initial_state;
> int i, ret;
>
> mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
> @@ -198,48 +254,18 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, muxc);
>
> - root = i2c_root_adapter(&parent->dev);
> -
> muxc->mux_locked = true;
>
> if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
> - initial_state = mux->data.idle;
> + mux->initial_state = mux->data.idle;
> muxc->deselect = i2c_mux_gpio_deselect;
> } else {
> - initial_state = mux->data.values[0];
> + mux->initial_state = mux->data.values[0];
> }
>
> - for (i = 0; i < mux->data.n_gpios; i++) {
> - struct device *gpio_dev;
> - struct gpio_desc *gpio_desc;
> -
> - ret = gpio_request(mux->gpio_base + mux->data.gpios[i],
> - "i2c-mux-gpio");
> - if (ret) {
> - dev_err(&pdev->dev, "Failed to request GPIO %d\n",
> - mux->data.gpios[i]);
> - goto err_request_gpio;
> - }
> -
> - ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> - initial_state & (1 << i));
> - if (ret) {
> - dev_err(&pdev->dev,
> - "Failed to set direction of GPIO %d to output\n",
> - mux->data.gpios[i]);
> - i++; /* gpio_request above succeeded, so must free */
> - goto err_request_gpio;
> - }
> -
> - gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
> - mux->gpios[i] = gpio_desc;
> -
> - if (!muxc->mux_locked)
> - continue;
> -
> - gpio_dev = &gpio_desc->gdev->dev;
> - muxc->mux_locked = i2c_root_adapter(gpio_dev) == root;
> - }
> + ret = i2c_mux_gpio_request_plat(mux, pdev);
> + if (ret)
> + goto alloc_failed;
>
> if (muxc->mux_locked)
> dev_info(&pdev->dev, "mux-locked i2c mux\n");
> @@ -260,10 +286,9 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
>
> add_adapter_failed:
> i2c_mux_del_adapters(muxc);
> - i = mux->data.n_gpios;
> -err_request_gpio:
> - for (; i > 0; i--)
> - gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
> +
> + i2c_mux_gpio_free(mux);
> +
> alloc_failed:
> i2c_put_adapter(parent);
>
> @@ -274,12 +299,10 @@ static int i2c_mux_gpio_remove(struct platform_device *pdev)
> {
> struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> struct gpiomux *mux = i2c_mux_priv(muxc);
> - int i;
>
> i2c_mux_del_adapters(muxc);
>
> - for (i = 0; i < mux->data.n_gpios; i++)
> - gpio_free(mux->gpio_base + mux->data.gpios[i]);
> + i2c_mux_gpio_free(mux);
>
> i2c_put_adapter(muxc->parent);
>
>
Powered by blists - more mailing lists