| Message ID | 20190425232028.9333-3-fancer.lancer@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Peter Rosin |
| Headers | show |
| Series | i2c-mux-gpio: Split plat- and dt-specific code up | expand |
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@gmail.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); > >
Hello Peter, On Sun, Jun 09, 2019 at 09:34:54PM +0000, Peter Rosin wrote: > 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? > The series was intended to add the gpiod support to the i2c-mux-gpio driver (see the cover letter of the series). So the last patch is the most valuable one. Without it the whole series is nothing but a small readability improvement. So it is pointless to merge the first patch only. Anyway since you refuse to add the last patch and the first patch is actually pointless without the rest of the series, and I would have to spend my time to resubmit the v3 of the first patch anyway, it was much easier to test the current version of the Linus' patch and make it working for OF-based platforms. Additionally the Linus' patch also reaches the main goal of this patchset. I don't know what would be the appropriate way to send the updated version of the Linus' patch. So I just attached the v4 of it to this email. Shall I better send it in reply to the Linus' patch series? Regards, -Sergey > 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@gmail.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); > > > > >
On 2019-06-14 18:31, Serge Semin wrote: > Hello Peter, > > On Sun, Jun 09, 2019 at 09:34:54PM +0000, Peter Rosin wrote: >> 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? >> > > The series was intended to add the gpiod support to the i2c-mux-gpio driver > (see the cover letter of the series). So the last patch is the most valuable > one. Without it the whole series is nothing but a small readability improvement. > So it is pointless to merge the first patch only. Agreed on all points, except perhaps for the "refuse" part below and that the readability improvement of patch 1/3 is perhaps not all that pointless. > Anyway since you refuse to add the last patch and the first patch is actually > pointless without the rest of the series, and I would have to spend my time to > resubmit the v3 of the first patch anyway, it was much easier to test the > current version of the Linus' patch and make it working for OF-based platforms. > Additionally the Linus' patch also reaches the main goal of this patchset. I'm very pleased that you do not feel totally put off, and are willing to help even if we end up storing your series in /dev/null. Kudos! > I don't know what would be the appropriate way to send the updated version of > the Linus' patch. So I just attached the v4 of it to this email. Shall I better > send it in reply to the Linus' patch series? I get the impression that you have already done the work? In that case, how I would proceed would depend on how big the difference is. If it's just a few one-liners here and there, I think I would make a detailed review comment so that it is easy for Linus to incorporate the needed changes. If it's anything even remotely complex I would post an incremental patch. Of course, the former does not exclude the latter, but I do think an incremental patch is better than a repost. Thanks again! Cheers, Peter
On Fri, Jun 14, 2019 at 06:04:33PM +0000, Peter Rosin wrote: > On 2019-06-14 18:31, Serge Semin wrote: > > Hello Peter, > > > > On Sun, Jun 09, 2019 at 09:34:54PM +0000, Peter Rosin wrote: > >> 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? > >> > > > > The series was intended to add the gpiod support to the i2c-mux-gpio driver > > (see the cover letter of the series). So the last patch is the most valuable > > one. Without it the whole series is nothing but a small readability improvement. > > So it is pointless to merge the first patch only. > > Agreed on all points, except perhaps for the "refuse" part below and > that the readability improvement of patch 1/3 is perhaps not all that > pointless. > > > Anyway since you refuse to add the last patch and the first patch is actually > > pointless without the rest of the series, and I would have to spend my time to > > resubmit the v3 of the first patch anyway, it was much easier to test the > > current version of the Linus' patch and make it working for OF-based platforms. > > Additionally the Linus' patch also reaches the main goal of this patchset. > > I'm very pleased that you do not feel totally put off, and are willing > to help even if we end up storing your series in /dev/null. Kudos! > > > I don't know what would be the appropriate way to send the updated version of > > the Linus' patch. So I just attached the v4 of it to this email. Shall I better > > send it in reply to the Linus' patch series? > > I get the impression that you have already done the work? In that case, > how I would proceed would depend on how big the difference is. If it's > just a few one-liners here and there, I think I would make a detailed > review comment so that it is easy for Linus to incorporate the needed > changes. If it's anything even remotely complex I would post an > incremental patch. Of course, the former does not exclude the latter, > but I do think an incremental patch is better than a repost. > Yes, I tested the Linus' patch on my OF-based platform (though had to port to kernel 4.9) and found two problems. The incremental patch, which fixes them is send to you with mailing lists and everyone involved being Cc'ed. Regards, -Sergey > Thanks again! > > Cheers, > Peter
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);
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. Signed-off-by: Serge Semin <fancer.lancer@gmail.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(-)