diff mbox series

regulator: s5m8767: Convert to GPIO descriptors

Message ID 20250318052709.1731747-1-peng.fan@oss.nxp.com (mailing list archive)
State New
Headers show
Series regulator: s5m8767: Convert to GPIO descriptors | expand

Commit Message

Peng Fan (OSS) March 18, 2025, 5:27 a.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

Update the driver to fetch buck_gpio and buck_ds as gpio descriptors.
Then drop the usage of 'of_gpio.h' which should be deprecated.
Take commit 84618d5e31cf ("regulator: max8997:
Convert to GPIO descriptors") as a reference to make the changes.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V1:
 Not have device to test, just do my best pratice to make the changes.

 drivers/regulator/s5m8767.c | 149 ++++++++++--------------------------
 1 file changed, 41 insertions(+), 108 deletions(-)

Comments

Andy Shevchenko March 18, 2025, 10:21 a.m. UTC | #1 
On Tue, Mar 18, 2025 at 01:27:09PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Update the driver to fetch buck_gpio and buck_ds as gpio descriptors.

GPIO

> Then drop the usage of 'of_gpio.h' which should be deprecated.

s/should/is/

> Take commit 84618d5e31cf ("regulator: max8997:

s/Take/Based on/

> Convert to GPIO descriptors") as a reference to make the changes.

> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>

Can be done via --to parameter to git-send-email.

...

> +	gpiod_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);

Can be simply done as !!(temp_index & BIT(2)).

> +	gpiod_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
> +	gpiod_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);

Ditto.

...

> +	gpiod_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
> +	gpiod_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
> +	gpiod_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);

As per above.

...

Also the commit message doesn't tell anything about the existing DTS files.
Do we have this device described in any in the kernel? Do we have any googled
examples? Why I'm asking because often the issue is the incorrect setting of
the polarity, which needs to be carefully checked, esp. for the voltage regulators
case.

...

> +	const char *gpiods_names[3] = {"S5M8767 DS2", "S5M8767 DS3", "S5M8767 DS4"};
> +	const char *gpiodvs_names[3] = {"S5M8767 SET1", "S5M8767 SET2", "S5M8767 SET3"};

Add spaces after { and before }.

...

> +		for (i = 0; i < 3; i++) {
> +			enum gpiod_flags flags;
>  
> +			if (s5m8767->buck_gpioindex & BIT(2 - i))
> +				flags = GPIOD_OUT_HIGH;
> +			else
> +				flags = GPIOD_OUT_LOW;
> +
> +			s5m8767->buck_gpios[i] = devm_gpiod_get_index(iodev->dev,
> +								      "s5m8767,pmic-buck-dvs",
> +								      i,
> +								      flags);

i and flags can be located on the same line, or I would rather move i to
the line with the con_id.

			s5m8767->buck_gpios[i] = devm_gpiod_get_index(iodev->dev,
								      "s5m8767,pmic-buck-dvs", i,
								      flags);

> +			if (IS_ERR(s5m8767->buck_gpios[i])) {
> +				ret = PTR_ERR(s5m8767->buck_gpios[i]);
> +				return dev_err_probe(iodev->dev, ret, "invalid gpio[%d]: %d\n",
> +						     i, ret);

ret will be printed twice. This should be as simple as

			if (IS_ERR(s5m8767->buck_gpios[i]))
				return dev_err_probe(iodev->dev, PTR_ERR(s5m8767->buck_gpios[i]),
						     "invalid gpio[%d]\n", i);

> +			}
>  
> +			gpiod_set_consumer_name(s5m8767->buck_gpios[i], gpiodvs_names[i]);
> +		}

...

> +	for (i = 0; i < 3; i++) {

Both comments as per above apply here, in this for-loop.

> +		s5m8767->buck_ds[i] = devm_gpiod_get_index(iodev->dev,
> +							   "s5m8767,pmic-buck-ds",
> +							   i, GPIOD_OUT_LOW);
> +		if (IS_ERR(s5m8767->buck_ds[i])) {
> +			ret = PTR_ERR(s5m8767->buck_ds[i]);
> +			return dev_err_probe(iodev->dev, ret, "can't get GPIO %d (%d)\n",
> +					     i, ret);
> +		}
> +		gpiod_set_consumer_name(s5m8767->buck_ds[i], gpiods_names[i]);
> +	}
Peng Fan March 18, 2025, 12:38 p.m. UTC | #2 
Hi Andy,

