[PATCH v2] pinctrl: samsung: Add support for pull-up and pull-down

[Vishnu Reddy]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski at linaro.org]
> Sent: 26 June 2024 18:31
> To: Vishnu Reddy <vishnu.reddy at samsung.com>; 'Krzysztof Kozlowski'
> <krzk at kernel.org>; s.nawrocki at samsung.com; alim.akhtar at samsung.com;
> linus.walleij at linaro.org
> Cc: linux-arm-kernel at lists.infradead.org; linux-samsung-
> soc at vger.kernel.org; linux-gpio at vger.kernel.org; linux-
> kernel at vger.kernel.org; pankaj.dubey at samsung.com;
> ravi.patel at samsung.com; gost.dev at 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 at kernel.org]
> >> Sent: 24 June 2024 19:27
> >> To: Vishnu Reddy <vishnu.reddy at samsung.com>;
> >> krzysztof.kozlowski at linaro.org; s.nawrocki at samsung.com;
> >> alim.akhtar at samsung.com; linus.walleij at linaro.org
> >> Cc: linux-arm-kernel at lists.infradead.org; linux-samsung-
> >> soc at vger.kernel.org; linux-gpio at vger.kernel.org; linux-
> >> kernel at vger.kernel.org; pankaj.dubey at samsung.com;
> >> ravi.patel at samsung.com; gost.dev at 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 at 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