| Message ID | 1472645613-5362-2-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wednesday, August 31, 2016 2:13:25 PM CEST Krzysztof Kozlowski wrote: > > +#define PIN_PULL_NONE 0 > +#define PIN_PULL_DOWN 1 > +#define PIN_PULL_UP 3 > + > +#define PIN_DRV_LV1 0 > +#define PIN_DRV_LV2 2 > +#define PIN_DRV_LV3 1 > +#define PIN_DRV_LV4 3 > + > +#define PIN_FUNC_INPUT 0 > +#define PIN_FUNC_OUTPUT 1 > +#define PIN_FUNC_SPC_2 2 > +#define PIN_FUNC_SPC_3 3 > +#define PIN_FUNC_SPC_4 4 > +#define PIN_FUNC_SPC_5 5 > +#define PIN_FUNC_SPC_F 0xf Any reason for having a copy in each of those files instead of one that is shared across all of them? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/31/2016 02:42 PM, Arnd Bergmann wrote: > On Wednesday, August 31, 2016 2:13:25 PM CEST Krzysztof Kozlowski wrote: >> >> +#define PIN_PULL_NONE 0 >> +#define PIN_PULL_DOWN 1 >> +#define PIN_PULL_UP 3 >> + >> +#define PIN_DRV_LV1 0 >> +#define PIN_DRV_LV2 2 >> +#define PIN_DRV_LV3 1 >> +#define PIN_DRV_LV4 3 >> + >> +#define PIN_FUNC_INPUT 0 >> +#define PIN_FUNC_OUTPUT 1 >> +#define PIN_FUNC_SPC_2 2 >> +#define PIN_FUNC_SPC_3 3 >> +#define PIN_FUNC_SPC_4 4 >> +#define PIN_FUNC_SPC_5 5 >> +#define PIN_FUNC_SPC_F 0xf > > Any reason for having a copy in each of those files instead of one > that is shared across all of them? The drive strengths differ between some of them. There are three groups of drive strengths: 1. Exynos3250, Exynos4 (all) and Exynos5250, 2. Exynos5260, 3. Exynos5410, 542x and 5800. Rest (functions and pull up/down) is the same so sharing the defines is possible but not that obvious. Solution would be for example adding a SoC-family prefix for PIN_DRV_LVx. Not that good... I could put it into three DTSI: - exynos3-pinctrl.dtsi (new file) - exynos5260-pinctrl.dtsi (like it is now) - exynos54xx-pinctrl.dtsi (new file) which would reduce the duplication. Other ideas? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, August 31, 2016 2:53:02 PM CEST Krzysztof Kozlowski wrote: > On 08/31/2016 02:42 PM, Arnd Bergmann wrote: > > On Wednesday, August 31, 2016 2:13:25 PM CEST Krzysztof Kozlowski wrote: > >> > >> +#define PIN_PULL_NONE 0 > >> +#define PIN_PULL_DOWN 1 > >> +#define PIN_PULL_UP 3 > >> + > >> +#define PIN_DRV_LV1 0 > >> +#define PIN_DRV_LV2 2 > >> +#define PIN_DRV_LV3 1 > >> +#define PIN_DRV_LV4 3 > >> + > >> +#define PIN_FUNC_INPUT 0 > >> +#define PIN_FUNC_OUTPUT 1 > >> +#define PIN_FUNC_SPC_2 2 > >> +#define PIN_FUNC_SPC_3 3 > >> +#define PIN_FUNC_SPC_4 4 > >> +#define PIN_FUNC_SPC_5 5 > >> +#define PIN_FUNC_SPC_F 0xf > > > > Any reason for having a copy in each of those files instead of one > > that is shared across all of them? > > The drive strengths differ between some of them. There are three groups > of drive strengths: > 1. Exynos3250, Exynos4 (all) and Exynos5250, > 2. Exynos5260, > 3. Exynos5410, 542x and 5800. I see. That sounds like an even stronger reason to not duplicate the definitions, as this is very confusing. > Rest (functions and pull up/down) is the same so sharing the defines is > possible but not that obvious. Solution would be for example adding a > SoC-family prefix for PIN_DRV_LVx. Not that good... > > I could put it into three DTSI: > - exynos3-pinctrl.dtsi (new file) > - exynos5260-pinctrl.dtsi (like it is now) > - exynos54xx-pinctrl.dtsi (new file) > > which would reduce the duplication. Other ideas? I think having the soc-family prefix is better, as it avoids defining the same symbol to a different value. Better make this as explicit as possible. I think overall, a better solution would have been to define the constants globally (shared with non-exynos) to start with, and have the driver translate generic numbers into vendor specific ones. Obviously it's too late for that now. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/31/2016 02:58 PM, Arnd Bergmann wrote: > On Wednesday, August 31, 2016 2:53:02 PM CEST Krzysztof Kozlowski wrote: >> On 08/31/2016 02:42 PM, Arnd Bergmann wrote: >>> On Wednesday, August 31, 2016 2:13:25 PM CEST Krzysztof Kozlowski wrote: >>>> >>>> +#define PIN_PULL_NONE 0 >>>> +#define PIN_PULL_DOWN 1 >>>> +#define PIN_PULL_UP 3 >>>> + >>>> +#define PIN_DRV_LV1 0 >>>> +#define PIN_DRV_LV2 2 >>>> +#define PIN_DRV_LV3 1 >>>> +#define PIN_DRV_LV4 3 >>>> + >>>> +#define PIN_FUNC_INPUT 0 >>>> +#define PIN_FUNC_OUTPUT 1 >>>> +#define PIN_FUNC_SPC_2 2 >>>> +#define PIN_FUNC_SPC_3 3 >>>> +#define PIN_FUNC_SPC_4 4 >>>> +#define PIN_FUNC_SPC_5 5 >>>> +#define PIN_FUNC_SPC_F 0xf >>> >>> Any reason for having a copy in each of those files instead of one >>> that is shared across all of them? >> >> The drive strengths differ between some of them. There are three groups >> of drive strengths: >> 1. Exynos3250, Exynos4 (all) and Exynos5250, >> 2. Exynos5260, >> 3. Exynos5410, 542x and 5800. > > I see. That sounds like an even stronger reason to not duplicate > the definitions, as this is very confusing. Okay. > >> Rest (functions and pull up/down) is the same so sharing the defines is >> possible but not that obvious. Solution would be for example adding a >> SoC-family prefix for PIN_DRV_LVx. Not that good... >> >> I could put it into three DTSI: >> - exynos3-pinctrl.dtsi (new file) >> - exynos5260-pinctrl.dtsi (like it is now) >> - exynos54xx-pinctrl.dtsi (new file) >> >> which would reduce the duplication. Other ideas? > > I think having the soc-family prefix is better, as it avoids > defining the same symbol to a different value. Better make this > as explicit as possible. Ok, sounds reasonable. I want to convert also older platforms S3C (drive strengths and pull up/down differ) and arm64 SoC: Exynos7. For the latter the problem is there is no common place for sharing DTS, except the headers. However this does not really belong to headers. I guess some level of duplication might be still exist. > I think overall, a better solution would have been to define the > constants globally (shared with non-exynos) to start with, > and have the driver translate generic numbers into vendor > specific ones. Obviously it's too late for that now. We could extend driver by adding new bindings accepting generic numbers (and still backward compatible) but this looks like an overkill. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, August 31, 2016 3:07:00 PM CEST Krzysztof Kozlowski wrote: > > Ok, sounds reasonable. I want to convert also older platforms S3C (drive > strengths and pull up/down differ) and arm64 SoC: Exynos7. For the > latter the problem is there is no common place for sharing DTS, except > the headers. However this does not really belong to headers. I guess > some level of duplication might be still exist. We have stuff in the headers that belongs way less there, so I wouldn't mind, but having a separate set of definitions for arm64 also isn't a problem at all. > > I think overall, a better solution would have been to define the > > constants globally (shared with non-exynos) to start with, > > and have the driver translate generic numbers into vendor > > specific ones. Obviously it's too late for that now. > > We could extend driver by adding new bindings accepting generic numbers > (and still backward compatible) but this looks like an overkill. Agreed, that would only make things more confusing, not less. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/exynos3250-pinctrl.dtsi b/arch/arm/boot/dts/exynos3250-pinctrl.dtsi index 40ea7de44933..645bc3669554 100644 --- a/arch/arm/boot/dts/exynos3250-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos3250-pinctrl.dtsi @@ -26,6 +26,11 @@ #define PIN_PDN_INPUT 2 #define PIN_PDN_PREV 3 +#define PIN_FUNC_INPUT 0 +#define PIN_FUNC_SPC_2 2 +#define PIN_FUNC_SPC_3 3 +#define PIN_FUNC_SPC_4 4 + #define PIN_IN(_pin, _pull, _drv) \ _pin { \ samsung,pins = #_pin; \ diff --git a/arch/arm/boot/dts/exynos4210-pinctrl.dtsi b/arch/arm/boot/dts/exynos4210-pinctrl.dtsi index 9331c6252eff..21129263e4e5 100644 --- a/arch/arm/boot/dts/exynos4210-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos4210-pinctrl.dtsi @@ -14,6 +14,23 @@ * published by the Free Software Foundation. */ +#define PIN_PULL_NONE 0 +#define PIN_PULL_DOWN 1 +#define PIN_PULL_UP 3 + +#define PIN_DRV_LV1 0 +#define PIN_DRV_LV2 2 +#define PIN_DRV_LV3 1 +#define PIN_DRV_LV4 3 + +#define PIN_FUNC_INPUT 0 +#define PIN_FUNC_OUTPUT 1 +#define PIN_FUNC_SPC_2 2 +#define PIN_FUNC_SPC_3 3 +#define PIN_FUNC_SPC_4 4 +#define PIN_FUNC_SPC_5 5 +#define PIN_FUNC_SPC_F 0xf + / { pinctrl@11400000 { gpa0: gpa0 { diff --git a/arch/arm/boot/dts/exynos4415-pinctrl.dtsi b/arch/arm/boot/dts/exynos4415-pinctrl.dtsi index 75af9c56123e..ae9d140a24d1 100644 --- a/arch/arm/boot/dts/exynos4415-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos4415-pinctrl.dtsi @@ -11,6 +11,23 @@ * published by the Free Software Foundation. */ +#define PIN_PULL_NONE 0 +#define PIN_PULL_DOWN 1 +#define PIN_PULL_UP 3 + +#define PIN_DRV_LV1 0 +#define PIN_DRV_LV2 2 +#define PIN_DRV_LV3 1 +#define PIN_DRV_LV4 3 + +#define PIN_FUNC_INPUT 0 +#define PIN_FUNC_OUTPUT 1 +#define PIN_FUNC_SPC_2 2 +#define PIN_FUNC_SPC_3 3 +#define PIN_FUNC_SPC_4 4 +#define PIN_FUNC_SPC_5 5 +#define PIN_FUNC_SPC_F 0xf + &pinctrl_0 { gpa0: gpa0 { gpio-controller; diff --git a/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi b/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi index 856b29254374..ba5865208d3e 100644 --- a/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos4x12-pinctrl.dtsi @@ -16,11 +16,24 @@ #define PIN_PULL_DOWN 1 #define PIN_PULL_UP 3 +#define PIN_DRV_LV1 0 +#define PIN_DRV_LV2 2 +#define PIN_DRV_LV3 1 +#define PIN_DRV_LV4 3 + #define PIN_PDN_OUT0 0 #define PIN_PDN_OUT1 1 #define PIN_PDN_INPUT 2 #define PIN_PDN_PREV 3 +#define PIN_FUNC_INPUT 0 +#define PIN_FUNC_OUTPUT 1 +#define PIN_FUNC_SPC_2 2 +#define PIN_FUNC_SPC_3 3 +#define PIN_FUNC_SPC_4 4 +#define PIN_FUNC_SPC_5 5 +#define PIN_FUNC_SPC_F 0xf + #define PIN_SLP(_pin, _mode, _pull) \ _pin { \ samsung,pins = #_pin; \ diff --git a/arch/arm/boot/dts/exynos5250-pinctrl.dtsi b/arch/arm/boot/dts/exynos5250-pinctrl.dtsi index 880917e508b2..5563dd80d4d0 100644 --- a/arch/arm/boot/dts/exynos5250-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos5250-pinctrl.dtsi @@ -12,6 +12,23 @@ * published by the Free Software Foundation. */ +#define PIN_PULL_NONE 0 +#define PIN_PULL_DOWN 1 +#define PIN_PULL_UP 3 + +#define PIN_DRV_LV1 0 +#define PIN_DRV_LV2 2 +#define PIN_DRV_LV3 1 +#define PIN_DRV_LV4 3 + +#define PIN_FUNC_INPUT 0 +#define PIN_FUNC_OUTPUT 1 +#define PIN_FUNC_SPC_2 2 +#define PIN_FUNC_SPC_3 3 +#define PIN_FUNC_SPC_4 4 +#define PIN_FUNC_SPC_5 5 +#define PIN_FUNC_SPC_F 0xf + &pinctrl_0 { gpa0: gpa0 { gpio-controller; diff --git a/arch/arm/boot/dts/exynos5260-pinctrl.dtsi b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi index f6ee55ea0708..efd01b816538 100644 --- a/arch/arm/boot/dts/exynos5260-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi @@ -12,9 +12,20 @@ * published by the Free Software Foundation. */ -#define PIN_PULL_NONE 0 -#define PIN_PULL_DOWN 1 -#define PIN_PULL_UP 3 +#define PIN_PULL_NONE 0 +#define PIN_PULL_DOWN 1 +#define PIN_PULL_UP 3 + +#define PIN_DRV_LV1 0 +#define PIN_DRV_LV2 1 +#define PIN_DRV_LV4 2 +#define PIN_DRV_LV6 3 + +#define PIN_FUNC_INPUT 0 +#define PIN_FUNC_OUTPUT 1 +#define PIN_FUNC_SPC_2 2 +#define PIN_FUNC_SPC_3 3 +#define PIN_FUNC_SPC_4 4 &pinctrl_0 { gpa0: gpa0 { diff --git a/arch/arm/boot/dts/exynos5410-pinctrl.dtsi b/arch/arm/boot/dts/exynos5410-pinctrl.dtsi index b58a0f29f42c..10272fa54c37 100644 --- a/arch/arm/boot/dts/exynos5410-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos5410-pinctrl.dtsi @@ -9,6 +9,22 @@ * published by the Free Software Foundation. */ +#define PIN_PULL_NONE 0 +#define PIN_PULL_DOWN 1 +#define PIN_PULL_UP 3 + +#define PIN_DRV_LV1 0 +#define PIN_DRV_LV2 1 +#define PIN_DRV_LV3 2 +#define PIN_DRV_LV4 3 + +#define PIN_FUNC_INPUT 0 +#define PIN_FUNC_OUTPUT 1 +#define PIN_FUNC_SPC_2 2 +#define PIN_FUNC_SPC_3 3 +#define PIN_FUNC_SPC_4 4 +#define PIN_FUNC_SPC_F 0xf + &pinctrl_0 { gpa0: gpa0 { gpio-controller; diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi index 14beb7e07323..41abc8ec474a 100644 --- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi @@ -12,6 +12,23 @@ * published by the Free Software Foundation. */ +#define PIN_PULL_NONE 0 +#define PIN_PULL_DOWN 1 +#define PIN_PULL_UP 3 + +#define PIN_DRV_LV1 0 +#define PIN_DRV_LV2 1 +#define PIN_DRV_LV3 2 +#define PIN_DRV_LV4 3 + +#define PIN_FUNC_INPUT 0 +#define PIN_FUNC_OUTPUT 1 +#define PIN_FUNC_SPC_2 2 +#define PIN_FUNC_SPC_3 3 +#define PIN_FUNC_SPC_4 4 +#define PIN_FUNC_SPC_5 5 +#define PIN_FUNC_SPC_F 0xf + &pinctrl_0 { gpy7: gpy7 { gpio-controller;
Add macros for replacing hard-coded GPIO configuration: pull up/down, drive strength and function. Although PIN_FUNC_SPC_2 does not bring much information about the function itself, it still is more descriptive then hard-coded number <2>. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- arch/arm/boot/dts/exynos3250-pinctrl.dtsi | 5 +++++ arch/arm/boot/dts/exynos4210-pinctrl.dtsi | 17 +++++++++++++++++ arch/arm/boot/dts/exynos4415-pinctrl.dtsi | 17 +++++++++++++++++ arch/arm/boot/dts/exynos4x12-pinctrl.dtsi | 13 +++++++++++++ arch/arm/boot/dts/exynos5250-pinctrl.dtsi | 17 +++++++++++++++++ arch/arm/boot/dts/exynos5260-pinctrl.dtsi | 17 ++++++++++++++--- arch/arm/boot/dts/exynos5410-pinctrl.dtsi | 16 ++++++++++++++++ arch/arm/boot/dts/exynos5420-pinctrl.dtsi | 17 +++++++++++++++++ 8 files changed, 116 insertions(+), 3 deletions(-)