| Message ID | 20241211051019.176131-1-chensong_2000@189.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] regulator:s5m8767: Fully convert to GPIO descriptors | expand |
On Wed, Dec 11, 2024 at 01:10:19PM +0800, Song Chen wrote: > This converts s5m8767 regulator driver to use GPIO descriptors. > > --- > v1 - v2: > 1, reedit commit message. > 2, remove development code. > 3, print error msg in dev_err_probe. > 4, doesn't set gpiod directions until successfully requesting > all gpiods. It's pretty much equivalent with original code. > > Signed-off-by: Song Chen <chensong_2000@189.cn> > --- Your signoff needs to be before the ---, and the changelog after the ---. The tools will get very confused and remove your signoff here.
On Wed, Dec 11, 2024 at 6:10 AM Song Chen <chensong_2000@189.cn> wrote: > > This converts s5m8767 regulator driver to use GPIO descriptors. > > --- > v1 - v2: > 1, reedit commit message. > 2, remove development code. > 3, print error msg in dev_err_probe. > 4, doesn't set gpiod directions until successfully requesting > all gpiods. It's pretty much equivalent with original code. > > Signed-off-by: Song Chen <chensong_2000@189.cn> > --- > drivers/regulator/s5m8767.c | 106 ++++++++++++++----------------- > include/linux/mfd/samsung/core.h | 4 +- > 2 files changed, 48 insertions(+), 62 deletions(-) > > diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c > index d25cd81e3f36..b23df037336b 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); > It seems to me that these GPIOs are always manipulated at once. Maybe consider adding fwnode_gpiod_get_index_array() to GPIO core and using it here to shrink the code a bit more? Bart
在 2024/12/11 21:51, Bartosz Golaszewski 写道: > On Wed, Dec 11, 2024 at 6:10 AM Song Chen <chensong_2000@189.cn> wrote: >> >> This converts s5m8767 regulator driver to use GPIO descriptors. >> >> --- >> v1 - v2: >> 1, reedit commit message. >> 2, remove development code. >> 3, print error msg in dev_err_probe. >> 4, doesn't set gpiod directions until successfully requesting >> all gpiods. It's pretty much equivalent with original code. >> >> Signed-off-by: Song Chen <chensong_2000@189.cn> >> --- >> drivers/regulator/s5m8767.c | 106 ++++++++++++++----------------- >> include/linux/mfd/samsung/core.h | 4 +- >> 2 files changed, 48 insertions(+), 62 deletions(-) >> >> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c >> index d25cd81e3f36..b23df037336b 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); >> > > It seems to me that these GPIOs are always manipulated at once. Maybe > consider adding fwnode_gpiod_get_index_array() to GPIO core and using > it here to shrink the code a bit more? > > Bart > If I understand you correctly, you mean introducing devm_fwnode_gpiod_get_index_array and in s5m8767_set_high calling gpiod_set_array_value to control s5m8767->buck_gpios. That is a good point and i can give it a try, but i'm not sure if gpio maintainers like it, they are cautious to introduce new helpers. Or we can use devm_gpiod_get_array, it's pretty much equivalent effect in s5m8767 even without fwnode specified. Furthermore, speaking of shrinking code, i'm thinking about using a bitmap to replace buck2_gpiodvs, buck3_gpiodvs and buck4_gpiodvs, below snippet can be optimized by this bitmap and __builtin_popcount. if (pdata->buck2_gpiodvs) { if (pdata->buck3_gpiodvs || pdata->buck4_gpiodvs) { dev_err(&pdev->dev, "S5M8767 GPIO DVS NOT VALID\n"); return -EINVAL; } } if (pdata->buck3_gpiodvs) { if (pdata->buck2_gpiodvs || pdata->buck4_gpiodvs) { dev_err(&pdev->dev, "S5M8767 GPIO DVS NOT VALID\n"); return -EINVAL; } } if (pdata->buck4_gpiodvs) { if (pdata->buck2_gpiodvs || pdata->buck3_gpiodvs) { dev_err(&pdev->dev, "S5M8767 GPIO DVS NOT VALID\n"); return -EINVAL; } } what do you think? Best regards, Song
Thanks for reminding, will be corrected in next version. Song 在 2024/12/11 21:25, Mark Brown 写道: > On Wed, Dec 11, 2024 at 01:10:19PM +0800, Song Chen wrote: >> This converts s5m8767 regulator driver to use GPIO descriptors. >> >> --- >> v1 - v2: >> 1, reedit commit message. >> 2, remove development code. >> 3, print error msg in dev_err_probe. >> 4, doesn't set gpiod directions until successfully requesting >> all gpiods. It's pretty much equivalent with original code. >> >> Signed-off-by: Song Chen <chensong_2000@189.cn> >> --- > > Your signoff needs to be before the ---, and the changelog after the > ---. The tools will get very confused and remove your signoff here.
On Thu, Dec 12, 2024 at 6:55 AM Song Chen <chensong_2000@189.cn> wrote: > > Or we can use devm_gpiod_get_array, it's pretty much equivalent effect > in s5m8767 even without fwnode specified. > Can you use it though? I was thinking you need the fwnode variant because it's the child (regulator) node of the device? If you can, that would be great. Bart
Hi Bart, 在 2024/12/12 18:29, Bartosz Golaszewski 写道: > On Thu, Dec 12, 2024 at 6:55 AM Song Chen <chensong_2000@189.cn> wrote: >> >> Or we can use devm_gpiod_get_array, it's pretty much equivalent effect >> in s5m8767 even without fwnode specified. >> > > Can you use it though? I was thinking you need the fwnode variant > because it's the child (regulator) node of the device? > > If you can, that would be great. > > Bart > yes, i believe so, eventually both devm_gpiod_get_array and devm_fwnode_gpiod_get_index use of_fwnode_handle(iodev->dev->of_node) in this case. But, i think it's a good idea to introduce a helper to get gpio descriptor array with fwnode, i have made a patch, would appreciate it if you can help me have a look. Best regards, Song
diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c index d25cd81e3f36..b23df037336b 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; } @@ -486,16 +486,19 @@ 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; + int i; + char label[32]; 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; + snprintf(label, sizeof(label), "%s%d", "S5M8767 SET", i + 1); + pdata->buck_gpios[i] = devm_fwnode_gpiod_get_index( + iodev->dev, + of_fwnode_handle(pmic_np), + "s5m8767,pmic-buck-dvs", + i, GPIOD_OUT_LOW, label); + if (IS_ERR(pdata->buck_gpios[i])) + return dev_err_probe(iodev->dev, PTR_ERR(pdata->buck_gpios[i]), + "can't get GPIO\n"); } return 0; } @@ -504,16 +507,19 @@ 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; + int i; + char label[32]; 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; + snprintf(label, sizeof(label), "%s%d", "S5M8767 DS", i + 2); + pdata->buck_ds[i] = devm_fwnode_gpiod_get_index( + iodev->dev, + of_fwnode_handle(pmic_np), + "s5m8767,pmic-buck-ds", + i, GPIOD_OUT_LOW, label); + if (IS_ERR(pdata->buck_ds[i])) + return dev_err_probe(iodev->dev, PTR_ERR(pdata->buck_ds[i]), + "can't get GPIO\n"); } return 0; } @@ -788,57 +794,37 @@ static int s5m8767_pmic_probe(struct platform_device *pdev) if (pdata->buck2_gpiodvs || pdata->buck3_gpiodvs || pdata->buck4_gpiodvs) { - if (!gpio_is_valid(pdata->buck_gpios[0]) || - !gpio_is_valid(pdata->buck_gpios[1]) || - !gpio_is_valid(pdata->buck_gpios[2])) { + if (IS_ERR(pdata->buck_gpios[0]) || + IS_ERR(pdata->buck_gpios[1]) || + IS_ERR(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; - /* SET1 GPIO */ - gpio_direction_output(pdata->buck_gpios[0], + gpiod_direction_output(pdata->buck_gpios[0], (s5m8767->buck_gpioindex >> 2) & 0x1); /* SET2 GPIO */ - gpio_direction_output(pdata->buck_gpios[1], + gpiod_direction_output(pdata->buck_gpios[1], (s5m8767->buck_gpioindex >> 1) & 0x1); /* SET3 GPIO */ - gpio_direction_output(pdata->buck_gpios[2], + gpiod_direction_output(pdata->buck_gpios[2], (s5m8767->buck_gpioindex >> 0) & 0x1); } - 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; + if (IS_ERR(pdata->buck_ds[0]) || + IS_ERR(pdata->buck_ds[1]) || + IS_ERR(pdata->buck_ds[2])) { + dev_err(&pdev->dev, "GPIO NOT VALID\n"); + return -EINVAL; + } /* DS2 GPIO */ - gpio_direction_output(pdata->buck_ds[0], 0x0); + gpiod_direction_output(pdata->buck_ds[0], 0x0); /* DS3 GPIO */ - gpio_direction_output(pdata->buck_ds[1], 0x0); + gpiod_direction_output(pdata->buck_ds[1], 0x0); /* DS4 GPIO */ - gpio_direction_output(pdata->buck_ds[2], 0x0); + gpiod_direction_output(pdata->buck_ds[2], 0x0); regmap_update_bits(s5m8767->iodev->regmap_pmic, S5M8767_REG_BUCK2CTRL, 1 << 1, diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h index 750274d41fc0..c06fff66e755 100644 --- a/include/linux/mfd/samsung/core.h +++ b/include/linux/mfd/samsung/core.h @@ -79,8 +79,8 @@ struct sec_platform_data { struct sec_opmode_data *opmode; int num_regulators; - int buck_gpios[3]; - int buck_ds[3]; + struct gpio_desc *buck_gpios[3]; + struct gpio_desc *buck_ds[3]; unsigned int buck2_voltage[8]; bool buck2_gpiodvs; unsigned int buck3_voltage[8];
This converts s5m8767 regulator driver to use GPIO descriptors. --- v1 - v2: 1, reedit commit message. 2, remove development code. 3, print error msg in dev_err_probe. 4, doesn't set gpiod directions until successfully requesting all gpiods. It's pretty much equivalent with original code. Signed-off-by: Song Chen <chensong_2000@189.cn> --- drivers/regulator/s5m8767.c | 106 ++++++++++++++----------------- include/linux/mfd/samsung/core.h | 4 +- 2 files changed, 48 insertions(+), 62 deletions(-)