* linux-next: build failure after merge of the gpio tree @ 2015-07-21 3:29 Stephen Rothwell 2015-07-21 6:59 ` [PATCH] regulator: rk808: make better use of the gpiod API Uwe Kleine-König 0 siblings, 1 reply; 12+ messages in thread From: Stephen Rothwell @ 2015-07-21 3:29 UTC (permalink / raw) To: Linus Walleij, Mark Brown, Liam Girdwood Cc: linux-next, linux-kernel, Chris Zhong, Uwe Kleine-König Hi Linus, After merging the gpio tree, today's linux-next build (x86_64 allmodconfig) failed like this: drivers/regulator/rk808-regulator.c: In function 'rk808_regulator_dt_parse_pdata': drivers/regulator/rk808-regulator.c:543:24: error: too few arguments to function 'gpiod_get_index' pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i); ^ In file included from include/asm-generic/gpio.h:13:0, from include/linux/gpio.h:51, from drivers/regulator/rk808-regulator.c:20: include/linux/gpio/consumer.h:53:32: note: declared here struct gpio_desc *__must_check gpiod_get_index(struct device *dev, ^ Caused by commit bad47ad2eef3 ("regulator: rk808: fixed the overshoot when adjust voltage") from the regulator tree interactings with commit b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions") from the gpio tree. I added teh following merge fix patch: From: Stephen Rothwell <sfr@canb.auug.org.au> Date: Tue, 21 Jul 2015 13:23:56 +1000 Subject: [PATCH] regulator: rk808: fix up for gpiod_get_index() API change Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au> --- drivers/regulator/rk808-regulator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c index ca913fd15598..cfec52c1a4a2 100644 --- a/drivers/regulator/rk808-regulator.c +++ b/drivers/regulator/rk808-regulator.c @@ -540,7 +540,7 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev, goto dt_parse_end; for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) { - pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i); + pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i, GPIOD_ASIS); if (IS_ERR(pdata->dvs_gpio[i])) { dev_warn(dev, "there is no dvs%d gpio\n", i); continue; -- 2.1.4 -- Cheers, Stephen Rothwell sfr@canb.auug.org.au ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-21 3:29 linux-next: build failure after merge of the gpio tree Stephen Rothwell @ 2015-07-21 6:59 ` Uwe Kleine-König 2015-07-21 9:56 ` Linus Walleij 2015-07-21 13:09 ` [PATCH] " Krzysztof Kozlowski 0 siblings, 2 replies; 12+ messages in thread From: Uwe Kleine-König @ 2015-07-21 6:59 UTC (permalink / raw) To: Linus Walleij, Mark Brown, Liam Girdwood Cc: linux-next, linux-kernel, Chris Zhong, kernel The gpiod functions include variants for managed gpiod resources. Use it to simplify the remove function. As the driver handles a device node without a specification of dvs gpios just fine, additionally use the variant of gpiod_get exactly for this use case. This makes error checking more strict. As a third benefit this patch makes the driver use the flags parameter of gpiod_get* which will not be optional any more after 4.2 and so prevents a build failure when the respective gpiod commit is merged. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, this is the more complete fix of the issue that Stephen found while creating next-20150721 (see commit f51ec04cf8be9f7ef795f1f39ada17c19f725650). Best regards Uwe drivers/regulator/rk808-regulator.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c index ca913fd15598..3738e49beb75 100644 --- a/drivers/regulator/rk808-regulator.c +++ b/drivers/regulator/rk808-regulator.c @@ -94,7 +94,7 @@ static int rk808_buck1_2_get_voltage_sel_regmap(struct regulator_dev *rdev) unsigned int val; int ret; - if (IS_ERR(gpio) || gpiod_get_value(gpio) == 0) + if (!gpio || gpiod_get_value(gpio) == 0) return regulator_get_voltage_sel_regmap(rdev); ret = regmap_read(rdev->regmap, @@ -168,7 +168,7 @@ static int rk808_buck1_2_set_voltage_sel(struct regulator_dev *rdev, unsigned old_sel; int ret, gpio_level; - if (IS_ERR(gpio)) + if (!gpio) return rk808_buck1_2_i2c_set_voltage_sel(rdev, sel); gpio_level = gpiod_get_value(gpio); @@ -205,7 +205,7 @@ static int rk808_buck1_2_set_voltage_time_sel(struct regulator_dev *rdev, struct gpio_desc *gpio = pdata->dvs_gpio[id]; /* if there is no dvs1/2 pin, we don't need wait extra time here. */ - if (IS_ERR(gpio)) + if (!gpio) return 0; return regulator_set_voltage_time_sel(rdev, old_selector, new_selector); @@ -540,14 +540,19 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev, goto dt_parse_end; for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) { - pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i); + pdata->dvs_gpio[i] = + devm_gpiod_get_index_optional(client_dev, "dvs", i, + GPIOD_OUT_LOW); if (IS_ERR(pdata->dvs_gpio[i])) { + dev_err(dev, "failed to get dvs%d gpio\n", i); + return PTR_ERR(pdata->dvs_gpio[i]); + } + + if (!pdata->dvs_gpio[i]) { dev_warn(dev, "there is no dvs%d gpio\n", i); continue; } - gpiod_direction_output(pdata->dvs_gpio[i], 0); - tmp = i ? RK808_DVS2_POL : RK808_DVS1_POL; ret = regmap_update_bits(map, RK808_IO_POL_REG, tmp, gpiod_is_active_low(pdata->dvs_gpio[i]) ? @@ -561,14 +566,6 @@ dt_parse_end: static int rk808_regulator_remove(struct platform_device *pdev) { - struct rk808_regulator_data *pdata = platform_get_drvdata(pdev); - int i; - - for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) { - if (!IS_ERR(pdata->dvs_gpio[i])) - gpiod_put(pdata->dvs_gpio[i]); - } - return 0; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-21 6:59 ` [PATCH] regulator: rk808: make better use of the gpiod API Uwe Kleine-König @ 2015-07-21 9:56 ` Linus Walleij 2015-07-21 11:04 ` Mark Brown 2015-07-21 13:09 ` [PATCH] " Krzysztof Kozlowski 1 sibling, 1 reply; 12+ messages in thread From: Linus Walleij @ 2015-07-21 9:56 UTC (permalink / raw) To: Uwe Kleine-König Cc: Mark Brown, Liam Girdwood, linux-next, linux-kernel, Chris Zhong, Sascha Hauer On Tue, Jul 21, 2015 at 8:59 AM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > The gpiod functions include variants for managed gpiod resources. Use it > to simplify the remove function. > > As the driver handles a device node without a specification of dvs gpios > just fine, additionally use the variant of gpiod_get exactly for this > use case. This makes error checking more strict. > > As a third benefit this patch makes the driver use the flags parameter > of gpiod_get* which will not be optional any more after 4.2 and so > prevents a build failure when the respective gpiod commit is merged. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > this is the more complete fix of the issue that Stephen found while creating > next-20150721 (see commit f51ec04cf8be9f7ef795f1f39ada17c19f725650). Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Mark, please apply this. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-21 9:56 ` Linus Walleij @ 2015-07-21 11:04 ` Mark Brown 2015-07-21 14:21 ` Uwe Kleine-König 0 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2015-07-21 11:04 UTC (permalink / raw) To: Linus Walleij Cc: Uwe Kleine-König, Liam Girdwood, linux-next, linux-kernel, Chris Zhong, Sascha Hauer [-- Attachment #1: Type: text/plain, Size: 966 bytes --] On Tue, Jul 21, 2015 at 11:56:49AM +0200, Linus Walleij wrote: > On Tue, Jul 21, 2015 at 8:59 AM, Uwe Kleine-König > > The gpiod functions include variants for managed gpiod resources. Use it > > to simplify the remove function. > > As the driver handles a device node without a specification of dvs gpios > > just fine, additionally use the variant of gpiod_get exactly for this > > use case. This makes error checking more strict. > > As a third benefit this patch makes the driver use the flags parameter > > of gpiod_get* which will not be optional any more after 4.2 and so > > prevents a build failure when the respective gpiod commit is merged. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Mark, please apply this. ...and reverted since it doesn't build as it's adding a use of a new API which doesn't actually exist in Linus' tree yet (hopefully it's in -next). I need a tag to pull if I'm going to use this patch. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-21 11:04 ` Mark Brown @ 2015-07-21 14:21 ` Uwe Kleine-König [not found] ` <1437489985-1362-1-git-send-email-uwe@kleine-koenig.org> 0 siblings, 1 reply; 12+ messages in thread From: Uwe Kleine-König @ 2015-07-21 14:21 UTC (permalink / raw) To: Mark Brown Cc: Linus Walleij, Liam Girdwood, linux-next, linux-kernel, Chris Zhong, kernel, Krzysztof Kozlowski On Tue, Jul 21, 2015 at 12:04:15PM +0100, Mark Brown wrote: > On Tue, Jul 21, 2015 at 11:56:49AM +0200, Linus Walleij wrote: > > On Tue, Jul 21, 2015 at 8:59 AM, Uwe Kleine-König > > > > The gpiod functions include variants for managed gpiod resources. Use it > > > to simplify the remove function. > > > > As the driver handles a device node without a specification of dvs gpios > > > just fine, additionally use the variant of gpiod_get exactly for this > > > use case. This makes error checking more strict. > > > > As a third benefit this patch makes the driver use the flags parameter > > > of gpiod_get* which will not be optional any more after 4.2 and so > > > prevents a build failure when the respective gpiod commit is merged. > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > Mark, please apply this. > > ...and reverted since it doesn't build as it's adding a use of a new API > which doesn't actually exist in Linus' tree yet (hopefully it's in > -next). I need a tag to pull if I'm going to use this patch. If you're refering to the build bot warning I got, too, then the problem is just a missing #include and the problem existed already before e.g. for gpiod_get_value with the .config used by the build bot. In reply to this mail I'll send a fixup patch for the missing include and a reworked version of my patch that fixes the reference leak pointed out by Krzysztof Kozlowski. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <1437489985-1362-1-git-send-email-uwe@kleine-koenig.org>]
* [PATCH v2 2/2] regulator: rk808: make better use of the gpiod API [not found] ` <1437489985-1362-1-git-send-email-uwe@kleine-koenig.org> @ 2015-07-21 14:46 ` Uwe Kleine-König 2015-07-22 2:21 ` Krzysztof Kozlowski 0 siblings, 1 reply; 12+ messages in thread From: Uwe Kleine-König @ 2015-07-21 14:46 UTC (permalink / raw) To: Linus Walleij, Mark Brown, Liam Girdwood Cc: Uwe Kleine-König, Chris Zhong, linux-next, linux-kernel, kernel From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> The gpiod functions include variants for managed gpiod resources. Use it to simplify the remove function. As the driver handles a device node without a specification of dvs gpios just fine, additionally use the variant of gpiod_get exactly for this use case. This makes error checking more strict. As a third benefit this patch makes the driver use the flags parameter of gpiod_get* which will not be optional any more after 4.2 and so prevents a build failure when the respective gpiod commit is merged. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- note the above mentioned gpiod change is already in next, so the driver fails to build there. Changes since (implicit) v1, sent with Message-Id: 1437461993-14860-1-git-send-email-u.kleine-koenig@pengutronix.de: - Assert that of_node_put is called in error path to not leak a reference and drop now empty remove callback. Thanks to Krzysztof Kozlowski for catching. Best regards Uwe drivers/regulator/rk808-regulator.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c index ac9436d2ea8d..d86a3dcd61e2 100644 --- a/drivers/regulator/rk808-regulator.c +++ b/drivers/regulator/rk808-regulator.c @@ -95,7 +95,7 @@ static int rk808_buck1_2_get_voltage_sel_regmap(struct regulator_dev *rdev) unsigned int val; int ret; - if (IS_ERR(gpio) || gpiod_get_value(gpio) == 0) + if (!gpio || gpiod_get_value(gpio) == 0) return regulator_get_voltage_sel_regmap(rdev); ret = regmap_read(rdev->regmap, @@ -169,7 +169,7 @@ static int rk808_buck1_2_set_voltage_sel(struct regulator_dev *rdev, unsigned old_sel; int ret, gpio_level; - if (IS_ERR(gpio)) + if (!gpio) return rk808_buck1_2_i2c_set_voltage_sel(rdev, sel); gpio_level = gpiod_get_value(gpio); @@ -206,7 +206,7 @@ static int rk808_buck1_2_set_voltage_time_sel(struct regulator_dev *rdev, struct gpio_desc *gpio = pdata->dvs_gpio[id]; /* if there is no dvs1/2 pin, we don't need wait extra time here. */ - if (IS_ERR(gpio)) + if (!gpio) return 0; return regulator_set_voltage_time_sel(rdev, old_selector, new_selector); @@ -541,14 +541,20 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev, goto dt_parse_end; for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) { - pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i); + pdata->dvs_gpio[i] = + devm_gpiod_get_index_optional(client_dev, "dvs", i, + GPIOD_OUT_LOW); if (IS_ERR(pdata->dvs_gpio[i])) { + ret = PTR_ERR(pdata->dvs_gpio[i]); + dev_err(dev, "failed to get dvs%d gpio (%d)\n", i, ret); + goto dt_parse_end; + } + + if (!pdata->dvs_gpio[i]) { dev_warn(dev, "there is no dvs%d gpio\n", i); continue; } - gpiod_direction_output(pdata->dvs_gpio[i], 0); - tmp = i ? RK808_DVS2_POL : RK808_DVS1_POL; ret = regmap_update_bits(map, RK808_IO_POL_REG, tmp, gpiod_is_active_low(pdata->dvs_gpio[i]) ? @@ -560,19 +566,6 @@ dt_parse_end: return ret; } -static int rk808_regulator_remove(struct platform_device *pdev) -{ - struct rk808_regulator_data *pdata = platform_get_drvdata(pdev); - int i; - - for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) { - if (!IS_ERR(pdata->dvs_gpio[i])) - gpiod_put(pdata->dvs_gpio[i]); - } - - return 0; -} - static int rk808_regulator_probe(struct platform_device *pdev) { struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent); @@ -619,7 +612,6 @@ static int rk808_regulator_probe(struct platform_device *pdev) static struct platform_driver rk808_regulator_driver = { .probe = rk808_regulator_probe, - .remove = rk808_regulator_remove, .driver = { .name = "rk808-regulator", .owner = THIS_MODULE, -- 2.1.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] regulator: rk808: make better use of the gpiod API 2015-07-21 14:46 ` [PATCH v2 2/2] " Uwe Kleine-König @ 2015-07-22 2:21 ` Krzysztof Kozlowski 0 siblings, 0 replies; 12+ messages in thread From: Krzysztof Kozlowski @ 2015-07-22 2:21 UTC (permalink / raw) To: Uwe Kleine-König Cc: Linus Walleij, Mark Brown, Liam Girdwood, Uwe Kleine-König, Chris Zhong, linux-next, linux-kernel, kernel 2015-07-21 23:46 GMT+09:00 Uwe Kleine-König <uwe@kleine-koenig.org>: > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > The gpiod functions include variants for managed gpiod resources. Use it > to simplify the remove function. > > As the driver handles a device node without a specification of dvs gpios > just fine, additionally use the variant of gpiod_get exactly for this > use case. This makes error checking more strict. > > As a third benefit this patch makes the driver use the flags parameter > of gpiod_get* which will not be optional any more after 4.2 and so > prevents a build failure when the respective gpiod commit is merged. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > note the above mentioned gpiod change is already in next, so the driver > fails to build there. > > Changes since (implicit) v1, sent with > Message-Id: 1437461993-14860-1-git-send-email-u.kleine-koenig@pengutronix.de: > > - Assert that of_node_put is called in error path to not leak a reference > and drop now empty remove callback. > Thanks to Krzysztof Kozlowski for catching. > > > Best regards > Uwe > drivers/regulator/rk808-regulator.c | 32 ++++++++++++-------------------- > 1 file changed, 12 insertions(+), 20 deletions(-) Looks good now: Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-21 6:59 ` [PATCH] regulator: rk808: make better use of the gpiod API Uwe Kleine-König 2015-07-21 9:56 ` Linus Walleij @ 2015-07-21 13:09 ` Krzysztof Kozlowski 2015-07-21 14:35 ` Uwe Kleine-König 1 sibling, 1 reply; 12+ messages in thread From: Krzysztof Kozlowski @ 2015-07-21 13:09 UTC (permalink / raw) To: Uwe Kleine-König Cc: Linus Walleij, Mark Brown, Liam Girdwood, linux-next, linux-kernel, Chris Zhong, kernel 2015-07-21 15:59 GMT+09:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > The gpiod functions include variants for managed gpiod resources. Use it > to simplify the remove function. > > As the driver handles a device node without a specification of dvs gpios > just fine, additionally use the variant of gpiod_get exactly for this > use case. This makes error checking more strict. > > As a third benefit this patch makes the driver use the flags parameter > of gpiod_get* which will not be optional any more after 4.2 and so > prevents a build failure when the respective gpiod commit is merged. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > this is the more complete fix of the issue that Stephen found while creating > next-20150721 (see commit f51ec04cf8be9f7ef795f1f39ada17c19f725650). > > Best regards > Uwe > > drivers/regulator/rk808-regulator.c | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-) > > diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c > index ca913fd15598..3738e49beb75 100644 > --- a/drivers/regulator/rk808-regulator.c > +++ b/drivers/regulator/rk808-regulator.c > @@ -94,7 +94,7 @@ static int rk808_buck1_2_get_voltage_sel_regmap(struct regulator_dev *rdev) > unsigned int val; > int ret; > > - if (IS_ERR(gpio) || gpiod_get_value(gpio) == 0) > + if (!gpio || gpiod_get_value(gpio) == 0) > return regulator_get_voltage_sel_regmap(rdev); > > ret = regmap_read(rdev->regmap, > @@ -168,7 +168,7 @@ static int rk808_buck1_2_set_voltage_sel(struct regulator_dev *rdev, > unsigned old_sel; > int ret, gpio_level; > > - if (IS_ERR(gpio)) > + if (!gpio) > return rk808_buck1_2_i2c_set_voltage_sel(rdev, sel); > > gpio_level = gpiod_get_value(gpio); > @@ -205,7 +205,7 @@ static int rk808_buck1_2_set_voltage_time_sel(struct regulator_dev *rdev, > struct gpio_desc *gpio = pdata->dvs_gpio[id]; > > /* if there is no dvs1/2 pin, we don't need wait extra time here. */ > - if (IS_ERR(gpio)) > + if (!gpio) > return 0; > > return regulator_set_voltage_time_sel(rdev, old_selector, new_selector); > @@ -540,14 +540,19 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev, > goto dt_parse_end; > > for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) { > - pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i); > + pdata->dvs_gpio[i] = > + devm_gpiod_get_index_optional(client_dev, "dvs", i, > + GPIOD_OUT_LOW); > if (IS_ERR(pdata->dvs_gpio[i])) { > + dev_err(dev, "failed to get dvs%d gpio\n", i); Missing of_node_put() from of_get_child_by_name() called before. > + return PTR_ERR(pdata->dvs_gpio[i]); > + } > + > + if (!pdata->dvs_gpio[i]) { > dev_warn(dev, "there is no dvs%d gpio\n", i); > continue; > } > > - gpiod_direction_output(pdata->dvs_gpio[i], 0); > - > tmp = i ? RK808_DVS2_POL : RK808_DVS1_POL; > ret = regmap_update_bits(map, RK808_IO_POL_REG, tmp, > gpiod_is_active_low(pdata->dvs_gpio[i]) ? > @@ -561,14 +566,6 @@ dt_parse_end: > > static int rk808_regulator_remove(struct platform_device *pdev) > { > - struct rk808_regulator_data *pdata = platform_get_drvdata(pdev); > - int i; > - > - for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) { > - if (!IS_ERR(pdata->dvs_gpio[i])) > - gpiod_put(pdata->dvs_gpio[i]); > - } > - > return 0; > } The function looks empty so it can be removed entirely. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-21 13:09 ` [PATCH] " Krzysztof Kozlowski @ 2015-07-21 14:35 ` Uwe Kleine-König 2015-07-21 14:41 ` Mark Brown 0 siblings, 1 reply; 12+ messages in thread From: Uwe Kleine-König @ 2015-07-21 14:35 UTC (permalink / raw) To: Krzysztof Kozlowski, Greg Kroah-Hartman Cc: Linus Walleij, Mark Brown, Liam Girdwood, linux-next, linux-kernel, Chris Zhong, kernel Hello, On Tue, Jul 21, 2015 at 10:09:32PM +0900, Krzysztof Kozlowski wrote: > 2015-07-21 15:59 GMT+09:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > > @@ -540,14 +540,19 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev, > > goto dt_parse_end; > > > > for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) { > > - pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i); > > + pdata->dvs_gpio[i] = > > + devm_gpiod_get_index_optional(client_dev, "dvs", i, > > + GPIOD_OUT_LOW); > > if (IS_ERR(pdata->dvs_gpio[i])) { > > + dev_err(dev, "failed to get dvs%d gpio\n", i); > > Missing of_node_put() from of_get_child_by_name() called before. Good catch, thanks. > > @@ -561,14 +566,6 @@ dt_parse_end: > > > > static int rk808_regulator_remove(struct platform_device *pdev) > > { > > - struct rk808_regulator_data *pdata = platform_get_drvdata(pdev); > > - int i; > > - > > - for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) { > > - if (!IS_ERR(pdata->dvs_gpio[i])) > > - gpiod_put(pdata->dvs_gpio[i]); > > - } > > - > > return 0; > > } > > The function looks empty so it can be removed entirely. I assumed that not having a remove function makes the device not detachable. Not sure about that. Looking at the code I found that not having a remove function can yield surprises, though. If your driver has a probe but no remove function the platform bus glue calls dev_pm_domain_attach(_dev, true); at probe time, but not dev_pm_domain_detach(_dev, true); at remove. I admit I don't know about that dev_pm_domain stuff, but it looks wrong to only have one but not the other. Greg? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-21 14:35 ` Uwe Kleine-König @ 2015-07-21 14:41 ` Mark Brown 2015-07-22 0:13 ` Krzysztof Kozlowski 0 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2015-07-21 14:41 UTC (permalink / raw) To: Uwe Kleine-König Cc: Krzysztof Kozlowski, Greg Kroah-Hartman, Linus Walleij, Liam Girdwood, linux-next, linux-kernel, Chris Zhong, kernel [-- Attachment #1: Type: text/plain, Size: 827 bytes --] On Tue, Jul 21, 2015 at 04:35:24PM +0200, Uwe Kleine-König wrote: > On Tue, Jul 21, 2015 at 10:09:32PM +0900, Krzysztof Kozlowski wrote: > > The function looks empty so it can be removed entirely. > I assumed that not having a remove function makes the device not > detachable. Not sure about that. No, of course not - the remove function is completely optional. > Looking at the code I found that not having a remove function can yield > surprises, though. If your driver has a probe but no remove function the > platform bus glue calls > dev_pm_domain_attach(_dev, true); > at probe time, but not > dev_pm_domain_detach(_dev, true); > at remove. I admit I don't know about that dev_pm_domain stuff, but it > looks wrong to only have one but not the other. Greg? That looks like a bug, yes. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-21 14:41 ` Mark Brown @ 2015-07-22 0:13 ` Krzysztof Kozlowski 2015-07-22 7:13 ` Uwe Kleine-König 0 siblings, 1 reply; 12+ messages in thread From: Krzysztof Kozlowski @ 2015-07-22 0:13 UTC (permalink / raw) To: Mark Brown, Uwe Kleine-König Cc: Greg Kroah-Hartman, Linus Walleij, Liam Girdwood, linux-next, linux-kernel, Chris Zhong, kernel, linux-pm, Rafael J. Wysocki, Ulf Hansson, Kevin Hilman On 21.07.2015 23:41, Mark Brown wrote: > On Tue, Jul 21, 2015 at 04:35:24PM +0200, Uwe Kleine-König wrote: >> On Tue, Jul 21, 2015 at 10:09:32PM +0900, Krzysztof Kozlowski wrote: > >>> The function looks empty so it can be removed entirely. > >> I assumed that not having a remove function makes the device not >> detachable. Not sure about that. > > No, of course not - the remove function is completely optional. > >> Looking at the code I found that not having a remove function can yield >> surprises, though. If your driver has a probe but no remove function the >> platform bus glue calls > >> dev_pm_domain_attach(_dev, true); > >> at probe time, but not > >> dev_pm_domain_detach(_dev, true); > >> at remove. I admit I don't know about that dev_pm_domain stuff, but it >> looks wrong to only have one but not the other. Greg? > > That looks like a bug, yes. Cc: linux-pm, Kevin, Rafael, Ulf I agree, device should be detached from domain regardless of presence of remove callback. Documentation (like Documentation/driver-model/driver.txt) does not mention that remove callback is necessary for unbinding devices. There is no sense in storing empty removal callbacks. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-22 0:13 ` Krzysztof Kozlowski @ 2015-07-22 7:13 ` Uwe Kleine-König 0 siblings, 0 replies; 12+ messages in thread From: Uwe Kleine-König @ 2015-07-22 7:13 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Mark Brown, Greg Kroah-Hartman, Linus Walleij, Liam Girdwood, linux-next, linux-kernel, Chris Zhong, kernel, linux-pm, Rafael J. Wysocki, Ulf Hansson, Kevin Hilman Hello, On Wed, Jul 22, 2015 at 09:13:51AM +0900, Krzysztof Kozlowski wrote: > On 21.07.2015 23:41, Mark Brown wrote: > > That looks like a bug, yes. > > Cc: linux-pm, Kevin, Rafael, Ulf Good idea. Note I already sent a patch to Greg that you might want to look at, too: http://article.gmane.org/gmane.linux.kernel/2001345 Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-07-22 7:13 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-21 3:29 linux-next: build failure after merge of the gpio tree Stephen Rothwell 2015-07-21 6:59 ` [PATCH] regulator: rk808: make better use of the gpiod API Uwe Kleine-König 2015-07-21 9:56 ` Linus Walleij 2015-07-21 11:04 ` Mark Brown 2015-07-21 14:21 ` Uwe Kleine-König [not found] ` <1437489985-1362-1-git-send-email-uwe@kleine-koenig.org> 2015-07-21 14:46 ` [PATCH v2 2/2] " Uwe Kleine-König 2015-07-22 2:21 ` Krzysztof Kozlowski 2015-07-21 13:09 ` [PATCH] " Krzysztof Kozlowski 2015-07-21 14:35 ` Uwe Kleine-König 2015-07-21 14:41 ` Mark Brown 2015-07-22 0:13 ` Krzysztof Kozlowski 2015-07-22 7:13 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).