Thanks for your patches, and sorry for the slow review...
On 2019-04-26 01:20, Serge Semin wrote:
> We can unpin a code specific for i2c-mux-gpio device declared
Unpin? I think the common phrase is "factor out"? That unpin is also
present in the subject. BTW, I prefer the subject to start with
[PATCH ...] i2c: mux: gpio: factor out...
> as platform device. In this case the platform data just needs to be
> copied to the private storage and if GPIO chip pointer is referring to
> a valid GPIO chip descriptor save it' base number for further GPIOs
> request and initialization. The rest of the code is common for both
> platform and OF-based setups.
>
> It's also pointless and might be even errors prone to proceed with
> further initialization if OF kernel config is disabled and plat-based
> initialization isn't defined. Just return an error in this case.
Hmm, there are a couple of other language issues, how about:
Subject: i2c: mux: gpio: factor out platform-based device init
We can factor out the probe code specific for i2c-mux-gpio when used as
a platform device. In this case the platform data just needs to be
copied to the private storage except if the GPIO chip pointer is
referring to a valid GPIO chip descriptor, in which case we save its
base number for further GPIO requests and init. The rest of the code
is common for both platform and OF-based setups.
It's also pointless and might even be error prone to proceed with
further initialization if neither OF nor platform-based parameters
are given. Just error out in this case.
> Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
>
> ---
> Changelog v2
> - Return an error if OF kconfig is disabled while dt-based GPIOs probe
> is called.
> ---
> drivers/i2c/muxes/i2c-mux-gpio.c | 69 ++++++++++++++++++--------------
> 1 file changed, 38 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 13882a2a4f60..54158b825acd 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -132,48 +132,55 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> struct platform_device *pdev)
> {
> - return 0;
> + return -EINVAL;
This is unrelated and should be a separate patch, as is almost always the
case when there is an "also" like you have in the commit message.
> }
> #endif
>
> +static int i2c_mux_gpio_probe_plat(struct gpiomux *mux,
> + struct platform_device *pdev)
I think you should spell out platform, and please align the arguments
vertically.
> +{
> + struct i2c_mux_gpio_platform_data *data = dev_get_platdata(&pdev->dev);
> + struct gpio_chip *gpio;
> +
> + /*
> + * If a GPIO chip name is provided, the GPIO pin numbers provided are
> + * relative to its base GPIO number. Otherwise they are absolute.
> + */
> + if (data->gpio_chip) {
> + gpio = gpiochip_find(data->gpio_chip,
> + match_gpio_chip_by_label);
> + if (!gpio)
> + return -EPROBE_DEFER;
> +
> + mux->gpio_base = gpio->base;
> + } else {
> + mux->gpio_base = 0;
This else-branch is pointless. I realize that you are just moving
code around, but mux->gpio_base is already zero here. Could be
simplified in a followup commit, I suppose.
Cheers,
Peter
> + }
> +
> + memcpy(&mux->data, data, sizeof(mux->data));
> +
> + return 0;
> +}
> +
> 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, gpio_base;
> + unsigned initial_state;
> int i, ret;
>
> mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
> if (!mux)
> return -ENOMEM;
>
> - if (!dev_get_platdata(&pdev->dev)) {
> + if (!dev_get_platdata(&pdev->dev))
> ret = i2c_mux_gpio_probe_dt(mux, pdev);
> - if (ret < 0)
> - return ret;
> - } else {
> - memcpy(&mux->data, dev_get_platdata(&pdev->dev),
> - sizeof(mux->data));
> - }
> -
> - /*
> - * If a GPIO chip name is provided, the GPIO pin numbers provided are
> - * relative to its base GPIO number. Otherwise they are absolute.
> - */
> - if (mux->data.gpio_chip) {
> - struct gpio_chip *gpio;
> -
> - gpio = gpiochip_find(mux->data.gpio_chip,
> - match_gpio_chip_by_label);
> - if (!gpio)
> - return -EPROBE_DEFER;
> -
> - gpio_base = gpio->base;
> - } else {
> - gpio_base = 0;
> - }
> + else
> + ret = i2c_mux_gpio_probe_plat(mux, pdev);
> + if (ret)
> + return ret;
>
> parent = i2c_get_adapter(mux->data.parent);
> if (!parent)
> @@ -194,7 +201,6 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> root = i2c_root_adapter(&parent->dev);
>
> muxc->mux_locked = true;
> - mux->gpio_base = gpio_base;
>
> if (mux->data.idle != I2C_MUX_GPIO_NO_IDLE) {
> initial_state = mux->data.idle;
> @@ -207,14 +213,15 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> struct device *gpio_dev;
> struct gpio_desc *gpio_desc;
>
> - ret = gpio_request(gpio_base + mux->data.gpios[i], "i2c-mux-gpio");
> + 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(gpio_base + mux->data.gpios[i],
> + ret = gpio_direction_output(mux->gpio_base + mux->data.gpios[i],
> initial_state & (1 << i));
> if (ret) {
> dev_err(&pdev->dev,
> @@ -224,7 +231,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> goto err_request_gpio;
> }
>
> - gpio_desc = gpio_to_desc(gpio_base + mux->data.gpios[i]);
> + gpio_desc = gpio_to_desc(mux->gpio_base + mux->data.gpios[i]);
> mux->gpios[i] = gpio_desc;
>
> if (!muxc->mux_locked)
> @@ -256,7 +263,7 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
> i = mux->data.n_gpios;
> err_request_gpio:
> for (; i > 0; i--)
> - gpio_free(gpio_base + mux->data.gpios[i - 1]);
> + gpio_free(mux->gpio_base + mux->data.gpios[i - 1]);
> alloc_failed:
> i2c_put_adapter(parent);
>
>
