| Message ID | 20241004120740.2887776-1-catalin.popescu@leica-geosystems.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control | expand |
On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote: > Add compatible value "mmc-pwrseq-simple-reset" to support reset control > instead of gpios. Reset controls being refcounted, they allow to use > shared resets or gpios across drivers. Support of reset control is > limited to one single reset control. Can't you do this without a binding change? Just use reset controls when there is only 1 GPIO. > > Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> > --- > .../bindings/mmc/mmc-pwrseq-simple.yaml | 21 +++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-)
On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote: > Add compatible value "mmc-pwrseq-simple-reset" to support reset control > instead of gpios. Reset controls being refcounted, they allow to use > shared resets or gpios across drivers. Support of reset control is > limited to one single reset control. Driver support is not that relevant to the bindings. > > Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> > --- > .../bindings/mmc/mmc-pwrseq-simple.yaml | 21 +++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml > index 00feaafc1063..da218260af01 100644 > --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml > +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml > @@ -16,12 +16,13 @@ description: > > properties: > compatible: > - const: mmc-pwrseq-simple > + enum: > + - mmc-pwrseq-simple > + - mmc-pwrseq-simple-reset Nope, that's the same device. > > reset-gpios: > minItems: 1 > # Put some limit to avoid false warnings > - maxItems: 32 > description: > contains a list of GPIO specifiers. The reset GPIOs are asserted > at initialization and prior we start the power up procedure of the card. > @@ -50,6 +51,22 @@ properties: > required: > - compatible > > +allOf: > + - if: > + properties: > + compatible: > + contains: > + enum: > + - mmc-pwrseq-simple-reset > + then: > + properties: > + reset-gpios: > + maxItems: 1 > + else: > + properties: > + reset-gpios: > + maxItems: 32 So basically they are the same... Sorry, this all patch makes little sense to me. You are not doing here much. It's exactly the same device which you now describe in two ways (first no-go) but the two ways are actually the same (second no-go). Best regards, Krzysztof
On 06/10/2024 14:37, Krzysztof Kozlowski wrote: > [Some people who received this message don't often get email from krzk@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email. > > > On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote: >> Add compatible value "mmc-pwrseq-simple-reset" to support reset control >> instead of gpios. Reset controls being refcounted, they allow to use >> shared resets or gpios across drivers. Support of reset control is >> limited to one single reset control. > Driver support is not that relevant to the bindings. > >> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> >> --- >> .../bindings/mmc/mmc-pwrseq-simple.yaml | 21 +++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml >> index 00feaafc1063..da218260af01 100644 >> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml >> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml >> @@ -16,12 +16,13 @@ description: >> >> properties: >> compatible: >> - const: mmc-pwrseq-simple >> + enum: >> + - mmc-pwrseq-simple >> + - mmc-pwrseq-simple-reset > Nope, that's the same device. > >> reset-gpios: >> minItems: 1 >> # Put some limit to avoid false warnings >> - maxItems: 32 >> description: >> contains a list of GPIO specifiers. The reset GPIOs are asserted >> at initialization and prior we start the power up procedure of the card. >> @@ -50,6 +51,22 @@ properties: >> required: >> - compatible >> >> +allOf: >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - mmc-pwrseq-simple-reset >> + then: >> + properties: >> + reset-gpios: >> + maxItems: 1 >> + else: >> + properties: >> + reset-gpios: >> + maxItems: 32 > So basically they are the same... Sorry, this all patch makes little > sense to me. You are not doing here much. It's exactly the same device > which you now describe in two ways (first no-go) but the two ways are > actually the same (second no-go). That's because the reset framework doesn't support a list of reset gpios (assuming that RESET_GPIO was enabled), but only a single reset gpio. Both gpio and reset frameworks are expecting the same DT property ("reset-gpios") so using a different compatible to differentiate b/w the "tool" (gpio or reset control) to use for the power sequence seemed fine to me. > > Best regards, > Krzysztof >
On 05/10/2024 20:26, Rob Herring wrote: > [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email. > > > On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote: >> Add compatible value "mmc-pwrseq-simple-reset" to support reset control >> instead of gpios. Reset controls being refcounted, they allow to use >> shared resets or gpios across drivers. Support of reset control is >> limited to one single reset control. > Can't you do this without a binding change? Just use reset controls when > there is only 1 GPIO. That's a good question. The idea was to keep in place the gpio support w/o impacting any platform using pwrseq-simple. Also, later on when support for a list of reset gpios will be added to the reset framework, this would not work anymore... > >> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> >> --- >> .../bindings/mmc/mmc-pwrseq-simple.yaml | 21 +++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-)
On Mon, Oct 07, 2024 at 03:32:42PM +0000, POPESCU Catalin wrote: > On 05/10/2024 20:26, Rob Herring wrote: > > [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > > > This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email. > > > > > > On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote: > >> Add compatible value "mmc-pwrseq-simple-reset" to support reset control > >> instead of gpios. Reset controls being refcounted, they allow to use > >> shared resets or gpios across drivers. Support of reset control is > >> limited to one single reset control. > > Can't you do this without a binding change? Just use reset controls when > > there is only 1 GPIO. > > That's a good question. The idea was to keep in place the gpio support > w/o impacting any platform using pwrseq-simple. Why would it matter? If not shared, then the behavior should be the same. If shared, we want to maintain the broken behavior? > > Also, later on when support for a list of reset gpios will be added to > the reset framework, this would not work anymore... Why not? How an OS handles reset-gpios is up to the OS. It can evolve. The binding can't evolve because it is an ABI. Also, a list is kind of broken to begin with for a "generic" binding. What's the order the lines should be asserted/deasserted? What about timing requirements? You don't know because every device is different. This binding would not be accepted now, so extending it is questionable. Rob
On 07/10/2024 17:59, Rob Herring wrote: > [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email. > > > On Mon, Oct 07, 2024 at 03:32:42PM +0000, POPESCU Catalin wrote: >> On 05/10/2024 20:26, Rob Herring wrote: >>> [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] >>> >>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email. >>> >>> >>> On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote: >>>> Add compatible value "mmc-pwrseq-simple-reset" to support reset control >>>> instead of gpios. Reset controls being refcounted, they allow to use >>>> shared resets or gpios across drivers. Support of reset control is >>>> limited to one single reset control. >>> Can't you do this without a binding change? Just use reset controls when >>> there is only 1 GPIO. >> That's a good question. The idea was to keep in place the gpio support >> w/o impacting any platform using pwrseq-simple. > Why would it matter? If not shared, then the behavior should be the > same. If shared, we want to maintain the broken behavior? Indeed, you're right. I will provide a new patchset w/o the binding change and using reset control for 1 gpio use-case. >> Also, later on when support for a list of reset gpios will be added to >> the reset framework, this would not work anymore... > Why not? > > How an OS handles reset-gpios is up to the OS. It can evolve. The > binding can't evolve because it is an ABI. > > Also, a list is kind of broken to begin with for a "generic" binding. > What's the order the lines should be asserted/deasserted? What about > timing requirements? You don't know because every device is different. > This binding would not be accepted now, so extending it is questionable. > > Rob
On 24-10-07, Rob Herring wrote: > On Mon, Oct 07, 2024 at 03:32:42PM +0000, POPESCU Catalin wrote: > > On 05/10/2024 20:26, Rob Herring wrote: > > > [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > > > > > This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email. > > > > > > > > > On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote: > > >> Add compatible value "mmc-pwrseq-simple-reset" to support reset control > > >> instead of gpios. Reset controls being refcounted, they allow to use > > >> shared resets or gpios across drivers. Support of reset control is > > >> limited to one single reset control. > > > Can't you do this without a binding change? Just use reset controls when > > > there is only 1 GPIO. > > > > That's a good question. The idea was to keep in place the gpio support > > w/o impacting any platform using pwrseq-simple. > > Why would it matter? If not shared, then the behavior should be the > same. If shared, we want to maintain the broken behavior? My two cents on this. This will break systems which haven't the RESET_GPIO driver enabled yet. Therefore we may get regressions mails. I'm fine with it if that is okay. Regards, Marco > > Also, later on when support for a list of reset gpios will be added to > > the reset framework, this would not work anymore... > > Why not? > > How an OS handles reset-gpios is up to the OS. It can evolve. The > binding can't evolve because it is an ABI. > > Also, a list is kind of broken to begin with for a "generic" binding. > What's the order the lines should be asserted/deasserted? What about > timing requirements? You don't know because every device is different. > This binding would not be accepted now, so extending it is questionable. > > Rob >
diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml index 00feaafc1063..da218260af01 100644 --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml @@ -16,12 +16,13 @@ description: properties: compatible: - const: mmc-pwrseq-simple + enum: + - mmc-pwrseq-simple + - mmc-pwrseq-simple-reset reset-gpios: minItems: 1 # Put some limit to avoid false warnings - maxItems: 32 description: contains a list of GPIO specifiers. The reset GPIOs are asserted at initialization and prior we start the power up procedure of the card. @@ -50,6 +51,22 @@ properties: required: - compatible +allOf: + - if: + properties: + compatible: + contains: + enum: + - mmc-pwrseq-simple-reset + then: + properties: + reset-gpios: + maxItems: 1 + else: + properties: + reset-gpios: + maxItems: 32 + additionalProperties: false examples:
Add compatible value "mmc-pwrseq-simple-reset" to support reset control instead of gpios. Reset controls being refcounted, they allow to use shared resets or gpios across drivers. Support of reset control is limited to one single reset control. Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> --- .../bindings/mmc/mmc-pwrseq-simple.yaml | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)