* 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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 2015-07-21 14:46 ` [PATCH 1/2] regulator: rk808: add #include for gpiod functions Uwe Kleine-König 0 siblings, 1 reply; 15+ 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] 15+ messages in thread
* [PATCH 1/2] regulator: rk808: add #include for gpiod functions 2015-07-21 14:21 ` Uwe Kleine-König @ 2015-07-21 14:46 ` Uwe Kleine-König 2015-07-21 14:46 ` [PATCH v2 2/2] regulator: rk808: make better use of the gpiod API Uwe Kleine-König 0 siblings, 1 reply; 15+ messages in thread From: Uwe Kleine-König @ 2015-07-21 14:46 UTC (permalink / raw) To: Mark Brown, Liam Girdwood Cc: linux-kernel, Doug Anderson, Chris Zhong, kernel This fixes a build problem on mips found by the kbuild test robot: drivers/regulator/rk808-regulator.c: In function 'rk808_buck1_2_get_voltage_sel_regmap': drivers/regulator/rk808-regulator.c:97:2: error: implicit declaration of function 'gpiod_get_value' [-Werror=implicit-function-declaration] if (IS_ERR(gpio) || gpiod_get_value(gpio) == 0) ^ Fixes: bad47ad2eef3 ("regulator: rk808: fixed the overshoot when adjust voltage") Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- drivers/regulator/rk808-regulator.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c index ca913fd15598..ac9436d2ea8d 100644 --- a/drivers/regulator/rk808-regulator.c +++ b/drivers/regulator/rk808-regulator.c @@ -25,6 +25,7 @@ #include <linux/mfd/rk808.h> #include <linux/regulator/driver.h> #include <linux/regulator/of_regulator.h> +#include <linux/gpio/consumer.h> /* Field Definitions */ #define RK808_BUCK_VSEL_MASK 0x3f -- 2.1.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] regulator: rk808: make better use of the gpiod API 2015-07-21 14:46 ` [PATCH 1/2] regulator: rk808: add #include for gpiod functions Uwe Kleine-König @ 2015-07-21 14:46 ` Uwe Kleine-König 2015-07-22 2:21 ` Krzysztof Kozlowski 0 siblings, 1 reply; 15+ 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] 15+ 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] regulator: rk808: make better use of the gpiod API Uwe Kleine-König @ 2015-07-22 2:21 ` Krzysztof Kozlowski 0 siblings, 0 replies; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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-21 15:08 ` [PATCH] base/platform: assert that dev_pm_domain callbacks are called unconditionally Uwe Kleine-König 2015-07-22 0:13 ` [PATCH] regulator: rk808: make better use of the gpiod API Krzysztof Kozlowski 0 siblings, 2 replies; 15+ 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] 15+ messages in thread
* [PATCH] base/platform: assert that dev_pm_domain callbacks are called unconditionally 2015-07-21 14:41 ` Mark Brown @ 2015-07-21 15:08 ` Uwe Kleine-König 2015-08-06 0:06 ` Greg Kroah-Hartman 2015-07-22 0:13 ` [PATCH] regulator: rk808: make better use of the gpiod API Krzysztof Kozlowski 1 sibling, 1 reply; 15+ messages in thread From: Uwe Kleine-König @ 2015-07-21 15:08 UTC (permalink / raw) To: Greg Kroah-Hartman, Mark Brown; +Cc: linux-kernel, kernel When a platform driver doesn't provide a .remove callback the function platform_drv_remove isn't called and so the call to dev_pm_domain_attach called at probe time isn't paired by dev_pm_domain_detach at remove time. To fix this (and similar issues if different callbacks are missing) hook up the driver callbacks unconditionally and make them aware that the platform callbacks might be missing. Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- Hello, I didn't see any breakage without this patch, but it looks wrong the way it is. Best regards Uwe drivers/base/platform.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 063f0ab15259..62debf4a1348 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -517,7 +517,7 @@ static int platform_drv_probe(struct device *_dev) return ret; ret = dev_pm_domain_attach(_dev, true); - if (ret != -EPROBE_DEFER) { + if (ret != -EPROBE_DEFER && drv->probe) { ret = drv->probe(dev); if (ret) dev_pm_domain_detach(_dev, true); @@ -542,7 +542,8 @@ static int platform_drv_remove(struct device *_dev) struct platform_device *dev = to_platform_device(_dev); int ret; - ret = drv->remove(dev); + if (drv->remove) + ret = drv->remove(dev); dev_pm_domain_detach(_dev, true); return ret; @@ -553,7 +554,8 @@ static void platform_drv_shutdown(struct device *_dev) struct platform_driver *drv = to_platform_driver(_dev->driver); struct platform_device *dev = to_platform_device(_dev); - drv->shutdown(dev); + if (drv->shutdown) + drv->shutdown(dev); dev_pm_domain_detach(_dev, true); } @@ -567,12 +569,9 @@ int __platform_driver_register(struct platform_driver *drv, { drv->driver.owner = owner; drv->driver.bus = &platform_bus_type; - if (drv->probe) - drv->driver.probe = platform_drv_probe; - if (drv->remove) - drv->driver.remove = platform_drv_remove; - if (drv->shutdown) - drv->driver.shutdown = platform_drv_shutdown; + drv->driver.probe = platform_drv_probe; + drv->driver.remove = platform_drv_remove; + drv->driver.shutdown = platform_drv_shutdown; return driver_register(&drv->driver); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] base/platform: assert that dev_pm_domain callbacks are called unconditionally 2015-07-21 15:08 ` [PATCH] base/platform: assert that dev_pm_domain callbacks are called unconditionally Uwe Kleine-König @ 2015-08-06 0:06 ` Greg Kroah-Hartman 0 siblings, 0 replies; 15+ messages in thread From: Greg Kroah-Hartman @ 2015-08-06 0:06 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Mark Brown, linux-kernel, kernel On Tue, Jul 21, 2015 at 05:08:35PM +0200, Uwe Kleine-König wrote: > When a platform driver doesn't provide a .remove callback the function > platform_drv_remove isn't called and so the call to dev_pm_domain_attach > called at probe time isn't paired by dev_pm_domain_detach at remove > time. > > To fix this (and similar issues if different callbacks are missing) hook > up the driver callbacks unconditionally and make them aware that the > platform callbacks might be missing. > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > --- > Hello, > > I didn't see any breakage without this patch, but it looks wrong the way > it is. > > Best regards > Uwe > > drivers/base/platform.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 063f0ab15259..62debf4a1348 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -517,7 +517,7 @@ static int platform_drv_probe(struct device *_dev) > return ret; > > ret = dev_pm_domain_attach(_dev, true); > - if (ret != -EPROBE_DEFER) { > + if (ret != -EPROBE_DEFER && drv->probe) { > ret = drv->probe(dev); > if (ret) > dev_pm_domain_detach(_dev, true); > @@ -542,7 +542,8 @@ static int platform_drv_remove(struct device *_dev) > struct platform_device *dev = to_platform_device(_dev); > int ret; > > - ret = drv->remove(dev); > + if (drv->remove) > + ret = drv->remove(dev); > dev_pm_domain_detach(_dev, true); This causes a build warning :( Please fix. thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-21 14:41 ` Mark Brown 2015-07-21 15:08 ` [PATCH] base/platform: assert that dev_pm_domain callbacks are called unconditionally Uwe Kleine-König @ 2015-07-22 0:13 ` Krzysztof Kozlowski 2015-07-22 7:13 ` Uwe Kleine-König 1 sibling, 1 reply; 15+ 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] 15+ messages in thread
* Re: [PATCH] regulator: rk808: make better use of the gpiod API 2015-07-22 0:13 ` [PATCH] regulator: rk808: make better use of the gpiod API Krzysztof Kozlowski @ 2015-07-22 7:13 ` Uwe Kleine-König 0 siblings, 0 replies; 15+ 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] 15+ messages in thread
end of thread, other threads:[~2015-08-06 0:06 UTC | newest] Thread overview: 15+ 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 2015-07-21 14:46 ` [PATCH 1/2] regulator: rk808: add #include for gpiod functions Uwe Kleine-König 2015-07-21 14:46 ` [PATCH v2 2/2] regulator: rk808: make better use of the gpiod API 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-21 15:08 ` [PATCH] base/platform: assert that dev_pm_domain callbacks are called unconditionally Uwe Kleine-König 2015-08-06 0:06 ` Greg Kroah-Hartman 2015-07-22 0:13 ` [PATCH] regulator: rk808: make better use of the gpiod API 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).