Hi Krzysztof,
2016-09-04 20:04 GMT+09:00 Krzysztof Kozlowski <[email protected]>:
>
> Hi,
>
> Changes since v2
> ================
> 1. Combine separate patchsets into one. Previously I sent separately the fixes
> and changes for S3C platforms.
> 2. Fix issues pointed during review.
> 3. Add review tags.
>
> Changes since v1
> ================
> 1. Follow Arnd's suggestion about moving the macros to common place.
> 2. Subjects: replace "GPIO" with "pinctrl".
> 3. There were some major changes here so I did not add Javier's
> reviewed-by and tested-by tags.
>
> Merging
> =======
> Patches #1 and #2 should probably go through pinctrl tree. In that case I would
> appreciate a stable branch/tag so DTS could base on top of it.
>
> Goal
> ====
> Increase readability:
> uart0_data: uart0-data {
> samsung,pins = "gpa0-0", "gpa0-1";
> - samsung,pin-function = <2>;
> - samsung,pin-pud = <0>;
> - samsung,pin-drv = <0>;
> + samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> + samsung,pin-drv = <EXYNOS4_PIN_DRV_LV1>;
I like the idea, thanks for cleaning this up. However I'd like to
bikeshed the prefix a bit. Since the properties are already prefixed
by "samsung,", I think it would make much more sense to also prefix
the generic values with "SAMSUNG_". Of course for soc/family-specific
values, the soc/family name prefix sounds right.
Similarly for rest of the value names, such as SAMSUNG_PIN_PUD instead
of SAMSUNG_PIN_PULL, which obviously sounds more like correct English,
however hurts the consistency and could confuse the people writing new
dts files.
Best regards,
Tomasz
On Sun, Oct 09, 2016 at 04:04:11PM +0900, Tomasz Figa wrote:
> Hi Krzysztof,
>
> 2016-09-04 20:04 GMT+09:00 Krzysztof Kozlowski <[email protected]>:
> >
> > Hi,
> >
> > Changes since v2
> > ================
> > 1. Combine separate patchsets into one. Previously I sent separately the fixes
> > and changes for S3C platforms.
> > 2. Fix issues pointed during review.
> > 3. Add review tags.
> >
> > Changes since v1
> > ================
> > 1. Follow Arnd's suggestion about moving the macros to common place.
> > 2. Subjects: replace "GPIO" with "pinctrl".
> > 3. There were some major changes here so I did not add Javier's
> > reviewed-by and tested-by tags.
> >
> > Merging
> > =======
> > Patches #1 and #2 should probably go through pinctrl tree. In that case I would
> > appreciate a stable branch/tag so DTS could base on top of it.
> >
> > Goal
> > ====
> > Increase readability:
> > uart0_data: uart0-data {
> > samsung,pins = "gpa0-0", "gpa0-1";
> > - samsung,pin-function = <2>;
> > - samsung,pin-pud = <0>;
> > - samsung,pin-drv = <0>;
> > + samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> > + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> > + samsung,pin-drv = <EXYNOS4_PIN_DRV_LV1>;
>
> I like the idea, thanks for cleaning this up. However I'd like to
> bikeshed the prefix a bit. Since the properties are already prefixed
> by "samsung,", I think it would make much more sense to also prefix
> the generic values with "SAMSUNG_". Of course for soc/family-specific
> values, the soc/family name prefix sounds right.
I am lost. Sorry, I don't get what kind of final prefixes you would like
to have.
SAMSUNG_EXYNOS4_PIN_DRV_LV1
SAMSUNG_EXYNOS5260_PIN_DRV_LV1
?
> Similarly for rest of the value names, such as SAMSUNG_PIN_PUD instead
> of SAMSUNG_PIN_PULL, which obviously sounds more like correct English,
> however hurts the consistency and could confuse the people writing new
> dts files.
SAMSUNG_S3C64XX_PIN_PUD_NONE
SAMSUNG_EXYNOS_PIN_PUD_NONE
?
Best regards,
Krzysztof
2016-10-10 1:39 GMT+09:00 Krzysztof Kozlowski <[email protected]>:
> On Sun, Oct 09, 2016 at 04:04:11PM +0900, Tomasz Figa wrote:
>> Hi Krzysztof,
>>
>> 2016-09-04 20:04 GMT+09:00 Krzysztof Kozlowski <[email protected]>:
>> >
>> > Hi,
>> >
>> > Changes since v2
>> > ================
>> > 1. Combine separate patchsets into one. Previously I sent separately the fixes
>> > and changes for S3C platforms.
>> > 2. Fix issues pointed during review.
>> > 3. Add review tags.
>> >
>> > Changes since v1
>> > ================
>> > 1. Follow Arnd's suggestion about moving the macros to common place.
>> > 2. Subjects: replace "GPIO" with "pinctrl".
>> > 3. There were some major changes here so I did not add Javier's
>> > reviewed-by and tested-by tags.
>> >
>> > Merging
>> > =======
>> > Patches #1 and #2 should probably go through pinctrl tree. In that case I would
>> > appreciate a stable branch/tag so DTS could base on top of it.
>> >
>> > Goal
>> > ====
>> > Increase readability:
>> > uart0_data: uart0-data {
>> > samsung,pins = "gpa0-0", "gpa0-1";
>> > - samsung,pin-function = <2>;
>> > - samsung,pin-pud = <0>;
>> > - samsung,pin-drv = <0>;
>> > + samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
>> > + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
>> > + samsung,pin-drv = <EXYNOS4_PIN_DRV_LV1>;
>>
>> I like the idea, thanks for cleaning this up. However I'd like to
>> bikeshed the prefix a bit. Since the properties are already prefixed
>> by "samsung,", I think it would make much more sense to also prefix
>> the generic values with "SAMSUNG_". Of course for soc/family-specific
>> values, the soc/family name prefix sounds right.
>
> I am lost. Sorry, I don't get what kind of final prefixes you would like
> to have.
>
> SAMSUNG_EXYNOS4_PIN_DRV_LV1
> SAMSUNG_EXYNOS5260_PIN_DRV_LV1
> ?
For SoC-specific definitions:
EXYNOS4_PIN_DRV_LV1
EXYNOS5260_PIN_DRV_LV1
>
>> Similarly for rest of the value names, such as SAMSUNG_PIN_PUD instead
>> of SAMSUNG_PIN_PULL, which obviously sounds more like correct English,
>> however hurts the consistency and could confuse the people writing new
>> dts files.
>
> SAMSUNG_S3C64XX_PIN_PUD_NONE
> SAMSUNG_EXYNOS_PIN_PUD_NONE
For definitions common for the whole Samsung pinctrl driver:
SAMSUNG_PIN_PUD_NONE
...
But actually I think I missed the fact that there is almost no common
definitions. Is that correct? Was that the missing part of my
understanding?
Best regards,
Tomasz
On Mon, Oct 10, 2016 at 02:49:01AM +0900, Tomasz Figa wrote:
> 2016-10-10 1:39 GMT+09:00 Krzysztof Kozlowski <[email protected]>:
> > On Sun, Oct 09, 2016 at 04:04:11PM +0900, Tomasz Figa wrote:
> >> Hi Krzysztof,
> >>
> >> 2016-09-04 20:04 GMT+09:00 Krzysztof Kozlowski <[email protected]>:
> >> >
> >> > Hi,
> >> >
> >> > Changes since v2
> >> > ================
> >> > 1. Combine separate patchsets into one. Previously I sent separately the fixes
> >> > and changes for S3C platforms.
> >> > 2. Fix issues pointed during review.
> >> > 3. Add review tags.
> >> >
> >> > Changes since v1
> >> > ================
> >> > 1. Follow Arnd's suggestion about moving the macros to common place.
> >> > 2. Subjects: replace "GPIO" with "pinctrl".
> >> > 3. There were some major changes here so I did not add Javier's
> >> > reviewed-by and tested-by tags.
> >> >
> >> > Merging
> >> > =======
> >> > Patches #1 and #2 should probably go through pinctrl tree. In that case I would
> >> > appreciate a stable branch/tag so DTS could base on top of it.
> >> >
> >> > Goal
> >> > ====
> >> > Increase readability:
> >> > uart0_data: uart0-data {
> >> > samsung,pins = "gpa0-0", "gpa0-1";
> >> > - samsung,pin-function = <2>;
> >> > - samsung,pin-pud = <0>;
> >> > - samsung,pin-drv = <0>;
> >> > + samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> >> > + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> >> > + samsung,pin-drv = <EXYNOS4_PIN_DRV_LV1>;
> >>
> >> I like the idea, thanks for cleaning this up. However I'd like to
> >> bikeshed the prefix a bit. Since the properties are already prefixed
> >> by "samsung,", I think it would make much more sense to also prefix
> >> the generic values with "SAMSUNG_". Of course for soc/family-specific
> >> values, the soc/family name prefix sounds right.
> >
> > I am lost. Sorry, I don't get what kind of final prefixes you would like
> > to have.
> >
> > SAMSUNG_EXYNOS4_PIN_DRV_LV1
> > SAMSUNG_EXYNOS5260_PIN_DRV_LV1
> > ?
>
> For SoC-specific definitions:
>
> EXYNOS4_PIN_DRV_LV1
> EXYNOS5260_PIN_DRV_LV1
ok... so no change needed in my patch.
>
> >
> >> Similarly for rest of the value names, such as SAMSUNG_PIN_PUD instead
> >> of SAMSUNG_PIN_PULL, which obviously sounds more like correct English,
> >> however hurts the consistency and could confuse the people writing new
> >> dts files.
> >
> > SAMSUNG_S3C64XX_PIN_PUD_NONE
> > SAMSUNG_EXYNOS_PIN_PUD_NONE
>
> For definitions common for the whole Samsung pinctrl driver:
>
> SAMSUNG_PIN_PUD_NONE
These are not the same. The "none" is the same but rest is not.
> But actually I think I missed the fact that there is almost no common
> definitions. Is that correct? Was that the missing part of my
> understanding?
Yes. The only common definition for all Samsung SoCs would be the
function of a pin. On the other hand this will bring inconsistency:
everything prefixed with SoC except the function.
Best regards,
Krzysztof