| Message ID | 20240620103410.35786-1-vishnu.reddy@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pinctrl: samsung: Add support for pull-up and pull-down | expand |
Hi Vishnu > -----Original Message----- > From: Vishnu Reddy <vishnu.reddy@samsung.com> > Sent: Thursday, June 20, 2024 4:04 PM > To: krzysztof.kozlowski@linaro.org; s.nawrocki@samsung.com; > alim.akhtar@samsung.com; linus.walleij@linaro.org > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; linux-gpio@vger.kernel.org; linux- > kernel@vger.kernel.org; pankaj.dubey@samsung.com; > ravi.patel@samsung.com; gost.dev@samsung.com > Subject: [PATCH v2] pinctrl: samsung: Add support for pull-up and pull-down > > gpiolib framework has the implementation of setting up the PUD > configuration for GPIO pins but there is no driver support. > > Add support to handle the PUD configuration request from the userspace in > samsung pinctrl driver. > > Signed-off-by: Vishnu Reddy <vishnu.reddy@samsung.com> > --- Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
On 20/06/2024 12:34, Vishnu Reddy wrote: > gpiolib framework has the implementation of setting up the > PUD configuration for GPIO pins but there is no driver support. > > Add support to handle the PUD configuration request from the > userspace in samsung pinctrl driver. > > Signed-off-by: Vishnu Reddy <vishnu.reddy@samsung.com> > --- > Verified the offset from the user manual of following Exynos SoC series > and found the current code is taking care of correct offset for pull-up > and pull-down > > Exynos-3250 > Exynos-3470 > Exynos-4412 > Exynos-4415 > Exynos-5250 > Exynos-5260 > Exynos-5410 > Exynos-5420 > Exynos-5422 > Exynos-7420 > Exynos-7580 > Exynos-7880 > Exynos-9820 > Exynos-9830 > Exynos-4210 > Exynos-S5PC210 > Exynos-S5PV310 > > This patch is tested on FSD platform You verified but... > diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h > index d50ba6f07d5d..758b623a4bea 100644 > --- a/drivers/pinctrl/samsung/pinctrl-samsung.h > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h > @@ -61,6 +61,13 @@ enum pincfg_type { > #define PIN_CON_FUNC_INPUT 0x0 > #define PIN_CON_FUNC_OUTPUT 0x1 > > +/* > + * Values for the pin PUD register. > + */ > +#define PIN_PUD_PULL_UP_DOWN_DISABLE 0x0 > +#define PIN_PUD_PULL_DOWN_ENABLE 0x1 > +#define PIN_PUD_PULL_UP_ENABLE 0x3 ... I said it is not correct, so you send the same? If you think I was wrong, then please respond and keep discussion going. Sending the same suggests you just ignored my comment. Look at two headers s5pv210-pinctrl.h and s3c64xx-pinctrl.h. How did you resolve these? Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzk@kernel.org] > Sent: 24 June 2024 19:27 > To: Vishnu Reddy <vishnu.reddy@samsung.com>; > krzysztof.kozlowski@linaro.org; s.nawrocki@samsung.com; > alim.akhtar@samsung.com; linus.walleij@linaro.org > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; linux-gpio@vger.kernel.org; linux- > kernel@vger.kernel.org; pankaj.dubey@samsung.com; > ravi.patel@samsung.com; gost.dev@samsung.com > Subject: Re: [PATCH v2] pinctrl: samsung: Add support for pull-up and pull- > down > > On 20/06/2024 12:34, Vishnu Reddy wrote: > > gpiolib framework has the implementation of setting up the > > PUD configuration for GPIO pins but there is no driver support. > > > > Add support to handle the PUD configuration request from the > > userspace in samsung pinctrl driver. > > > > Signed-off-by: Vishnu Reddy <vishnu.reddy@samsung.com> > > --- > > Verified the offset from the user manual of following Exynos SoC series > > and found the current code is taking care of correct offset for pull-up > > and pull-down > > > > Exynos-3250 > > Exynos-3470 > > Exynos-4412 > > Exynos-4415 > > Exynos-5250 > > Exynos-5260 > > Exynos-5410 > > Exynos-5420 > > Exynos-5422 > > Exynos-7420 > > Exynos-7580 > > Exynos-7880 > > Exynos-9820 > > Exynos-9830 > > Exynos-4210 > > Exynos-S5PC210 > > Exynos-S5PV310 > > > > This patch is tested on FSD platform > > You verified but... > > > diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h > b/drivers/pinctrl/samsung/pinctrl-samsung.h > > index d50ba6f07d5d..758b623a4bea 100644 > > --- a/drivers/pinctrl/samsung/pinctrl-samsung.h > > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h > > @@ -61,6 +61,13 @@ enum pincfg_type { > > #define PIN_CON_FUNC_INPUT 0x0 > > #define PIN_CON_FUNC_OUTPUT 0x1 > > > > +/* > > + * Values for the pin PUD register. > > + */ > > +#define PIN_PUD_PULL_UP_DOWN_DISABLE 0x0 > > +#define PIN_PUD_PULL_DOWN_ENABLE 0x1 > > +#define PIN_PUD_PULL_UP_ENABLE 0x3 > > ... I said it is not correct, so you send the same? If you think I was > wrong, then please respond and keep discussion going. Sending the same > suggests you just ignored my comment. > > Look at two headers s5pv210-pinctrl.h and s3c64xx-pinctrl.h. How did you > resolve these? Thank you for sharing the s5pv210-pinctrl.h and s3c64xx-pinctrl.h file names for the pin value information. I have not ignored your comment. Unfortunately, I don't have the user manuals for the s3c64xx and s5pv210 series. I have an idea to handle the PIN_PULL_UP value of the s3c64xx and s5pv210 series by checking the compatibility with the of_device_is_compatible API. Will it be okay or do you have any other suggestions? > > Best regards, > Krzysztof
On 26/06/2024 13:49, Vishnu Reddy wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski [mailto:krzk@kernel.org] >> Sent: 24 June 2024 19:27 >> To: Vishnu Reddy <vishnu.reddy@samsung.com>; >> krzysztof.kozlowski@linaro.org; s.nawrocki@samsung.com; >> alim.akhtar@samsung.com; linus.walleij@linaro.org >> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- >> soc@vger.kernel.org; linux-gpio@vger.kernel.org; linux- >> kernel@vger.kernel.org; pankaj.dubey@samsung.com; >> ravi.patel@samsung.com; gost.dev@samsung.com >> Subject: Re: [PATCH v2] pinctrl: samsung: Add support for pull-up and pull- >> down >> >> On 20/06/2024 12:34, Vishnu Reddy wrote: >>> gpiolib framework has the implementation of setting up the >>> PUD configuration for GPIO pins but there is no driver support. >>> >>> Add support to handle the PUD configuration request from the >>> userspace in samsung pinctrl driver. >>> >>> Signed-off-by: Vishnu Reddy <vishnu.reddy@samsung.com> >>> --- >>> Verified the offset from the user manual of following Exynos SoC series >>> and found the current code is taking care of correct offset for pull-up >>> and pull-down >>> >>> Exynos-3250 >>> Exynos-3470 >>> Exynos-4412 >>> Exynos-4415 >>> Exynos-5250 >>> Exynos-5260 >>> Exynos-5410 >>> Exynos-5420 >>> Exynos-5422 >>> Exynos-7420 >>> Exynos-7580 >>> Exynos-7880 >>> Exynos-9820 >>> Exynos-9830 >>> Exynos-4210 >>> Exynos-S5PC210 >>> Exynos-S5PV310 >>> >>> This patch is tested on FSD platform >> >> You verified but... >> >>> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h >> b/drivers/pinctrl/samsung/pinctrl-samsung.h >>> index d50ba6f07d5d..758b623a4bea 100644 >>> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h >>> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h >>> @@ -61,6 +61,13 @@ enum pincfg_type { >>> #define PIN_CON_FUNC_INPUT 0x0 >>> #define PIN_CON_FUNC_OUTPUT 0x1 >>> >>> +/* >>> + * Values for the pin PUD register. >>> + */ >>> +#define PIN_PUD_PULL_UP_DOWN_DISABLE 0x0 >>> +#define PIN_PUD_PULL_DOWN_ENABLE 0x1 >>> +#define PIN_PUD_PULL_UP_ENABLE 0x3 >> >> ... I said it is not correct, so you send the same? If you think I was >> wrong, then please respond and keep discussion going. Sending the same >> suggests you just ignored my comment. >> >> Look at two headers s5pv210-pinctrl.h and s3c64xx-pinctrl.h. How did you >> resolve these? > Thank you for sharing the s5pv210-pinctrl.h and s3c64xx-pinctrl.h file names for the pin value information. > I have not ignored your comment. Unfortunately, I don't have the user manuals for the s3c64xx and s5pv210 series. > I have an idea to handle the PIN_PULL_UP value of the s3c64xx and s5pv210 series by checking the compatibility with the of_device_is_compatible API. > Will it be okay or do you have any other suggestions? I don't remember the code used here, but usually such choices are determined by driver match data (and flags or value customized per variant). Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org] > Sent: 26 June 2024 18:31 > To: Vishnu Reddy <vishnu.reddy@samsung.com>; 'Krzysztof Kozlowski' > <krzk@kernel.org>; s.nawrocki@samsung.com; alim.akhtar@samsung.com; > linus.walleij@linaro.org > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; linux-gpio@vger.kernel.org; linux- > kernel@vger.kernel.org; pankaj.dubey@samsung.com; > ravi.patel@samsung.com; gost.dev@samsung.com > Subject: Re: [PATCH v2] pinctrl: samsung: Add support for pull-up and pull- > down > > On 26/06/2024 13:49, Vishnu Reddy wrote: > > > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski [mailto:krzk@kernel.org] > >> Sent: 24 June 2024 19:27 > >> To: Vishnu Reddy <vishnu.reddy@samsung.com>; > >> krzysztof.kozlowski@linaro.org; s.nawrocki@samsung.com; > >> alim.akhtar@samsung.com; linus.walleij@linaro.org > >> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- > >> soc@vger.kernel.org; linux-gpio@vger.kernel.org; linux- > >> kernel@vger.kernel.org; pankaj.dubey@samsung.com; > >> ravi.patel@samsung.com; gost.dev@samsung.com > >> Subject: Re: [PATCH v2] pinctrl: samsung: Add support for pull-up and > >> pull- down > >> > >> On 20/06/2024 12:34, Vishnu Reddy wrote: > >>> gpiolib framework has the implementation of setting up the PUD > >>> configuration for GPIO pins but there is no driver support. > >>> > >>> Add support to handle the PUD configuration request from the > >>> userspace in samsung pinctrl driver. > >>> > >>> Signed-off-by: Vishnu Reddy <vishnu.reddy@samsung.com> > >>> --- > >>> Verified the offset from the user manual of following Exynos SoC > >>> series and found the current code is taking care of correct offset > >>> for pull-up and pull-down > >>> > >>> Exynos-3250 > >>> Exynos-3470 > >>> Exynos-4412 > >>> Exynos-4415 > >>> Exynos-5250 > >>> Exynos-5260 > >>> Exynos-5410 > >>> Exynos-5420 > >>> Exynos-5422 > >>> Exynos-7420 > >>> Exynos-7580 > >>> Exynos-7880 > >>> Exynos-9820 > >>> Exynos-9830 > >>> Exynos-4210 > >>> Exynos-S5PC210 > >>> Exynos-S5PV310 > >>> > >>> This patch is tested on FSD platform > >> > >> You verified but... > >> > >>> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h > >> b/drivers/pinctrl/samsung/pinctrl-samsung.h > >>> index d50ba6f07d5d..758b623a4bea 100644 > >>> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h > >>> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h > >>> @@ -61,6 +61,13 @@ enum pincfg_type { > >>> #define PIN_CON_FUNC_INPUT 0x0 > >>> #define PIN_CON_FUNC_OUTPUT 0x1 > >>> > >>> +/* > >>> + * Values for the pin PUD register. > >>> + */ > >>> +#define PIN_PUD_PULL_UP_DOWN_DISABLE 0x0 > >>> +#define PIN_PUD_PULL_DOWN_ENABLE 0x1 > >>> +#define PIN_PUD_PULL_UP_ENABLE 0x3 > >> > >> ... I said it is not correct, so you send the same? If you think I > >> was wrong, then please respond and keep discussion going. Sending the > >> same suggests you just ignored my comment. > >> > >> Look at two headers s5pv210-pinctrl.h and s3c64xx-pinctrl.h. How did > >> you resolve these? > > Thank you for sharing the s5pv210-pinctrl.h and s3c64xx-pinctrl.h file > names for the pin value information. > > I have not ignored your comment. Unfortunately, I don't have the user > manuals for the s3c64xx and s5pv210 series. > > I have an idea to handle the PIN_PULL_UP value of the s3c64xx and > s5pv210 series by checking the compatibility with the > of_device_is_compatible API. > > Will it be okay or do you have any other suggestions? > > I don't remember the code used here, but usually such choices are > determined by driver match data (and flags or value customized per variant). Hi, Thanks for suggestion. I have gone through this and found that driver match data in this driver is stored in the __initconst section, which is freed up after kernel initialization. So we have two options: 1: Keep this platform specific data in driver match data and then populate driver_data field in probe function. 2: Use compatible matching and set different values during set_config. First approach will result in many changes, such as populating driver match data for all platforms and then storing the same in driver_data in probe. In the second approach, we can handle this using simple if/else based on a compatible match. IMO, second approach would be simpler and introduce less changes. Any suggestions from your end? > > Best regards, > Krzysztof
On 27/06/2024 15:35, Vishnu Reddy wrote: >> >> I don't remember the code used here, but usually such choices are >> determined by driver match data (and flags or value customized per variant). > Hi, Thanks for suggestion. > I have gone through this and found that driver match data in this driver is stored in the __initconst section, which is freed up after kernel initialization. So we have two options: > 1: Keep this platform specific data in driver match data and then populate driver_data field in probe function. > 2: Use compatible matching and set different values during set_config. > > First approach will result in many changes, such as populating driver match data for all platforms and then storing the same in driver_data in probe. > > In the second approach, we can handle this using simple if/else based on a compatible match. > > IMO, second approach would be simpler and introduce less changes. Any suggestions from your end? Please wrap your email according to mailing list style. Both options are not the way because you introduce a new, different style of handling per-variant customization. The driver already parses match data and stores such per-variant-details in different places, like samsung_pin_bank or samsung_pinctrl_drv_data. This seems like a value fixed per entire device, so could go to samsung_pinctrl_drv_data. Best regards, Krzysztof
On 27/06/2024 17:22, Krzysztof Kozlowski wrote: > On 27/06/2024 15:35, Vishnu Reddy wrote: >>> >>> I don't remember the code used here, but usually such choices are >>> determined by driver match data (and flags or value customized per variant). >> Hi, Thanks for suggestion. >> I have gone through this and found that driver match data in this driver is stored in the __initconst section, which is freed up after kernel initialization. So we have two options: >> 1: Keep this platform specific data in driver match data and then populate driver_data field in probe function. >> 2: Use compatible matching and set different values during set_config. >> >> First approach will result in many changes, such as populating driver match data for all platforms and then storing the same in driver_data in probe. >> >> In the second approach, we can handle this using simple if/else based on a compatible match. >> >> IMO, second approach would be simpler and introduce less changes. Any suggestions from your end? > > Please wrap your email according to mailing list style. > > Both options are not the way because you introduce a new, different > style of handling per-variant customization. The driver already parses > match data and stores such per-variant-details in different places, like > samsung_pin_bank or samsung_pinctrl_drv_data. This seems like a value > fixed per entire device, so could go to samsung_pinctrl_drv_data. ... although maybe this matches your first option? Not sure. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzk@kernel.org] > Sent: 27 June 2024 20:58 > To: Vishnu Reddy <vishnu.reddy@samsung.com>; 'Krzysztof Kozlowski' > <krzysztof.kozlowski@linaro.org>; s.nawrocki@samsung.com; > alim.akhtar@samsung.com; linus.walleij@linaro.org > Cc: linux-arm-kernel@lists.infradead.org; linux-samsung- > soc@vger.kernel.org; linux-gpio@vger.kernel.org; linux- > kernel@vger.kernel.org; pankaj.dubey@samsung.com; > ravi.patel@samsung.com; gost.dev@samsung.com > Subject: Re: [PATCH v2] pinctrl: samsung: Add support for pull-up and pull- > down > > On 27/06/2024 17:22, Krzysztof Kozlowski wrote: > > On 27/06/2024 15:35, Vishnu Reddy wrote: > >>> > >>> I don't remember the code used here, but usually such choices are > >>> determined by driver match data (and flags or value customized per > variant). > >> Hi, Thanks for suggestion. > >> I have gone through this and found that driver match data in this driver is > stored in the __initconst section, which is freed up after kernel initialization. > So we have two options: > >> 1: Keep this platform specific data in driver match data and then populate > driver_data field in probe function. > >> 2: Use compatible matching and set different values during set_config. > >> > >> First approach will result in many changes, such as populating driver > match data for all platforms and then storing the same in driver_data in > probe. > >> > >> In the second approach, we can handle this using simple if/else based on > a compatible match. > >> > >> IMO, second approach would be simpler and introduce less changes. Any > suggestions from your end? > > > > Please wrap your email according to mailing list style. > > > > Both options are not the way because you introduce a new, different > > style of handling per-variant customization. The driver already parses > > match data and stores such per-variant-details in different places, like > > samsung_pin_bank or samsung_pinctrl_drv_data. This seems like a value > > fixed per entire device, so could go to samsung_pinctrl_drv_data. > > ... although maybe this matches your first option? Not sure. My understanding is same that the value is fixed per entire device. We can add the pud data into samsung_pinctrl_drv_data and then fetch the pud values during probe. I will test this and submit v3 soon. > > Best regards, > Krzysztof
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c index 623df65a5d6f..13e8109d002a 100644 --- a/drivers/pinctrl/samsung/pinctrl-samsung.c +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c @@ -997,6 +997,58 @@ static int samsung_pinctrl_unregister(struct platform_device *pdev, return 0; } +/* + * samsung_gpio_set_pud will enable or disable the pull-down and + * pull-up for the gpio pins in the PUD register. + */ +static void samsung_gpio_set_pud(struct gpio_chip *gc, unsigned int offset, + unsigned int value) +{ + struct samsung_pin_bank *bank = gpiochip_get_data(gc); + const struct samsung_pin_bank_type *type = bank->type; + void __iomem *reg; + unsigned int data, mask; + + reg = bank->pctl_base + bank->pctl_offset; + data = readl(reg + type->reg_offset[PINCFG_TYPE_PUD]); + mask = (1 << type->fld_width[PINCFG_TYPE_PUD]) - 1; + data &= ~(mask << (offset * type->fld_width[PINCFG_TYPE_PUD])); + data |= value << (offset * type->fld_width[PINCFG_TYPE_PUD]); + writel(data, reg + type->reg_offset[PINCFG_TYPE_PUD]); +} + +/* + * samsung_gpio_set_config will identify the type of PUD config based + * on the gpiolib request to enable or disable the PUD configuration. + */ +static int samsung_gpio_set_config(struct gpio_chip *gc, unsigned int offset, + unsigned long config) +{ + struct samsung_pin_bank *bank = gpiochip_get_data(gc); + unsigned long flags; + unsigned int value = 0; + + switch (pinconf_to_config_param(config)) { + case PIN_CONFIG_BIAS_DISABLE: + value = PIN_PUD_PULL_UP_DOWN_DISABLE; + break; + case PIN_CONFIG_BIAS_PULL_DOWN: + value = PIN_PUD_PULL_DOWN_ENABLE; + break; + case PIN_CONFIG_BIAS_PULL_UP: + value = PIN_PUD_PULL_UP_ENABLE; + break; + default: + return -ENOTSUPP; + } + + raw_spin_lock_irqsave(&bank->slock, flags); + samsung_gpio_set_pud(gc, offset, value); + raw_spin_unlock_irqrestore(&bank->slock, flags); + + return 0; +} + static const struct gpio_chip samsung_gpiolib_chip = { .request = gpiochip_generic_request, .free = gpiochip_generic_free, @@ -1006,6 +1058,7 @@ static const struct gpio_chip samsung_gpiolib_chip = { .direction_output = samsung_gpio_direction_output, .to_irq = samsung_gpio_to_irq, .add_pin_ranges = samsung_add_pin_ranges, + .set_config = samsung_gpio_set_config, .owner = THIS_MODULE, }; diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h index d50ba6f07d5d..758b623a4bea 100644 --- a/drivers/pinctrl/samsung/pinctrl-samsung.h +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h @@ -61,6 +61,13 @@ enum pincfg_type { #define PIN_CON_FUNC_INPUT 0x0 #define PIN_CON_FUNC_OUTPUT 0x1 +/* + * Values for the pin PUD register. + */ +#define PIN_PUD_PULL_UP_DOWN_DISABLE 0x0 +#define PIN_PUD_PULL_DOWN_ENABLE 0x1 +#define PIN_PUD_PULL_UP_ENABLE 0x3 + /** * enum eint_type - possible external interrupt types. * @EINT_TYPE_NONE: bank does not support external interrupts
gpiolib framework has the implementation of setting up the PUD configuration for GPIO pins but there is no driver support. Add support to handle the PUD configuration request from the userspace in samsung pinctrl driver. Signed-off-by: Vishnu Reddy <vishnu.reddy@samsung.com> --- Verified the offset from the user manual of following Exynos SoC series and found the current code is taking care of correct offset for pull-up and pull-down Exynos-3250 Exynos-3470 Exynos-4412 Exynos-4415 Exynos-5250 Exynos-5260 Exynos-5410 Exynos-5420 Exynos-5422 Exynos-7420 Exynos-7580 Exynos-7880 Exynos-9820 Exynos-9830 Exynos-4210 Exynos-S5PC210 Exynos-S5PV310 This patch is tested on FSD platform drivers/pinctrl/samsung/pinctrl-samsung.c | 53 +++++++++++++++++++++++ drivers/pinctrl/samsung/pinctrl-samsung.h | 7 +++ 2 files changed, 60 insertions(+)