| Message ID | 20180514080640.12515-11-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | 0369e02b75e6381f892e3bd45f1d8d6330d855fb |
| Headers | show |
| Series | Refactor fixed and GPIO regulators | expand |
On Mon, May 14, 2018 at 10:06 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > Instead of passing a global GPIO number for the enable GPIO, pass > a descriptor looked up with the standard devm_gpiod_get_optional() > call. > > This regulator supports passing platform data, but enable/sleep > regulators are looked up from the device tree exclusively, so > we can need not touch other files. > > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Sangbeom Kim <sbkim73@samsung.com> > Cc: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v2->v3: > - Resending. > ChangeLog v1->v2: > - Rebase the patch on the other changes. > --- > drivers/regulator/s2mps11.c | 46 ++++++++++++++++++------------------- > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c > index 7726b874e539..9a1dca26362e 100644 > --- a/drivers/regulator/s2mps11.c > +++ b/drivers/regulator/s2mps11.c > @@ -18,7 +18,7 @@ > > #include <linux/bug.h> > #include <linux/err.h> > -#include <linux/gpio.h> > +#include <linux/gpio/consumer.h> > #include <linux/slab.h> > #include <linux/module.h> > #include <linux/of.h> > @@ -27,7 +27,6 @@ > #include <linux/regulator/driver.h> > #include <linux/regulator/machine.h> > #include <linux/regulator/of_regulator.h> > -#include <linux/of_gpio.h> > #include <linux/mfd/samsung/core.h> > #include <linux/mfd/samsung/s2mps11.h> > #include <linux/mfd/samsung/s2mps13.h> > @@ -57,7 +56,7 @@ struct s2mps11_info { > * Array (size: number of regulators) with GPIO-s for external > * sleep control. > */ > - int *ext_control_gpio; > + struct gpio_desc **ext_control_gpiod; > }; > > static int get_ramp_delay(int ramp_delay) > @@ -524,7 +523,7 @@ static int s2mps14_regulator_enable(struct regulator_dev *rdev) > case S2MPS14X: > if (test_bit(rdev_get_id(rdev), s2mps11->suspend_state)) > val = S2MPS14_ENABLE_SUSPEND; > - else if (gpio_is_valid(s2mps11->ext_control_gpio[rdev_get_id(rdev)])) > + else if (s2mps11->ext_control_gpiod[rdev_get_id(rdev)]) > val = S2MPS14_ENABLE_EXT_CONTROL; > else > val = rdev->desc->enable_mask; > @@ -818,7 +817,7 @@ static int s2mps14_pmic_enable_ext_control(struct s2mps11_info *s2mps11, > static void s2mps14_pmic_dt_parse_ext_control_gpio(struct platform_device *pdev, > struct of_regulator_match *rdata, struct s2mps11_info *s2mps11) > { > - int *gpio = s2mps11->ext_control_gpio; > + struct gpio_desc **gpio = s2mps11->ext_control_gpiod; > unsigned int i; > unsigned int valid_regulators[3] = { S2MPS14_LDO10, S2MPS14_LDO11, > S2MPS14_LDO12 }; > @@ -829,11 +828,20 @@ static void s2mps14_pmic_dt_parse_ext_control_gpio(struct platform_device *pdev, > if (!rdata[reg].init_data || !rdata[reg].of_node) > continue; > > - gpio[reg] = of_get_named_gpio(rdata[reg].of_node, > - "samsung,ext-control-gpios", 0); > - if (gpio_is_valid(gpio[reg])) > - dev_dbg(&pdev->dev, "Using GPIO %d for ext-control over %d/%s\n", > - gpio[reg], reg, rdata[reg].name); > + gpio[reg] = devm_gpiod_get_from_of_node(&pdev->dev, > + rdata[reg].of_node, > + "samsung,ext-control-gpios", > + 0, > + GPIOD_OUT_HIGH, > + "s2mps11-LDO"); Now I noticed that the name is okay because GPIO control can be used only for LDOs. No need to change: Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Tested on Odroid XU3 (with s2mps11 although not using any GPIOs for regulators, so at least default paths are not broken): Tested-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
On Mon, May 14, 2018 at 10:06:31AM +0200, Linus Walleij wrote: > Instead of passing a global GPIO number for the enable GPIO, pass > a descriptor looked up with the standard devm_gpiod_get_optional() > call. > > This regulator supports passing platform data, but enable/sleep > regulators are looked up from the device tree exclusively, so > we can need not touch other files. This seems to have broken the boot on Odroid XU3 so I'm going to revert it: https://storage.kernelci.org/next/master/v4.17-rc6-9523-g47b9cef0672d/arm/multi_v7_defconfig/lab-baylibre-seattle/boot-exynos5422-odroidxu3.html
On Sat, May 26, 2018 at 12:02 PM, Mark Brown <broonie@kernel.org> wrote: > On Mon, May 14, 2018 at 10:06:31AM +0200, Linus Walleij wrote: >> Instead of passing a global GPIO number for the enable GPIO, pass >> a descriptor looked up with the standard devm_gpiod_get_optional() >> call. >> >> This regulator supports passing platform data, but enable/sleep >> regulators are looked up from the device tree exclusively, so >> we can need not touch other files. > > This seems to have broken the boot on Odroid XU3 so I'm going to revert > it: > > https://storage.kernelci.org/next/master/v4.17-rc6-9523-g47b9cef0672d/arm/multi_v7_defconfig/lab-baylibre-seattle/boot-exynos5422-odroidxu3.html How annoying. I will check with a colleague who might have this board so I can test it on hardware. Yours, Linus Walleij
Hi Linus, On Monday, May 28, 2018 10:41:31 AM Linus Walleij wrote: > On Sat, May 26, 2018 at 12:02 PM, Mark Brown <broonie@kernel.org> wrote: > > On Mon, May 14, 2018 at 10:06:31AM +0200, Linus Walleij wrote: > >> Instead of passing a global GPIO number for the enable GPIO, pass > >> a descriptor looked up with the standard devm_gpiod_get_optional() > >> call. > >> > >> This regulator supports passing platform data, but enable/sleep > >> regulators are looked up from the device tree exclusively, so > >> we can need not touch other files. > > > > This seems to have broken the boot on Odroid XU3 so I'm going to revert > > it: > > > > https://storage.kernelci.org/next/master/v4.17-rc6-9523-g47b9cef0672d/arm/multi_v7_defconfig/lab-baylibre-seattle/boot-exynos5422-odroidxu3.html > > How annoying. I will check with a colleague who might have this > board so I can test it on hardware. I've reproduced the problem on TM2e board and the patch below fixes it (old code always initialized the structure, new code does it only in case GPIO properties are provided). I've also tested the new code (with fixup) on Artik5 board (which actually uses GPIO properties) and discovered the other problem, the GPIO core code doesn't handle shared GPIOs which are used by many platforms. Old code: [ 1.094950] s2mps11-pmic s2mps14-regulator: Using GPIO 21 for ext-control over 10/LDO11 [ 1.095210] s2mps11-pmic s2mps14-regulator: Using GPIO 21 for ext-control over 11/LDO12 New code (with fixup): [ 1.114288] s2mps11-pmic s2mps14-regulator: Using GPIO for ext-control over 10/LDO11 [ 1.143209] s2mps11-pmic s2mps14-regulator: Failed to get control GPIO for 11/LDO12 [ It fails with -EBUSY on gpiod_request() in gpiod_get_from_of_node(). ] Therefore it seems that more work is needed before s2mps11 driver can be converted to use GPIO descriptors. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics Index: b/drivers/regulator/s2mps11.c =================================================================== --- a/drivers/regulator/s2mps11.c 2018-05-28 10:53:58.121322472 +0200 +++ b/drivers/regulator/s2mps11.c 2018-05-28 11:33:38.613382421 +0200 @@ -1147,7 +1147,7 @@ static int s2mps11_pmic_probe(struct pla return -EINVAL; } - s2mps11->ext_control_gpiod = devm_kmalloc(&pdev->dev, + s2mps11->ext_control_gpiod = devm_kzalloc(&pdev->dev, sizeof(*s2mps11->ext_control_gpiod) * rdev_num, GFP_KERNEL); if (!s2mps11->ext_control_gpiod)
On Mon, May 28, 2018 at 2:29 PM, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > - s2mps11->ext_control_gpiod = devm_kmalloc(&pdev->dev, > + s2mps11->ext_control_gpiod = devm_kzalloc(&pdev->dev, > sizeof(*s2mps11->ext_control_gpiod) * rdev_num, > GFP_KERNEL); devm_kcalloc() I would rather use. -- With Best Regards, Andy Shevchenko
On Mon, May 28, 2018 at 01:29:07PM +0200, Bartlomiej Zolnierkiewicz wrote: > I've also tested the new code (with fixup) on Artik5 board (which > actually uses GPIO properties) and discovered the other problem, > the GPIO core code doesn't handle shared GPIOs which are used by > many platforms. If the GPIO code is moved to use the regulator API GPIO support that should hopefully deal with it (or we have a lot more boards with problems).
On Mon, May 28, 2018 at 2:26 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, May 28, 2018 at 2:29 PM, Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> wrote: > >> - s2mps11->ext_control_gpiod = devm_kmalloc(&pdev->dev, >> + s2mps11->ext_control_gpiod = devm_kzalloc(&pdev->dev, >> sizeof(*s2mps11->ext_control_gpiod) * rdev_num, >> GFP_KERNEL); > > devm_kcalloc() I would rather use. The patch replace kmalloc with kzalloc amd kcalloc does not initialize memory to zero. We don't have demv_kzcalloc() I'm afraid, I wonder it even makes sense to add? Yours, Linus Walleij
On Monday, May 28, 2018 01:29:07 PM Bartlomiej Zolnierkiewicz wrote: > > Hi Linus, > > On Monday, May 28, 2018 10:41:31 AM Linus Walleij wrote: > > On Sat, May 26, 2018 at 12:02 PM, Mark Brown <broonie@kernel.org> wrote: > > > On Mon, May 14, 2018 at 10:06:31AM +0200, Linus Walleij wrote: > > >> Instead of passing a global GPIO number for the enable GPIO, pass > > >> a descriptor looked up with the standard devm_gpiod_get_optional() > > >> call. > > >> > > >> This regulator supports passing platform data, but enable/sleep > > >> regulators are looked up from the device tree exclusively, so > > >> we can need not touch other files. > > > > > > This seems to have broken the boot on Odroid XU3 so I'm going to revert > > > it: > > > > > > https://storage.kernelci.org/next/master/v4.17-rc6-9523-g47b9cef0672d/arm/multi_v7_defconfig/lab-baylibre-seattle/boot-exynos5422-odroidxu3.html > > > > How annoying. I will check with a colleague who might have this > > board so I can test it on hardware. > > I've reproduced the problem on TM2e board and the patch below fixes > it (old code always initialized the structure, new code does it only > in case GPIO properties are provided). > > I've also tested the new code (with fixup) on Artik5 board (which > actually uses GPIO properties) and discovered the other problem, > the GPIO core code doesn't handle shared GPIOs which are used by > many platforms. > > Old code: > [ 1.094950] s2mps11-pmic s2mps14-regulator: Using GPIO 21 for ext-control over 10/LDO11 > [ 1.095210] s2mps11-pmic s2mps14-regulator: Using GPIO 21 for ext-control over 11/LDO12 > > New code (with fixup): > [ 1.114288] s2mps11-pmic s2mps14-regulator: Using GPIO for ext-control over 10/LDO11 > [ 1.143209] s2mps11-pmic s2mps14-regulator: Failed to get control GPIO for 11/LDO12 > > [ It fails with -EBUSY on gpiod_request() in gpiod_get_from_of_node(). ] > > Therefore it seems that more work is needed before s2mps11 driver > can be converted to use GPIO descriptors. The similar issue with shared GPIOs seems to be present in commit c89c00e2b8f0 ("regulator: max77686: Pass descriptor instead of GPIO number") and Midas board (arch/arm/boot/dts/exynos4412-midas.dtsi). I don't have the board itself to test it but from the code audit it seems that commit c89c00e2b8f0 should also be reverted. Marek has also discovered separate problem on Trats2 board with commit 3c6b38d45fa5 ("regulator: wm8994: Pass descriptor instead of GPIO number"): [ 2.278330] wm8994 4-001a: Failed to get supply 'DBVDD1': -517 [ 2.282773] wm8994 4-001a: Failed to get supplies: -517 [ 2.291919] ------------[ cut here ]------------ [ 2.295210] WARNING: CPU: 3 PID: 1 at drivers/gpio/gpiolib.c:2399 release_nodes+0x178/0x1fc [ 2.303562] Modules linked in: [ 2.306483] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.17.0-rc7-next-20180530-00001-g06c3288f5beb #811 [ 2.315920] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) [ 2.321975] [<c011313c>] (unwind_backtrace) from [<c010ed20>] (show_stack+0x10/0x14) [ 2.329715] [<c010ed20>] (show_stack) from [<c0a3c070>] (dump_stack+0x98/0xc4) [ 2.336913] [<c0a3c070>] (dump_stack) from [<c0127a50>] (__warn+0x10c/0x124) [ 2.343935] [<c0127a50>] (__warn) from [<c0127b7c>] (warn_slowpath_null+0x40/0x48) [ 2.351495] [<c0127b7c>] (warn_slowpath_null) from [<c057e564>] (release_nodes+0x178/0x1fc) [ 2.359838] [<c057e564>] (release_nodes) from [<c0579dd8>] (driver_probe_device+0xe8/0x4a4) [ 2.368169] [<c0579dd8>] (driver_probe_device) from [<c057a298>] (__driver_attach+0x104/0x120) [ 2.376765] [<c057a298>] (__driver_attach) from [<c0577ef8>] (bus_for_each_dev+0x68/0xb4) [ 2.384920] [<c0577ef8>] (bus_for_each_dev) from [<c057915c>] (bus_add_driver+0x1a8/0x268) [ 2.393166] [<c057915c>] (bus_add_driver) from [<c057b3ec>] (driver_register+0x78/0x10c) [ 2.401243] [<c057b3ec>] (driver_register) from [<c06ce0b4>] (i2c_register_driver+0x3c/0xa8) [ 2.409664] [<c06ce0b4>] (i2c_register_driver) from [<c0103188>] (do_one_initcall+0x8c/0x4b0) [ 2.418172] [<c0103188>] (do_one_initcall) from [<c0f01268>] (kernel_init_freeable+0x3e4/0x570) [ 2.426854] [<c0f01268>] (kernel_init_freeable) from [<c0a52418>] (kernel_init+0x8/0x10c) [ 2.435003] [<c0a52418>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20) [ 2.442542] Exception stack(0xef0bbfb0 to 0xef0bbff8) [ 2.447549] bfa0: 00000000 00000000 00000000 00000000 [ 2.455745] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 2.463903] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 2.470568] irq event stamp: 331409 [ 2.474034] hardirqs last enabled at (331493): [<c01935a4>] console_unlock+0x4a4/0x660 [ 2.481962] hardirqs last disabled at (331500): [<c01931cc>] console_unlock+0xcc/0x660 [ 2.489923] softirqs last enabled at (331530): [<c01023e0>] __do_softirq+0x2b8/0x650 [ 2.497733] softirqs last disabled at (331541): [<c0130068>] irq_exit+0x158/0x164 [ 2.505190] ---[ end trace 54f70738f2594ecf ]--- [ Reverting the commit in question fixes the issue. ] Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On Wed, May 30, 2018 at 03:44:08PM +0200, Bartlomiej Zolnierkiewicz wrote: > The similar issue with shared GPIOs seems to be present in commit > c89c00e2b8f0 ("regulator: max77686: Pass descriptor instead of GPIO > number") and Midas board (arch/arm/boot/dts/exynos4412-midas.dtsi). > I don't have the board itself to test it but from the code audit it > seems that commit c89c00e2b8f0 should also be reverted. > Marek has also discovered separate problem on Trats2 board with > commit 3c6b38d45fa5 ("regulator: wm8994: Pass descriptor instead of > GPIO number"): I've reverted both.
diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c index 7726b874e539..9a1dca26362e 100644 --- a/drivers/regulator/s2mps11.c +++ b/drivers/regulator/s2mps11.c @@ -18,7 +18,7 @@ #include <linux/bug.h> #include <linux/err.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/slab.h> #include <linux/module.h> #include <linux/of.h> @@ -27,7 +27,6 @@ #include <linux/regulator/driver.h> #include <linux/regulator/machine.h> #include <linux/regulator/of_regulator.h> -#include <linux/of_gpio.h> #include <linux/mfd/samsung/core.h> #include <linux/mfd/samsung/s2mps11.h> #include <linux/mfd/samsung/s2mps13.h> @@ -57,7 +56,7 @@ struct s2mps11_info { * Array (size: number of regulators) with GPIO-s for external * sleep control. */ - int *ext_control_gpio; + struct gpio_desc **ext_control_gpiod; }; static int get_ramp_delay(int ramp_delay) @@ -524,7 +523,7 @@ static int s2mps14_regulator_enable(struct regulator_dev *rdev) case S2MPS14X: if (test_bit(rdev_get_id(rdev), s2mps11->suspend_state)) val = S2MPS14_ENABLE_SUSPEND; - else if (gpio_is_valid(s2mps11->ext_control_gpio[rdev_get_id(rdev)])) + else if (s2mps11->ext_control_gpiod[rdev_get_id(rdev)]) val = S2MPS14_ENABLE_EXT_CONTROL; else val = rdev->desc->enable_mask; @@ -818,7 +817,7 @@ static int s2mps14_pmic_enable_ext_control(struct s2mps11_info *s2mps11, static void s2mps14_pmic_dt_parse_ext_control_gpio(struct platform_device *pdev, struct of_regulator_match *rdata, struct s2mps11_info *s2mps11) { - int *gpio = s2mps11->ext_control_gpio; + struct gpio_desc **gpio = s2mps11->ext_control_gpiod; unsigned int i; unsigned int valid_regulators[3] = { S2MPS14_LDO10, S2MPS14_LDO11, S2MPS14_LDO12 }; @@ -829,11 +828,20 @@ static void s2mps14_pmic_dt_parse_ext_control_gpio(struct platform_device *pdev, if (!rdata[reg].init_data || !rdata[reg].of_node) continue; - gpio[reg] = of_get_named_gpio(rdata[reg].of_node, - "samsung,ext-control-gpios", 0); - if (gpio_is_valid(gpio[reg])) - dev_dbg(&pdev->dev, "Using GPIO %d for ext-control over %d/%s\n", - gpio[reg], reg, rdata[reg].name); + gpio[reg] = devm_gpiod_get_from_of_node(&pdev->dev, + rdata[reg].of_node, + "samsung,ext-control-gpios", + 0, + GPIOD_OUT_HIGH, + "s2mps11-LDO"); + if (IS_ERR(gpio[reg])) { + dev_err(&pdev->dev, "Failed to get control GPIO for %d/%s\n", + reg, rdata[reg].name); + continue; + } + if (gpio[reg]) + dev_dbg(&pdev->dev, "Using GPIO for ext-control over %d/%s\n", + reg, rdata[reg].name); } } @@ -1139,17 +1147,11 @@ static int s2mps11_pmic_probe(struct platform_device *pdev) return -EINVAL; } - s2mps11->ext_control_gpio = devm_kmalloc(&pdev->dev, - sizeof(*s2mps11->ext_control_gpio) * rdev_num, + s2mps11->ext_control_gpiod = devm_kmalloc(&pdev->dev, + sizeof(*s2mps11->ext_control_gpiod) * rdev_num, GFP_KERNEL); - if (!s2mps11->ext_control_gpio) + if (!s2mps11->ext_control_gpiod) return -ENOMEM; - /* - * 0 is a valid GPIO so initialize all GPIO-s to negative value - * to indicate that external control won't be used for this regulator. - */ - for (i = 0; i < rdev_num; i++) - s2mps11->ext_control_gpio[i] = -EINVAL; if (!iodev->dev->of_node) { if (iodev->pdata) { @@ -1179,8 +1181,6 @@ static int s2mps11_pmic_probe(struct platform_device *pdev) config.dev = &pdev->dev; config.regmap = iodev->regmap_pmic; config.driver_data = s2mps11; - config.ena_gpio_flags = GPIOF_OUT_INIT_HIGH; - config.ena_gpio_initialized = true; for (i = 0; i < rdev_num; i++) { struct regulator_dev *regulator; @@ -1191,7 +1191,7 @@ static int s2mps11_pmic_probe(struct platform_device *pdev) config.init_data = rdata[i].init_data; config.of_node = rdata[i].of_node; } - config.ena_gpio = s2mps11->ext_control_gpio[i]; + config.ena_gpiod = s2mps11->ext_control_gpiod[i]; regulator = devm_regulator_register(&pdev->dev, ®ulators[i], &config); @@ -1202,7 +1202,7 @@ static int s2mps11_pmic_probe(struct platform_device *pdev) goto out; } - if (gpio_is_valid(s2mps11->ext_control_gpio[i])) { + if (s2mps11->ext_control_gpiod[i]) { ret = s2mps14_pmic_enable_ext_control(s2mps11, regulator); if (ret < 0) {
Instead of passing a global GPIO number for the enable GPIO, pass a descriptor looked up with the standard devm_gpiod_get_optional() call. This regulator supports passing platform data, but enable/sleep regulators are looked up from the device tree exclusively, so we can need not touch other files. Cc: Krzysztof Kozlowski <krzk@kernel.org> Cc: Sangbeom Kim <sbkim73@samsung.com> Cc: Chanwoo Choi <cw00.choi@samsung.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v2->v3: - Resending. ChangeLog v1->v2: - Rebase the patch on the other changes. --- drivers/regulator/s2mps11.c | 46 ++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 23 deletions(-) -- 2.17.0