> Subject: Re: [PATCH] regulator: s5m8767: Convert to GPIO descriptors
> 
...

> > +	gpiod_set_value(s5m8767->buck_gpios[0], (temp_index >> 2)
> & 0x1);
> 
> Can be simply done as !!(temp_index & BIT(2)).
> 
> > +	gpiod_set_value(s5m8767->buck_gpios[1], (temp_index >> 1)
> & 0x1);
> > +	gpiod_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
> 
> Ditto.
> 
> ...
> 
> > +	gpiod_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
> > +	gpiod_set_value(s5m8767->buck_gpios[1], (temp_index >> 1)
> & 0x1);
> > +	gpiod_set_value(s5m8767->buck_gpios[0], (temp_index >> 2)
> & 0x1);
> 
> As per above.
> 
> ...
> 
> Also the commit message doesn't tell anything about the existing DTS
> files.
> Do we have this device described in any in the kernel? Do we have any
> googled examples? Why I'm asking because often the issue is the
> incorrect setting of the polarity, which needs to be carefully checked,
> esp. for the voltage regulators case.


Under arch/arm/boot/dts/samsung/, a few dtsi files have the property 
with results from output of
`grep "s5m8767" ./arch/arm/boot/dts/samsung/ -rn | grep gpios`

Exynos5250-spring.dts uses GPIO_ACTIVE_LOW.
Others use GPIO_ACTIVE_HIGH.

The current changing to using GPIO descriptors should be ok per
my understanding.

