| Message ID | 1385122474-14926-4-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 11/22/2013 05:14 AM, Mika Westerberg wrote: > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > All platforms using this driver are now converted to the new > descriptor-based GPIO interface. Don't you want to remove the fields from the pdata structure too, since it's pointless to set them anymore IIUC? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 22, 2013 at 9:14 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > All platforms using this driver are now converted to the new > descriptor-based GPIO interface. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > net/rfkill/rfkill-gpio.c | 61 ++++++++++-------------------------------------- > 1 file changed, 12 insertions(+), 49 deletions(-) > > diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c > index 503432154616..bd2a5b90400c 100644 > --- a/net/rfkill/rfkill-gpio.c > +++ b/net/rfkill/rfkill-gpio.c > @@ -68,35 +68,6 @@ static const struct rfkill_ops rfkill_gpio_ops = { > .set_block = rfkill_gpio_set_power, > }; > > -static int rfkill_gpio_convert_to_desc(struct platform_device *pdev, > - struct rfkill_gpio_data *rfkill) > -{ > - struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data; > - int ret; > - > - if (gpio_is_valid(pdata->reset_gpio)) { > - ret = devm_gpio_request_one(&pdev->dev, pdata->reset_gpio, > - 0, rfkill->reset_name); > - if (ret) { > - dev_err(&pdev->dev, "failed to get reset gpio.\n"); > - return ret; > - } > - rfkill->reset_gpio = gpio_to_desc(pdata->reset_gpio); > - } > - > - if (gpio_is_valid(pdata->shutdown_gpio)) { > - ret = devm_gpio_request_one(&pdev->dev, pdata->shutdown_gpio, > - 0, rfkill->shutdown_name); > - if (ret) { > - dev_err(&pdev->dev, "failed to get shutdown gpio.\n"); > - return ret; > - } > - rfkill->shutdown_gpio = gpio_to_desc(pdata->shutdown_gpio); > - } > - > - return 0; > -} > - > static int rfkill_gpio_acpi_probe(struct device *dev, > struct rfkill_gpio_data *rfkill) > { > @@ -117,6 +88,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev) > struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data; > struct rfkill_gpio_data *rfkill; > const char *clk_name = NULL; > + struct gpio_desc *gpio; > int ret; > int len; > > @@ -150,29 +122,20 @@ static int rfkill_gpio_probe(struct platform_device *pdev) > > rfkill->clk = devm_clk_get(&pdev->dev, clk_name); > > - if (pdata && (pdata->reset_gpio || pdata->shutdown_gpio)) { > - ret = rfkill_gpio_convert_to_desc(pdev, rfkill); > + gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); > + if (!IS_ERR(gpio)) { > + ret = gpiod_direction_output(gpio, 0); > if (ret) > return ret; > - } else { > - struct gpio_desc *gpio; > - > - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); > - if (!IS_ERR(gpio)) { > - ret = gpiod_direction_output(gpio, 0); > - if (ret) > - return ret; > - rfkill->reset_gpio = gpio; > - } > + rfkill->reset_gpio = gpio; > + } > > - gpio = devm_gpiod_get_index(&pdev->dev, > - rfkill->shutdown_name, 1); > - if (!IS_ERR(gpio)) { > - ret = gpiod_direction_output(gpio, 0); > - if (ret) > - return ret; > - rfkill->shutdown_gpio = gpio; > - } > + gpio = devm_gpiod_get_index(&pdev->dev, rfkill->shutdown_name, 1); > + if (!IS_ERR(gpio)) { > + ret = gpiod_direction_output(gpio, 0); > + if (ret) > + return ret; > + rfkill->shutdown_gpio = gpio; > } > > /* Make sure at-least one of the GPIO is defined and that Wouldn't it be possible (and simpler) to move patch 2 of your series to first position, and then to merge patch 1 and 3 together in second position? It seems to me that you are basically undoing much of the work of your first patch here (notably by removing rfkill_gpio_convert_to_desc() which ends up having a very short life) and that this could be avoided if you defined the platform lookup tables first. Doing so would avoid prevent you from using gpio_to_desc() which you should never ever use anyway. :P -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 23, 2013 at 05:59:30PM +0900, Alexandre Courbot wrote: > Wouldn't it be possible (and simpler) to move patch 2 of your series > to first position, and then to merge patch 1 and 3 together in second > position? It seems to me that you are basically undoing much of the > work of your first patch here (notably by removing > rfkill_gpio_convert_to_desc() which ends up having a very short life) > and that this could be avoided if you defined the platform lookup > tables first. > > Doing so would avoid prevent you from using gpio_to_desc() which you > should never ever use anyway. :P Adding the lookup table in first patch and then changing the driver in the second creates a point to the history where this driver stops working on this platform, which is something I'm not willing to do. But, we can make one patch out of all three if everybody is OK with that.
On 11/25/2013 05:41 PM, Heikki Krogerus wrote: > On Sat, Nov 23, 2013 at 05:59:30PM +0900, Alexandre Courbot wrote: >> Wouldn't it be possible (and simpler) to move patch 2 of your series >> to first position, and then to merge patch 1 and 3 together in second >> position? It seems to me that you are basically undoing much of the >> work of your first patch here (notably by removing >> rfkill_gpio_convert_to_desc() which ends up having a very short life) >> and that this could be avoided if you defined the platform lookup >> tables first. >> >> Doing so would avoid prevent you from using gpio_to_desc() which you >> should never ever use anyway. :P > > Adding the lookup table in first patch and then changing the driver in > the second creates a point to the history where this driver stops > working on this platform, which is something I'm not willing to do. Does it? If you just add a lookup table and keep using the integer-based GPIO interface, then your lookup table will not be used by anyone and will basically be a no-op. Then you can switch to the GPIO descriptor interface and take advantage of the lookup table. Unless I missed something there should not be any point that breaks in the git history. (to be clear: the first patch should *only* contain the lookup table, and the second be a merge of the current patches 1 and 3 of this series.) > But, we can make one patch out of all three if everybody is OK with > that. IIUC platform changes should be distinct from drivers whenever possible, so this is probably not the best choice here. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 25, 2013 at 05:47:38PM +0900, Alex Courbot wrote: > On 11/25/2013 05:41 PM, Heikki Krogerus wrote: > >Adding the lookup table in first patch and then changing the driver in > >the second creates a point to the history where this driver stops > >working on this platform, which is something I'm not willing to do. > > Does it? If you just add a lookup table and keep using the > integer-based GPIO interface, then your lookup table will not be > used by anyone and will basically be a no-op. Then you can switch to > the GPIO descriptor interface and take advantage of the lookup > table. Unless I missed something there should not be any point that > breaks in the git history. > > (to be clear: the first patch should *only* contain the lookup > table, and the second be a merge of the current patches 1 and 3 of > this series.) OK, I agree. If I don't remove the old gpio numbers in in the first patch, there is no problem. We can do this with the two patches. Thanks,
On 11/25/2013 06:02 PM, Heikki Krogerus wrote: > On Mon, Nov 25, 2013 at 05:47:38PM +0900, Alex Courbot wrote: >> On 11/25/2013 05:41 PM, Heikki Krogerus wrote: >>> Adding the lookup table in first patch and then changing the driver in >>> the second creates a point to the history where this driver stops >>> working on this platform, which is something I'm not willing to do. >> >> Does it? If you just add a lookup table and keep using the >> integer-based GPIO interface, then your lookup table will not be >> used by anyone and will basically be a no-op. Then you can switch to >> the GPIO descriptor interface and take advantage of the lookup >> table. Unless I missed something there should not be any point that >> breaks in the git history. >> >> (to be clear: the first patch should *only* contain the lookup >> table, and the second be a merge of the current patches 1 and 3 of >> this series.) > > OK, I agree. If I don't remove the old gpio numbers in in the first > patch, there is no problem. We can do this with the two patches. Yep, that's what I meant. :) It would make the series considerably easier to understand by removing its temporary code. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c index 503432154616..bd2a5b90400c 100644 --- a/net/rfkill/rfkill-gpio.c +++ b/net/rfkill/rfkill-gpio.c @@ -68,35 +68,6 @@ static const struct rfkill_ops rfkill_gpio_ops = { .set_block = rfkill_gpio_set_power, }; -static int rfkill_gpio_convert_to_desc(struct platform_device *pdev, - struct rfkill_gpio_data *rfkill) -{ - struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data; - int ret; - - if (gpio_is_valid(pdata->reset_gpio)) { - ret = devm_gpio_request_one(&pdev->dev, pdata->reset_gpio, - 0, rfkill->reset_name); - if (ret) { - dev_err(&pdev->dev, "failed to get reset gpio.\n"); - return ret; - } - rfkill->reset_gpio = gpio_to_desc(pdata->reset_gpio); - } - - if (gpio_is_valid(pdata->shutdown_gpio)) { - ret = devm_gpio_request_one(&pdev->dev, pdata->shutdown_gpio, - 0, rfkill->shutdown_name); - if (ret) { - dev_err(&pdev->dev, "failed to get shutdown gpio.\n"); - return ret; - } - rfkill->shutdown_gpio = gpio_to_desc(pdata->shutdown_gpio); - } - - return 0; -} - static int rfkill_gpio_acpi_probe(struct device *dev, struct rfkill_gpio_data *rfkill) { @@ -117,6 +88,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev) struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data; struct rfkill_gpio_data *rfkill; const char *clk_name = NULL; + struct gpio_desc *gpio; int ret; int len; @@ -150,29 +122,20 @@ static int rfkill_gpio_probe(struct platform_device *pdev) rfkill->clk = devm_clk_get(&pdev->dev, clk_name); - if (pdata && (pdata->reset_gpio || pdata->shutdown_gpio)) { - ret = rfkill_gpio_convert_to_desc(pdev, rfkill); + gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); + if (!IS_ERR(gpio)) { + ret = gpiod_direction_output(gpio, 0); if (ret) return ret; - } else { - struct gpio_desc *gpio; - - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); - if (!IS_ERR(gpio)) { - ret = gpiod_direction_output(gpio, 0); - if (ret) - return ret; - rfkill->reset_gpio = gpio; - } + rfkill->reset_gpio = gpio; + } - gpio = devm_gpiod_get_index(&pdev->dev, - rfkill->shutdown_name, 1); - if (!IS_ERR(gpio)) { - ret = gpiod_direction_output(gpio, 0); - if (ret) - return ret; - rfkill->shutdown_gpio = gpio; - } + gpio = devm_gpiod_get_index(&pdev->dev, rfkill->shutdown_name, 1); + if (!IS_ERR(gpio)) { + ret = gpiod_direction_output(gpio, 0); + if (ret) + return ret; + rfkill->shutdown_gpio = gpio; } /* Make sure at-least one of the GPIO is defined and that