| Message ID | 20220929125639.143953-3-benedikt.niedermayr@siemens.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | gpmc wait pin additions | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Thu, Sep 29, 2022 at 02:56:39PM +0200, B. Niedermayr wrote: > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > The GPMC controller has the ability to configure the polarity for the > wait pin. The current properties do not allow this configuration. > This binding directly configures the WAITPIN<X>POLARITY bit > in the GPMC_CONFIG register by setting the gpmc,wait-pin-polarity > dt-property. > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > --- > .../bindings/memory-controllers/ti,gpmc-child.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > index 6e3995bb1630..477189973334 100644 > --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > @@ -230,6 +230,13 @@ properties: > Wait-pin used by client. Must be less than "gpmc,num-waitpins". > $ref: /schemas/types.yaml#/definitions/uint32 > > + gpmc,wait-pin-polarity: 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'. > + description: | > + Set the desired polarity for the selected wait pin. > + 1 for active low, 0 for active high. Well that looks backwards. I assume from the commit msg above, it's the register value, but that's not what the description says. Please go with the logical state here and do the inversion in the driver. > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1] > + > gpmc,wait-on-read: > description: Enables wait monitoring on reads. > type: boolean > -- > 2.25.1 > >
On 30/09/2022 21:42, Rob Herring wrote: > On Thu, Sep 29, 2022 at 02:56:39PM +0200, B. Niedermayr wrote: >> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> >> >> The GPMC controller has the ability to configure the polarity for the >> wait pin. The current properties do not allow this configuration. >> This binding directly configures the WAITPIN<X>POLARITY bit >> in the GPMC_CONFIG register by setting the gpmc,wait-pin-polarity >> dt-property. >> >> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> >> --- >> .../bindings/memory-controllers/ti,gpmc-child.yaml | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml >> index 6e3995bb1630..477189973334 100644 >> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml >> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml >> @@ -230,6 +230,13 @@ properties: >> Wait-pin used by client. Must be less than "gpmc,num-waitpins". >> $ref: /schemas/types.yaml#/definitions/uint32 >> >> + gpmc,wait-pin-polarity: > > 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'. > >> + description: | >> + Set the desired polarity for the selected wait pin. >> + 1 for active low, 0 for active high. > > Well that looks backwards. I assume from the commit msg above, it's the > register value, but that's not what the description says. Please go with > the logical state here and do the inversion in the driver. This was actually my suggestion to keep the same value as ACTIVE_HIGH/LOW in standard GPIO flags. The DTS could reuse the defines. Best regards, Krzysztof
On Fri, 2022-09-30 at 22:01 +0200, Krzysztof Kozlowski wrote: > On 30/09/2022 21:42, Rob Herring wrote: > > On Thu, Sep 29, 2022 at 02:56:39PM +0200, B. Niedermayr wrote: > > > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > > > > > The GPMC controller has the ability to configure the polarity for the > > > wait pin. The current properties do not allow this configuration. > > > This binding directly configures the WAITPIN<X>POLARITY bit > > > in the GPMC_CONFIG register by setting the gpmc,wait-pin-polarity > > > dt-property. > > > > > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > > --- > > > .../bindings/memory-controllers/ti,gpmc-child.yaml | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- > > > child.yaml > > > index 6e3995bb1630..477189973334 100644 > > > --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > > > +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > > > @@ -230,6 +230,13 @@ properties: > > > Wait-pin used by client. Must be less than "gpmc,num-waitpins". > > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > > > + gpmc,wait-pin-polarity: > > > > 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'. > > > > > + description: | > > > + Set the desired polarity for the selected wait pin. > > > + 1 for active low, 0 for active high. > > > > Well that looks backwards. I assume from the commit msg above, it's the > > register value, but that's not what the description says. Please go with > > the logical state here and do the inversion in the driver. > > This was actually my suggestion to keep the same value as > ACTIVE_HIGH/LOW in standard GPIO flags. The DTS could reuse the defines. > Ok, but how to proceed know? IMHO it makes more sense to use the value which actually lands in the register since most people will use the value found in the Datasheet. We already had a discussion with Roger about the GPIO bindings vs. wait-pin binding. The point was that we do not use GPIO bindings in this case, or? > Best regards, > Krzysztof > Cheers, Benedikt
On Fri, 2022-09-30 at 14:42 -0500, Rob Herring wrote: > On Thu, Sep 29, 2022 at 02:56:39PM +0200, B. Niedermayr wrote: > > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > > > The GPMC controller has the ability to configure the polarity for the > > wait pin. The current properties do not allow this configuration. > > This binding directly configures the WAITPIN<X>POLARITY bit > > in the GPMC_CONFIG register by setting the gpmc,wait-pin-polarity > > dt-property. > > > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > --- > > .../bindings/memory-controllers/ti,gpmc-child.yaml | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- > > child.yaml > > index 6e3995bb1630..477189973334 100644 > > --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > > +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > > @@ -230,6 +230,13 @@ properties: > > Wait-pin used by client. Must be less than "gpmc,num-waitpins". > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > + gpmc,wait-pin-polarity: > > 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'. You are right. But nevertheless I can't agree with that in this patch series. I don't want to break consistency, since all bindings currently use 'gpmc'. At least this applies to the "ti,gpmc-child.yaml". I think it makes more sense to create a complete new patch series for that specific change? This change wouldn't fit thematically the current patch series. > > > + description: | > > + Set the desired polarity for the selected wait pin. > > + 1 for active low, 0 for active high. > > Well that looks backwards. I assume from the commit msg above, it's the > register value, but that's not what the description says. Please go with > the logical state here and do the inversion in the driver. > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [0, 1] > > + > > gpmc,wait-on-read: > > description: Enables wait monitoring on reads. > > type: boolean > > -- > > 2.25.1 > > > > Cheers, Benedikt
On 05/10/2022 12:13, Niedermayr, BENEDIKT wrote: >>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- >>> child.yaml >>> index 6e3995bb1630..477189973334 100644 >>> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml >>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml >>> @@ -230,6 +230,13 @@ properties: >>> Wait-pin used by client. Must be less than "gpmc,num-waitpins". >>> $ref: /schemas/types.yaml#/definitions/uint32 >>> >>> + gpmc,wait-pin-polarity: >> >> 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'. > > You are right. But nevertheless I can't agree with that in this patch series. > I don't want to break consistency, since all bindings currently use 'gpmc'. At least this applies > to the "ti,gpmc-child.yaml". > > I think it makes more sense to create a complete new patch series for that specific change? This change > wouldn't fit thematically the current patch series. > So you want to add new property incorrectly named and immediately new patch which fixes the name? No, please squash this new patch into this. Best regards, Krzysztof
On 05/10/2022 12:05, Niedermayr, BENEDIKT wrote: > On Fri, 2022-09-30 at 22:01 +0200, Krzysztof Kozlowski wrote: >> On 30/09/2022 21:42, Rob Herring wrote: >>> On Thu, Sep 29, 2022 at 02:56:39PM +0200, B. Niedermayr wrote: >>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> >>>> >>>> The GPMC controller has the ability to configure the polarity for the >>>> wait pin. The current properties do not allow this configuration. >>>> This binding directly configures the WAITPIN<X>POLARITY bit >>>> in the GPMC_CONFIG register by setting the gpmc,wait-pin-polarity >>>> dt-property. >>>> >>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> >>>> --- >>>> .../bindings/memory-controllers/ti,gpmc-child.yaml | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- >>>> child.yaml >>>> index 6e3995bb1630..477189973334 100644 >>>> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml >>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml >>>> @@ -230,6 +230,13 @@ properties: >>>> Wait-pin used by client. Must be less than "gpmc,num-waitpins". >>>> $ref: /schemas/types.yaml#/definitions/uint32 >>>> >>>> + gpmc,wait-pin-polarity: >>> >>> 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'. >>> >>>> + description: | >>>> + Set the desired polarity for the selected wait pin. >>>> + 1 for active low, 0 for active high. >>> >>> Well that looks backwards. I assume from the commit msg above, it's the >>> register value, but that's not what the description says. Please go with >>> the logical state here and do the inversion in the driver. >> >> This was actually my suggestion to keep the same value as >> ACTIVE_HIGH/LOW in standard GPIO flags. The DTS could reuse the defines. >> > Ok, but how to proceed know? IMHO it makes more sense to use the value which actually lands in the register since most > people will use the value found in the Datasheet. > > We already had a discussion with Roger about the GPIO bindings vs. wait-pin binding. The point was that we do not use GPIO bindings > in this case, or? Go with Rob's suggestion, so back to previous version. Best regards, Krzysztof
On Wed, 2022-10-05 at 13:00 +0200, Krzysztof Kozlowski wrote: > On 05/10/2022 12:13, Niedermayr, BENEDIKT wrote: > > > > diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- > > > > child.yaml > > > > index 6e3995bb1630..477189973334 100644 > > > > --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > > > > @@ -230,6 +230,13 @@ properties: > > > > Wait-pin used by client. Must be less than "gpmc,num-waitpins". > > > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > > > > > + gpmc,wait-pin-polarity: > > > > > > 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'. > > > > You are right. But nevertheless I can't agree with that in this patch series. > > I don't want to break consistency, since all bindings currently use 'gpmc'. At least this applies > > to the "ti,gpmc-child.yaml". > > > > I think it makes more sense to create a complete new patch series for that specific change? This change > > wouldn't fit thematically the current patch series. > > > > So you want to add new property incorrectly named and immediately new > patch which fixes the name? No, please squash this new patch into this. > No that's not what I meant. Currently all bindings in "ti,gpmc-child.yaml" start with "gpmc," and introducing a single binding in this file with "ti," feels like breaking consistency. The "new" patch series should address **all** bindings in this file and all device trees currently using "gpmc," bindings. So finally we have the current patch series introducing the wait pin handling in a consisten way and then another patch series which changes all "gpmc," to "ti,". If it makes more sense to directly introduce the "ti,wait-pin-polarity" instead of "gpmc,wait-pin-polarity" then I will do. Just give me a short feedback. > Best regards, > Krzysztof > Cheers, Benedikt
On 05/10/2022 13:15, Niedermayr, BENEDIKT wrote: > On Wed, 2022-10-05 at 13:00 +0200, Krzysztof Kozlowski wrote: >> On 05/10/2022 12:13, Niedermayr, BENEDIKT wrote: >>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- >>>>> child.yaml >>>>> index 6e3995bb1630..477189973334 100644 >>>>> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml >>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml >>>>> @@ -230,6 +230,13 @@ properties: >>>>> Wait-pin used by client. Must be less than "gpmc,num-waitpins". >>>>> $ref: /schemas/types.yaml#/definitions/uint32 >>>>> >>>>> + gpmc,wait-pin-polarity: >>>> >>>> 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'. >>> >>> You are right. But nevertheless I can't agree with that in this patch series. >>> I don't want to break consistency, since all bindings currently use 'gpmc'. At least this applies >>> to the "ti,gpmc-child.yaml". >>> >>> I think it makes more sense to create a complete new patch series for that specific change? This change >>> wouldn't fit thematically the current patch series. >>> >> >> So you want to add new property incorrectly named and immediately new >> patch which fixes the name? No, please squash this new patch into this. >> > No that's not what I meant. Currently all bindings in "ti,gpmc-child.yaml" start with "gpmc," and introducing > a single binding in this file with "ti," feels like breaking consistency. > > The "new" patch series should address **all** bindings in this file and all device trees currently using "gpmc," > bindings. So finally we have the current patch series introducing the wait pin handling in a consisten way and then another > patch series which changes all "gpmc," to "ti,". Isn't this exactly what I said? First add gpmc (so incorrectly named property) and then fix it to proper name (TI)? So squash that part of second patch which relates to this one, into this patch. Please do not commit incorrect properties, just because they are consistent. > If it makes more sense to directly introduce the "ti,wait-pin-polarity" instead of "gpmc,wait-pin-polarity" then I will do. Just > give me a short feedback. > You got it already: 'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml index 6e3995bb1630..477189973334 100644 --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml @@ -230,6 +230,13 @@ properties: Wait-pin used by client. Must be less than "gpmc,num-waitpins". $ref: /schemas/types.yaml#/definitions/uint32 + gpmc,wait-pin-polarity: + description: | + Set the desired polarity for the selected wait pin. + 1 for active low, 0 for active high. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + gpmc,wait-on-read: description: Enables wait monitoring on reads. type: boolean