Not able to find any public datasheet for this pmic (:

> 
> ...
> 
> 
Other non-replied comments are accepted, I will address them in v2.

Thanks,
Peng.
Andy Shevchenko March 18, 2025, 1:42 p.m. UTC | #3 
On Tue, Mar 18, 2025 at 12:38:15PM +0000, Peng Fan wrote:

...

> > Also the commit message doesn't tell anything about the existing DTS
> > files.
> > Do we have this device described in any in the kernel? Do we have any
> > googled examples? Why I'm asking because often the issue is the
> > incorrect setting of the polarity, which needs to be carefully checked,
> > esp. for the voltage regulators case.
> 
> Under arch/arm/boot/dts/samsung/, a few dtsi files have the property 
> with results from output of
> `grep "s5m8767" ./arch/arm/boot/dts/samsung/ -rn | grep gpios`

Side note: `git grep -n s5m8767` is a better command. You can just look for the
exact GPIOs, usually not so bit amount of them, or do it recursively with

`git grep -n 'gpios' -- $(git grep -lw s5m8767 -- Documentation/devicetree/bindings)`

> Exynos5250-spring.dts uses GPIO_ACTIVE_LOW.
> Others use GPIO_ACTIVE_HIGH.

So, one of this needs to be fixed.

> The current changing to using GPIO descriptors should be ok per
> my understanding.
> 
> Not able to find any public datasheet for this pmic (:
Krzysztof Kozlowski March 18, 2025, 1:48 p.m. UTC | #4 
On 18/03/2025 13:38, Peng Fan wrote:
>> Also the commit message doesn't tell anything about the existing DTS
>> files.
>> Do we have this device described in any in the kernel? Do we have any
>> googled examples? Why I'm asking because often the issue is the
>> incorrect setting of the polarity, which needs to be carefully checked,
>> esp. for the voltage regulators case.
> 
> 
> Under arch/arm/boot/dts/samsung/, a few dtsi files have the property 
> with results from output of
> `grep "s5m8767" ./arch/arm/boot/dts/samsung/ -rn | grep gpios`
> 
> Exynos5250-spring.dts uses GPIO_ACTIVE_LOW.
> Others use GPIO_ACTIVE_HIGH.
> 
These are old devices and not many people are actually providing tests,
so you need to preserve existing ABI. IOW, if previously GPIO flags were
ignored, meaning "1" is ACTIVE_HIGH, then you must preserve this behavior.

Best regards,
Krzysztof
Peng Fan (OSS) March 24, 2025, 4:21 a.m. UTC | #5 
On Tue, Mar 18, 2025 at 02:48:05PM +0100, Krzysztof Kozlowski wrote:
>On 18/03/2025 13:38, Peng Fan wrote:
>>> Also the commit message doesn't tell anything about the existing DTS
>>> files.
>>> Do we have this device described in any in the kernel? Do we have any
>>> googled examples? Why I'm asking because often the issue is the
>>> incorrect setting of the polarity, which needs to be carefully checked,
>>> esp. for the voltage regulators case.
>> 
>> 
>> Under arch/arm/boot/dts/samsung/, a few dtsi files have the property 
>> with results from output of
>> `grep "s5m8767" ./arch/arm/boot/dts/samsung/ -rn | grep gpios`
>> 
>> Exynos5250-spring.dts uses GPIO_ACTIVE_LOW.
>> Others use GPIO_ACTIVE_HIGH.
>> 
>These are old devices and not many people are actually providing tests,
>so you need to preserve existing ABI. IOW, if previously GPIO flags were
>ignored, meaning "1" is ACTIVE_HIGH, then you must preserve this behavior.

Per google,
Manual Reset function is for Hardware reset in the Active mode.
If MR1B and MR2B is kept low during the VLDO3 is active state, the
system makes all internal presetting registers as default in the
active mode (automatic power on sequence). If this hardware reset
function is not required, connect MRB pin to high.

So the reset is ACTIVE LOW if my understanding is correct.

To keep DTS unchanged, we need update polarity in gpiolib to
force GPIO_ACTIVE_LOW.

please see whether this is ok for you.

Thanks,
Peng

>
>Best regards,
>Krzysztof
Krzysztof Kozlowski March 25, 2025, 9:30 a.m. UTC | #6 
On 24/03/2025 05:21, Peng Fan wrote:
> On Tue, Mar 18, 2025 at 02:48:05PM +0100, Krzysztof Kozlowski wrote:
>> On 18/03/2025 13:38, Peng Fan wrote:
>>>> Also the commit message doesn't tell anything about the existing DTS
>>>> files.
>>>> Do we have this device described in any in the kernel? Do we have any
>>>> googled examples? Why I'm asking because often the issue is the
>>>> incorrect setting of the polarity, which needs to be carefully checked,
>>>> esp. for the voltage regulators case.
>>>
>>>
>>> Under arch/arm/boot/dts/samsung/, a few dtsi files have the property 
>>> with results from output of
>>> `grep "s5m8767" ./arch/arm/boot/dts/samsung/ -rn | grep gpios`
>>>
>>> Exynos5250-spring.dts uses GPIO_ACTIVE_LOW.
>>> Others use GPIO_ACTIVE_HIGH.
>>>
>> These are old devices and not many people are actually providing tests,
>> so you need to preserve existing ABI. IOW, if previously GPIO flags were
>> ignored, meaning "1" is ACTIVE_HIGH, then you must preserve this behavior.
> 
> Per google,
> Manual Reset function is for Hardware reset in the Active mode.

Why are you mentioning the reset functions? Which properties are these?


> If MR1B and MR2B is kept low during the VLDO3 is active state, the
> system makes all internal presetting registers as default in the
> active mode (automatic power on sequence). If this hardware reset
> function is not required, connect MRB pin to high.
> 
> So the reset is ACTIVE LOW if my understanding is correct.
> 
> To keep DTS unchanged, we need update polarity in gpiolib to
> force GPIO_ACTIVE_LOW.

How are you going to achieve it if one DTS has LOW and other has HIGH?

> 
> please see whether this is ok for you.

I don't understand how this is related to the issue I raised.

Best regards,
Krzysztof
Peng Fan March 25, 2025, 11:26 a.m. UTC | #7 
> Subject: Re: [PATCH] regulator: s5m8767: Convert to GPIO descriptors
> 
> On 24/03/2025 05:21, Peng Fan wrote:
> > On Tue, Mar 18, 2025 at 02:48:05PM +0100, Krzysztof Kozlowski
> wrote:
> >> On 18/03/2025 13:38, Peng Fan wrote:
> >>>> Also the commit message doesn't tell anything about the existing
> >>>> DTS files.
> >>>> Do we have this device described in any in the kernel? Do we
> have
> >>>> any googled examples? Why I'm asking because often the issue is
> the
> >>>> incorrect setting of the polarity, which needs to be carefully
> >>>> checked, esp. for the voltage regulators case.
> >>>
> >>>
> >>> Under arch/arm/boot/dts/samsung/, a few dtsi files have the
> property
> >>> with results from output of `grep "s5m8767"
> >>> ./arch/arm/boot/dts/samsung/ -rn | grep gpios`
> >>>
> >>> Exynos5250-spring.dts uses GPIO_ACTIVE_LOW.
> >>> Others use GPIO_ACTIVE_HIGH.
> >>>
> >> These are old devices and not many people are actually providing
> >> tests, so you need to preserve existing ABI. IOW, if previously GPIO
> >> flags were ignored, meaning "1" is ACTIVE_HIGH, then you must
> preserve this behavior.
> >
> > Per google,
> > Manual Reset function is for Hardware reset in the Active mode.
> 
> Why are you mentioning the reset functions? Which properties are
> these?

Oops, I got messed with the ASoC patches I did.


> 
> 
> > If MR1B and MR2B is kept low during the VLDO3 is active state, the
> > system makes all internal presetting registers as default in the
> > active mode (automatic power on sequence). If this hardware reset
> > function is not required, connect MRB pin to high.
> >
> > So the reset is ACTIVE LOW if my understanding is correct.
> >
> > To keep DTS unchanged, we need update polarity in gpiolib to force
> > GPIO_ACTIVE_LOW.
> 
> How are you going to achieve it if one DTS has LOW and other has
> HIGH?

With this gpiolib-of change, to fix polarity as HIGH.

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index eb667f8f1ead..aa5d112c6189 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -229,6 +229,10 @@ static void of_gpio_try_fixup_polarity(const struct device_node *np,
                 * treats it as "active low".
                 */
                { "ti,tsc2005",         "reset-gpios",  false },
+#endif
+#if IS_ENABLED(CONFIG_REGULATOR_S5M8767)
+               { "samsung,s5m8767-pmic", s5m8767,pmic-buck-dvs-gpios, false },
+               { "samsung,s5m8767-pmic", s5m8767,pmic-buck-ds-gpios, false },
 #endif
        };
        unsigned int i;

Thanks,
Peng.

> 
> >
> > please see whether this is ok for you.
> 
> I don't understand how this is related to the issue I raised.
> 
> Best regards,
> Krzysztof
Linus Walleij March 25, 2025, 1:09 p.m. UTC | #8 
On Tue, Mar 25, 2025 at 12:26 PM Peng Fan <peng.fan@nxp.com> wrote:
> > On 24/03/2025 05:21, Peng Fan wrote:
> > > On Tue, Mar 18, 2025 at 02:48:05PM +0100, Krzysztof Kozlowski

> > > To keep DTS unchanged, we need update polarity in gpiolib to force
> > > GPIO_ACTIVE_LOW.
> >
> > How are you going to achieve it if one DTS has LOW and other has
> > HIGH?
>
> With this gpiolib-of change, to fix polarity as HIGH.

Yep those quirks is what we have done to handle legacy cases,
mostly from old devicetrees using bindings where proper polarity
wasn't enforced or properly used because the drivers would
override it anyway.

Ideally the bindings schema should enforce consumers to use
GPIO_ACTIVE_LOW but I don't know if that is possible, but maybe
Krzysztof has ideas!

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index d25cd81e3f36..57a6476f217f 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -5,7 +5,7 @@ 
 
 #include <linux/cleanup.h>
 #include <linux/err.h>
-#include <linux/of_gpio.h>
+#include <linux/of.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -35,8 +35,8 @@  struct s5m8767_info {
 	u8 buck2_vol[8];
 	u8 buck3_vol[8];
 	u8 buck4_vol[8];
-	int buck_gpios[3];
-	int buck_ds[3];
+	struct gpio_desc *buck_gpios[3];
+	struct gpio_desc *buck_ds[3];
 	int buck_gpioindex;
 };
 
@@ -272,9 +272,9 @@  static inline int s5m8767_set_high(struct s5m8767_info *s5m8767)
 {
 	int temp_index = s5m8767->buck_gpioindex;
 
-	gpio_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);
-	gpio_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
-	gpio_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
+	gpiod_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);
+	gpiod_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
+	gpiod_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
 
 	return 0;
 }
@@ -283,9 +283,9 @@  static inline int s5m8767_set_low(struct s5m8767_info *s5m8767)
 {
 	int temp_index = s5m8767->buck_gpioindex;
 
-	gpio_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
-	gpio_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
-	gpio_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);
+	gpiod_set_value(s5m8767->buck_gpios[2], temp_index & 0x1);
+	gpiod_set_value(s5m8767->buck_gpios[1], (temp_index >> 1) & 0x1);
+	gpiod_set_value(s5m8767->buck_gpios[0], (temp_index >> 2) & 0x1);
 
 	return 0;
 }
@@ -482,42 +482,6 @@  static int s5m8767_enable_ext_control(struct s5m8767_info *s5m8767,
 
 
 #ifdef CONFIG_OF
-static int s5m8767_pmic_dt_parse_dvs_gpio(struct sec_pmic_dev *iodev,
-			struct sec_platform_data *pdata,
-			struct device_node *pmic_np)
-{
-	int i, gpio;
-
-	for (i = 0; i < 3; i++) {
-		gpio = of_get_named_gpio(pmic_np,
-					"s5m8767,pmic-buck-dvs-gpios", i);
-		if (!gpio_is_valid(gpio)) {
-			dev_err(iodev->dev, "invalid gpio[%d]: %d\n", i, gpio);
-			return -EINVAL;
-		}
-		pdata->buck_gpios[i] = gpio;
-	}
-	return 0;
-}
-
-static int s5m8767_pmic_dt_parse_ds_gpio(struct sec_pmic_dev *iodev,
-			struct sec_platform_data *pdata,
-			struct device_node *pmic_np)
-{
-	int i, gpio;
-
-	for (i = 0; i < 3; i++) {
-		gpio = of_get_named_gpio(pmic_np,
-					"s5m8767,pmic-buck-ds-gpios", i);
-		if (!gpio_is_valid(gpio)) {
-			dev_err(iodev->dev, "invalid gpio[%d]: %d\n", i, gpio);
-			return -EINVAL;
-		}
-		pdata->buck_ds[i] = gpio;
-	}
-	return 0;
-}
-
 static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 					struct sec_platform_data *pdata)
 {
@@ -525,7 +489,7 @@  static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 	struct device_node *pmic_np, *reg_np;
 	struct sec_regulator_data *rdata;
 	struct sec_opmode_data *rmode;
-	unsigned int i, dvs_voltage_nr = 8, ret;
+	unsigned int i, dvs_voltage_nr = 8;
 
 	pmic_np = iodev->dev->of_node;
 	if (!pmic_np) {
@@ -635,10 +599,6 @@  static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 
 	if (pdata->buck2_gpiodvs || pdata->buck3_gpiodvs ||
 						pdata->buck4_gpiodvs) {
-		ret = s5m8767_pmic_dt_parse_dvs_gpio(iodev, pdata, pmic_np);
-		if (ret)
-			return -EINVAL;
-
 		if (of_property_read_u32(pmic_np,
 				"s5m8767,pmic-buck-default-dvs-idx",
 				&pdata->buck_default_idx)) {
@@ -652,10 +612,6 @@  static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 		}
 	}
 
-	ret = s5m8767_pmic_dt_parse_ds_gpio(iodev, pdata, pmic_np);
-	if (ret)
-		return -EINVAL;
-
 	pdata->buck2_ramp_enable = of_property_read_bool(pmic_np, "s5m8767,pmic-buck2-ramp-enable");
 	pdata->buck3_ramp_enable = of_property_read_bool(pmic_np, "s5m8767,pmic-buck3-ramp-enable");
 	pdata->buck4_ramp_enable = of_property_read_bool(pmic_np, "s5m8767,pmic-buck4-ramp-enable");
@@ -684,6 +640,8 @@  static int s5m8767_pmic_probe(struct platform_device *pdev)
 	struct regulator_config config = { };
 	struct s5m8767_info *s5m8767;
 	int i, ret, buck_init;
+	const char *gpiods_names[3] = {"S5M8767 DS2", "S5M8767 DS3", "S5M8767 DS4"};
+	const char *gpiodvs_names[3] = {"S5M8767 SET1", "S5M8767 SET2", "S5M8767 SET3"};
 
 	if (!pdata) {
 		dev_err(pdev->dev.parent, "Platform data not supplied\n");
@@ -731,12 +689,6 @@  static int s5m8767_pmic_probe(struct platform_device *pdev)
 	s5m8767->buck2_gpiodvs = pdata->buck2_gpiodvs;
 	s5m8767->buck3_gpiodvs = pdata->buck3_gpiodvs;
 	s5m8767->buck4_gpiodvs = pdata->buck4_gpiodvs;
-	s5m8767->buck_gpios[0] = pdata->buck_gpios[0];
-	s5m8767->buck_gpios[1] = pdata->buck_gpios[1];
-	s5m8767->buck_gpios[2] = pdata->buck_gpios[2];
-	s5m8767->buck_ds[0] = pdata->buck_ds[0];
-	s5m8767->buck_ds[1] = pdata->buck_ds[1];
-	s5m8767->buck_ds[2] = pdata->buck_ds[2];
 
 	s5m8767->ramp_delay = pdata->buck_ramp_delay;
 	s5m8767->buck2_ramp = pdata->buck2_ramp_enable;
@@ -787,58 +739,39 @@  static int s5m8767_pmic_probe(struct platform_device *pdev)
 
 	if (pdata->buck2_gpiodvs || pdata->buck3_gpiodvs ||
 						pdata->buck4_gpiodvs) {
+		for (i = 0; i < 3; i++) {
+			enum gpiod_flags flags;
 
-		if (!gpio_is_valid(pdata->buck_gpios[0]) ||
-			!gpio_is_valid(pdata->buck_gpios[1]) ||
-			!gpio_is_valid(pdata->buck_gpios[2])) {
-			dev_err(&pdev->dev, "GPIO NOT VALID\n");
-			return -EINVAL;
-		}
-
-		ret = devm_gpio_request(&pdev->dev, pdata->buck_gpios[0],
-					"S5M8767 SET1");
-		if (ret)
-			return ret;
-
-		ret = devm_gpio_request(&pdev->dev, pdata->buck_gpios[1],
-					"S5M8767 SET2");
-		if (ret)
-			return ret;
-
-		ret = devm_gpio_request(&pdev->dev, pdata->buck_gpios[2],
-					"S5M8767 SET3");
-		if (ret)
-			return ret;
+			if (s5m8767->buck_gpioindex & BIT(2 - i))
+				flags = GPIOD_OUT_HIGH;
+			else
+				flags = GPIOD_OUT_LOW;
+
+			s5m8767->buck_gpios[i] = devm_gpiod_get_index(iodev->dev,
+								      "s5m8767,pmic-buck-dvs",
+								      i,
+								      flags);
+			if (IS_ERR(s5m8767->buck_gpios[i])) {
+				ret = PTR_ERR(s5m8767->buck_gpios[i]);
+				return dev_err_probe(iodev->dev, ret, "invalid gpio[%d]: %d\n",
+						     i, ret);
+			}
 
-		/* SET1 GPIO */
-		gpio_direction_output(pdata->buck_gpios[0],
-				(s5m8767->buck_gpioindex >> 2) & 0x1);
-		/* SET2 GPIO */
-		gpio_direction_output(pdata->buck_gpios[1],
-				(s5m8767->buck_gpioindex >> 1) & 0x1);
-		/* SET3 GPIO */
-		gpio_direction_output(pdata->buck_gpios[2],
-				(s5m8767->buck_gpioindex >> 0) & 0x1);
+			gpiod_set_consumer_name(s5m8767->buck_gpios[i], gpiodvs_names[i]);
+		}
 	}
 
-	ret = devm_gpio_request(&pdev->dev, pdata->buck_ds[0], "S5M8767 DS2");
-	if (ret)
-		return ret;
-
-	ret = devm_gpio_request(&pdev->dev, pdata->buck_ds[1], "S5M8767 DS3");
-	if (ret)
-		return ret;
-
-	ret = devm_gpio_request(&pdev->dev, pdata->buck_ds[2], "S5M8767 DS4");
-	if (ret)
-		return ret;
-
-	/* DS2 GPIO */
-	gpio_direction_output(pdata->buck_ds[0], 0x0);
-	/* DS3 GPIO */
-	gpio_direction_output(pdata->buck_ds[1], 0x0);
-	/* DS4 GPIO */
-	gpio_direction_output(pdata->buck_ds[2], 0x0);
+	for (i = 0; i < 3; i++) {
+		s5m8767->buck_ds[i] = devm_gpiod_get_index(iodev->dev,
+							   "s5m8767,pmic-buck-ds",
+							   i, GPIOD_OUT_LOW);
+		if (IS_ERR(s5m8767->buck_ds[i])) {
+			ret = PTR_ERR(s5m8767->buck_ds[i]);
+			return dev_err_probe(iodev->dev, ret, "can't get GPIO %d (%d)\n",
+					     i, ret);
+		}
+		gpiod_set_consumer_name(s5m8767->buck_ds[i], gpiods_names[i]);
+	}
 
 	regmap_update_bits(s5m8767->iodev->regmap_pmic,
 			   S5M8767_REG_BUCK2CTRL, 1 << 1,