[PATCH 0/6] Add Apple Mac System Management Controller GPIOs

[PATCH 0/6] Add Apple Mac System Management Controller GPIOs

[Russell King (Oracle)]

Hi,

This series adds support for the Apple Mac GPIO driver. These GPIOs
are hadled via the System Management Controller.

The first two patches add the DT binding documentation for the new
drivers.

The second two patches add the core System Management Controller
support.

The last two patches add the GPIO support.

DT updates will follow once the bindings have been reviewed.

Patches taken from the Asahi project.

 .../devicetree/bindings/gpio/gpio-macsmc.yaml      |  28 ++
 .../devicetree/bindings/mfd/apple,smc.yaml         |  57 +++
 drivers/gpio/Kconfig                               |  11 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-macsmc.c                         | 394 ++++++++++++++++++
 drivers/platform/Kconfig                           |   2 +
 drivers/platform/Makefile                          |   1 +
 drivers/platform/apple/Kconfig                     |  49 +++
 drivers/platform/apple/Makefile                    |  11 +
 drivers/platform/apple/smc.h                       |  28 ++
 drivers/platform/apple/smc_core.c                  | 249 ++++++++++++
 drivers/platform/apple/smc_rtkit.c                 | 451 +++++++++++++++++++++
 drivers/soc/apple/rtkit.c                          |   6 +
 include/linux/mfd/macsmc.h                         |  86 ++++
 include/linux/soc/apple/rtkit.h                    |  12 +
 15 files changed, 1386 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
 create mode 100644 drivers/gpio/gpio-macsmc.c
 create mode 100644 drivers/platform/apple/Kconfig
 create mode 100644 drivers/platform/apple/Makefile
 create mode 100644 drivers/platform/apple/smc.h
 create mode 100644 drivers/platform/apple/smc_core.c
 create mode 100644 drivers/platform/apple/smc_rtkit.c
 create mode 100644 include/linux/mfd/macsmc.h

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

Add a DT binding for the Apple Mac System Management Controller.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../devicetree/bindings/mfd/apple,smc.yaml    | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml

diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
new file mode 100644
index 000000000000..794d3a6eb04a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Mac System Management Controller
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+description:
+  Apple Mac System Management Controller implements various functions
+  such as GPIO, RTC, power, reboot.
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - apple,t8103-smc
+        - apple,t8112-smc
+        - apple,t6000-smc
+      - const: apple,smc
+
+  reg:
+    description: Two regions, one for the SMC area and one for the SRAM area.
+
+  reg-names:
+    items:
+      - const: smc
+      - const: sram
+
+  mboxes:
+    description:
+      A phandle to the mailbox channel
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - mboxes
+
+examples:
+  - |
+    smc@23e400000 {
+      compatible = "apple,t8103-smc", "apple,smc";
+      reg = <0x2 0x3e400000 0x0 0x4000>,
+             <0x2 0x3fe00000 0x0 0x100000>;
+      reg-names = "smc", "sram";
+      mboxes = <&smc_mbox>;
+    };
-- 
2.30.2

[PATCH 2/6] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

Add the DT binding for the Apple Mac System Management Controller GPIOs.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
 .../devicetree/bindings/mfd/apple,smc.yaml    |  4 +++
 2 files changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
new file mode 100644
index 000000000000..ee620fe50ca8
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Mac System Management Controller GPIO
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+description:
+  This describes the binding for the Apple Mac System Management Controller
+  GPIO block.
+
+properties:
+  gpio-controller: true
+  '#gpio-cells':
+    const: 2
+
+additionalProperties: false
+
+examples:
+  - |
+    smc_gpio: gpio {
+      gpio-controller;
+      #gpio-cells = <2>;
+    };
diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
index 794d3a6eb04a..bb799a27638d 100644
--- a/Documentation/devicetree/bindings/mfd/apple,smc.yaml
+++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
@@ -34,6 +34,10 @@ title: Apple Mac System Management Controller
     description:
       A phandle to the mailbox channel
 
+patternProperties:
+  gpio:
+    $ref: /schemas/gpio/gpio-macsmc.yaml
+
 additionalProperties: false
 
 required:
-- 
2.30.2

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 01/09/2022 16:54, Russell King (Oracle) wrote:
> Add a DT binding for the Apple Mac System Management Controller.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thank you for your patch. There is something to discuss/improve.

> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description:
> +  Apple Mac System Management Controller implements various functions
> +  such as GPIO, RTC, power, reboot.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - apple,t8103-smc

You miss two spaces of indentation on this level.

> +        - apple,t8112-smc
> +        - apple,t6000-smc

Bring some order here - either alphabetical or by date of release (as in
other Apple schemas). I think t6000 was before t8112, so it's none of
that orders.

> +      - const: apple,smc
> +
> +  reg:
> +    description: Two regions, one for the SMC area and one for the SRAM area.

You need constraints for size/order, so in this context list with
described items.

> +
> +  reg-names:
> +    items:
> +      - const: smc
> +      - const: sram
> +
> +  mboxes:
> +    description:
> +      A phandle to the mailbox channel

Missing maxItems

> +
> +additionalProperties: false
> +
Best regards,
Krzysztof

Re: [PATCH 2/6] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 01/09/2022 16:54, Russell King (Oracle) wrote:
> Add the DT binding for the Apple Mac System Management Controller GPIOs.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
>  .../devicetree/bindings/mfd/apple,smc.yaml    |  4 +++
>  2 files changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> new file mode 100644
> index 000000000000..ee620fe50ca8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#

filename based on the compatible... which is missing here, so this leads
to the question - why this is separate device/child node? It won't bind
to anything, won't be populated, right?

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple Mac System Management Controller GPIO
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description:
> +  This describes the binding for the Apple Mac System Management Controller
> +  GPIO block.
> +
> +properties:
> +  gpio-controller: true
> +  '#gpio-cells':
> +    const: 2
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    smc_gpio: gpio {
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +    };
> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> index 794d3a6eb04a..bb799a27638d 100644
> --- a/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> @@ -34,6 +34,10 @@ title: Apple Mac System Management Controller
>      description:
>        A phandle to the mailbox channel
>  
> +patternProperties:

This is not a pattern, so "gpio" goes to "properties:".

> +  gpio:
> +    $ref: /schemas/gpio/gpio-macsmc.yaml
> +
>  additionalProperties: false
>  
>  required:


Best regards,
Krzysztof

Re: [PATCH 0/6] Add Apple Mac System Management Controller GPIOs

[Krzysztof Kozlowski]

On 01/09/2022 16:54, Russell King (Oracle) wrote:
> Hi,
> 
> This series adds support for the Apple Mac GPIO driver. These GPIOs
> are hadled via the System Management Controller.
> 

Thank you for your patch. There is something to discuss/improve.

> The first two patches add the DT binding documentation for the new
> drivers.
> 
> The second two patches add the core System Management Controller
> support.
> 
> The last two patches add the GPIO support.
> 
> DT updates will follow once the bindings have been reviewed.

...and how can we validate that your DTS actually implements the
bindings? Typical submission includes everything, either in one patchset
or at least linked to each other so we can see the user of the bindings.

Best regards,
Krzysztof

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Sep 01, 2022 at 06:06:17PM +0300, Krzysztof Kozlowski wrote:
> On 01/09/2022 16:54, Russell King (Oracle) wrote:
> > Add a DT binding for the Apple Mac System Management Controller.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +
> > +maintainers:
> > +  - Hector Martin <marcan@marcan.st>
> > +
> > +description:
> > +  Apple Mac System Management Controller implements various functions
> > +  such as GPIO, RTC, power, reboot.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +        - apple,t8103-smc
> 
> You miss two spaces of indentation on this level.

Should that be picked up by the dt checker?

> > +        - apple,t8112-smc
> > +        - apple,t6000-smc
> 
> Bring some order here - either alphabetical or by date of release (as in
> other Apple schemas). I think t6000 was before t8112, so it's none of
> that orders.

Ok.

> > +      - const: apple,smc
> > +
> > +  reg:
> > +    description: Two regions, one for the SMC area and one for the SRAM area.
> 
> You need constraints for size/order, so in this context list with
> described items.

How do I do that? I tried maxItems/minItems set to 2, but the dt checker
objected to it.

> > +  reg-names:
> > +    items:
> > +      - const: smc
> > +      - const: sram
> > +
> > +  mboxes:
> > +    description:
> > +      A phandle to the mailbox channel
> 
> Missing maxItems

Ok. Would be helpful if the dt checker identified that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/6] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

On Thu, Sep 01, 2022 at 06:11:30PM +0300, Krzysztof Kozlowski wrote:
> On 01/09/2022 16:54, Russell King (Oracle) wrote:
> > Add the DT binding for the Apple Mac System Management Controller GPIOs.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
> >  .../devicetree/bindings/mfd/apple,smc.yaml    |  4 +++
> >  2 files changed, 32 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> > new file mode 100644
> > index 000000000000..ee620fe50ca8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> > @@ -0,0 +1,28 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
> 
> filename based on the compatible... which is missing here, so this leads
> to the question - why this is separate device/child node? It won't bind
> to anything, won't be populated, right?

The node is a child of the apple,smc mfd device, and is referenced from
that description. Should this be embedded in the DT description for the
apple,smc device?

It'll also have other nodes as well, for the RTC etc.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 01/09/2022 18:12, Russell King (Oracle) wrote:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +        - apple,t8103-smc
>>
>> You miss two spaces of indentation on this level.
> 
> Should that be picked up by the dt checker?

I think yamllint complains about it. It is not a hard-dependency, so
maybe you don't have it installed.

> 
>>> +        - apple,t8112-smc
>>> +        - apple,t6000-smc
>>
>> Bring some order here - either alphabetical or by date of release (as in
>> other Apple schemas). I think t6000 was before t8112, so it's none of
>> that orders.
> 
> Ok.
> 
>>> +      - const: apple,smc
>>> +
>>> +  reg:
>>> +    description: Two regions, one for the SMC area and one for the SRAM area.
>>
>> You need constraints for size/order, so in this context list with
>> described items.
> 
> How do I do that? I tried maxItems/minItems set to 2, but the dt checker
> objected to it.

One way:
reg:
  items:
    - description: SMC area
    - description: SRAM area

but actually this is very similar what you wrote for reg-names - kind of
obvious, so easier way:

reg:
  maxItems: 2


> 
>>> +  reg-names:
>>> +    items:
>>> +      - const: smc
>>> +      - const: sram
>>> +
>>> +  mboxes:
>>> +    description:
>>> +      A phandle to the mailbox channel
>>
>> Missing maxItems
> 
> Ok. Would be helpful if the dt checker identified that.

Patches are welcomed :)

Best regards,
Krzysztof

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote:
> On 01/09/2022 18:12, Russell King (Oracle) wrote:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +        - apple,t8103-smc
> >>
> >> You miss two spaces of indentation on this level.
> > 
> > Should that be picked up by the dt checker?
> 
> I think yamllint complains about it. It is not a hard-dependency, so
> maybe you don't have it installed.
> 
> > 
> >>> +        - apple,t8112-smc
> >>> +        - apple,t6000-smc
> >>
> >> Bring some order here - either alphabetical or by date of release (as in
> >> other Apple schemas). I think t6000 was before t8112, so it's none of
> >> that orders.
> > 
> > Ok.
> > 
> >>> +      - const: apple,smc
> >>> +
> >>> +  reg:
> >>> +    description: Two regions, one for the SMC area and one for the SRAM area.
> >>
> >> You need constraints for size/order, so in this context list with
> >> described items.
> > 
> > How do I do that? I tried maxItems/minItems set to 2, but the dt checker
> > objected to it.
> 
> One way:
> reg:
>   items:
>     - description: SMC area
>     - description: SRAM area
> 
> but actually this is very similar what you wrote for reg-names - kind of
> obvious, so easier way:
> 
> reg:
>   maxItems: 2

Doesn't work. With maxItems: 2, the example fails, yet it correctly lists
two regs which are 64-bit address and 64-bit size - so in total 8 32-bit
ints.

Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long
        From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml

Hence, I originally had maxItems: 2, and ended up deleting it because of
the dt checker.

With the two descriptions, it's the same failure.

I think the problem is that the checker has no knowledge in the example
of how big each address and size element of the reg property is. So,
it's interpreting it as four entries of 32-bit address,size pairs
instead of two entries of 64-bit address,size pairs. Yep, that's it,
if I increase the number of "- description" entries to four then it's
happy.

So, what's the solution?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 01/09/2022 18:24, Russell King (Oracle) wrote:
> On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote:
>> On 01/09/2022 18:12, Russell King (Oracle) wrote:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - enum:
>>>>> +        - apple,t8103-smc
>>>>
>>>> You miss two spaces of indentation on this level.
>>>
>>> Should that be picked up by the dt checker?
>>
>> I think yamllint complains about it. It is not a hard-dependency, so
>> maybe you don't have it installed.
>>
>>>
>>>>> +        - apple,t8112-smc
>>>>> +        - apple,t6000-smc
>>>>
>>>> Bring some order here - either alphabetical or by date of release (as in
>>>> other Apple schemas). I think t6000 was before t8112, so it's none of
>>>> that orders.
>>>
>>> Ok.
>>>
>>>>> +      - const: apple,smc
>>>>> +
>>>>> +  reg:
>>>>> +    description: Two regions, one for the SMC area and one for the SRAM area.
>>>>
>>>> You need constraints for size/order, so in this context list with
>>>> described items.
>>>
>>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker
>>> objected to it.
>>
>> One way:
>> reg:
>>   items:
>>     - description: SMC area
>>     - description: SRAM area
>>
>> but actually this is very similar what you wrote for reg-names - kind of
>> obvious, so easier way:
>>
>> reg:
>>   maxItems: 2
> 
> Doesn't work. With maxItems: 2, the example fails, yet it correctly lists
> two regs which are 64-bit address and 64-bit size - so in total 8 32-bit
> ints.
> 
> Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long
>         From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> 
> Hence, I originally had maxItems: 2, and ended up deleting it because of
> the dt checker.
> 
> With the two descriptions, it's the same failure.

Yeah, they should create same result.

> 
> I think the problem is that the checker has no knowledge in the example
> of how big each address and size element of the reg property is. So,
> it's interpreting it as four entries of 32-bit address,size pairs
> instead of two entries of 64-bit address,size pairs. Yep, that's it,
> if I increase the number of "- description" entries to four then it's
> happy.
> 
> So, what's the solution?
> 

If you open generated DTS examples (in your
kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which
address/size cells are expected. By default it is I think address/size
cells=1, so you need a bus node setting it to 2.

Best regards,
Krzysztof

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Sep 01, 2022 at 06:45:52PM +0300, Krzysztof Kozlowski wrote:
> On 01/09/2022 18:24, Russell King (Oracle) wrote:
> > On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote:
> >> On 01/09/2022 18:12, Russell King (Oracle) wrote:
> >>>>> +  compatible:
> >>>>> +    items:
> >>>>> +      - enum:
> >>>>> +        - apple,t8103-smc
> >>>>
> >>>> You miss two spaces of indentation on this level.
> >>>
> >>> Should that be picked up by the dt checker?
> >>
> >> I think yamllint complains about it. It is not a hard-dependency, so
> >> maybe you don't have it installed.
> >>
> >>>
> >>>>> +        - apple,t8112-smc
> >>>>> +        - apple,t6000-smc
> >>>>
> >>>> Bring some order here - either alphabetical or by date of release (as in
> >>>> other Apple schemas). I think t6000 was before t8112, so it's none of
> >>>> that orders.
> >>>
> >>> Ok.
> >>>
> >>>>> +      - const: apple,smc
> >>>>> +
> >>>>> +  reg:
> >>>>> +    description: Two regions, one for the SMC area and one for the SRAM area.
> >>>>
> >>>> You need constraints for size/order, so in this context list with
> >>>> described items.
> >>>
> >>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker
> >>> objected to it.
> >>
> >> One way:
> >> reg:
> >>   items:
> >>     - description: SMC area
> >>     - description: SRAM area
> >>
> >> but actually this is very similar what you wrote for reg-names - kind of
> >> obvious, so easier way:
> >>
> >> reg:
> >>   maxItems: 2
> > 
> > Doesn't work. With maxItems: 2, the example fails, yet it correctly lists
> > two regs which are 64-bit address and 64-bit size - so in total 8 32-bit
> > ints.
> > 
> > Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long
> >         From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > 
> > Hence, I originally had maxItems: 2, and ended up deleting it because of
> > the dt checker.
> > 
> > With the two descriptions, it's the same failure.
> 
> Yeah, they should create same result.
> 
> > 
> > I think the problem is that the checker has no knowledge in the example
> > of how big each address and size element of the reg property is. So,
> > it's interpreting it as four entries of 32-bit address,size pairs
> > instead of two entries of 64-bit address,size pairs. Yep, that's it,
> > if I increase the number of "- description" entries to four then it's
> > happy.
> > 
> > So, what's the solution?
> > 
> 
> If you open generated DTS examples (in your
> kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which
> address/size cells are expected. By default it is I think address/size
> cells=1, so you need a bus node setting it to 2.

Thanks, that works. The patch with all those points addressed now looks
like:

8<===
From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
 Controller

Add a DT binding for the Apple Mac System Management Controller.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../devicetree/bindings/mfd/apple,smc.yaml    | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml

diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
new file mode 100644
index 000000000000..168f237c2962
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Mac System Management Controller
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+description:
+  Apple Mac System Management Controller implements various functions
+  such as GPIO, RTC, power, reboot.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - apple,t6000-smc
+          - apple,t8103-smc
+          - apple,t8112-smc
+      - const: apple,smc
+
+  reg:
+    items:
+      - description: SMC area
+      - description: SRAM area
+
+  reg-names:
+    items:
+      - const: smc
+      - const: sram
+
+  mboxes:
+    maxItems: 1
+    description:
+      A phandle to the mailbox channel
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - mboxes
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      smc@23e400000 {
+        compatible = "apple,t8103-smc", "apple,smc";
+        reg = <0x2 0x3e400000 0x0 0x4000>,
+               <0x2 0x3fe00000 0x0 0x100000>;
+        reg-names = "smc", "sram";
+        mboxes = <&smc_mbox>;
+      };
+    };
-- 
2.30.2


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 01/09/2022 18:56, Russell King (Oracle) wrote:
> 
> 8<===
> From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
>  Controller
> 
> Add a DT binding for the Apple Mac System Management Controller.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Yes, looks good.

I won't add Reviewed-by tag, because I think it would confuse Patchwork,
so please send a v2 at some point.

Best regards,
Krzysztof

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote:
> On 01/09/2022 18:56, Russell King (Oracle) wrote:
> > 
> > 8<===
> > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
> >  Controller
> > 
> > Add a DT binding for the Apple Mac System Management Controller.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> Yes, looks good.
> 
> I won't add Reviewed-by tag, because I think it would confuse Patchwork,
> so please send a v2 at some point.

Thanks. Do you have any suggestions for patch 2? Should I merge the
description in patch 2 into this file?

The full dts for this series looks like this:

                smc: smc@23e400000 {
                        compatible = "apple,t8103-smc", "apple,smc";
                        reg = <0x2 0x3e400000 0x0 0x4000>,
                                <0x2 0x3fe00000 0x0 0x100000>;
                        reg-names = "smc", "sram";
                        mboxes = <&smc_mbox>;

                        smc_gpio: gpio {
                                gpio-controller;
                                #gpio-cells = <2>;
                        };
                };

but the fuller version in the asahi linux tree looks like:

		smc: smc@23e400000 {
			compatible = "apple,t8103-smc", "apple,smc";
			reg = <0x2 0x3e400000 0x0 0x4000>,
				<0x2 0x3fe00000 0x0 0x100000>;
			reg-names = "smc", "sram";
			mboxes = <&smc_mbox>;

			smc_gpio: gpio {
				gpio-controller;
				#gpio-cells = <2>;
			};

			smc_rtc: rtc {
				nvmem-cells = <&rtc_offset>;
				nvmem-cell-names = "rtc_offset";
			};

			smc_reboot: reboot {
				nvmem-cells = <&shutdown_flag>, <&boot_stage>,
					<&boot_error_count>, <&panic_count>, <&pm_setting>;
				nvmem-cell-names = "shutdown_flag", "boot_stage",
					"boot_error_count", "panic_count", "pm_setting";
			};
		};

So whatever is decided for the gpio node would eventually also apply
to the other nodes too.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Thu, 01 Sep 2022 14:54:25 +0100, Russell King (Oracle) wrote:
> Add a DT binding for the Apple Mac System Management Controller.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/mfd/apple,smc.yaml    | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/mfd/apple,smc.yaml:20:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Thu, Sep 1, 2022 at 10:56 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Sep 01, 2022 at 06:45:52PM +0300, Krzysztof Kozlowski wrote:
> > On 01/09/2022 18:24, Russell King (Oracle) wrote:
> > > On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote:
> > >> On 01/09/2022 18:12, Russell King (Oracle) wrote:
> > >>>>> +  compatible:
> > >>>>> +    items:
> > >>>>> +      - enum:
> > >>>>> +        - apple,t8103-smc
> > >>>>
> > >>>> You miss two spaces of indentation on this level.
> > >>>
> > >>> Should that be picked up by the dt checker?

I have a problem that Krzysztof is quicker. ;) Maybe I should stop
screening the emails (for the times I break things mostly).

> > >>
> > >> I think yamllint complains about it. It is not a hard-dependency, so
> > >> maybe you don't have it installed.
> > >>
> > >>>
> > >>>>> +        - apple,t8112-smc
> > >>>>> +        - apple,t6000-smc
> > >>>>
> > >>>> Bring some order here - either alphabetical or by date of release (as in
> > >>>> other Apple schemas). I think t6000 was before t8112, so it's none of
> > >>>> that orders.
> > >>>
> > >>> Ok.
> > >>>
> > >>>>> +      - const: apple,smc
> > >>>>> +
> > >>>>> +  reg:
> > >>>>> +    description: Two regions, one for the SMC area and one for the SRAM area.
> > >>>>
> > >>>> You need constraints for size/order, so in this context list with
> > >>>> described items.
> > >>>
> > >>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker
> > >>> objected to it.
> > >>
> > >> One way:
> > >> reg:
> > >>   items:
> > >>     - description: SMC area
> > >>     - description: SRAM area
> > >>
> > >> but actually this is very similar what you wrote for reg-names - kind of
> > >> obvious, so easier way:
> > >>
> > >> reg:
> > >>   maxItems: 2
> > >
> > > Doesn't work. With maxItems: 2, the example fails, yet it correctly lists
> > > two regs which are 64-bit address and 64-bit size - so in total 8 32-bit
> > > ints.
> > >
> > > Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long
> > >         From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > >
> > > Hence, I originally had maxItems: 2, and ended up deleting it because of
> > > the dt checker.
> > >
> > > With the two descriptions, it's the same failure.
> >
> > Yeah, they should create same result.
> >
> > >
> > > I think the problem is that the checker has no knowledge in the example
> > > of how big each address and size element of the reg property is. So,
> > > it's interpreting it as four entries of 32-bit address,size pairs
> > > instead of two entries of 64-bit address,size pairs. Yep, that's it,
> > > if I increase the number of "- description" entries to four then it's
> > > happy.
> > >
> > > So, what's the solution?
> > >
> >
> > If you open generated DTS examples (in your
> > kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which
> > address/size cells are expected. By default it is I think address/size
> > cells=1, so you need a bus node setting it to 2.
>
> Thanks, that works. The patch with all those points addressed now looks
> like:
>
> 8<===
> From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
>  Controller
>
> Add a DT binding for the Apple Mac System Management Controller.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/mfd/apple,smc.yaml    | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> new file mode 100644
> index 000000000000..168f237c2962
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple Mac System Management Controller
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description:
> +  Apple Mac System Management Controller implements various functions
> +  such as GPIO, RTC, power, reboot.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t6000-smc
> +          - apple,t8103-smc
> +          - apple,t8112-smc
> +      - const: apple,smc
> +
> +  reg:
> +    items:
> +      - description: SMC area
> +      - description: SRAM area

Based on the disjoint addresses, is this really one device? Perhaps
the SRAM area should use mmio-sram binding? That already supports
sub-dividing the sram for different uses. I'll comment more on the
full example.

Rob

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote:
> > On 01/09/2022 18:56, Russell King (Oracle) wrote:
> > >
> > > 8<===
> > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
> > >  Controller
> > >
> > > Add a DT binding for the Apple Mac System Management Controller.
> > >
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> >
> > Yes, looks good.
> >
> > I won't add Reviewed-by tag, because I think it would confuse Patchwork,
> > so please send a v2 at some point.
>
> Thanks. Do you have any suggestions for patch 2? Should I merge the
> description in patch 2 into this file?
>
> The full dts for this series looks like this:
>
>                 smc: smc@23e400000 {
>                         compatible = "apple,t8103-smc", "apple,smc";
>                         reg = <0x2 0x3e400000 0x0 0x4000>,
>                                 <0x2 0x3fe00000 0x0 0x100000>;
>                         reg-names = "smc", "sram";
>                         mboxes = <&smc_mbox>;
>
>                         smc_gpio: gpio {
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>                         };
>                 };
>
> but the fuller version in the asahi linux tree looks like:
>
>                 smc: smc@23e400000 {
>                         compatible = "apple,t8103-smc", "apple,smc";
>                         reg = <0x2 0x3e400000 0x0 0x4000>,
>                                 <0x2 0x3fe00000 0x0 0x100000>;
>                         reg-names = "smc", "sram";
>                         mboxes = <&smc_mbox>;
>
>                         smc_gpio: gpio {
>                                 gpio-controller;
>                                 #gpio-cells = <2>;

Only 2 properties doesn't really need its own schema doc. However, I
would just move these to the parent node.

>                         };
>
>                         smc_rtc: rtc {
>                                 nvmem-cells = <&rtc_offset>;
>                                 nvmem-cell-names = "rtc_offset";
>                         };
>
>                         smc_reboot: reboot {
>                                 nvmem-cells = <&shutdown_flag>, <&boot_stage>,
>                                         <&boot_error_count>, <&panic_count>, <&pm_setting>;
>                                 nvmem-cell-names = "shutdown_flag", "boot_stage",
>                                         "boot_error_count", "panic_count", "pm_setting";

Not really much reason to split these up either because you can just
fetch the entry you want by name.

How confident are the asahi folks that this is a complete binding?

Rob

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Mark Kettenis]

> From: Rob Herring <robh+dt@kernel.org>
> Date: Thu, 1 Sep 2022 17:26:18 -0500
> 
> On Thu, Sep 1, 2022 at 10:56 AM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, Sep 01, 2022 at 06:45:52PM +0300, Krzysztof Kozlowski wrote:
> > > On 01/09/2022 18:24, Russell King (Oracle) wrote:
> > > > On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote:
> > > >> On 01/09/2022 18:12, Russell King (Oracle) wrote:
> > > >>>>> +  compatible:
> > > >>>>> +    items:
> > > >>>>> +      - enum:
> > > >>>>> +        - apple,t8103-smc
> > > >>>>
> > > >>>> You miss two spaces of indentation on this level.
> > > >>>
> > > >>> Should that be picked up by the dt checker?
> 
> I have a problem that Krzysztof is quicker. ;) Maybe I should stop
> screening the emails (for the times I break things mostly).
> 
> > > >>
> > > >> I think yamllint complains about it. It is not a hard-dependency, so
> > > >> maybe you don't have it installed.
> > > >>
> > > >>>
> > > >>>>> +        - apple,t8112-smc
> > > >>>>> +        - apple,t6000-smc
> > > >>>>
> > > >>>> Bring some order here - either alphabetical or by date of release (as in
> > > >>>> other Apple schemas). I think t6000 was before t8112, so it's none of
> > > >>>> that orders.
> > > >>>
> > > >>> Ok.
> > > >>>
> > > >>>>> +      - const: apple,smc
> > > >>>>> +
> > > >>>>> +  reg:
> > > >>>>> +    description: Two regions, one for the SMC area and one for the SRAM area.
> > > >>>>
> > > >>>> You need constraints for size/order, so in this context list with
> > > >>>> described items.
> > > >>>
> > > >>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker
> > > >>> objected to it.
> > > >>
> > > >> One way:
> > > >> reg:
> > > >>   items:
> > > >>     - description: SMC area
> > > >>     - description: SRAM area
> > > >>
> > > >> but actually this is very similar what you wrote for reg-names - kind of
> > > >> obvious, so easier way:
> > > >>
> > > >> reg:
> > > >>   maxItems: 2
> > > >
> > > > Doesn't work. With maxItems: 2, the example fails, yet it correctly lists
> > > > two regs which are 64-bit address and 64-bit size - so in total 8 32-bit
> > > > ints.
> > > >
> > > > Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long
> > > >         From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > >
> > > > Hence, I originally had maxItems: 2, and ended up deleting it because of
> > > > the dt checker.
> > > >
> > > > With the two descriptions, it's the same failure.
> > >
> > > Yeah, they should create same result.
> > >
> > > >
> > > > I think the problem is that the checker has no knowledge in the example
> > > > of how big each address and size element of the reg property is. So,
> > > > it's interpreting it as four entries of 32-bit address,size pairs
> > > > instead of two entries of 64-bit address,size pairs. Yep, that's it,
> > > > if I increase the number of "- description" entries to four then it's
> > > > happy.
> > > >
> > > > So, what's the solution?
> > > >
> > >
> > > If you open generated DTS examples (in your
> > > kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which
> > > address/size cells are expected. By default it is I think address/size
> > > cells=1, so you need a bus node setting it to 2.
> >
> > Thanks, that works. The patch with all those points addressed now looks
> > like:
> >
> > 8<===
> > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
> >  Controller
> >
> > Add a DT binding for the Apple Mac System Management Controller.
> >
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../devicetree/bindings/mfd/apple,smc.yaml    | 61 +++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > new file mode 100644
> > index 000000000000..168f237c2962
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Apple Mac System Management Controller
> > +
> > +maintainers:
> > +  - Hector Martin <marcan@marcan.st>
> > +
> > +description:
> > +  Apple Mac System Management Controller implements various functions
> > +  such as GPIO, RTC, power, reboot.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - apple,t6000-smc
> > +          - apple,t8103-smc
> > +          - apple,t8112-smc
> > +      - const: apple,smc
> > +
> > +  reg:
> > +    items:
> > +      - description: SMC area
> > +      - description: SRAM area
> 
> Based on the disjoint addresses, is this really one device? Perhaps
> the SRAM area should use mmio-sram binding? That already supports
> sub-dividing the sram for different uses. I'll comment more on the
> full example.

To me it does look as if the SRAM is part of the SMC coprocessor
block.  It is probably part of a larger SRAM on the side of the SMC
coprocessor.  There is a gap, but the addresses are close.  The only
thing in between is the SMC mailbox, which is represented by a
separate node.

The address of the SRAM can be discovered by sending SMC commands.  I
believe Hector added it in order to verify the address that the SMC
firmware provides.  My OpenBSD driver doesn't use it, so in that sense
changing the binding to use a separate node with a "mmio-sram"
compatible (and presumably an "apple,sram" property to reference that
node using a phandle) would work fine.  The extra level of indirection
obviously would mean more code in the Linux SMC driver though.

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Mark Kettenis]

> From: Rob Herring <robh+dt@kernel.org>
> Date: Thu, 1 Sep 2022 17:33:31 -0500
> 
> On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote:
> > > On 01/09/2022 18:56, Russell King (Oracle) wrote:
> > > >
> > > > 8<===
> > > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
> > > >  Controller
> > > >
> > > > Add a DT binding for the Apple Mac System Management Controller.
> > > >
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > >
> > > Yes, looks good.
> > >
> > > I won't add Reviewed-by tag, because I think it would confuse Patchwork,
> > > so please send a v2 at some point.
> >
> > Thanks. Do you have any suggestions for patch 2? Should I merge the
> > description in patch 2 into this file?
> >
> > The full dts for this series looks like this:
> >
> >                 smc: smc@23e400000 {
> >                         compatible = "apple,t8103-smc", "apple,smc";
> >                         reg = <0x2 0x3e400000 0x0 0x4000>,
> >                                 <0x2 0x3fe00000 0x0 0x100000>;
> >                         reg-names = "smc", "sram";
> >                         mboxes = <&smc_mbox>;
> >
> >                         smc_gpio: gpio {
> >                                 gpio-controller;
> >                                 #gpio-cells = <2>;
> >                         };
> >                 };
> >
> > but the fuller version in the asahi linux tree looks like:
> >
> >                 smc: smc@23e400000 {
> >                         compatible = "apple,t8103-smc", "apple,smc";
> >                         reg = <0x2 0x3e400000 0x0 0x4000>,
> >                                 <0x2 0x3fe00000 0x0 0x100000>;
> >                         reg-names = "smc", "sram";
> >                         mboxes = <&smc_mbox>;
> >
> >                         smc_gpio: gpio {
> >                                 gpio-controller;
> >                                 #gpio-cells = <2>;
> 
> Only 2 properties doesn't really need its own schema doc. However, I
> would just move these to the parent node.

When we designed the bindings, it was our understanding that having
separate nodes better matches Linux's MFD driver model.

Please be aware that OpenBSD is already using these bindings.  If
there are good reasons for moving things, we can probably deal with
that.  But this sounds a bit like a toss up.

> 
> >                         };
> >
> >                         smc_rtc: rtc {
> >                                 nvmem-cells = <&rtc_offset>;
> >                                 nvmem-cell-names = "rtc_offset";
> >                         };
> >
> >                         smc_reboot: reboot {
> >                                 nvmem-cells = <&shutdown_flag>, <&boot_stage>,
> >                                         <&boot_error_count>, <&panic_count>, <&pm_setting>;
> >                                 nvmem-cell-names = "shutdown_flag", "boot_stage",
> >                                         "boot_error_count", "panic_count", "pm_setting";
> 
> Not really much reason to split these up either because you can just
> fetch the entry you want by name.

Again the separate nodes are there because the RTC and the reboot
functionality are logically separate and handled by different MFD
sub-drivers in Linux.

> How confident are the asahi folks that this is a complete binding?

There is a lot of functionality in the SMC that is still largely
unexplored.  The idea of the design of the binding is that additional
functionality may be added by adding more subnodes to the smc node.
But we expect that the main SMC node itself should be complete with
the "smc" and "sram" regions and the reference to the mailbox.

Cheers,

Mark

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Fri, Sep 02, 2022 at 04:49:37PM +0200, Mark Kettenis wrote:
> > From: Rob Herring <robh+dt@kernel.org>
> > Date: Thu, 1 Sep 2022 17:26:18 -0500
> > 
> > On Thu, Sep 1, 2022 at 10:56 AM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Thu, Sep 01, 2022 at 06:45:52PM +0300, Krzysztof Kozlowski wrote:
> > > > On 01/09/2022 18:24, Russell King (Oracle) wrote:
> > > > > On Thu, Sep 01, 2022 at 06:15:46PM +0300, Krzysztof Kozlowski wrote:
> > > > >> On 01/09/2022 18:12, Russell King (Oracle) wrote:
> > > > >>>>> +  compatible:
> > > > >>>>> +    items:
> > > > >>>>> +      - enum:
> > > > >>>>> +        - apple,t8103-smc
> > > > >>>>
> > > > >>>> You miss two spaces of indentation on this level.
> > > > >>>
> > > > >>> Should that be picked up by the dt checker?
> > 
> > I have a problem that Krzysztof is quicker. ;) Maybe I should stop
> > screening the emails (for the times I break things mostly).
> > 
> > > > >>
> > > > >> I think yamllint complains about it. It is not a hard-dependency, so
> > > > >> maybe you don't have it installed.
> > > > >>
> > > > >>>
> > > > >>>>> +        - apple,t8112-smc
> > > > >>>>> +        - apple,t6000-smc
> > > > >>>>
> > > > >>>> Bring some order here - either alphabetical or by date of release (as in
> > > > >>>> other Apple schemas). I think t6000 was before t8112, so it's none of
> > > > >>>> that orders.
> > > > >>>
> > > > >>> Ok.
> > > > >>>
> > > > >>>>> +      - const: apple,smc
> > > > >>>>> +
> > > > >>>>> +  reg:
> > > > >>>>> +    description: Two regions, one for the SMC area and one for the SRAM area.
> > > > >>>>
> > > > >>>> You need constraints for size/order, so in this context list with
> > > > >>>> described items.
> > > > >>>
> > > > >>> How do I do that? I tried maxItems/minItems set to 2, but the dt checker
> > > > >>> objected to it.
> > > > >>
> > > > >> One way:
> > > > >> reg:
> > > > >>   items:
> > > > >>     - description: SMC area
> > > > >>     - description: SRAM area
> > > > >>
> > > > >> but actually this is very similar what you wrote for reg-names - kind of
> > > > >> obvious, so easier way:
> > > > >>
> > > > >> reg:
> > > > >>   maxItems: 2
> > > > >
> > > > > Doesn't work. With maxItems: 2, the example fails, yet it correctly lists
> > > > > two regs which are 64-bit address and 64-bit size - so in total 8 32-bit
> > > > > ints.
> > > > >
> > > > > Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: reg: [[2, 1044381696], [0, 16384], [2, 1071644672], [0, 1048576]] is too long
> > > > >         From schema: /home/rmk/git/linux-rmk/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > >
> > > > > Hence, I originally had maxItems: 2, and ended up deleting it because of
> > > > > the dt checker.
> > > > >
> > > > > With the two descriptions, it's the same failure.
> > > >
> > > > Yeah, they should create same result.
> > > >
> > > > >
> > > > > I think the problem is that the checker has no knowledge in the example
> > > > > of how big each address and size element of the reg property is. So,
> > > > > it's interpreting it as four entries of 32-bit address,size pairs
> > > > > instead of two entries of 64-bit address,size pairs. Yep, that's it,
> > > > > if I increase the number of "- description" entries to four then it's
> > > > > happy.
> > > > >
> > > > > So, what's the solution?
> > > > >
> > > >
> > > > If you open generated DTS examples (in your
> > > > kbuild-output/Documentation/devicetree/bindings/mfd/) you will see which
> > > > address/size cells are expected. By default it is I think address/size
> > > > cells=1, so you need a bus node setting it to 2.
> > >
> > > Thanks, that works. The patch with all those points addressed now looks
> > > like:
> > >
> > > 8<===
> > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
> > >  Controller
> > >
> > > Add a DT binding for the Apple Mac System Management Controller.
> > >
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  .../devicetree/bindings/mfd/apple,smc.yaml    | 61 +++++++++++++++++++
> > >  1 file changed, 61 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > new file mode 100644
> > > index 000000000000..168f237c2962
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > @@ -0,0 +1,61 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Apple Mac System Management Controller
> > > +
> > > +maintainers:
> > > +  - Hector Martin <marcan@marcan.st>
> > > +
> > > +description:
> > > +  Apple Mac System Management Controller implements various functions
> > > +  such as GPIO, RTC, power, reboot.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - apple,t6000-smc
> > > +          - apple,t8103-smc
> > > +          - apple,t8112-smc
> > > +      - const: apple,smc
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: SMC area
> > > +      - description: SRAM area
> > 
> > Based on the disjoint addresses, is this really one device? Perhaps
> > the SRAM area should use mmio-sram binding? That already supports
> > sub-dividing the sram for different uses. I'll comment more on the
> > full example.
> 
> To me it does look as if the SRAM is part of the SMC coprocessor
> block.  It is probably part of a larger SRAM on the side of the SMC
> coprocessor.  There is a gap, but the addresses are close.  The only
> thing in between is the SMC mailbox, which is represented by a
> separate node.

Okay, fair enough. Let's keep them together.

Rob

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Fri, Sep 02, 2022 at 05:06:43PM +0200, Mark Kettenis wrote:
> > From: Rob Herring <robh+dt@kernel.org>
> > Date: Thu, 1 Sep 2022 17:33:31 -0500
> > 
> > On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote:
> > > > On 01/09/2022 18:56, Russell King (Oracle) wrote:
> > > > >
> > > > > 8<===
> > > > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > > > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
> > > > >  Controller
> > > > >
> > > > > Add a DT binding for the Apple Mac System Management Controller.
> > > > >
> > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > >
> > > > Yes, looks good.
> > > >
> > > > I won't add Reviewed-by tag, because I think it would confuse Patchwork,
> > > > so please send a v2 at some point.
> > >
> > > Thanks. Do you have any suggestions for patch 2? Should I merge the
> > > description in patch 2 into this file?
> > >
> > > The full dts for this series looks like this:
> > >
> > >                 smc: smc@23e400000 {
> > >                         compatible = "apple,t8103-smc", "apple,smc";
> > >                         reg = <0x2 0x3e400000 0x0 0x4000>,
> > >                                 <0x2 0x3fe00000 0x0 0x100000>;
> > >                         reg-names = "smc", "sram";
> > >                         mboxes = <&smc_mbox>;
> > >
> > >                         smc_gpio: gpio {
> > >                                 gpio-controller;
> > >                                 #gpio-cells = <2>;
> > >                         };
> > >                 };
> > >
> > > but the fuller version in the asahi linux tree looks like:
> > >
> > >                 smc: smc@23e400000 {
> > >                         compatible = "apple,t8103-smc", "apple,smc";
> > >                         reg = <0x2 0x3e400000 0x0 0x4000>,
> > >                                 <0x2 0x3fe00000 0x0 0x100000>;
> > >                         reg-names = "smc", "sram";
> > >                         mboxes = <&smc_mbox>;
> > >
> > >                         smc_gpio: gpio {
> > >                                 gpio-controller;
> > >                                 #gpio-cells = <2>;
> > 
> > Only 2 properties doesn't really need its own schema doc. However, I
> > would just move these to the parent node.
> 
> When we designed the bindings, it was our understanding that having
> separate nodes better matches Linux's MFD driver model.

Well, it is convenient to have subnodes with compatibles so that your 
drivers automagically probe. So yes, a 1:1 relationship of nodes to 
drivers is nice and tidy. But h/w is not always packaged up neatly and 
it's not DT's job to try to abstract it such that it is. Also, we 
shouldn't design bindings around the *current* driver partitioning of 
some OS.

This one is actually pretty odd in that the child nodes don't have a 
compatible string which breaks the automagical probing.

> Please be aware that OpenBSD is already using these bindings.  If
> there are good reasons for moving things, we can probably deal with
> that.  But this sounds a bit like a toss up.

Sigh. If there are other bindings in use, please submit them even if the 
Linux driver isn't ready. If a Linux subsystem maintainer doesn't want 
to take it, then I will. 

It is a toss up though...

> > >                         };
> > >
> > >                         smc_rtc: rtc {
> > >                                 nvmem-cells = <&rtc_offset>;
> > >                                 nvmem-cell-names = "rtc_offset";
> > >                         };
> > >
> > >                         smc_reboot: reboot {
> > >                                 nvmem-cells = <&shutdown_flag>, <&boot_stage>,
> > >                                         <&boot_error_count>, <&panic_count>, <&pm_setting>;
> > >                                 nvmem-cell-names = "shutdown_flag", "boot_stage",
> > >                                         "boot_error_count", "panic_count", "pm_setting";
> > 
> > Not really much reason to split these up either because you can just
> > fetch the entry you want by name.
> 
> Again the separate nodes are there because the RTC and the reboot
> functionality are logically separate and handled by different MFD
> sub-drivers in Linux.

It's really a question of whether the subset of functionality is going 
to get reused on its own or has its own resources in DT. MFD bindings 
are done both ways.

Rob

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Fri, Sep 02, 2022 at 12:28:08PM -0500, Rob Herring wrote:
> On Fri, Sep 02, 2022 at 05:06:43PM +0200, Mark Kettenis wrote:
> > > From: Rob Herring <robh+dt@kernel.org>
> > > Date: Thu, 1 Sep 2022 17:33:31 -0500
> > > 
> > > On Thu, Sep 1, 2022 at 11:47 AM Russell King (Oracle)
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Thu, Sep 01, 2022 at 07:25:03PM +0300, Krzysztof Kozlowski wrote:
> > > > > On 01/09/2022 18:56, Russell King (Oracle) wrote:
> > > > > >
> > > > > > 8<===
> > > > > > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > > > > > Subject: [PATCH] dt-bindings: mfd: add binding for Apple Mac System Management
> > > > > >  Controller
> > > > > >
> > > > > > Add a DT binding for the Apple Mac System Management Controller.
> > > > > >
> > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > >
> > > > > Yes, looks good.
> > > > >
> > > > > I won't add Reviewed-by tag, because I think it would confuse Patchwork,
> > > > > so please send a v2 at some point.
> > > >
> > > > Thanks. Do you have any suggestions for patch 2? Should I merge the
> > > > description in patch 2 into this file?
> > > >
> > > > The full dts for this series looks like this:
> > > >
> > > >                 smc: smc@23e400000 {
> > > >                         compatible = "apple,t8103-smc", "apple,smc";
> > > >                         reg = <0x2 0x3e400000 0x0 0x4000>,
> > > >                                 <0x2 0x3fe00000 0x0 0x100000>;
> > > >                         reg-names = "smc", "sram";
> > > >                         mboxes = <&smc_mbox>;
> > > >
> > > >                         smc_gpio: gpio {
> > > >                                 gpio-controller;
> > > >                                 #gpio-cells = <2>;
> > > >                         };
> > > >                 };
> > > >
> > > > but the fuller version in the asahi linux tree looks like:
> > > >
> > > >                 smc: smc@23e400000 {
> > > >                         compatible = "apple,t8103-smc", "apple,smc";
> > > >                         reg = <0x2 0x3e400000 0x0 0x4000>,
> > > >                                 <0x2 0x3fe00000 0x0 0x100000>;
> > > >                         reg-names = "smc", "sram";
> > > >                         mboxes = <&smc_mbox>;
> > > >
> > > >                         smc_gpio: gpio {
> > > >                                 gpio-controller;
> > > >                                 #gpio-cells = <2>;
> > > 
> > > Only 2 properties doesn't really need its own schema doc. However, I
> > > would just move these to the parent node.
> > 
> > When we designed the bindings, it was our understanding that having
> > separate nodes better matches Linux's MFD driver model.
> 
> Well, it is convenient to have subnodes with compatibles so that your 
> drivers automagically probe. So yes, a 1:1 relationship of nodes to 
> drivers is nice and tidy. But h/w is not always packaged up neatly and 
> it's not DT's job to try to abstract it such that it is. Also, we 
> shouldn't design bindings around the *current* driver partitioning of 
> some OS.
> 
> This one is actually pretty odd in that the child nodes don't have a 
> compatible string which breaks the automagical probing.
> 
> > Please be aware that OpenBSD is already using these bindings.  If
> > there are good reasons for moving things, we can probably deal with
> > that.  But this sounds a bit like a toss up.
> 
> Sigh. If there are other bindings in use, please submit them even if the 
> Linux driver isn't ready. If a Linux subsystem maintainer doesn't want 
> to take it, then I will. 
> 
> It is a toss up though...
> 
> > > >                         };
> > > >
> > > >                         smc_rtc: rtc {
> > > >                                 nvmem-cells = <&rtc_offset>;
> > > >                                 nvmem-cell-names = "rtc_offset";
> > > >                         };
> > > >
> > > >                         smc_reboot: reboot {
> > > >                                 nvmem-cells = <&shutdown_flag>, <&boot_stage>,
> > > >                                         <&boot_error_count>, <&panic_count>, <&pm_setting>;
> > > >                                 nvmem-cell-names = "shutdown_flag", "boot_stage",
> > > >                                         "boot_error_count", "panic_count", "pm_setting";
> > > 
> > > Not really much reason to split these up either because you can just
> > > fetch the entry you want by name.
> > 
> > Again the separate nodes are there because the RTC and the reboot
> > functionality are logically separate and handled by different MFD
> > sub-drivers in Linux.
> 
> It's really a question of whether the subset of functionality is going 
> to get reused on its own or has its own resources in DT. MFD bindings 
> are done both ways.

I'm guessing this series is blocked until a resolution is found for the
DT binding description.

Please can the Asahi folk and the DT maintainers sort this out and let
me know when I can proceed with this patch set. I'm just the man in
the middle here.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Fri, Sep 02, 2022 at 12:28:08PM -0500, Rob Herring wrote:
> This one is actually pretty odd in that the child nodes don't have a 
> compatible string which breaks the automagical probing.

I don't think that is necessarily true, and I don't think it's true in
this case.

The SMC core driver instructs the MFD core to create devices for the
individual functional items:

static const struct mfd_cell apple_smc_devs[] = {
        {
                .name = "macsmc-gpio",
        },
        {
                .name = "macsmc-hid",
        },
        {
                .name = "macsmc-power",
        },
        {
                .name = "macsmc-reboot",
        },
        {
                .name = "macsmc-rtc",
        },
};

Since MFD uses platform devices for these, they get all the normal
functionality that these devices have, which include matching by
device name ot the driver name, and udev events being appropriately
triggered. As long as the platform drivers for these devices have the
correct modalias lines, autoloading of the modules will work and the
drivers will be correctly matched and probed.

The Asahi kernel builds most of the platform support as modules,
including these, so we know it works (if it didn't, then lots of
module autoloading would be broken on non-DT platforms.)

> > Again the separate nodes are there because the RTC and the reboot
> > functionality are logically separate and handled by different MFD
> > sub-drivers in Linux.
> 
> It's really a question of whether the subset of functionality is going 
> to get reused on its own or has its own resources in DT. MFD bindings 
> are done both ways.

I think the current position on what to do about these is that everyone
is looking for someone else to make a decision, and no one wants to!

Firstly, I don't think that the number of properties in a node should
have a bearing on the design of the DT binding - what should have a
bearing is the logical partitioning of functionality.

Mark suggests that it would take six months for OpenBSD to transition to
some other description - for example, if we merged the nodes.

Hector says that MacOS's firmware description has the nodes merged, but
their description is a mess.

The overall preference seems to be to keep the sub-nodes unless there
is a strong technical reason not to.

The feeling I am getting from the review is that there doesn't seem to
be a strong technical reason to merge the nodes - there are desires and
preferences, but nothing concrete.

So at this point, I think it would make sense if I post a v2 with all
the updates so far (sorry, given the long drawn out discussions on
this, I've lost track of what changes have been made to the code, so
I won't include a detailed change log.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Mark Kettenis]

> Date: Tue, 6 Sep 2022 10:04:45 +0100
> From: "Russell King (Oracle)" <linux@armlinux.org.uk>
> 
> On Fri, Sep 02, 2022 at 12:28:08PM -0500, Rob Herring wrote:
> > This one is actually pretty odd in that the child nodes don't have a 
> > compatible string which breaks the automagical probing.
> 
> I don't think that is necessarily true, and I don't think it's true in
> this case.
> 
> The SMC core driver instructs the MFD core to create devices for the
> individual functional items:
> 
> static const struct mfd_cell apple_smc_devs[] = {
>         {
>                 .name = "macsmc-gpio",
>         },
>         {
>                 .name = "macsmc-hid",
>         },
>         {
>                 .name = "macsmc-power",
>         },
>         {
>                 .name = "macsmc-reboot",
>         },
>         {
>                 .name = "macsmc-rtc",
>         },
> };
> 
> Since MFD uses platform devices for these, they get all the normal
> functionality that these devices have, which include matching by
> device name ot the driver name, and udev events being appropriately
> triggered. As long as the platform drivers for these devices have the
> correct modalias lines, autoloading of the modules will work and the
> drivers will be correctly matched and probed.
> 
> The Asahi kernel builds most of the platform support as modules,
> including these, so we know it works (if it didn't, then lots of
> module autoloading would be broken on non-DT platforms.)
> 
> > > Again the separate nodes are there because the RTC and the reboot
> > > functionality are logically separate and handled by different MFD
> > > sub-drivers in Linux.
> > 
> > It's really a question of whether the subset of functionality is going 
> > to get reused on its own or has its own resources in DT. MFD bindings 
> > are done both ways.
> 
> I think the current position on what to do about these is that everyone
> is looking for someone else to make a decision, and no one wants to!
> 
> Firstly, I don't think that the number of properties in a node should
> have a bearing on the design of the DT binding - what should have a
> bearing is the logical partitioning of functionality.
> 
> Mark suggests that it would take six months for OpenBSD to transition to
> some other description - for example, if we merged the nodes.

Note that we're totally willing to do that if there is a good
technical reason for changes to the binding.  We just have to find a
way to upgrade existing installations of OpenBSD 7.1 which is a bit of
a challenge as the SMC GPIOs are essential for wifi which on the
laptops is the only network connection available.

> Hector says that MacOS's firmware description has the nodes merged, but
> their description is a mess.

To elaborate on that a bit more, the use of sub-nodes provides a nice
separation between the SMC hardware interface (the main node) and the
functionality offered by the firmware running on the SMC (the sub-nodes).

Another argument for having sub-nodes is that the firmware actually
exposes *two* GPIO controllers.  For now we only support the "master"
PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a
separate set of SMC "keys".  We currently don't need any of the pins
on the "slave", so we don't expose it in the DT yet.

> The overall preference seems to be to keep the sub-nodes unless there
> is a strong technical reason not to.
> 
> The feeling I am getting from the review is that there doesn't seem to
> be a strong technical reason to merge the nodes - there are desires and
> preferences, but nothing concrete.
> 
> So at this point, I think it would make sense if I post a v2 with all
> the updates so far (sorry, given the long drawn out discussions on
> this, I've lost track of what changes have been made to the code, so
> I won't include a detailed change log.)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 
> 

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Linus Walleij]

On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> Another argument for having sub-nodes is that the firmware actually
> exposes *two* GPIO controllers.  For now we only support the "master"
> PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a
> separate set of SMC "keys".  We currently don't need any of the pins
> on the "slave", so we don't expose it in the DT yet.

That sounds backward, like we don't expose device X as DT node
because $OS doesn't use it yet. DT should just expose (by nodes or
other ways) all hardware that exist or at least all hardware we know
about no matter what $OS is using.

FWIW I think nodes makes most sense because no doubt for example
the RTC is a separate hardware unit somewhere, and so is the
GPIO. The fact that it is hidden behind a software abstraction doesn't
change the fact that the HW definitely has these discrete units.

Yours,
Linus Walleij

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Hector Martin]

On 06/09/2022 20.22, Linus Walleij wrote:
> On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
>> Another argument for having sub-nodes is that the firmware actually
>> exposes *two* GPIO controllers.  For now we only support the "master"
>> PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a
>> separate set of SMC "keys".  We currently don't need any of the pins
>> on the "slave", so we don't expose it in the DT yet.
> 
> That sounds backward, like we don't expose device X as DT node
> because $OS doesn't use it yet. DT should just expose (by nodes or
> other ways) all hardware that exist or at least all hardware we know
> about no matter what $OS is using.

How so? The are piles and piles of unused hardware not exposed in the
DT, and piles and piles of hardware that will be used but we haven't
figured out how to do it yet, so it's not exposed. For example, we know
there are like 8 or so UARTs, but we don't define them in the DT because
they are not connected to anything on any existing device and we don't
need them. Apple does the same thing in their DTs (only used hardware is
defined).

I don't really see the point of exposing a GPIO controller when we don't
actually do anything with the pins yet, and might never do so. Having
drivers bind and stay unused just increases the amount of code running
without any ultimate purpose, so why do it? It's not like any other OS
would use the hardware either - GPIOs are only useful if they are
referenced in the DT for something, and we don't have anything that
would reference these.

For SMC in particular, there's a huge amount of functionality we don't
have drivers for yet, and I don't see the point of trying to conjure up
DT bindings for it until someone writes a driver (and has a reason to do
so) :)

> FWIW I think nodes makes most sense because no doubt for example
> the RTC is a separate hardware unit somewhere, and so is the
> GPIO. The fact that it is hidden behind a software abstraction doesn't
> change the fact that the HW definitely has these discrete units.

The RTC and the GPIO happen to be part of the same physical IC (PMU),
but yes, I agree.

- Hector

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Linus Walleij]

On Tue, Sep 6, 2022 at 1:36 PM Hector Martin <marcan@marcan.st> wrote:
> On 06/09/2022 20.22, Linus Walleij wrote:
> > On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> >> Another argument for having sub-nodes is that the firmware actually
> >> exposes *two* GPIO controllers.  For now we only support the "master"
> >> PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a
> >> separate set of SMC "keys".  We currently don't need any of the pins
> >> on the "slave", so we don't expose it in the DT yet.
> >
> > That sounds backward, like we don't expose device X as DT node
> > because $OS doesn't use it yet. DT should just expose (by nodes or
> > other ways) all hardware that exist or at least all hardware we know
> > about no matter what $OS is using.
>
> How so? The are piles and piles of unused hardware not exposed in the
> DT, and piles and piles of hardware that will be used but we haven't
> figured out how to do it yet, so it's not exposed. For example, we know
> there are like 8 or so UARTs, but we don't define them in the DT because
> they are not connected to anything on any existing device and we don't
> need them. Apple does the same thing in their DTs (only used hardware is
> defined).
>
> I don't really see the point of exposing a GPIO controller when we don't
> actually do anything with the pins yet, and might never do so. Having
> drivers bind and stay unused just increases the amount of code running
> without any ultimate purpose, so why do it? It's not like any other OS
> would use the hardware either - GPIOs are only useful if they are
> referenced in the DT for something, and we don't have anything that
> would reference these.

This comes from the FDT background in OpenFirmware, and there is
definitely a timeline perspective (also called "waterfall model") in that
line of thinking: a DT is written that describes the hardware and flashed
into the BIOS and never changed, then the operating system is
implemented at a later point. This is how e.g. the PC ACPI BIOS tables
are also thinking about themselves.

Of course the world does not work like that, but as a standard process
DT and ACPI works with the same ambition as any such process: slowly and
impersonal, like the planets, or the plants.

The ambition that a DT should always remain backward compatible
comes from the same historical root and reasoning.

Any agile development or (heh) hardware discovers through reverse
engineering will of course drive a truck through that line of thinking.

Realistically, use the same approach as with everything else, I like the
IETF motto "rough consensus and running code", any ambition taken too
far will just stifle development. So I'd say make an honest effort to
resonably describe the nodes we see will be useful in the foreseeable
future.

Yours,
Linus Walleij

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Hector Martin]

On 06/09/2022 20.57, Linus Walleij wrote:
> On Tue, Sep 6, 2022 at 1:36 PM Hector Martin <marcan@marcan.st> wrote:
>> On 06/09/2022 20.22, Linus Walleij wrote:
>>> On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>>
>>>> Another argument for having sub-nodes is that the firmware actually
>>>> exposes *two* GPIO controllers.  For now we only support the "master"
>>>> PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a
>>>> separate set of SMC "keys".  We currently don't need any of the pins
>>>> on the "slave", so we don't expose it in the DT yet.
>>>
>>> That sounds backward, like we don't expose device X as DT node
>>> because $OS doesn't use it yet. DT should just expose (by nodes or
>>> other ways) all hardware that exist or at least all hardware we know
>>> about no matter what $OS is using.
>>
>> How so? The are piles and piles of unused hardware not exposed in the
>> DT, and piles and piles of hardware that will be used but we haven't
>> figured out how to do it yet, so it's not exposed. For example, we know
>> there are like 8 or so UARTs, but we don't define them in the DT because
>> they are not connected to anything on any existing device and we don't
>> need them. Apple does the same thing in their DTs (only used hardware is
>> defined).
>>
>> I don't really see the point of exposing a GPIO controller when we don't
>> actually do anything with the pins yet, and might never do so. Having
>> drivers bind and stay unused just increases the amount of code running
>> without any ultimate purpose, so why do it? It's not like any other OS
>> would use the hardware either - GPIOs are only useful if they are
>> referenced in the DT for something, and we don't have anything that
>> would reference these.
> 
> This comes from the FDT background in OpenFirmware, and there is
> definitely a timeline perspective (also called "waterfall model") in that
> line of thinking: a DT is written that describes the hardware and flashed
> into the BIOS and never changed, then the operating system is
> implemented at a later point. This is how e.g. the PC ACPI BIOS tables
> are also thinking about themselves.

Yes, but again, that only makes sense from the point of view of
describing hardware that exists, is useful, and could be used by the OS.

For any given platform X, if platform X does not use the secondary GPIO
controller for any service describable in the DT, then there is no point
in describing that GPIO controller. Same way ACPI tables do not describe
every single physical GPIO available on a platform, just whatever is
used by the AML.

This is the case for every existing Apple device to my knowledge.
Apple's device trees have zero references to the secondary controller's
GPIOs. It's possible they are only used internally by SMC, or they were
added to support future platforms that don't exist yet, or for debugging
purposes, or who knows.

If some day we find a use for those GPIOs, that would require a DT
change *anyway*, to describe that usage, and the controller could be
described then (we did something like that, using a GPIO that Apple
doesn't, for the interim display-backlight power control support, though
that is a temporary hack that will go away). Heck, we don't even know
what these GPIOs are connected to right now!

Ultimately, we're working with a reverse engineered platform here, and
DTs will inevitaby be incremental. But in this particular case, of
hardware that has no known useful purpose to an OS, I don't see the
point in gratuitously describing it. And besides, the way we set things
up, forward-compatible DT upgrades are trivial, and no actual user on
this platform is going to be stuck with an old DT and newer software (if
their software supports the platform properly, and that takes more than
the relatively trivial DT upgrade stuff anyway). I'm a lot more
interested in getting bindings upstreamed ASAP (so we can start
guaranteeing no backwards-compat breaks, which is important to avoid
outright breakage) than I am in trying to be exhaustive up front with
device instances. It's perfectly fine to say that users have to upgrade
both their DTs and kernels to get newer hardware support, on these
platforms.

- Hector

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Tue, Sep 06, 2022 at 10:28:25PM +0900, Hector Martin wrote:
> Ultimately, we're working with a reverse engineered platform here, and
> DTs will inevitaby be incremental. But in this particular case, of
> hardware that has no known useful purpose to an OS, I don't see the
> point in gratuitously describing it. And besides, the way we set things
> up, forward-compatible DT upgrades are trivial, and no actual user on
> this platform is going to be stuck with an old DT and newer software (if
> their software supports the platform properly, and that takes more than
> the relatively trivial DT upgrade stuff anyway). I'm a lot more
> interested in getting bindings upstreamed ASAP (so we can start
> guaranteeing no backwards-compat breaks, which is important to avoid
> outright breakage) than I am in trying to be exhaustive up front with
> device instances. It's perfectly fine to say that users have to upgrade
> both their DTs and kernels to get newer hardware support, on these
> platforms.

It's also a very common process for SoCs - almost no one writes the
DT first and then writes the drivers. You always see on the mailing
list series of patches that add a driver for some bit of hardware,
along with patches adding the DT binding and the DT description.

I don't think you're doing anything different here to what is common
practice within the mainline kernel community with this approach.

The exception to that is when adding a driver for feature X in a SoC,
it's common to add all instances of X to the dtsi with ``status =
"disabled"'' and only enable the appropriate blocks on platforms that
need it.

So, for example, if a SoC has three network interfaces, all of them
identical, when adding a network driver and the bindings for the
network hardware, one would add all three to the SoC description
whether or not the platform one was working with makes use of them.

It means that one has to think about how to support all instances
of the hardware on the platform and design the binding to allow
that flexibility, rather than having to augment the binding later.

In the case of gpio-macsmc, how would we later add support for the
slave PMU GPIOs, given that these use keys "gpXX" rather than "gPxx"?
How do we tell the gpio-macsmc code to use a different set of keys?
Should DT describe the key "prefix" (in other words "gp" vs "gP"),
or should it describe it some other way. What if Apple decides to
instantiate another GPIO controller in a later platform with a
different prefix, could that be accomodated without breaking any
solution we come up today?

Maybe the solution to this would be to describe the key prefix in DT
as that's effectively its "reg". Or maybe we use "reg" to describe
it somehow (which is value of the key, which seems to have an
"address" like quality to it?)

We don't have to implement code for this now, we just need to get a
reasonably correct DT binding for the gpio controller.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Linus Walleij]

On Tue, Sep 6, 2022 at 3:28 PM Hector Martin <marcan@marcan.st> wrote:
> On 06/09/2022 20.57, Linus Walleij wrote:
> > On Tue, Sep 6, 2022 at 1:36 PM Hector Martin <marcan@marcan.st> wrote:

> > This comes from the FDT background in OpenFirmware, and there is
> > definitely a timeline perspective (also called "waterfall model") in that
> > line of thinking: a DT is written that describes the hardware and flashed
> > into the BIOS and never changed, then the operating system is
> > implemented at a later point. This is how e.g. the PC ACPI BIOS tables
> > are also thinking about themselves.
>
> Yes, but again, that only makes sense from the point of view of
> describing hardware that exists, is useful, and could be used by the OS.
>
> For any given platform X, if platform X does not use the secondary GPIO
> controller for any service describable in the DT, then there is no point
> in describing that GPIO controller. Same way ACPI tables do not describe
> every single physical GPIO available on a platform, just whatever is
> used by the AML.

Good point. I don't know what ambition DT should have here.
If there is a discrete component on I2C for example, we tend to
describe it fully, this kind of stuff with misc "dark silicon" didn't
exist when DT was first conceived.

> If some day we find a use for those GPIOs, that would require a DT
> change *anyway*, to describe that usage, and the controller could be
> described then (we did something like that, using a GPIO that Apple
> doesn't, for the interim display-backlight power control support, though
> that is a temporary hack that will go away). Heck, we don't even know
> what these GPIOs are connected to right now!
>
> Ultimately, we're working with a reverse engineered platform here, and
> DTs will inevitaby be incremental.
(...)

That's OK, I think the patch series is good enough as it is and should
be merged, so I have added my Reviewed-by. I think the world
is a better place with support for Apple silicon being developed
in-tree.

> I'm a lot more
> interested in getting bindings upstreamed ASAP (so we can start
> guaranteeing no backwards-compat breaks, which is important to avoid
> outright breakage) than I am in trying to be exhaustive up front with
> device instances. It's perfectly fine to say that users have to upgrade
> both their DTs and kernels to get newer hardware support, on these
> platforms.

I agree.

Yours,
Linus Walleij

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Hector Martin]

On 06/09/2022 22.43, Russell King (Oracle) wrote:
> On Tue, Sep 06, 2022 at 10:28:25PM +0900, Hector Martin wrote:
>> Ultimately, we're working with a reverse engineered platform here, and
>> DTs will inevitaby be incremental. But in this particular case, of
>> hardware that has no known useful purpose to an OS, I don't see the
>> point in gratuitously describing it. And besides, the way we set things
>> up, forward-compatible DT upgrades are trivial, and no actual user on
>> this platform is going to be stuck with an old DT and newer software (if
>> their software supports the platform properly, and that takes more than
>> the relatively trivial DT upgrade stuff anyway). I'm a lot more
>> interested in getting bindings upstreamed ASAP (so we can start
>> guaranteeing no backwards-compat breaks, which is important to avoid
>> outright breakage) than I am in trying to be exhaustive up front with
>> device instances. It's perfectly fine to say that users have to upgrade
>> both their DTs and kernels to get newer hardware support, on these
>> platforms.
> 
> It's also a very common process for SoCs - almost no one writes the
> DT first and then writes the drivers. You always see on the mailing
> list series of patches that add a driver for some bit of hardware,
> along with patches adding the DT binding and the DT description.
> 
> I don't think you're doing anything different here to what is common
> practice within the mainline kernel community with this approach.
> 
> The exception to that is when adding a driver for feature X in a SoC,
> it's common to add all instances of X to the dtsi with ``status =
> "disabled"'' and only enable the appropriate blocks on platforms that
> need it.
> 
> So, for example, if a SoC has three network interfaces, all of them
> identical, when adding a network driver and the bindings for the
> network hardware, one would add all three to the SoC description
> whether or not the platform one was working with makes use of them.

Right, and that makes sense for SoCs that third parties can incorporate
into any arbitrary platform, and which have documentation listing those
instances, because then board-level DTs can just enable the ones they need.

But here we're working with SoCs where the only reference for what
hardware exists *is* Apple's DT, and it only lists hardware that is in
use on existing platforms. So there is no sane way for us to know what
hardware exists, except what we can infer from reading the tea leaves
(e.g. you can tell how many UARTs there are from the power domains and
leaked schematics, but this doesn't extend to everything else). For
example, we also know that these SoCs have a second Neural Engine that
Apple have disabled on all shipping platforms, that the t600x series has
an unused USB3 controller, etc... but there's no way we can safely
describe these things in the DT if we can't test that they work and we
don't have documentation even telling us how they're supposed to work :)

> In the case of gpio-macsmc, how would we later add support for the
> slave PMU GPIOs, given that these use keys "gpXX" rather than "gPxx"?
> How do we tell the gpio-macsmc code to use a different set of keys?
> Should DT describe the key "prefix" (in other words "gp" vs "gP"),
> or should it describe it some other way. What if Apple decides to
> instantiate another GPIO controller in a later platform with a
> different prefix, could that be accomodated without breaking any
> solution we come up today?
> 
> Maybe the solution to this would be to describe the key prefix in DT
> as that's effectively its "reg". Or maybe we use "reg" to describe
> it somehow (which is value of the key, which seems to have an
> "address" like quality to it?)
> 
> We don't have to implement code for this now, we just need to get a
> reasonably correct DT binding for the gpio controller.

I agree that this is something to think about (I was about to reply on
the subject).

I can think of two ways: using `reg` for the key name, but that feels
icky since it's ASCII and not *really* a register number/address, or
something like this:

gpio@0 {
	apple,smc-key-base = "gP00";
	...
}

gpio@1 {
	apple,smc-key-base = "gp00";
	...
}

But this ties back to the device enumeration too, since right now the DT
does not drive that (we'd have to add the subdevice to the mfd subdevice
list somehow anyway, if we don't switch to compatibles).

I'd love to hear Rob's opinion on this one, and also whether the
existing Linux and OpenBSD code would currently find gpio@0 {} instead
of gpio {} for backwards compat.

- Hector

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Mark Kettenis]

> Date: Tue, 6 Sep 2022 22:53:47 +0900
> From: Hector Martin <marcan@marcan.st>
> 
> On 06/09/2022 22.43, Russell King (Oracle) wrote:
> > In the case of gpio-macsmc, how would we later add support for the
> > slave PMU GPIOs, given that these use keys "gpXX" rather than "gPxx"?
> > How do we tell the gpio-macsmc code to use a different set of keys?
> > Should DT describe the key "prefix" (in other words "gp" vs "gP"),
> > or should it describe it some other way. What if Apple decides to
> > instantiate another GPIO controller in a later platform with a
> > different prefix, could that be accomodated without breaking any
> > solution we come up today?
> > 
> > Maybe the solution to this would be to describe the key prefix in DT
> > as that's effectively its "reg". Or maybe we use "reg" to describe
> > it somehow (which is value of the key, which seems to have an
> > "address" like quality to it?)
> > 
> > We don't have to implement code for this now, we just need to get a
> > reasonably correct DT binding for the gpio controller.
> 
> I agree that this is something to think about (I was about to reply on
> the subject).
> 
> I can think of two ways: using `reg` for the key name, but that feels
> icky since it's ASCII and not *really* a register number/address, or
> something like this:
> 
> gpio@0 {
> 	apple,smc-key-base = "gP00";
> 	...
> }
> 
> gpio@1 {
> 	apple,smc-key-base = "gp00";
> 	...
> }

This would still require us to add a (one-cell) "reg" property and
would require adding the appropriate "#address-cells" and
"#size-cells" properties to the SMC node.

> But this ties back to the device enumeration too, since right now the DT
> does not drive that (we'd have to add the subdevice to the mfd subdevice
> list somehow anyway, if we don't switch to compatibles).
> 
> I'd love to hear Rob's opinion on this one, and also whether the
> existing Linux and OpenBSD code would currently find gpio@0 {} instead
> of gpio {} for backwards compat.

The OpenBSD driver does a lookup by name and the "@0" is part of that
name.  So that would break backwards compat.

Maybe just name the slave GPIO controller "gpio-slave"?  If we add
compatibles, the compatibles for the nodes should propbably be
different such that we can switch to do a lookup by compatible?

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Tue, Sep 06, 2022 at 04:25:49PM +0200, Mark Kettenis wrote:
> > Date: Tue, 6 Sep 2022 22:53:47 +0900
> > From: Hector Martin <marcan@marcan.st>
> > 
> > I agree that this is something to think about (I was about to reply on
> > the subject).
> > 
> > I can think of two ways: using `reg` for the key name, but that feels
> > icky since it's ASCII and not *really* a register number/address, or
> > something like this:
> > 
> > gpio@0 {
> > 	apple,smc-key-base = "gP00";
> > 	...
> > }
> > 
> > gpio@1 {
> > 	apple,smc-key-base = "gp00";
> > 	...
> > }
> 
> This would still require us to add a (one-cell) "reg" property and
> would require adding the appropriate "#address-cells" and
> "#size-cells" properties to the SMC node.

Yes, and at that point, as I suggested, it probably would be better
to use:

	#address-cells = <1>;
	#size-cells = <0>;

	gpio@67503030 {
		reg = <0x67503030>;
	};

	gpio@67703030 {
		reg = <0x67703030>;
	};

Then the "reg" has a meaning that is directly related to the SMC.

> > But this ties back to the device enumeration too, since right now the DT
> > does not drive that (we'd have to add the subdevice to the mfd subdevice
> > list somehow anyway, if we don't switch to compatibles).
> > 
> > I'd love to hear Rob's opinion on this one, and also whether the
> > existing Linux and OpenBSD code would currently find gpio@0 {} instead
> > of gpio {} for backwards compat.
> 
> The OpenBSD driver does a lookup by name and the "@0" is part of that
> name.  So that would break backwards compat.

Oh, that's annoying - and is a different behaviour to Linux.

On Linux, we only look at the node name up to the @ when matching (see
of_node_name_eq() in drivers/of/base.c, so it doesn't matter to Linux
what follows the @ when you try to look up a node named "gpio" - you'll
find gpio@anythingyoulike.

> Maybe just name the slave GPIO controller "gpio-slave"?  If we add
> compatibles, the compatibles for the nodes should propbably be
> different such that we can switch to do a lookup by compatible?

I don't think the DT folk would be happy with "gpio-slave" because
node names are supposed to be generic. Also, "slave" probably isn't
a good choice of name in this modern era given past history.

Rather than the above, we could use "reg" to indicate which GPIO
controller we're talking about, and lookup the reg value in a table
to give the key. So gpio@0, reg=<0> => gP00, gpio@1, reg=<1> => gp00.
gpio@2, reg=<2> => whatever next.

That sounds like it won't break the existing OpenBSD.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Tue, Sep 06, 2022 at 08:36:25PM +0900, Hector Martin wrote:
> On 06/09/2022 20.22, Linus Walleij wrote:
> > On Tue, Sep 6, 2022 at 11:31 AM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > 
> >> Another argument for having sub-nodes is that the firmware actually
> >> exposes *two* GPIO controllers.  For now we only support the "master"
> >> PMU GPIOs, but there also is a "slave" PMU GPIO controller that uses a
> >> separate set of SMC "keys".  We currently don't need any of the pins
> >> on the "slave", so we don't expose it in the DT yet.
> > 
> > That sounds backward, like we don't expose device X as DT node
> > because $OS doesn't use it yet. DT should just expose (by nodes or
> > other ways) all hardware that exist or at least all hardware we know
> > about no matter what $OS is using.
> 
> How so? The are piles and piles of unused hardware not exposed in the
> DT, and piles and piles of hardware that will be used but we haven't
> figured out how to do it yet, so it's not exposed. For example, we know
> there are like 8 or so UARTs, but we don't define them in the DT because
> they are not connected to anything on any existing device and we don't
> need them. Apple does the same thing in their DTs (only used hardware is
> defined).
> 
> I don't really see the point of exposing a GPIO controller when we don't
> actually do anything with the pins yet, and might never do so. Having
> drivers bind and stay unused just increases the amount of code running
> without any ultimate purpose, so why do it? It's not like any other OS
> would use the hardware either - GPIOs are only useful if they are
> referenced in the DT for something, and we don't have anything that
> would reference these.
> 
> For SMC in particular, there's a huge amount of functionality we don't
> have drivers for yet, and I don't see the point of trying to conjure up
> DT bindings for it until someone writes a driver (and has a reason to do
> so) :)

Exposing in a DT is one thing. Defining in the binding is another. Even 
if it's not complete bindings, but just a fuller description of what 
functionality the MFD contains is. For example, just knowing there 
are 2 instances of GPIO, I'm much more inclined to agree GPIO should be 
a subnode.

Rob

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Mark Kettenis]

> Date: Tue, 6 Sep 2022 15:54:50 +0100
> From: "Russell King (Oracle)" <linux@armlinux.org.uk>
> 
> On Tue, Sep 06, 2022 at 04:25:49PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 6 Sep 2022 22:53:47 +0900
> > > From: Hector Martin <marcan@marcan.st>
> > > 
> > > I agree that this is something to think about (I was about to reply on
> > > the subject).
> > > 
> > > I can think of two ways: using `reg` for the key name, but that feels
> > > icky since it's ASCII and not *really* a register number/address, or
> > > something like this:
> > > 
> > > gpio@0 {
> > > 	apple,smc-key-base = "gP00";
> > > 	...
> > > }
> > > 
> > > gpio@1 {
> > > 	apple,smc-key-base = "gp00";
> > > 	...
> > > }
> > 
> > This would still require us to add a (one-cell) "reg" property and
> > would require adding the appropriate "#address-cells" and
> > "#size-cells" properties to the SMC node.
> 
> Yes, and at that point, as I suggested, it probably would be better
> to use:
> 
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	gpio@67503030 {
> 		reg = <0x67503030>;
> 	};
> 
> 	gpio@67703030 {
> 		reg = <0x67703030>;
> 	};
> 
> Then the "reg" has a meaning that is directly related to the SMC.
> 
> > > But this ties back to the device enumeration too, since right now the DT
> > > does not drive that (we'd have to add the subdevice to the mfd subdevice
> > > list somehow anyway, if we don't switch to compatibles).
> > > 
> > > I'd love to hear Rob's opinion on this one, and also whether the
> > > existing Linux and OpenBSD code would currently find gpio@0 {} instead
> > > of gpio {} for backwards compat.
> > 
> > The OpenBSD driver does a lookup by name and the "@0" is part of that
> > name.  So that would break backwards compat.
> 
> Oh, that's annoying - and is a different behaviour to Linux.
> 
> On Linux, we only look at the node name up to the @ when matching (see
> of_node_name_eq() in drivers/of/base.c, so it doesn't matter to Linux
> what follows the @ when you try to look up a node named "gpio" - you'll
> find gpio@anythingyoulike.

So maybe I should change our code to match what Linux does.  OpenBSD
7.2 is scheduled for release on November 1st, and I can probably get
that change in quickly.  If we can hold off updating the device trees
in the Asahi installer until then the transition should be smooth
enough.

> > Maybe just name the slave GPIO controller "gpio-slave"?  If we add
> > compatibles, the compatibles for the nodes should propbably be
> > different such that we can switch to do a lookup by compatible?
> 
> I don't think the DT folk would be happy with "gpio-slave" because
> node names are supposed to be generic.

I don't think that rule applies to "named" sub-nodes like this where
the name is actually significant.  

> Also, "slave" probably isn't a good choice of name in this modern
> era given past history.

Yes, we don't have to follow Apple's terminology here.

> Rather than the above, we could use "reg" to indicate which GPIO
> controller we're talking about, and lookup the reg value in a table
> to give the key. So gpio@0, reg=<0> => gP00, gpio@1, reg=<1> => gp00.
> gpio@2, reg=<2> => whatever next.
> 
> That sounds like it won't break the existing OpenBSD.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Tue, Sep 06, 2022 at 03:54:50PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 06, 2022 at 04:25:49PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 6 Sep 2022 22:53:47 +0900
> > > From: Hector Martin <marcan@marcan.st>
> > > 
> > > I agree that this is something to think about (I was about to reply on
> > > the subject).
> > > 
> > > I can think of two ways: using `reg` for the key name, but that feels
> > > icky since it's ASCII and not *really* a register number/address, or
> > > something like this:
> > > 
> > > gpio@0 {
> > > 	apple,smc-key-base = "gP00";
> > > 	...
> > > }
> > > 
> > > gpio@1 {
> > > 	apple,smc-key-base = "gp00";
> > > 	...
> > > }
> > 
> > This would still require us to add a (one-cell) "reg" property and
> > would require adding the appropriate "#address-cells" and
> > "#size-cells" properties to the SMC node.
> 
> Yes, and at that point, as I suggested, it probably would be better
> to use:
> 
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 
> 	gpio@67503030 {
> 		reg = <0x67503030>;
> 	};
> 
> 	gpio@67703030 {
> 		reg = <0x67703030>;
> 	};
> 
> Then the "reg" has a meaning that is directly related to the SMC.

That's certainly odd, but if that's how it is addressed, then okay I 
suppose.

> 
> > > But this ties back to the device enumeration too, since right now the DT
> > > does not drive that (we'd have to add the subdevice to the mfd subdevice
> > > list somehow anyway, if we don't switch to compatibles).
> > > 
> > > I'd love to hear Rob's opinion on this one, and also whether the
> > > existing Linux and OpenBSD code would currently find gpio@0 {} instead
> > > of gpio {} for backwards compat.

Node names are generally not considered ABI except when they are. :)

Generally, core code doesn't care. Specific bindings with defined child 
nodes often do, but finding nodes by compatible is strongly preferred. 
Linux drivers can bind by node name (w/o unit-address), but that's 
really only ever done on ancient h/w.

> > 
> > The OpenBSD driver does a lookup by name and the "@0" is part of that
> > name.  So that would break backwards compat.
> 
> Oh, that's annoying - and is a different behaviour to Linux.
> 
> On Linux, we only look at the node name up to the @ when matching (see
> of_node_name_eq() in drivers/of/base.c, so it doesn't matter to Linux
> what follows the @ when you try to look up a node named "gpio" - you'll
> find gpio@anythingyoulike.
>
> > Maybe just name the slave GPIO controller "gpio-slave"?  If we add
> > compatibles, the compatibles for the nodes should propbably be
> > different such that we can switch to do a lookup by compatible?
> 
> I don't think the DT folk would be happy with "gpio-slave" because
> node names are supposed to be generic. Also, "slave" probably isn't
> a good choice of name in this modern era given past history.

Yeah, not a great choice for both reasons.
 
> Rather than the above, we could use "reg" to indicate which GPIO
> controller we're talking about, and lookup the reg value in a table
> to give the key. So gpio@0, reg=<0> => gP00, gpio@1, reg=<1> => gp00.
> gpio@2, reg=<2> => whatever next.

Keep in mind that for any level, there is only 1 address space. So if 
there's anything else with multiple instances, they share the same 
address space. IOW, you couldn't have say 'rtc@1'.

(Another example of why I want to see a full picture.)
> 
> That sounds like it won't break the existing OpenBSD.

No? Isn't OpenBSD looking for 'gpio' which wouldn't find 'gpio@0'?

Rob

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Tue, Sep 06, 2022 at 10:04:45AM +0100, Russell King (Oracle) wrote:
> On Fri, Sep 02, 2022 at 12:28:08PM -0500, Rob Herring wrote:
> > This one is actually pretty odd in that the child nodes don't have a 
> > compatible string which breaks the automagical probing.
> 
> I don't think that is necessarily true, and I don't think it's true in
> this case.

It is, because you are creating the devices in the driver rather than 
creating devices based on child nodes that exist.

> 
> The SMC core driver instructs the MFD core to create devices for the
> individual functional items:
> 
> static const struct mfd_cell apple_smc_devs[] = {
>         {
>                 .name = "macsmc-gpio",
>         },
>         {
>                 .name = "macsmc-hid",
>         },
>         {
>                 .name = "macsmc-power",
>         },
>         {
>                 .name = "macsmc-reboot",
>         },
>         {
>                 .name = "macsmc-rtc",
>         },
> };
> 
> Since MFD uses platform devices for these, they get all the normal
> functionality that these devices have, which include matching by
> device name ot the driver name, and udev events being appropriately
> triggered. As long as the platform drivers for these devices have the
> correct modalias lines, autoloading of the modules will work and the
> drivers will be correctly matched and probed.

Yes, and that's how MFD devices with no child nodes work. The difference 
is those get any DT info out of the parent node. Here you are presumably 
manually getting the child DT node you want for each driver.


> The Asahi kernel builds most of the platform support as modules,
> including these, so we know it works (if it didn't, then lots of
> module autoloading would be broken on non-DT platforms.)
> 
> > > Again the separate nodes are there because the RTC and the reboot
> > > functionality are logically separate and handled by different MFD
> > > sub-drivers in Linux.
> > 
> > It's really a question of whether the subset of functionality is going 
> > to get reused on its own or has its own resources in DT. MFD bindings 
> > are done both ways.
> 
> I think the current position on what to do about these is that everyone
> is looking for someone else to make a decision, and no one wants to!

I'm just looking for more information first.

> Firstly, I don't think that the number of properties in a node should
> have a bearing on the design of the DT binding - what should have a
> bearing is the logical partitioning of functionality.

It's not strictly about number of properties. Though nodes with only a 
compatible string is generally a red flag for me.

> Mark suggests that it would take six months for OpenBSD to transition to
> some other description - for example, if we merged the nodes.

The risk you take when using undocumented bindings...

> Hector says that MacOS's firmware description has the nodes merged, but
> their description is a mess.
> 
> The overall preference seems to be to keep the sub-nodes unless there
> is a strong technical reason not to.
> 
> The feeling I am getting from the review is that there doesn't seem to
> be a strong technical reason to merge the nodes - there are desires and
> preferences, but nothing concrete.
> 
> So at this point, I think it would make sense if I post a v2 with all
> the updates so far (sorry, given the long drawn out discussions on
> this, I've lost track of what changes have been made to the code, so
> I won't include a detailed change log.)

As I said elsewhere, sub-nodes is probably the right choice here. I 
think they need compatible strings in the child nodes, and addressing 
has to be sorted out which it seems may also break OpenBSD.

Rob

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Hector Martin]

On 07/09/2022 01.10, Rob Herring wrote:
>> So at this point, I think it would make sense if I post a v2 with all
>> the updates so far (sorry, given the long drawn out discussions on
>> this, I've lost track of what changes have been made to the code, so
>> I won't include a detailed change log.)
> 
> As I said elsewhere, sub-nodes is probably the right choice here. I 
> think they need compatible strings in the child nodes, and addressing 
> has to be sorted out which it seems may also break OpenBSD.

So addressing only makes sense for GPIO, out of the nodes we have so far
- that's the only thing with two discrete instances whose access can be
entirely described by a single base key name, and which are otherwise
compatible.

Everything else is pretty much single-instance, and talks to multiple
keys, so there isn't one single "address" key that would make semantic
sense to use as the node address. There are some indexed keys, but at a
deeper level (e.g. multiple battery cells part of the charge control
subsystem, multiple Type C ports as part of the AC/power input
subsystem, etc.). And in those cases, these subdevices are mostly
homogeneous and we would never need multiple nodes for them at the DT
level, they'd just be implicitly handled by those drivers.

GPIO is quite special in that 1) it only has a single key type (which is
overloaded using advanced features to provide, effectively,
sub-registers to control all the GPIO features per pin), 2) a single key
represents a single pin, 3) keys are numbered in a reasoanble way, and
4) there are two prefixes for two discrete GPIO controllers. For pretty
much everything else SMC does, we just have a bag of keys with no real
rhyme nor reason from the point of view of an "address space".

Given that, how would this work? Is it legal/reasonable for only the
gpio nodes to have addressing/reg properties, and everything else to
just have a node name with no concept of address? Does it even make
sense to special case gpio in this way, vs. just having something like
gpio {} / gpio-sec {} (if we ever even need gpio-sec, which is an open
question)?

- Hector

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Wed, Sep 07, 2022 at 02:00:53AM +0900, Hector Martin wrote:
> On 07/09/2022 01.10, Rob Herring wrote:
> >> So at this point, I think it would make sense if I post a v2 with all
> >> the updates so far (sorry, given the long drawn out discussions on
> >> this, I've lost track of what changes have been made to the code, so
> >> I won't include a detailed change log.)
> > 
> > As I said elsewhere, sub-nodes is probably the right choice here. I 
> > think they need compatible strings in the child nodes, and addressing 
> > has to be sorted out which it seems may also break OpenBSD.
> 
> So addressing only makes sense for GPIO, out of the nodes we have so far
> - that's the only thing with two discrete instances whose access can be
> entirely described by a single base key name, and which are otherwise
> compatible.
> 
> Everything else is pretty much single-instance, and talks to multiple
> keys, so there isn't one single "address" key that would make semantic
> sense to use as the node address. 

Unit-addresses are just the first address in 'reg'. So multiple 
addresses or not doesn't really matter.

> There are some indexed keys, but at a
> deeper level (e.g. multiple battery cells part of the charge control
> subsystem, multiple Type C ports as part of the AC/power input
> subsystem, etc.). And in those cases, these subdevices are mostly
> homogeneous and we would never need multiple nodes for them at the DT
> level, they'd just be implicitly handled by those drivers.

Type-C will be fun depending on how much of the muxing/altmode 
details have to get exposed. 

> GPIO is quite special in that 1) it only has a single key type (which is
> overloaded using advanced features to provide, effectively,
> sub-registers to control all the GPIO features per pin), 2) a single key
> represents a single pin, 3) keys are numbered in a reasoanble way, and
> 4) there are two prefixes for two discrete GPIO controllers. For pretty
> much everything else SMC does, we just have a bag of keys with no real
> rhyme nor reason from the point of view of an "address space".
> 
> Given that, how would this work? Is it legal/reasonable for only the
> gpio nodes to have addressing/reg properties, and everything else to
> just have a node name with no concept of address? 

Not ideal, but allowed.

> Does it even make
> sense to special case gpio in this way, vs. just having something like
> gpio {} / gpio-sec {} (if we ever even need gpio-sec, which is an open
> question)?

If not unit-addresses, the 2nd choice we do is 'gpio-0', 'gpio-1', etc. 
Though that is mainly in the GPIO based consumer bindings like gpio-pwm, 
spi-gpio, etc. where there's really not anything to use for an address.

If only those 2 nodes, then I really don't care so much and gpio-sec 
would be fine. It's 1 node in 1 binding.

Rob

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Sven Peter]

On Tue, Sep 6, 2022, at 19:35, Rob Herring wrote:
> On Wed, Sep 07, 2022 at 02:00:53AM +0900, Hector Martin wrote:
>> On 07/09/2022 01.10, Rob Herring wrote:
>> >> So at this point, I think it would make sense if I post a v2 with all
>> >> the updates so far (sorry, given the long drawn out discussions on
>> >> this, I've lost track of what changes have been made to the code, so
>> >> I won't include a detailed change log.)
>> > 
>> > As I said elsewhere, sub-nodes is probably the right choice here. I 
>> > think they need compatible strings in the child nodes, and addressing 
>> > has to be sorted out which it seems may also break OpenBSD.
>> 
>> So addressing only makes sense for GPIO, out of the nodes we have so far
>> - that's the only thing with two discrete instances whose access can be
>> entirely described by a single base key name, and which are otherwise
>> compatible.
>> 
>> Everything else is pretty much single-instance, and talks to multiple
>> keys, so there isn't one single "address" key that would make semantic
>> sense to use as the node address. 
>
> Unit-addresses are just the first address in 'reg'. So multiple 
> addresses or not doesn't really matter.
>
>> There are some indexed keys, but at a
>> deeper level (e.g. multiple battery cells part of the charge control
>> subsystem, multiple Type C ports as part of the AC/power input
>> subsystem, etc.). And in those cases, these subdevices are mostly
>> homogeneous and we would never need multiple nodes for them at the DT
>> level, they'd just be implicitly handled by those drivers.
>
> Type-C will be fun depending on how much of the muxing/altmode 
> details have to get exposed. 

Type-C is going to be a lot of "fun", but the SMC is not directly involved.

I still don't have a full picture but these boards have TPS6598x chips
which trigger the entire mess whenever a new mode was negotiated and the
"Apple Type-C PHY" contains the actual mux.

The SMC has a side channel to these TPS6598x chips as well but it seems
to only handle charging without having to communicate with whatever kernel
is running on the main processor.


Sven

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Hector Martin]

On 07/09/2022 02.35, Rob Herring wrote:
> On Wed, Sep 07, 2022 at 02:00:53AM +0900, Hector Martin wrote:
>> On 07/09/2022 01.10, Rob Herring wrote:
>>>> So at this point, I think it would make sense if I post a v2 with all
>>>> the updates so far (sorry, given the long drawn out discussions on
>>>> this, I've lost track of what changes have been made to the code, so
>>>> I won't include a detailed change log.)
>>>
>>> As I said elsewhere, sub-nodes is probably the right choice here. I 
>>> think they need compatible strings in the child nodes, and addressing 
>>> has to be sorted out which it seems may also break OpenBSD.
>>
>> So addressing only makes sense for GPIO, out of the nodes we have so far
>> - that's the only thing with two discrete instances whose access can be
>> entirely described by a single base key name, and which are otherwise
>> compatible.
>>
>> Everything else is pretty much single-instance, and talks to multiple
>> keys, so there isn't one single "address" key that would make semantic
>> sense to use as the node address. 
> 
> Unit-addresses are just the first address in 'reg'. So multiple 
> addresses or not doesn't really matter.

Okay, but we're obviously not going to list every single SMC key used by
any given node in the device tree (that'd be a giant mess, other than
for hwmon which is a story for another time), and it doesn't make sense
to pick an arbitrary one as the reg address and then ignore it in the
driver.

>> There are some indexed keys, but at a
>> deeper level (e.g. multiple battery cells part of the charge control
>> subsystem, multiple Type C ports as part of the AC/power input
>> subsystem, etc.). And in those cases, these subdevices are mostly
>> homogeneous and we would never need multiple nodes for them at the DT
>> level, they'd just be implicitly handled by those drivers.
> 
> Type-C will be fun depending on how much of the muxing/altmode 
> details have to get exposed. 

As sven mentioned that will be fun, but thankfully not part of this
binding (SMC only cares for power supply purposes, and since our direct
driver already exposes power config info in Linux (or should, and we can
improve that), I don't particularly have plans to expose the SMC Type-C
data other than perhaps something in the existing power supply driver to
indicate which port is the currently chosen power input.

> If only those 2 nodes, then I really don't care so much and gpio-sec 
> would be fine. It's 1 node in 1 binding.

I think it might make sense to just go with this then. If Apple ever
introduces yet another GPIO sub-controller we can just add another one,
and honestly I don't think that's very likely, given they don't even use
any of the GPIOs from the second one from the AP yet. I don't see SMC
growing a big list of GPIO controllers any time soon, such that we
regret doing it this way. And then the node-name can just map to a given
key prefix statically in the driver, and thus we don't even need a
property for that (gpio would be gP?? and gpio-sec gp?? right now).

- Hector

Re: [PATCH 1/6] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Mark Kettenis]

> Date: Wed, 7 Sep 2022 03:38:09 +0900
> From: Hector Martin <marcan@marcan.st>
> 
> On 07/09/2022 02.35, Rob Herring wrote:
> 
> > If only those 2 nodes, then I really don't care so much and gpio-sec 
> > would be fine. It's 1 node in 1 binding.
> 
> I think it might make sense to just go with this then. If Apple ever
> introduces yet another GPIO sub-controller we can just add another one,
> and honestly I don't think that's very likely, given they don't even use
> any of the GPIOs from the second one from the AP yet. I don't see SMC
> growing a big list of GPIO controllers any time soon, such that we
> regret doing it this way. And then the node-name can just map to a given
> key prefix statically in the driver, and thus we don't even need a
> property for that (gpio would be gP?? and gpio-sec gp?? right now).

We could also use a compatible property to map the key prefix.  For
example we could have "apple,smc-gpio-primary" map to gP??  and
"apple,smc-gpio-secondary" map to gp??.  Then we can keep the generic
"gpio" name for both GPIO nodes.  And if Apple introduces yet another
GPIO sub-controller we just have to invent a new compatible for it.
Probably the cleanest solution if Rob still thinks it is better for
these nodes to have a compatible property anyway.

Re: [PATCH 0/6] Add Apple Mac System Management Controller GPIOs

[Russell King (Oracle)]

On Thu, Sep 01, 2022 at 02:54:01PM +0100, Russell King (Oracle) wrote:
> Hi,
> 
> This series adds support for the Apple Mac GPIO driver. These GPIOs
> are hadled via the System Management Controller.
> 
> The first two patches add the DT binding documentation for the new
> drivers.
> 
> The second two patches add the core System Management Controller
> support.
> 
> The last two patches add the GPIO support.
> 
> DT updates will follow once the bindings have been reviewed.
> 
> Patches taken from the Asahi project.

All,

As the discussion on this series was _extremely_ long, and did not
take account of my posting of v2, I will shortly be posting v3, but
as a much smaller series.

My plan is to get the two patches for GPIO driver sorted as an
independent series from the rest of the patches (that being the
platform or mfd bits).

I believe I've addressed all the comments for the GPIO driver, so
expect that part shortly. The other part will take longer due to
further changes being required.

Russell.


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH v3 0/7] Add Apple Mac System Management Controller GPIOs

[Russell King (Oracle)]

Hi,

This is version 3 of the series adds support for the Apple Mac GPIO
driver. These GPIOs are hadled via the System Management Controller.

The first patch adds the core SMC support to the MFD subsystem.

Patch 2 prepares the printf code for printing generic fourccs, as
required by patch 4 and 7.

Patch 3 adds the DT binding documentation for the rtkit backend.

Patch 4 adds the rtkit backend for the SMC core.

Patch 5 adds the device tree updates for the rtkit communication
channel and gpio sub device to DT.

Patch 6 adds the DT binding documentation for the gpio client.

Patch 7 adds the gpio client.

Patches mostly taken from the Asahi project and then modified due to
review comments from the previous postings.

I may have missed some comments as there's been quite a lot of
discussion on previous postings.

 Documentation/core-api/printk-formats.rst          |  32 ++
 .../devicetree/bindings/gpio/apple,smc-gpio.yaml   |  37 ++
 .../devicetree/bindings/mfd/apple,smc.yaml         |  67 +++
 arch/arm64/boot/dts/apple/t8103.dtsi               |  27 ++
 drivers/gpio/Kconfig                               |  11 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-macsmc.c                         | 245 +++++++++++
 drivers/mfd/Kconfig                                |   4 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/macsmc.c                               | 239 +++++++++++
 drivers/platform/Kconfig                           |   2 +
 drivers/platform/Makefile                          |   1 +
 drivers/platform/apple/Kconfig                     |  34 ++
 drivers/platform/apple/Makefile                    |   7 +
 drivers/platform/apple/macsmc-rtkit.c              | 455 +++++++++++++++++++++
 include/linux/mfd/macsmc.h                         | 104 +++++
 lib/test_printf.c                                  |  39 +-
 lib/vsprintf.c                                     |  35 +-
 18 files changed, 1328 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
 create mode 100644 drivers/gpio/gpio-macsmc.c
 create mode 100644 drivers/mfd/macsmc.c
 create mode 100644 drivers/platform/apple/Kconfig
 create mode 100644 drivers/platform/apple/Makefile
 create mode 100644 drivers/platform/apple/macsmc-rtkit.c
 create mode 100644 include/linux/mfd/macsmc.h

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH v3 1/7] mfd: Add core Apple Mac SMC driver

[Russell King]

From: Hector Martin <marcan@marcan.st>

This driver implements support for the SMC (System Management
Controller) in Apple Macs. In contrast to the existing applesmc driver,
it uses pluggable backends that allow it to support different SMC
implementations, and uses the MFD subsystem to expose the core SMC
functionality so that specific features (gpio, hwmon, battery, etc.) can
be implemented by separate drivers in their respective downstream
subsystems.

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/mfd/Kconfig        |   4 +
 drivers/mfd/Makefile       |   1 +
 drivers/mfd/macsmc.c       | 239 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/macsmc.h | 104 ++++++++++++++++
 4 files changed, 348 insertions(+)
 create mode 100644 drivers/mfd/macsmc.c
 create mode 100644 include/linux/mfd/macsmc.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8b93856de432..f73e098b7228 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -224,6 +224,10 @@ config MFD_CROS_EC_DEV
 	  To compile this driver as a module, choose M here: the module will be
 	  called cros-ec-dev.
 
+config MFD_MACSMC
+	tristate
+	select MFD_CORE
+
 config MFD_MADERA
 	tristate "Cirrus Logic Madera codecs"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 7ed3ef4a698c..a5271b578d31 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
 obj-$(CONFIG_MFD_ENE_KB3930)	+= ene-kb3930.o
 obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
 obj-$(CONFIG_MFD_GATEWORKS_GSC)	+= gateworks-gsc.o
+obj-$(CONFIG_MFD_MACSMC)	+= macsmc.o
 
 obj-$(CONFIG_HTC_PASIC3)	+= htc-pasic3.o
 obj-$(CONFIG_HTC_I2CPLD)	+= htc-i2cpld.o
diff --git a/drivers/mfd/macsmc.c b/drivers/mfd/macsmc.c
new file mode 100644
index 000000000000..e5c3957efea4
--- /dev/null
+++ b/drivers/mfd/macsmc.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SMC core framework
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include <linux/device.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/macsmc.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+
+struct apple_smc {
+	struct device *dev;
+
+	void *be_cookie;
+	const struct apple_smc_backend_ops *be;
+
+	struct mutex mutex;
+
+	u32 key_count;
+	smc_key first_key;
+	smc_key last_key;
+
+	struct blocking_notifier_head event_handlers;
+};
+
+static const struct mfd_cell apple_smc_devs[] = {
+	MFD_CELL_OF("macsmc-gpio", NULL, NULL, 0, 0, "apple,smc-gpio"),
+	MFD_CELL_NAME("macsmc-hid"),
+	MFD_CELL_NAME("macsmc-power"),
+	MFD_CELL_NAME("macsmc-reboot"),
+	MFD_CELL_OF("macsmc-rtc", NULL, NULL, 0, 0, "apple,smc-rtc"),
+};
+
+int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->read_key(smc->be_cookie, key, buf, size);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_read);
+
+int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->write_key(smc->be_cookie, key, buf, size);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_write);
+
+int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size)
+{
+	int ret;
+
+	/*
+	 * Will fail if SMC is busy. This is only used by SMC reboot/poweroff
+	 * final calls, so it doesn't really matter at that point.
+	 */
+	if (!mutex_trylock(&smc->mutex))
+		return -EBUSY;
+
+	ret = smc->be->write_key_atomic(smc->be_cookie, key, buf, size);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_write_atomic);
+
+int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
+		 void *rbuf, size_t rsize)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->rw_key(smc->be_cookie, key, wbuf, wsize, rbuf, rsize);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_rw);
+
+int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->get_key_by_index(smc->be_cookie, index, key);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_get_key_by_index);
+
+int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->get_key_info(smc->be_cookie, key, info);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_get_key_info);
+
+int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key)
+{
+	int start = 0, count = smc->key_count;
+	int ret;
+
+	if (key <= smc->first_key)
+		return 0;
+	if (key > smc->last_key)
+		return smc->key_count;
+
+	while (count > 1) {
+		int pivot = start + ((count - 1) >> 1);
+		smc_key pkey;
+
+		ret = apple_smc_get_key_by_index(smc, pivot, &pkey);
+		if (ret < 0)
+			return ret;
+
+		if (pkey == key)
+			return pivot;
+
+		pivot++;
+
+		if (pkey < key) {
+			count -= pivot - start;
+			start = pivot;
+		} else {
+			count = pivot - start;
+		}
+	}
+
+	return start;
+}
+EXPORT_SYMBOL(apple_smc_find_first_key_index);
+
+int apple_smc_get_key_count(struct apple_smc *smc)
+{
+	return smc->key_count;
+}
+EXPORT_SYMBOL(apple_smc_get_key_count);
+
+void apple_smc_event_received(struct apple_smc *smc, uint32_t event)
+{
+	dev_dbg(smc->dev, "Event: 0x%08x\n", event);
+	blocking_notifier_call_chain(&smc->event_handlers, event, NULL);
+}
+EXPORT_SYMBOL(apple_smc_event_received);
+
+int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n)
+{
+	return blocking_notifier_chain_register(&smc->event_handlers, n);
+}
+EXPORT_SYMBOL(apple_smc_register_notifier);
+
+int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n)
+{
+	return blocking_notifier_chain_unregister(&smc->event_handlers, n);
+}
+EXPORT_SYMBOL(apple_smc_unregister_notifier);
+
+void *apple_smc_get_cookie(struct apple_smc *smc)
+{
+	return smc->be_cookie;
+}
+EXPORT_SYMBOL(apple_smc_get_cookie);
+
+struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, void *cookie)
+{
+	struct apple_smc *smc;
+	u32 count;
+	int ret;
+
+	smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL);
+	if (!smc)
+		return ERR_PTR(-ENOMEM);
+
+	smc->dev = dev;
+	smc->be_cookie = cookie;
+	smc->be = ops;
+	mutex_init(&smc->mutex);
+	BLOCKING_INIT_NOTIFIER_HEAD(&smc->event_handlers);
+
+	ret = apple_smc_read_u32(smc, SMC_KEY(#KEY), &count);
+	if (ret)
+		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get key count"));
+	smc->key_count = be32_to_cpu(count);
+
+	ret = apple_smc_get_key_by_index(smc, 0, &smc->first_key);
+	if (ret)
+		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get first key"));
+
+	ret = apple_smc_get_key_by_index(smc, smc->key_count - 1, &smc->last_key);
+	if (ret)
+		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get last key"));
+
+	/* Enable notifications */
+	apple_smc_write_flag(smc, SMC_KEY(NTAP), 1);
+
+	dev_info(dev, "Initialized (%d keys %p4ch..%p4ch)\n",
+		 smc->key_count, &smc->first_key, &smc->last_key);
+
+	dev_set_drvdata(dev, smc);
+
+	ret = mfd_add_devices(dev, -1, apple_smc_devs, ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL);
+	if (ret)
+		return ERR_PTR(dev_err_probe(dev, ret, "Subdevice initialization failed"));
+
+	return smc;
+}
+EXPORT_SYMBOL(apple_smc_probe);
+
+int apple_smc_remove(struct apple_smc *smc)
+{
+	mfd_remove_devices(smc->dev);
+
+	/* Disable notifications */
+	apple_smc_write_flag(smc, SMC_KEY(NTAP), 1);
+
+	return 0;
+}
+EXPORT_SYMBOL(apple_smc_remove);
+
+MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_DESCRIPTION("Apple SMC core");
diff --git a/include/linux/mfd/macsmc.h b/include/linux/mfd/macsmc.h
new file mode 100644
index 000000000000..99cfa23f27bd
--- /dev/null
+++ b/include/linux/mfd/macsmc.h
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SMC core definitions
+ * Copyright (C) The Asahi Linux Contributors
+ */
+
+#ifndef _LINUX_MFD_MACSMC_H
+#define _LINUX_MFD_MACSMC_H
+
+struct apple_smc;
+
+typedef u32 smc_key;
+
+#define SMC_KEY(s) (smc_key)(_SMC_KEY(#s))
+#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
+
+#define APPLE_SMC_READABLE BIT(7)
+#define APPLE_SMC_WRITABLE BIT(6)
+#define APPLE_SMC_FUNCTION BIT(4)
+
+struct apple_smc_key_info {
+	u8 size;
+	u32 type_code;
+	u8 flags;
+};
+
+int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size);
+int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size);
+int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size);
+int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
+		 void *rbuf, size_t rsize);
+
+int apple_smc_get_key_count(struct apple_smc *smc);
+int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key);
+int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key);
+int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info);
+
+static inline bool apple_smc_key_exists(struct apple_smc *smc, smc_key key)
+{
+	return apple_smc_get_key_info(smc, key, NULL) >= 0;
+}
+
+#define APPLE_SMC_TYPE_OPS(type) \
+	static inline int apple_smc_read_##type(struct apple_smc *smc, smc_key key, type *p) \
+	{ \
+		int ret = apple_smc_read(smc, key, p, sizeof(*p)); \
+		return (ret < 0) ? ret : ((ret != sizeof(*p)) ? -EINVAL : 0); \
+	} \
+	static inline int apple_smc_write_##type(struct apple_smc *smc, smc_key key, type p) \
+	{ \
+		return apple_smc_write(smc, key, &p, sizeof(p)); \
+	} \
+	static inline int apple_smc_write_##type##_atomic(struct apple_smc *smc, smc_key key, type p) \
+	{ \
+		return apple_smc_write_atomic(smc, key, &p, sizeof(p)); \
+	} \
+	static inline int apple_smc_rw_##type(struct apple_smc *smc, smc_key key, \
+					      type w, type *r) \
+	{ \
+		int ret = apple_smc_rw(smc, key, &w, sizeof(w), r, sizeof(*r)); \
+		return (ret < 0) ? ret : ((ret != sizeof(*r)) ? -EINVAL : 0); \
+	}
+
+APPLE_SMC_TYPE_OPS(u64)
+APPLE_SMC_TYPE_OPS(u32)
+APPLE_SMC_TYPE_OPS(u16)
+APPLE_SMC_TYPE_OPS(u8)
+APPLE_SMC_TYPE_OPS(s64)
+APPLE_SMC_TYPE_OPS(s32)
+APPLE_SMC_TYPE_OPS(s16)
+APPLE_SMC_TYPE_OPS(s8)
+
+static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key)
+{
+	u8 val;
+	int ret = apple_smc_read_u8(smc, key, &val);
+	if (ret < 0)
+		return ret;
+	return val ? 1 : 0;
+}
+#define apple_smc_write_flag apple_smc_write_u8
+
+int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n);
+int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n);
+
+/* backend interface */
+
+struct apple_smc_backend_ops {
+	int (*read_key)(void *cookie, smc_key key, void *buf, size_t size);
+	int (*write_key)(void *cookie, smc_key key, void *buf, size_t size);
+	int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size);
+	int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize,
+		      void *rbuf, size_t rsize);
+	int (*get_key_by_index)(void *cookie, int index, smc_key *key);
+	int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info);
+};
+
+struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops,
+				  void *cookie);
+void *apple_smc_get_cookie(struct apple_smc *smc);
+int apple_smc_remove(struct apple_smc *smc);
+void apple_smc_event_received(struct apple_smc *smc, uint32_t event);
+
+#endif
-- 
2.30.2

[PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Russell King]

From: Hector Martin <marcan@marcan.st>

%p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
it's useful to be able to print generic 4-character codes formatted as
an integer. Extend it to add format specifiers for printing generic
32-bit FOURCCs with various endian semantics:

%p4ch   Host-endian
%p4cl	Little-endian
%p4cb	Big-endian
%p4cr	Reverse-endian

The endianness determines how bytes are interpreted as a u32, and the
FOURCC is then always printed MSByte-first (this is the opposite of
V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
allow printing LSByte-first FOURCCs stored in host endian order
(other than the hex form being in character order, not the integer
value).

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++
 lib/test_printf.c                         | 39 +++++++++++++++++++----
 lib/vsprintf.c                            | 35 ++++++++++++++++----
 3 files changed, 93 insertions(+), 13 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index dbe1aacc79d0..92a488884cf8 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -625,6 +625,38 @@ Passed by reference.
 	%p4cc	Y10  little-endian (0x20303159)
 	%p4cc	NV12 big-endian (0xb231564e)
 
+Generic FourCC code
+-------------------
+
+::
+	%p4c[hrbl]	gP00 (0x67503030)
+
+Print a generic FourCC code, as both ASCII characters and its numerical
+value as hexadecimal.
+
+The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify
+host, reversed, big or little endian order data respectively. Host endian
+order means the data is interpreted as a 32-bit integer and the most
+significant byte is printed first; that is, the character code as printed
+matches the byte order stored in memory on big-endian systems, and is reversed
+on little-endian systems.
+
+Passed by reference.
+
+Examples for a little-endian machine, given &(u32)0x67503030::
+
+	%p4ch	gP00 (0x67503030)
+	%p4cr	00Pg (0x30305067)
+	%p4cb	00Pg (0x30305067)
+	%p4cl	gP00 (0x67503030)
+
+Examples for a big-endian machine, given &(u32)0x67503030::
+
+	%p4ch	gP00 (0x67503030)
+	%p4cr	00Pg (0x30305067)
+	%p4cb	gP00 (0x67503030)
+	%p4cl	00Pg (0x30305067)
+
 Rust
 ----
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 4bd15a593fbd..77a9128a6b5a 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -731,21 +731,46 @@ static void __init fwnode_pointer(void)
 	software_node_unregister_nodes(softnodes);
 }
 
+struct fourcc_struct {
+	u32 code;
+	const char *str;
+};
+
+static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
+				       const char *fmt)
+{
+	size_t i;
+
+	for (i = 0; i < n; i++)
+		test(fc[i].str, fmt, &fc[i].code);
+}
+
 static void __init fourcc_pointer(void)
 {
-	struct {
-		u32 code;
-		char *str;
-	} const try[] = {
+	struct fourcc_struct const try_cc[] = {
 		{ 0x3231564e, "NV12 little-endian (0x3231564e)", },
 		{ 0xb231564e, "NV12 big-endian (0xb231564e)", },
 		{ 0x10111213, ".... little-endian (0x10111213)", },
 		{ 0x20303159, "Y10  little-endian (0x20303159)", },
 	};
-	unsigned int i;
+	struct fourcc_struct const try_ch = {
+		0x41424344, "ABCD (0x41424344)",
+	};
+	struct fourcc_struct const try_cr = {
+		0x41424344, "DCBA (0x44434241)",
+	};
+	struct fourcc_struct const try_cl = {
+		le32_to_cpu(0x41424344), "ABCD (0x41424344)",
+	};
+	struct fourcc_struct const try_cb = {
+		be32_to_cpu(0x41424344), "ABCD (0x41424344)",
+	};
 
-	for (i = 0; i < ARRAY_SIZE(try); i++)
-		test(try[i].str, "%p4cc", &try[i].code);
+	fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc");
+	fourcc_pointer_test(&try_ch, 1, "%p4ch");
+	fourcc_pointer_test(&try_cr, 1, "%p4cr");
+	fourcc_pointer_test(&try_cl, 1, "%p4cl");
+	fourcc_pointer_test(&try_cb, 1, "%p4cb");
 }
 
 static void __init
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 24f37bab8bc1..17064b839f19 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1762,27 +1762,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
 	char output[sizeof("0123 little-endian (0x01234567)")];
 	char *p = output;
 	unsigned int i;
+	bool pixel_fmt = false;
 	u32 orig, val;
 
-	if (fmt[1] != 'c' || fmt[2] != 'c')
+	if (fmt[1] != 'c')
 		return error_string(buf, end, "(%p4?)", spec);
 
 	if (check_pointer(&buf, end, fourcc, spec))
 		return buf;
 
 	orig = get_unaligned(fourcc);
-	val = orig & ~BIT(31);
+	switch (fmt[2]) {
+	case 'h':
+		val = orig;
+		break;
+	case 'r':
+		val = orig = swab32(orig);
+		break;
+	case 'l':
+		val = orig = le32_to_cpu(orig);
+		break;
+	case 'b':
+		val = orig = be32_to_cpu(orig);
+		break;
+	case 'c':
+		/* Pixel formats are printed LSB-first */
+		val = swab32(orig & ~BIT(31));
+		pixel_fmt = true;
+		break;
+	default:
+		return error_string(buf, end, "(%p4?)", spec);
+	}
 
 	for (i = 0; i < sizeof(u32); i++) {
-		unsigned char c = val >> (i * 8);
+		unsigned char c = val >> ((3 - i) * 8);
 
 		/* Print non-control ASCII characters as-is, dot otherwise */
 		*p++ = isascii(c) && isprint(c) ? c : '.';
 	}
 
-	*p++ = ' ';
-	strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
-	p += strlen(p);
+	if (pixel_fmt) {
+		*p++ = ' ';
+		strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
+		p += strlen(p);
+	}
 
 	*p++ = ' ';
 	*p++ = '(';
-- 
2.30.2

[PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

Add a DT binding for the Apple Mac System Management Controller.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml

diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
new file mode 100644
index 000000000000..014eba5a1bbc
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Mac System Management Controller
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+description:
+  Apple Mac System Management Controller implements various functions
+  such as GPIO, RTC, power, reboot.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - apple,t6000-smc
+          - apple,t8103-smc
+          - apple,t8112-smc
+      - const: apple,smc
+
+  reg:
+    items:
+      - description: SMC area
+      - description: SRAM area
+
+  reg-names:
+    items:
+      - const: smc
+      - const: sram
+
+  mboxes:
+    maxItems: 1
+
+  gpio:
+    $ref: /schemas/gpio/gpio-macsmc.yaml
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - mboxes
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      smc@23e400000 {
+        compatible = "apple,t8103-smc", "apple,smc";
+        reg = <0x2 0x3e400000 0x0 0x4000>,
+               <0x2 0x3fe00000 0x0 0x100000>;
+        reg-names = "smc", "sram";
+        mboxes = <&smc_mbox>;
+
+        smc_gpio: gpio {
+          gpio-controller;
+          #gpio-cells = <2>;
+        };
+      };
+    };
-- 
2.30.2

[PATCH v3 4/7] platform/apple: Add new Apple Mac SMC driver

[Russell King]

From: Hector Martin <marcan@marcan.st>

This driver implements support for the SMC (System Management
Controller) in Apple Macs. In contrast to the existing applesmc driver,
it uses pluggable backends that allow it to support different SMC
implementations, and uses the MFD subsystem to expose the core SMC
functionality so that specific features (gpio, hwmon, battery, etc.) can
be implemented by separate drivers in their respective downstream
subsystems.

The initial RTKit backend adds support for Apple Silicon Macs (M1 et
al). We hope a backend for T2 Macs will be written in the future
(since those are not supported by applesmc), and eventually an x86
backend would allow us to fully deprecate applesmc in favor of this
driver.

Signed-off-by: Hector Martin <marcan@marcan.st>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/platform/Kconfig              |   2 +
 drivers/platform/Makefile             |   1 +
 drivers/platform/apple/Kconfig        |  34 ++
 drivers/platform/apple/Makefile       |   7 +
 drivers/platform/apple/macsmc-rtkit.c | 455 ++++++++++++++++++++++++++
 5 files changed, 499 insertions(+)
 create mode 100644 drivers/platform/apple/Kconfig
 create mode 100644 drivers/platform/apple/Makefile
 create mode 100644 drivers/platform/apple/macsmc-rtkit.c

diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
index dbd327712205..31a361ecfc5e 100644
--- a/drivers/platform/Kconfig
+++ b/drivers/platform/Kconfig
@@ -15,4 +15,6 @@ source "drivers/platform/olpc/Kconfig"
 
 source "drivers/platform/surface/Kconfig"
 
+source "drivers/platform/apple/Kconfig"
+
 source "drivers/platform/x86/Kconfig"
diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
index 41640172975a..d2baa4eb4f13 100644
--- a/drivers/platform/Makefile
+++ b/drivers/platform/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OLPC_EC)		+= olpc/
 obj-$(CONFIG_GOLDFISH)		+= goldfish/
 obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
 obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
+obj-$(CONFIG_APPLE_PLATFORMS)	+= apple/
diff --git a/drivers/platform/apple/Kconfig b/drivers/platform/apple/Kconfig
new file mode 100644
index 000000000000..35963c348d05
--- /dev/null
+++ b/drivers/platform/apple/Kconfig
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Apple Platform-Specific Drivers
+#
+
+menuconfig APPLE_PLATFORMS
+	bool "Apple Mac Platform-Specific Device Drivers"
+	depends on ARCH_APPLE || X86 || COMPILE_TEST
+	default ARCH_APPLE
+	help
+	  Say Y here to get to see options for platform-specific device drivers
+	  for Apple devices. This option alone does not add any kernel code.
+
+	  If you say N, all options in this submenu will be skipped and disabled.
+
+if APPLE_PLATFORMS
+
+config APPLE_SMC_RTKIT
+	tristate "Apple SMC RTKit backend"
+	depends on ARCH_APPLE || (COMPILE_TEST && 64BIT)
+	depends on APPLE_RTKIT
+	default ARCH_APPLE
+	select MFD_MACSMC
+	help
+	  Build support for the Apple System Management Controller found in
+	  Apple Macs, communicated with via the RTKit backend. This is required
+	  for Apple Silicon Macs.
+
+	  Say Y here if you have an Apple Silicon Mac.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called macsmc-rtkit.
+
+endif
diff --git a/drivers/platform/apple/Makefile b/drivers/platform/apple/Makefile
new file mode 100644
index 000000000000..16cce9061ee8
--- /dev/null
+++ b/drivers/platform/apple/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for linux/drivers/platform/apple
+# Apple Platform-Specific Drivers
+#
+
+obj-$(CONFIG_APPLE_SMC_RTKIT)		+= macsmc-rtkit.o
diff --git a/drivers/platform/apple/macsmc-rtkit.c b/drivers/platform/apple/macsmc-rtkit.c
new file mode 100644
index 000000000000..81560fb73e06
--- /dev/null
+++ b/drivers/platform/apple/macsmc-rtkit.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SMC RTKit backend
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/mfd/macsmc.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/soc/apple/rtkit.h>
+#include <asm/unaligned.h>
+
+#define SMC_ENDPOINT			0x20
+
+/* Guess */
+#define SMC_SHMEM_SIZE			0x1000
+#define SMC_MAX_SIZE			255
+
+#define SMC_MSG_READ_KEY		0x10
+#define SMC_MSG_WRITE_KEY		0x11
+#define SMC_MSG_GET_KEY_BY_INDEX	0x12
+#define SMC_MSG_GET_KEY_INFO		0x13
+#define SMC_MSG_INITIALIZE		0x17
+#define SMC_MSG_NOTIFICATION		0x18
+#define SMC_MSG_RW_KEY			0x20
+
+#define SMC_DATA			GENMASK(63, 32)
+#define SMC_WSIZE			GENMASK(31, 24)
+#define SMC_SIZE			GENMASK(23, 16)
+#define SMC_ID				GENMASK(15, 12)
+#define SMC_MSG				GENMASK(7, 0)
+#define SMC_RESULT			SMC_MSG
+
+#define SMC_RECV_TIMEOUT		500
+
+struct apple_smc_rtkit {
+	struct device *dev;
+	struct apple_smc *core;
+	struct apple_rtkit *rtk;
+
+	struct completion init_done;
+	bool initialized;
+	bool alive;
+
+	struct resource *sram;
+	void __iomem *sram_base;
+	struct apple_rtkit_shmem shmem;
+
+	unsigned int msg_id;
+
+	bool atomic_pending;
+	struct completion cmd_done;
+	u64 cmd_ret;
+};
+
+static int apple_smc_rtkit_write_key_atomic(void *cookie, smc_key key, void *buf, size_t size)
+{
+	struct apple_smc_rtkit *smc = cookie;
+	int ret;
+	u64 msg;
+	u8 result;
+
+	if (size > SMC_MAX_SIZE || size == 0)
+		return -EINVAL;
+
+	if (!smc->alive)
+		return -EIO;
+
+	memcpy_toio(smc->shmem.iomem, buf, size);
+	smc->msg_id = (smc->msg_id + 1) & 0xf;
+	msg = (FIELD_PREP(SMC_MSG, SMC_MSG_WRITE_KEY) |
+	       FIELD_PREP(SMC_SIZE, size) |
+	       FIELD_PREP(SMC_ID, smc->msg_id) |
+	       FIELD_PREP(SMC_DATA, key));
+	smc->atomic_pending = true;
+
+	ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, msg, NULL, true);
+	if (ret < 0) {
+		dev_err(smc->dev, "Failed to send command (%d)\n", ret);
+		return ret;
+	}
+
+	while (smc->atomic_pending) {
+		ret = apple_rtkit_poll(smc->rtk);
+		if (ret < 0) {
+			dev_err(smc->dev, "RTKit poll failed (%llx)", msg);
+			return ret;
+		}
+		udelay(100);
+	}
+
+	if (FIELD_GET(SMC_ID, smc->cmd_ret) != smc->msg_id) {
+		dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n",
+			smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret));
+		return -EIO;
+	}
+
+	result = FIELD_GET(SMC_RESULT, smc->cmd_ret);
+	if (result != 0)
+		return -result;
+
+	return FIELD_GET(SMC_SIZE, smc->cmd_ret);
+}
+
+static int apple_smc_cmd(struct apple_smc_rtkit *smc, u64 cmd, u64 arg,
+			 u64 size, u64 wsize, u32 *ret_data)
+{
+	int ret;
+	u64 msg;
+	u8 result;
+
+	if (!smc->alive)
+		return -EIO;
+
+	reinit_completion(&smc->cmd_done);
+
+	smc->msg_id = (smc->msg_id + 1) & 0xf;
+	msg = (FIELD_PREP(SMC_MSG, cmd) |
+	       FIELD_PREP(SMC_SIZE, size) |
+	       FIELD_PREP(SMC_WSIZE, wsize) |
+	       FIELD_PREP(SMC_ID, smc->msg_id) |
+	       FIELD_PREP(SMC_DATA, arg));
+
+	ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, msg, NULL, false);
+	if (ret < 0) {
+		dev_err(smc->dev, "Failed to send command\n");
+		return ret;
+	}
+
+	do {
+		if (wait_for_completion_timeout(&smc->cmd_done,
+						msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) {
+			dev_err(smc->dev, "Command timed out (%llx)", msg);
+			return -ETIMEDOUT;
+		}
+		if (FIELD_GET(SMC_ID, smc->cmd_ret) == smc->msg_id)
+			break;
+		dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n",
+			smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret));
+	} while(1);
+
+	result = FIELD_GET(SMC_RESULT, smc->cmd_ret);
+	if (result != 0)
+		return -result;
+
+	if (ret_data)
+		*ret_data = FIELD_GET(SMC_DATA, smc->cmd_ret);
+
+	return FIELD_GET(SMC_SIZE, smc->cmd_ret);
+}
+
+static int _apple_smc_rtkit_read_key(struct apple_smc_rtkit *smc, smc_key key,
+				     void *buf, size_t size, size_t wsize)
+{
+	int ret;
+	u32 rdata;
+	u64 cmd;
+
+	if (size > SMC_MAX_SIZE || size == 0)
+		return -EINVAL;
+
+	cmd = wsize ? SMC_MSG_RW_KEY : SMC_MSG_READ_KEY;
+
+	ret = apple_smc_cmd(smc, cmd, key, size, wsize, &rdata);
+	if (ret < 0)
+		return ret;
+
+	if (size <= 4)
+		memcpy(buf, &rdata, size);
+	else
+		memcpy_fromio(buf, smc->shmem.iomem, size);
+
+	return ret;
+}
+
+static int apple_smc_rtkit_read_key(void *cookie, smc_key key, void *buf, size_t size)
+{
+	return _apple_smc_rtkit_read_key(cookie, key, buf, size, 0);
+}
+
+static int apple_smc_rtkit_write_key(void *cookie, smc_key key, void *buf, size_t size)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	if (size > SMC_MAX_SIZE || size == 0)
+		return -EINVAL;
+
+	memcpy_toio(smc->shmem.iomem, buf, size);
+	return apple_smc_cmd(smc, SMC_MSG_WRITE_KEY, key, size, 0, NULL);
+}
+
+static int apple_smc_rtkit_rw_key(void *cookie, smc_key key,
+				  void *wbuf, size_t wsize, void *rbuf, size_t rsize)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	if (wsize > SMC_MAX_SIZE || wsize == 0)
+		return -EINVAL;
+
+	memcpy_toio(smc->shmem.iomem, wbuf, wsize);
+	return _apple_smc_rtkit_read_key(smc, key, rbuf, rsize, wsize);
+}
+
+static int apple_smc_rtkit_get_key_by_index(void *cookie, int index, smc_key *key)
+{
+	struct apple_smc_rtkit *smc = cookie;
+	int ret;
+
+	ret = apple_smc_cmd(smc, SMC_MSG_GET_KEY_BY_INDEX, index, 0, 0, key);
+
+	*key = swab32(*key);
+	return ret;
+}
+
+static int apple_smc_rtkit_get_key_info(void *cookie, smc_key key, struct apple_smc_key_info *info)
+{
+	struct apple_smc_rtkit *smc = cookie;
+	u8 key_info[6];
+	int ret;
+
+	ret = apple_smc_cmd(smc, SMC_MSG_GET_KEY_INFO, key, 0, 0, NULL);
+	if (ret >= 0 && info) {
+		memcpy_fromio(key_info, smc->shmem.iomem, sizeof(key_info));
+		info->size = key_info[0];
+		info->type_code = get_unaligned_be32(&key_info[1]);
+		info->flags = key_info[5];
+	}
+	return ret;
+}
+
+static const struct apple_smc_backend_ops apple_smc_rtkit_be_ops = {
+	.read_key = apple_smc_rtkit_read_key,
+	.write_key = apple_smc_rtkit_write_key,
+	.write_key_atomic = apple_smc_rtkit_write_key_atomic,
+	.rw_key = apple_smc_rtkit_rw_key,
+	.get_key_by_index = apple_smc_rtkit_get_key_by_index,
+	.get_key_info = apple_smc_rtkit_get_key_info,
+};
+
+static void apple_smc_rtkit_crashed(void *cookie)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	dev_err(smc->dev, "SMC crashed! Your system will reboot in a few seconds...\n");
+	smc->alive = false;
+}
+
+static int apple_smc_rtkit_shmem_setup(void *cookie, struct apple_rtkit_shmem *bfr)
+{
+	struct apple_smc_rtkit *smc = cookie;
+	struct resource res = {
+		.start = bfr->iova,
+		.end = bfr->iova + bfr->size - 1,
+		.name = "rtkit_map",
+		.flags = smc->sram->flags,
+	};
+
+	if (!bfr->iova) {
+		dev_err(smc->dev, "RTKit wants a RAM buffer\n");
+		return -EIO;
+	}
+
+	if (res.end < res.start || !resource_contains(smc->sram, &res)) {
+		dev_err(smc->dev,
+			"RTKit buffer request outside SRAM region: %pR", &res);
+		return -EFAULT;
+	}
+
+	bfr->iomem = smc->sram_base + (res.start - smc->sram->start);
+	bfr->is_mapped = true;
+
+	return 0;
+}
+
+static void apple_smc_rtkit_shmem_destroy(void *cookie, struct apple_rtkit_shmem *bfr)
+{
+	// no-op
+}
+
+static bool apple_smc_rtkit_recv_early(void *cookie, u8 endpoint, u64 message)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	if (endpoint != SMC_ENDPOINT) {
+		dev_err(smc->dev, "Received message for unknown endpoint 0x%x\n", endpoint);
+		return false;
+	}
+
+	if (!smc->initialized) {
+		int ret;
+
+		smc->shmem.iova = message;
+		smc->shmem.size = SMC_SHMEM_SIZE;
+		ret = apple_smc_rtkit_shmem_setup(smc, &smc->shmem);
+		if (ret < 0)
+			dev_err(smc->dev, "Failed to initialize shared memory\n");
+		else
+			smc->alive = true;
+		smc->initialized = true;
+		complete(&smc->init_done);
+	} else if (FIELD_GET(SMC_MSG, message) == SMC_MSG_NOTIFICATION) {
+		/* Handle these in the RTKit worker thread */
+		return false;
+	} else {
+		smc->cmd_ret = message;
+		if (smc->atomic_pending) {
+			smc->atomic_pending = false;
+		} else {
+			complete(&smc->cmd_done);
+		}
+	}
+
+	return true;
+}
+
+static void apple_smc_rtkit_recv(void *cookie, u8 endpoint, u64 message)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	if (endpoint != SMC_ENDPOINT) {
+		dev_err(smc->dev, "Received message for unknown endpoint 0x%x\n", endpoint);
+		return;
+	}
+
+	if (FIELD_GET(SMC_MSG, message) != SMC_MSG_NOTIFICATION) {
+		dev_err(smc->dev, "Received unknown message from worker: 0x%llx\n", message);
+		return;
+	}
+
+	apple_smc_event_received(smc->core, FIELD_GET(SMC_DATA, message));
+}
+
+static const struct apple_rtkit_ops apple_smc_rtkit_ops = {
+	.crashed = apple_smc_rtkit_crashed,
+	.recv_message = apple_smc_rtkit_recv,
+	.recv_message_early = apple_smc_rtkit_recv_early,
+	.shmem_setup = apple_smc_rtkit_shmem_setup,
+	.shmem_destroy = apple_smc_rtkit_shmem_destroy,
+};
+
+static int apple_smc_rtkit_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct apple_smc_rtkit *smc;
+	int ret;
+
+	smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL);
+	if (!smc)
+		return -ENOMEM;
+
+	smc->dev = dev;
+
+	smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
+	if (!smc->sram)
+		return dev_err_probe(dev, EIO,
+				     "No SRAM region");
+
+	smc->sram_base = devm_ioremap_resource(dev, smc->sram);
+	if (IS_ERR(smc->sram_base))
+		return dev_err_probe(dev, PTR_ERR(smc->sram_base),
+				     "Failed to map SRAM region");
+
+	smc->rtk =
+		devm_apple_rtkit_init(dev, smc, NULL, 0, &apple_smc_rtkit_ops);
+	if (IS_ERR(smc->rtk))
+		return dev_err_probe(dev, PTR_ERR(smc->rtk),
+				     "Failed to intialize RTKit");
+
+	ret = apple_rtkit_wake(smc->rtk);
+	if (ret != 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to wake up SMC");
+
+	ret = apple_rtkit_start_ep(smc->rtk, SMC_ENDPOINT);
+	if (ret != 0) {
+		dev_err(dev, "Failed to start endpoint");
+		goto cleanup;
+	}
+
+	init_completion(&smc->init_done);
+	init_completion(&smc->cmd_done);
+
+	ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT,
+				       FIELD_PREP(SMC_MSG, SMC_MSG_INITIALIZE), NULL, false);
+	if (ret < 0) {
+		ret = dev_err_probe(dev, ret, "Failed to send init message");
+		goto cleanup;
+	}
+
+	if (wait_for_completion_timeout(&smc->init_done,
+					msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) {
+		ret = -ETIMEDOUT;
+		dev_err(dev, "Timed out initializing SMC");
+		goto cleanup;
+	}
+
+	if (!smc->alive) {
+		ret = -EIO;
+		goto cleanup;
+	}
+
+	smc->core = apple_smc_probe(dev, &apple_smc_rtkit_be_ops, smc);
+	if (IS_ERR(smc->core)) {
+		ret = PTR_ERR(smc->core);
+		goto cleanup;
+	}
+
+	return 0;
+
+cleanup:
+	/* Try to shut down RTKit, if it's not completely wedged */
+	if (apple_rtkit_is_running(smc->rtk))
+		apple_rtkit_quiesce(smc->rtk);
+
+	return ret;
+}
+
+static int apple_smc_rtkit_remove(struct platform_device *pdev)
+{
+	struct apple_smc *core = platform_get_drvdata(pdev);
+	struct apple_smc_rtkit *smc = apple_smc_get_cookie(core);
+
+	apple_smc_remove(core);
+
+	if (apple_rtkit_is_running(smc->rtk))
+		apple_rtkit_quiesce(smc->rtk);
+
+	return 0;
+}
+
+static const struct of_device_id apple_smc_rtkit_of_match[] = {
+	{ .compatible = "apple,smc" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, apple_smc_rtkit_of_match);
+
+static struct platform_driver apple_smc_rtkit_driver = {
+	.driver = {
+		.name = "macsmc-rtkit",
+		.owner = THIS_MODULE,
+		.of_match_table = apple_smc_rtkit_of_match,
+	},
+	.probe = apple_smc_rtkit_probe,
+	.remove = apple_smc_rtkit_remove,
+};
+module_platform_driver(apple_smc_rtkit_driver);
+
+MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_DESCRIPTION("Apple SMC RTKit backend driver");
-- 
2.30.2

[PATCH v3 5/7] arm64: dts: apple: Add SMC node to t8103 devicetrees

[Russell King]

From: Hector Martin <marcan@marcan.st>

Signed-off-by: Hector Martin <marcan@marcan.st>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 arch/arm64/boot/dts/apple/t8103.dtsi | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index 51a63b29d404..e44ba4e541f6 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -336,6 +336,33 @@ wdt: watchdog@23d2b0000 {
 			interrupts = <AIC_IRQ 338 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		smc: smc@23e400000 {
+			compatible = "apple,t8103-smc", "apple,smc";
+			reg = <0x2 0x3e400000 0x0 0x4000>,
+				<0x2 0x3fe00000 0x0 0x100000>;
+			reg-names = "smc", "sram";
+			mboxes = <&smc_mbox>;
+
+			smc_gpio: gpio {
+				compatible = "apple,smc-gpio";
+				gpio-controller;
+				#gpio-cells = <2>;
+			};
+		};
+
+		smc_mbox: mbox@23e408000 {
+			compatible = "apple,t8103-asc-mailbox", "apple,asc-mailbox-v4";
+			reg = <0x2 0x3e408000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 400 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_IRQ 401 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_IRQ 402 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_IRQ 403 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "send-empty", "send-not-empty",
+				"recv-empty", "recv-not-empty";
+			#mbox-cells = <0>;
+		};
+
 		pinctrl_smc: pinctrl@23e820000 {
 			compatible = "apple,t8103-pinctrl", "apple,pinctrl";
 			reg = <0x2 0x3e820000 0x0 0x4000>;
-- 
2.30.2

[PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

Add the DT binding for the Apple Mac System Management Controller GPIOs.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
new file mode 100644
index 000000000000..1a415b78760b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Mac System Management Controller GPIO
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+description:
+  Apple Mac System Management Controller GPIO block.
+
+properties:
+  compatible:
+    const: apple,smc-gpio
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+required:
+  - compatible
+  - gpio-controller
+  - '#gpio-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio {
+        compatible = "apple,smc-gpio";
+        gpio-controller;
+        #gpio-cells = <2>;
+    };
-- 
2.30.2

[PATCH v3 7/7] gpio: Add new gpio-macsmc driver for Apple Macs

[Russell King]

From: Hector Martin <marcan@marcan.st>

This driver implements the GPIO service on top of the SMC framework
on Apple Mac machines. In particular, these are the GPIOs present in the
PMU IC which are used to control power to certain on-board devices.

Although the underlying hardware supports various pin config settings
(input/output, open drain, etc.), this driver does not implement that
functionality and leaves it up to the firmware to configure things
properly. We also don't yet support interrupts/events. This is
sufficient for device power control, which is the only thing we need to
support at this point. More features will be implemented when needed.

To our knowledge, only Apple Silicon Macs implement this SMC feature.

Signed-off-by: Hector Martin <marcan@marcan.st>
Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/gpio/Kconfig       |  11 ++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-macsmc.c | 245 +++++++++++++++++++++++++++++++++++++
 3 files changed, 257 insertions(+)
 create mode 100644 drivers/gpio/gpio-macsmc.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a01af1180616..08138322010a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1283,6 +1283,17 @@ config GPIO_LP87565
 	  This driver can also be built as a module. If so, the module will be
 	  called gpio-lp87565.
 
+config GPIO_MACSMC
+	tristate "Apple Mac SMC GPIO"
+	depends on MFD_MACSMC
+	default ARCH_APPLE
+	help
+	  Support for GPIOs controlled by the SMC microcontroller on Apple Mac
+	  systems.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-macsmc.
+
 config GPIO_MADERA
 	tristate "Cirrus Logic Madera class codecs"
 	depends on PINCTRL_MADERA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 29e3beb6548c..467833cc9a9d 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_GPIO_LP873X)		+= gpio-lp873x.o
 obj-$(CONFIG_GPIO_LP87565)		+= gpio-lp87565.o
 obj-$(CONFIG_GPIO_LPC18XX)		+= gpio-lpc18xx.o
 obj-$(CONFIG_GPIO_LPC32XX)		+= gpio-lpc32xx.o
+obj-$(CONFIG_GPIO_MACSMC)		+= gpio-macsmc.o
 obj-$(CONFIG_GPIO_MADERA)		+= gpio-madera.o
 obj-$(CONFIG_GPIO_MAX3191X)		+= gpio-max3191x.o
 obj-$(CONFIG_GPIO_MAX7300)		+= gpio-max7300.o
diff --git a/drivers/gpio/gpio-macsmc.c b/drivers/gpio/gpio-macsmc.c
new file mode 100644
index 000000000000..24ec98ad18f7
--- /dev/null
+++ b/drivers/gpio/gpio-macsmc.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SMC GPIO driver
+ * Copyright The Asahi Linux Contributors
+ *
+ * This driver implements basic SMC PMU GPIO support that can read inputs
+ * and write outputs. Mode changes and IRQ config are not yet implemented.
+ */
+
+#include <linux/bitmap.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/macsmc.h>
+
+#define MAX_GPIO 64
+
+/*
+ * Commands 0-6 are, presumably, the intended API.
+ * Command 0xff lets you get/set the pin configuration in detail directly,
+ * but the bit meanings seem not to be stable between devices/PMU hardware
+ * versions.
+ *
+ * We're going to try to make do with the low commands for now.
+ * We don't implement pin mode changes at this time.
+ */
+
+#define CMD_ACTION	(0 << 24)
+#define CMD_OUTPUT	(1 << 24)
+#define CMD_INPUT	(2 << 24)
+#define CMD_PINMODE	(3 << 24)
+#define CMD_IRQ_ENABLE	(4 << 24)
+#define CMD_IRQ_ACK	(5 << 24)
+#define CMD_IRQ_MODE	(6 << 24)
+#define CMD_CONFIG	(0xff << 24)
+
+#define MODE_INPUT	0
+#define MODE_OUTPUT	1
+#define MODE_VALUE_0	0
+#define MODE_VALUE_1	2
+
+#define IRQ_MODE_HIGH		0
+#define IRQ_MODE_LOW		1
+#define IRQ_MODE_RISING		2
+#define IRQ_MODE_FALLING	3
+#define IRQ_MODE_BOTH		4
+
+#define CONFIG_MASK	GENMASK(23, 16)
+#define CONFIG_VAL	GENMASK(7, 0)
+
+#define CONFIG_OUTMODE	GENMASK(7, 6)
+#define CONFIG_IRQMODE	GENMASK(5, 3)
+#define CONFIG_PULLDOWN	BIT(2)
+#define CONFIG_PULLUP	BIT(1)
+#define CONFIG_OUTVAL	BIT(0)
+
+/*
+ * Output modes seem to differ depending on the PMU in use... ?
+ * j274 / M1 (Sera PMU):
+ *   0 = input
+ *   1 = output
+ *   2 = open drain
+ *   3 = disable
+ * j314 / M1Pro (Maverick PMU):
+ *   0 = input
+ *   1 = open drain
+ *   2 = output
+ *   3 = ?
+ */
+
+struct macsmc_gpio {
+	struct device *dev;
+	struct apple_smc *smc;
+	struct gpio_chip gc;
+
+	int first_index;
+};
+
+static int macsmc_gpio_nr(smc_key key)
+{
+	int low = hex_to_bin(key & 0xff);
+	int high = hex_to_bin((key >> 8) & 0xff);
+
+	if (low < 0 || high < 0)
+		return -1;
+
+	return low | (high << 4);
+}
+
+static int macsmc_gpio_key(unsigned int offset)
+{
+	return _SMC_KEY("gP\0\0") | hex_asc_hi(offset) << 8 | hex_asc_lo(offset);
+}
+
+static int macsmc_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
+	smc_key key = macsmc_gpio_key(offset);
+	u32 val;
+	int ret;
+
+	/* First try reading the explicit pin mode register */
+	ret = apple_smc_rw_u32(smcgp->smc, key, CMD_PINMODE, &val);
+	if (!ret)
+		return (val & MODE_OUTPUT) ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+
+	/*
+	 * Less common IRQ configs cause CMD_PINMODE to fail, and so does open drain mode.
+	 * Fall back to reading IRQ mode, which will only succeed for inputs.
+	 */
+	ret = apple_smc_rw_u32(smcgp->smc, key, CMD_IRQ_MODE, &val);
+	return ret ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
+static int macsmc_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
+	smc_key key = macsmc_gpio_key(offset);
+	u32 cmd, val;
+	int ret;
+
+	ret = macsmc_gpio_get_direction(gc, offset);
+	if (ret < 0)
+		return ret;
+
+	if (ret == GPIO_LINE_DIRECTION_OUT)
+		cmd = CMD_OUTPUT;
+	else
+		cmd = CMD_INPUT;
+
+	ret = apple_smc_rw_u32(smcgp->smc, key, cmd, &val);
+	if (ret < 0)
+		return ret;
+
+	return val ? 1 : 0;
+}
+
+static void macsmc_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
+	smc_key key = macsmc_gpio_key(offset);
+	int ret;
+
+	value |= CMD_OUTPUT;
+	ret = apple_smc_write_u32(smcgp->smc, key, CMD_OUTPUT | value);
+	if (ret < 0)
+		dev_err(smcgp->dev, "GPIO set failed %p4ch = 0x%x\n", &key, value);
+}
+
+static int macsmc_gpio_init_valid_mask(struct gpio_chip *gc,
+				       unsigned long *valid_mask, unsigned int ngpios)
+{
+	struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
+	int count;
+	int i;
+
+	count = apple_smc_get_key_count(smcgp->smc) - smcgp->first_index;
+	if (count > MAX_GPIO)
+		count = MAX_GPIO;
+
+	bitmap_zero(valid_mask, ngpios);
+
+	for (i = 0; i < count; i++) {
+		int ret, gpio_nr;
+		smc_key key;
+
+		ret = apple_smc_get_key_by_index(smcgp->smc, smcgp->first_index + i, &key);
+		if (ret < 0)
+			return ret;
+
+		if (key > SMC_KEY(gPff))
+			break;
+
+		gpio_nr = macsmc_gpio_nr(key);
+		if (gpio_nr < 0 || gpio_nr > MAX_GPIO) {
+			dev_err(smcgp->dev, "Bad GPIO key %p4ch\n", &key);
+			continue;
+		}
+
+		set_bit(gpio_nr, valid_mask);
+	}
+
+	return 0;
+}
+
+static int macsmc_gpio_probe(struct platform_device *pdev)
+{
+	struct macsmc_gpio *smcgp;
+	struct apple_smc *smc = dev_get_drvdata(pdev->dev.parent);
+	smc_key key;
+	int ret;
+
+	smcgp = devm_kzalloc(&pdev->dev, sizeof(*smcgp), GFP_KERNEL);
+	if (!smcgp)
+		return -ENOMEM;
+
+	smcgp->dev = &pdev->dev;
+	smcgp->smc = smc;
+	smcgp->first_index = apple_smc_find_first_key_index(smc, SMC_KEY(gP00));
+
+	if (smcgp->first_index >= apple_smc_get_key_count(smc))
+		return -ENODEV;
+
+	ret = apple_smc_get_key_by_index(smc, smcgp->first_index, &key);
+	if (ret < 0)
+		return ret;
+
+	if (key > macsmc_gpio_key(MAX_GPIO - 1))
+		return -ENODEV;
+
+	dev_info(smcgp->dev, "First GPIO key: %p4ch\n", &key);
+
+	smcgp->gc.label = "macsmc-pmu-gpio";
+	smcgp->gc.owner = THIS_MODULE;
+	smcgp->gc.get = macsmc_gpio_get;
+	smcgp->gc.set = macsmc_gpio_set;
+	smcgp->gc.get_direction = macsmc_gpio_get_direction;
+	smcgp->gc.init_valid_mask = macsmc_gpio_init_valid_mask;
+	smcgp->gc.can_sleep = true;
+	smcgp->gc.ngpio = MAX_GPIO;
+	smcgp->gc.base = -1;
+	smcgp->gc.parent = &pdev->dev;
+
+	return devm_gpiochip_add_data(&pdev->dev, &smcgp->gc, smcgp);
+}
+
+static const struct of_device_id macsmc_gpio_of_table[] = {
+	{ .compatible = "apple,smc-gpio", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, macsmc_gpio_of_table);
+
+static struct platform_driver macsmc_gpio_driver = {
+	.driver = {
+		.name = "macsmc-gpio",
+		.of_match_table = macsmc_gpio_of_table,
+	},
+	.probe = macsmc_gpio_probe,
+};
+module_platform_driver(macsmc_gpio_driver);
+
+MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_DESCRIPTION("Apple SMC GPIO driver");
+MODULE_ALIAS("platform:macsmc-gpio");
-- 
2.30.2

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Linus Walleij]

On Tue, Nov 8, 2022 at 5:33 PM Russell King (Oracle)
<rmk+kernel@armlinux.org.uk> wrote:

> Add a DT binding for the Apple Mac System Management Controller.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Looks good to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 08/11/2022 17:33, Russell King (Oracle) wrote:
> Add a DT binding for the Apple Mac System Management Controller.

Drop the second, redundant "binding" from subject. It's already in prefix.

> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> new file mode 100644
> index 000000000000..014eba5a1bbc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple Mac System Management Controller
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description:
> +  Apple Mac System Management Controller implements various functions
> +  such as GPIO, RTC, power, reboot.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t6000-smc
> +          - apple,t8103-smc
> +          - apple,t8112-smc
> +      - const: apple,smc
> +
> +  reg:
> +    items:
> +      - description: SMC area
> +      - description: SRAM area
> +
> +  reg-names:
> +    items:
> +      - const: smc
> +      - const: sram
> +
> +  mboxes:
> +    maxItems: 1
> +
> +  gpio:
> +    $ref: /schemas/gpio/gpio-macsmc.yaml

So this depends on other patch, so:
1. You need mention the dependency in cover letter (nothing there),
2. Re-order patches.

The GPIO cannot go separate tree and this must be explicitly communicated.

> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - mboxes
> +
> +examples:
> +  - |
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      smc@23e400000 {

Usually these are called system-controller, to have a generic name (as
asked by DT spec).

> +        compatible = "apple,t8103-smc", "apple,smc";
> +        reg = <0x2 0x3e400000 0x0 0x4000>,
> +               <0x2 0x3fe00000 0x0 0x100000>;

Align the items (opening <).

With three above:
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 08/11/2022 17:33, Russell King (Oracle) wrote:
> Add the DT binding for the Apple Mac System Management Controller GPIOs.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> new file mode 100644
> index 000000000000..1a415b78760b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple Mac System Management Controller GPIO
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description:
> +  Apple Mac System Management Controller GPIO block.
> +

Best regards,
Krzysztof

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
> On 08/11/2022 17:33, Russell King (Oracle) wrote:
> > Add the DT binding for the Apple Mac System Management Controller GPIOs.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> > new file mode 100644
> > index 000000000000..1a415b78760b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> > @@ -0,0 +1,37 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).

Oh ffs. DT bindings are utterly impossible to get correct.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> On 08/11/2022 17:33, Russell King (Oracle) wrote:
> > Add a DT binding for the Apple Mac System Management Controller.
> 
> Drop the second, redundant "binding" from subject. It's already in prefix.

Yet another thing that's been there from the start... how many more
things are you going to pick up in subsequent versions of the patch?
When does this stop?

In any case, taking your comment literally,

"dt-bindings: mfd: add for Apple Mac System Management Controller"

makes no sense, so presumably you want something more than that.

In any case, I see several recent cases already merged which follow
the pattern that I've used and that you've reviewed.

> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > new file mode 100644
> > index 000000000000..014eba5a1bbc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Apple Mac System Management Controller
> > +
> > +maintainers:
> > +  - Hector Martin <marcan@marcan.st>
> > +
> > +description:
> > +  Apple Mac System Management Controller implements various functions
> > +  such as GPIO, RTC, power, reboot.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - apple,t6000-smc
> > +          - apple,t8103-smc
> > +          - apple,t8112-smc
> > +      - const: apple,smc
> > +
> > +  reg:
> > +    items:
> > +      - description: SMC area
> > +      - description: SRAM area
> > +
> > +  reg-names:
> > +    items:
> > +      - const: smc
> > +      - const: sram
> > +
> > +  mboxes:
> > +    maxItems: 1
> > +
> > +  gpio:
> > +    $ref: /schemas/gpio/gpio-macsmc.yaml
> 
> So this depends on other patch, so:
> 1. You need mention the dependency in cover letter (nothing there),
> 2. Re-order patches.
> 
> The GPIO cannot go separate tree and this must be explicitly communicated.

Sigh, getting an order that is sensible is really bloody difficult.
I'm quite sure Lee is only going to want to apply the mfd bits. Then
what do we do with the other bits? GPIO stuff via the GPIO tree, then
wait a cycle before the rest can be merged. Or what?

> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - mboxes
> > +
> > +examples:
> > +  - |
> > +    soc {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      smc@23e400000 {
> 
> Usually these are called system-controller, to have a generic name (as
> asked by DT spec).

I'll defer to Hector for his response on this one, but you've had
had plenty of opportunities to bring this up in the past - it's been
there since the first posting.

Frustrating is definitely the word for this drip-drip-drip review.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Tue, 08 Nov 2022 16:33:27 +0000, Russell King (Oracle) wrote:
> Add a DT binding for the Apple Mac System Management Controller.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/mfd/apple,smc.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: gpio: False schema does not allow {'gpio-controller': True, '#gpio-cells': [[2]]}
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/apple,smc.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Rob Herring]

On Tue, 08 Nov 2022 16:33:42 +0000, Russell King (Oracle) wrote:
> Add the DT binding for the Apple Mac System Management Controller GPIOs.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/gpio/apple,smc-gpio.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Hector Martin]

On 09/11/2022 07.09, Russell King (Oracle) wrote:
> On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>> Add the DT binding for the Apple Mac System Management Controller GPIOs.
>>>
>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> ---
>>>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>>>  1 file changed, 37 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>> new file mode 100644
>>> index 000000000000..1a415b78760b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>> @@ -0,0 +1,37 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
>>
>> Does not look like you tested the bindings. Please run `make
>> dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> 
> Oh ffs. DT bindings are utterly impossible to get correct.

I'd be happy to wrap the bindings up in another cycle & take them via
asahi-soc, if you want. That will also allow us to add the nodes to the
t6000 DTs which are on that tree for this cycle, and unblock merging the
driver bits of this series. Bindings updates are not a hard dependency
for drivers, only for the DTs themselves.

That is:

1-2,4 via mfd tree (if Lee agrees to merging the RTKit driver platform
bits that way)
3,5-6 via asahi-soc
7 via GPIO (I think)

- Hector

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 08/11/2022 23:22, Russell King (Oracle) wrote:
> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>> Add a DT binding for the Apple Mac System Management Controller.
>>
>> Drop the second, redundant "binding" from subject. It's already in prefix.
> 
> Yet another thing that's been there from the start... how many more
> things are you going to pick up in subsequent versions of the patch?
> When does this stop?
> 
> In any case, taking your comment literally,
> 
> "dt-bindings: mfd: add for Apple Mac System Management Controller"
> 
> makes no sense, so presumably you want something more than that.

dt-bindings: mfd: add Apple Mac System Management Controller

> 
> In any case, I see several recent cases already merged which follow
> the pattern that I've used and that you've reviewed.

Any many received comments to fix. I wouldn't mention it if there was no
second issue - your patchset is non-bisectable. Since you must resend,
please adjust the subject.

> 
>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> ---
>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>>>  1 file changed, 67 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>> new file mode 100644
>>> index 000000000000..014eba5a1bbc
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>> @@ -0,0 +1,67 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Apple Mac System Management Controller
>>> +
>>> +maintainers:
>>> +  - Hector Martin <marcan@marcan.st>
>>> +
>>> +description:
>>> +  Apple Mac System Management Controller implements various functions
>>> +  such as GPIO, RTC, power, reboot.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - apple,t6000-smc
>>> +          - apple,t8103-smc
>>> +          - apple,t8112-smc
>>> +      - const: apple,smc
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: SMC area
>>> +      - description: SRAM area
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: smc
>>> +      - const: sram
>>> +
>>> +  mboxes:
>>> +    maxItems: 1
>>> +
>>> +  gpio:
>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
>>
>> So this depends on other patch, so:
>> 1. You need mention the dependency in cover letter (nothing there),
>> 2. Re-order patches.
>>
>> The GPIO cannot go separate tree and this must be explicitly communicated.
> 
> Sigh, getting an order that is sensible is really bloody difficult.
> I'm quite sure Lee is only going to want to apply the mfd bits. Then
> what do we do with the other bits? GPIO stuff via the GPIO tree, then
> wait a cycle before the rest can be merged. Or what?

It's nothing new... bindings headers, drivers and DTS all have it since
years. Your case is actually easy to solve:
1. Re-order patches, describe dependency, get ack from one maintainer so
it can go via other.
2. Squash patches and describe dependency. Only one maintainer will need
to pick it up (with second's ack).

I don't understand why we talk about such basics, you are far way more
experienced in kernel development than many of us...

Best regards,
Krzysztof

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 08/11/2022 23:09, Russell King (Oracle) wrote:
> On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>> Add the DT binding for the Apple Mac System Management Controller GPIOs.
>>>
>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> ---
>>>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>>>  1 file changed, 37 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>> new file mode 100644
>>> index 000000000000..1a415b78760b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>> @@ -0,0 +1,37 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
>>
>> Does not look like you tested the bindings. Please run `make
>> dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> 
> Oh ffs. DT bindings are utterly impossible to get correct.

Why? You write, you test and then you see the error. Srsly...

Best regards,
Krzysztof

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

On Wed, Nov 09, 2022 at 09:36:12AM +0100, Krzysztof Kozlowski wrote:
> On 08/11/2022 23:09, Russell King (Oracle) wrote:
> > On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
> >> On 08/11/2022 17:33, Russell King (Oracle) wrote:
> >>> Add the DT binding for the Apple Mac System Management Controller GPIOs.
> >>>
> >>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> >>> ---
> >>>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
> >>>  1 file changed, 37 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> >>> new file mode 100644
> >>> index 000000000000..1a415b78760b
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> >>> @@ -0,0 +1,37 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
> >>
> >> Does not look like you tested the bindings. Please run `make
> >> dt_binding_check` (see
> >> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> > 
> > Oh ffs. DT bindings are utterly impossible to get correct.
> 
> Why? You write, you test and then you see the error. Srsly...

It's utterly impossible for me to remember the dependencies. It's
utterly impossible for me to remember the make incantation to test
this bloody things - especially as I need to limit it otherwise it
takes a very long time to test.

The whole bloody thing is just too sodding difficult.

I'm not going to bother with this patch set any more. Too frustrated
with this piecemeal approach and the crappy slow tooling and the
idiotic incomprehensible error messages it spits out.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 09/11/2022 10:12, Russell King (Oracle) wrote:
> On Wed, Nov 09, 2022 at 09:36:12AM +0100, Krzysztof Kozlowski wrote:
>> On 08/11/2022 23:09, Russell King (Oracle) wrote:
>>> On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
>>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>>>> Add the DT binding for the Apple Mac System Management Controller GPIOs.
>>>>>
>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>>>> ---
>>>>>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>>>>>  1 file changed, 37 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..1a415b78760b
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>>> @@ -0,0 +1,37 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
>>>>
>>>> Does not look like you tested the bindings. Please run `make
>>>> dt_binding_check` (see
>>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>>
>>> Oh ffs. DT bindings are utterly impossible to get correct.
>>
>> Why? You write, you test and then you see the error. Srsly...
> 
> It's utterly impossible for me to remember the dependencies. It's
> utterly impossible for me to remember the make incantation to test
> this bloody things - especially as I need to limit it otherwise it
> takes a very long time to test.

Then this is where I can help - you can test just one, specific binding
or few of them:

make dt_binding_check DT_SCHEMA_FILES=apple,smc-gpio.yaml

make dt_binding_check DT_SCHEMA_FILES=apple,smc

So before you format patches, just do `git rebase -i HEAD~7`, change to
re-word or edit and  go one-by-one testing your bisectability. The same
we are always doing for regular kernel builds when having a big
patchset, right? Each commit must compile, not just the last one in
patchset.

Or push each of the commit to a service which can do it for you. For
example, for kernel building (not yet ready for DT binding check) I am
using something like:
https://github.com/krzk/tools/tree/master/git-build-kernel

> 
> The whole bloody thing is just too sodding difficult.
> 
> I'm not going to bother with this patch set any more. Too frustrated
> with this piecemeal approach and the crappy slow tooling and the
> idiotic incomprehensible error messages it spits out.

Best regards,
Krzysztof

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> > On 08/11/2022 17:33, Russell King (Oracle) wrote:
> > > Add a DT binding for the Apple Mac System Management Controller.
> > 
> > Drop the second, redundant "binding" from subject. It's already in prefix.
> 
> Yet another thing that's been there from the start... how many more
> things are you going to pick up in subsequent versions of the patch?
> When does this stop?
> 
> In any case, taking your comment literally,
> 
> "dt-bindings: mfd: add for Apple Mac System Management Controller"
> 
> makes no sense, so presumably you want something more than that.
> 
> In any case, I see several recent cases already merged which follow
> the pattern that I've used and that you've reviewed.
> 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> > >  1 file changed, 67 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > new file mode 100644
> > > index 000000000000..014eba5a1bbc
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > @@ -0,0 +1,67 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Apple Mac System Management Controller
> > > +
> > > +maintainers:
> > > +  - Hector Martin <marcan@marcan.st>
> > > +
> > > +description:
> > > +  Apple Mac System Management Controller implements various functions
> > > +  such as GPIO, RTC, power, reboot.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - apple,t6000-smc
> > > +          - apple,t8103-smc
> > > +          - apple,t8112-smc
> > > +      - const: apple,smc
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: SMC area
> > > +      - description: SRAM area
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: smc
> > > +      - const: sram
> > > +
> > > +  mboxes:
> > > +    maxItems: 1
> > > +
> > > +  gpio:
> > > +    $ref: /schemas/gpio/gpio-macsmc.yaml
> > 
> > So this depends on other patch, so:
> > 1. You need mention the dependency in cover letter (nothing there),
> > 2. Re-order patches.
> > 
> > The GPIO cannot go separate tree and this must be explicitly communicated.
> 
> Sigh, getting an order that is sensible is really bloody difficult.

It's not. Sub-devices before the MFD. The only time that doesn't work is 
when the sub-devices put the parent MFD in their example. The solution 
there is don't do that. Just 1 complete example in the MFD schema and no 
examples in the sub-devices.

> I'm quite sure Lee is only going to want to apply the mfd bits. 

Indeed. I can't seem to make Lee care... All the schemas should really 
be applied together.

> Then
> what do we do with the other bits? GPIO stuff via the GPIO tree, then
> wait a cycle before the rest can be merged. Or what?

The schemas must be picked up in the same cycle. I don't care so much 
if subsystem maintainers' trees have warnings if they don't care, but I 
do care for linux-next. If the subsystem bits aren't picked up, then 
I'll pick them up if it comes to that.

Rob

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Hector Martin]

On 10/11/2022 07.17, Rob Herring wrote:
> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>>> Add a DT binding for the Apple Mac System Management Controller.
>>>
>>> Drop the second, redundant "binding" from subject. It's already in prefix.
>>
>> Yet another thing that's been there from the start... how many more
>> things are you going to pick up in subsequent versions of the patch?
>> When does this stop?
>>
>> In any case, taking your comment literally,
>>
>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
>>
>> makes no sense, so presumably you want something more than that.
>>
>> In any case, I see several recent cases already merged which follow
>> the pattern that I've used and that you've reviewed.
>>
>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>>> ---
>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>>>>  1 file changed, 67 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>> new file mode 100644
>>>> index 000000000000..014eba5a1bbc
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>> @@ -0,0 +1,67 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Apple Mac System Management Controller
>>>> +
>>>> +maintainers:
>>>> +  - Hector Martin <marcan@marcan.st>
>>>> +
>>>> +description:
>>>> +  Apple Mac System Management Controller implements various functions
>>>> +  such as GPIO, RTC, power, reboot.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>>> +      - enum:
>>>> +          - apple,t6000-smc
>>>> +          - apple,t8103-smc
>>>> +          - apple,t8112-smc
>>>> +      - const: apple,smc
>>>> +
>>>> +  reg:
>>>> +    items:
>>>> +      - description: SMC area
>>>> +      - description: SRAM area
>>>> +
>>>> +  reg-names:
>>>> +    items:
>>>> +      - const: smc
>>>> +      - const: sram
>>>> +
>>>> +  mboxes:
>>>> +    maxItems: 1
>>>> +
>>>> +  gpio:
>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
>>>
>>> So this depends on other patch, so:
>>> 1. You need mention the dependency in cover letter (nothing there),
>>> 2. Re-order patches.
>>>
>>> The GPIO cannot go separate tree and this must be explicitly communicated.
>>
>> Sigh, getting an order that is sensible is really bloody difficult.
> 
> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> when the sub-devices put the parent MFD in their example. The solution 
> there is don't do that. Just 1 complete example in the MFD schema and no 
> examples in the sub-devices.
> 
>> I'm quite sure Lee is only going to want to apply the mfd bits. 
> 
> Indeed. I can't seem to make Lee care... All the schemas should really 
> be applied together.
> 
>> Then
>> what do we do with the other bits? GPIO stuff via the GPIO tree, then
>> wait a cycle before the rest can be merged. Or what?
> 
> The schemas must be picked up in the same cycle. I don't care so much 
> if subsystem maintainers' trees have warnings if they don't care, but I 
> do care for linux-next. If the subsystem bits aren't picked up, then 
> I'll pick them up if it comes to that.
> 

We can take all the schemas and DT changes via asahi-soc if that works
for you. This also lets us move forward with more related DT changes
that would apply on top of things already in that branch. Then Lee only
has to take the mfd core bits (and possibly the RTKit platform part, or
we can figure something else out for that).


- Hector

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> > On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> > > On 08/11/2022 17:33, Russell King (Oracle) wrote:
> > > > Add a DT binding for the Apple Mac System Management Controller.
> > > 
> > > Drop the second, redundant "binding" from subject. It's already in prefix.
> > 
> > Yet another thing that's been there from the start... how many more
> > things are you going to pick up in subsequent versions of the patch?
> > When does this stop?
> > 
> > In any case, taking your comment literally,
> > 
> > "dt-bindings: mfd: add for Apple Mac System Management Controller"
> > 
> > makes no sense, so presumably you want something more than that.
> > 
> > In any case, I see several recent cases already merged which follow
> > the pattern that I've used and that you've reviewed.
> > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > new file mode 100644
> > > > index 000000000000..014eba5a1bbc
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > @@ -0,0 +1,67 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Apple Mac System Management Controller
> > > > +
> > > > +maintainers:
> > > > +  - Hector Martin <marcan@marcan.st>
> > > > +
> > > > +description:
> > > > +  Apple Mac System Management Controller implements various functions
> > > > +  such as GPIO, RTC, power, reboot.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - enum:
> > > > +          - apple,t6000-smc
> > > > +          - apple,t8103-smc
> > > > +          - apple,t8112-smc
> > > > +      - const: apple,smc
> > > > +
> > > > +  reg:
> > > > +    items:
> > > > +      - description: SMC area
> > > > +      - description: SRAM area
> > > > +
> > > > +  reg-names:
> > > > +    items:
> > > > +      - const: smc
> > > > +      - const: sram
> > > > +
> > > > +  mboxes:
> > > > +    maxItems: 1
> > > > +
> > > > +  gpio:
> > > > +    $ref: /schemas/gpio/gpio-macsmc.yaml
> > > 
> > > So this depends on other patch, so:
> > > 1. You need mention the dependency in cover letter (nothing there),
> > > 2. Re-order patches.
> > > 
> > > The GPIO cannot go separate tree and this must be explicitly communicated.
> > 
> > Sigh, getting an order that is sensible is really bloody difficult.
> 
> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> when the sub-devices put the parent MFD in their example. The solution 
> there is don't do that. Just 1 complete example in the MFD schema and no 
> examples in the sub-devices.

Meanwhile, I was told by Krzysztof that DT schemas must always have an
example. So, different person, different story.

Anyway, I've washed my hands of this farce with this series. I'm not
planning to post another version of it. I've had enough of this crap.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 10/11/2022 12:48, Russell King (Oracle) wrote:
> On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
>> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
>>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
>>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>>>> Add a DT binding for the Apple Mac System Management Controller.
>>>>
>>>> Drop the second, redundant "binding" from subject. It's already in prefix.
>>>
>>> Yet another thing that's been there from the start... how many more
>>> things are you going to pick up in subsequent versions of the patch?
>>> When does this stop?
>>>
>>> In any case, taking your comment literally,
>>>
>>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
>>>
>>> makes no sense, so presumably you want something more than that.
>>>
>>> In any case, I see several recent cases already merged which follow
>>> the pattern that I've used and that you've reviewed.
>>>
>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>>>> ---
>>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>>>>>  1 file changed, 67 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..014eba5a1bbc
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>> @@ -0,0 +1,67 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Apple Mac System Management Controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Hector Martin <marcan@marcan.st>
>>>>> +
>>>>> +description:
>>>>> +  Apple Mac System Management Controller implements various functions
>>>>> +  such as GPIO, RTC, power, reboot.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - enum:
>>>>> +          - apple,t6000-smc
>>>>> +          - apple,t8103-smc
>>>>> +          - apple,t8112-smc
>>>>> +      - const: apple,smc
>>>>> +
>>>>> +  reg:
>>>>> +    items:
>>>>> +      - description: SMC area
>>>>> +      - description: SRAM area
>>>>> +
>>>>> +  reg-names:
>>>>> +    items:
>>>>> +      - const: smc
>>>>> +      - const: sram
>>>>> +
>>>>> +  mboxes:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  gpio:
>>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
>>>>
>>>> So this depends on other patch, so:
>>>> 1. You need mention the dependency in cover letter (nothing there),
>>>> 2. Re-order patches.
>>>>
>>>> The GPIO cannot go separate tree and this must be explicitly communicated.
>>>
>>> Sigh, getting an order that is sensible is really bloody difficult.
>>
>> It's not. Sub-devices before the MFD. The only time that doesn't work is 
>> when the sub-devices put the parent MFD in their example. The solution 
>> there is don't do that. Just 1 complete example in the MFD schema and no 
>> examples in the sub-devices.
> 
> Meanwhile, I was told by Krzysztof that DT schemas must always have an
> example. So, different person, different story.

Hm, where do you see a message I told you to always have examples? Maybe
in some discussion I mentioned that examples are desired, but not
always. There is no point in having example in MFD child device schema
if it is already part of the parent MFD binding, where it is actually
required for complete picture.

Best regards,
Krzysztof

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Nov 10, 2022 at 03:00:16PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2022 12:48, Russell King (Oracle) wrote:
> > On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
> >> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> >>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
> >>>>> Add a DT binding for the Apple Mac System Management Controller.
> >>>>
> >>>> Drop the second, redundant "binding" from subject. It's already in prefix.
> >>>
> >>> Yet another thing that's been there from the start... how many more
> >>> things are you going to pick up in subsequent versions of the patch?
> >>> When does this stop?
> >>>
> >>> In any case, taking your comment literally,
> >>>
> >>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
> >>>
> >>> makes no sense, so presumably you want something more than that.
> >>>
> >>> In any case, I see several recent cases already merged which follow
> >>> the pattern that I've used and that you've reviewed.
> >>>
> >>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> >>>>> ---
> >>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> >>>>>  1 file changed, 67 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..014eba5a1bbc
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>> @@ -0,0 +1,67 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Apple Mac System Management Controller
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Hector Martin <marcan@marcan.st>
> >>>>> +
> >>>>> +description:
> >>>>> +  Apple Mac System Management Controller implements various functions
> >>>>> +  such as GPIO, RTC, power, reboot.
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    items:
> >>>>> +      - enum:
> >>>>> +          - apple,t6000-smc
> >>>>> +          - apple,t8103-smc
> >>>>> +          - apple,t8112-smc
> >>>>> +      - const: apple,smc
> >>>>> +
> >>>>> +  reg:
> >>>>> +    items:
> >>>>> +      - description: SMC area
> >>>>> +      - description: SRAM area
> >>>>> +
> >>>>> +  reg-names:
> >>>>> +    items:
> >>>>> +      - const: smc
> >>>>> +      - const: sram
> >>>>> +
> >>>>> +  mboxes:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  gpio:
> >>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
> >>>>
> >>>> So this depends on other patch, so:
> >>>> 1. You need mention the dependency in cover letter (nothing there),
> >>>> 2. Re-order patches.
> >>>>
> >>>> The GPIO cannot go separate tree and this must be explicitly communicated.
> >>>
> >>> Sigh, getting an order that is sensible is really bloody difficult.
> >>
> >> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> >> when the sub-devices put the parent MFD in their example. The solution 
> >> there is don't do that. Just 1 complete example in the MFD schema and no 
> >> examples in the sub-devices.
> > 
> > Meanwhile, I was told by Krzysztof that DT schemas must always have an
> > example. So, different person, different story.
> 
> Hm, where do you see a message I told you to always have examples? Maybe
> in some discussion I mentioned that examples are desired, but not
> always. There is no point in having example in MFD child device schema
> if it is already part of the parent MFD binding, where it is actually
> required for complete picture.

35ed6e48-40e6-eb14-72de-9a0a4f5b38f8@linaro.org

and

2e2356f2-ded1-3cbf-4456-20054a8defda@linaro.org

For the GPIO macsec binding. So I'm getting contradictory information.
First you say that I need an example in the gpio macsec DT binding
yaml document.

Now I'm told it should go in the parent.

Make up your bloody minds and stop pissing me about. This is why I've
given up trying to get this in.

Getting a consistent message would be nice, but it seems impossible.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 10/11/2022 15:14, Russell King (Oracle) wrote:
> On Thu, Nov 10, 2022 at 03:00:16PM +0100, Krzysztof Kozlowski wrote:
>> On 10/11/2022 12:48, Russell King (Oracle) wrote:
>>> On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
>>>> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
>>>>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>>>>>> Add a DT binding for the Apple Mac System Management Controller.
>>>>>>
>>>>>> Drop the second, redundant "binding" from subject. It's already in prefix.
>>>>>
>>>>> Yet another thing that's been there from the start... how many more
>>>>> things are you going to pick up in subsequent versions of the patch?
>>>>> When does this stop?
>>>>>
>>>>> In any case, taking your comment literally,
>>>>>
>>>>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
>>>>>
>>>>> makes no sense, so presumably you want something more than that.
>>>>>
>>>>> In any case, I see several recent cases already merged which follow
>>>>> the pattern that I've used and that you've reviewed.
>>>>>
>>>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>>>>>>>  1 file changed, 67 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..014eba5a1bbc
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>>>> @@ -0,0 +1,67 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Apple Mac System Management Controller
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Hector Martin <marcan@marcan.st>
>>>>>>> +
>>>>>>> +description:
>>>>>>> +  Apple Mac System Management Controller implements various functions
>>>>>>> +  such as GPIO, RTC, power, reboot.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    items:
>>>>>>> +      - enum:
>>>>>>> +          - apple,t6000-smc
>>>>>>> +          - apple,t8103-smc
>>>>>>> +          - apple,t8112-smc
>>>>>>> +      - const: apple,smc
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    items:
>>>>>>> +      - description: SMC area
>>>>>>> +      - description: SRAM area
>>>>>>> +
>>>>>>> +  reg-names:
>>>>>>> +    items:
>>>>>>> +      - const: smc
>>>>>>> +      - const: sram
>>>>>>> +
>>>>>>> +  mboxes:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  gpio:
>>>>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
>>>>>>
>>>>>> So this depends on other patch, so:
>>>>>> 1. You need mention the dependency in cover letter (nothing there),
>>>>>> 2. Re-order patches.
>>>>>>
>>>>>> The GPIO cannot go separate tree and this must be explicitly communicated.
>>>>>
>>>>> Sigh, getting an order that is sensible is really bloody difficult.
>>>>
>>>> It's not. Sub-devices before the MFD. The only time that doesn't work is 
>>>> when the sub-devices put the parent MFD in their example. The solution 
>>>> there is don't do that. Just 1 complete example in the MFD schema and no 
>>>> examples in the sub-devices.
>>>
>>> Meanwhile, I was told by Krzysztof that DT schemas must always have an
>>> example. So, different person, different story.
>>
>> Hm, where do you see a message I told you to always have examples? Maybe
>> in some discussion I mentioned that examples are desired, but not
>> always. There is no point in having example in MFD child device schema
>> if it is already part of the parent MFD binding, where it is actually
>> required for complete picture.
> 
> 35ed6e48-40e6-eb14-72de-9a0a4f5b38f8@linaro.org
> 
> and

That was independent schema, no references to MFD, thus my comment. If
you post such stuff alone without indication it is part of MFD, what do
you expect from reviewers?

> 
> 2e2356f2-ded1-3cbf-4456-20054a8defda@linaro.org

Which was comment about MFD, right? It is expected to have example for
MFD. I never said it is mandatory for every schema, which you implied in
previous mailing.


> 
> For the GPIO macsec binding. So I'm getting contradictory information.
> First you say that I need an example in the gpio macsec DT binding
> yaml document.

First you split patches making reviewers life difficult. Then reviewers
don't get entire concept and they answer based what they got.

> 
> Now I'm told it should go in the parent.

After posting entire patchset with context you can get better review,
yes, that's right.

> 
> Make up your bloody minds and stop pissing me about. This is why I've
> given up trying to get this in.

I don't think it is constructive to discuss this with you anymore.

> 
> Getting a consistent message would be nice, but it seems impossible.
> 

Best regards,
Krzysztof

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Nov 10, 2022 at 03:21:35PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2022 15:14, Russell King (Oracle) wrote:
> > On Thu, Nov 10, 2022 at 03:00:16PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/11/2022 12:48, Russell King (Oracle) wrote:
> >>> On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
> >>>> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> >>>>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> >>>>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
> >>>>>>> Add a DT binding for the Apple Mac System Management Controller.
> >>>>>>
> >>>>>> Drop the second, redundant "binding" from subject. It's already in prefix.
> >>>>>
> >>>>> Yet another thing that's been there from the start... how many more
> >>>>> things are you going to pick up in subsequent versions of the patch?
> >>>>> When does this stop?
> >>>>>
> >>>>> In any case, taking your comment literally,
> >>>>>
> >>>>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
> >>>>>
> >>>>> makes no sense, so presumably you want something more than that.
> >>>>>
> >>>>> In any case, I see several recent cases already merged which follow
> >>>>> the pattern that I've used and that you've reviewed.
> >>>>>
> >>>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> >>>>>>> ---
> >>>>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> >>>>>>>  1 file changed, 67 insertions(+)
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..014eba5a1bbc
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>>>> @@ -0,0 +1,67 @@
> >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: Apple Mac System Management Controller
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> +  - Hector Martin <marcan@marcan.st>
> >>>>>>> +
> >>>>>>> +description:
> >>>>>>> +  Apple Mac System Management Controller implements various functions
> >>>>>>> +  such as GPIO, RTC, power, reboot.
> >>>>>>> +
> >>>>>>> +properties:
> >>>>>>> +  compatible:
> >>>>>>> +    items:
> >>>>>>> +      - enum:
> >>>>>>> +          - apple,t6000-smc
> >>>>>>> +          - apple,t8103-smc
> >>>>>>> +          - apple,t8112-smc
> >>>>>>> +      - const: apple,smc
> >>>>>>> +
> >>>>>>> +  reg:
> >>>>>>> +    items:
> >>>>>>> +      - description: SMC area
> >>>>>>> +      - description: SRAM area
> >>>>>>> +
> >>>>>>> +  reg-names:
> >>>>>>> +    items:
> >>>>>>> +      - const: smc
> >>>>>>> +      - const: sram
> >>>>>>> +
> >>>>>>> +  mboxes:
> >>>>>>> +    maxItems: 1
> >>>>>>> +
> >>>>>>> +  gpio:
> >>>>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
> >>>>>>
> >>>>>> So this depends on other patch, so:
> >>>>>> 1. You need mention the dependency in cover letter (nothing there),
> >>>>>> 2. Re-order patches.
> >>>>>>
> >>>>>> The GPIO cannot go separate tree and this must be explicitly communicated.
> >>>>>
> >>>>> Sigh, getting an order that is sensible is really bloody difficult.
> >>>>
> >>>> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> >>>> when the sub-devices put the parent MFD in their example. The solution 
> >>>> there is don't do that. Just 1 complete example in the MFD schema and no 
> >>>> examples in the sub-devices.
> >>>
> >>> Meanwhile, I was told by Krzysztof that DT schemas must always have an
> >>> example. So, different person, different story.
> >>
> >> Hm, where do you see a message I told you to always have examples? Maybe
> >> in some discussion I mentioned that examples are desired, but not
> >> always. There is no point in having example in MFD child device schema
> >> if it is already part of the parent MFD binding, where it is actually
> >> required for complete picture.
> > 
> > 35ed6e48-40e6-eb14-72de-9a0a4f5b38f8@linaro.org
> > 
> > and
> 
> That was independent schema, no references to MFD, thus my comment. If
> you post such stuff alone without indication it is part of MFD, what do
> you expect from reviewers?
> 
> > 
> > 2e2356f2-ded1-3cbf-4456-20054a8defda@linaro.org
> 
> Which was comment about MFD, right? It is expected to have example for
> MFD. I never said it is mandatory for every schema, which you implied in
> previous mailing.
> 
> 
> > 
> > For the GPIO macsec binding. So I'm getting contradictory information.
> > First you say that I need an example in the gpio macsec DT binding
> > yaml document.
> 
> First you split patches making reviewers life difficult. Then reviewers
> don't get entire concept and they answer based what they got.
> 
> > 
> > Now I'm told it should go in the parent.
> 
> After posting entire patchset with context you can get better review,
> yes, that's right.
> 
> > 
> > Make up your bloody minds and stop pissing me about. This is why I've
> > given up trying to get this in.
> 
> I don't think it is constructive to discuss this with you anymore.
> 
> > 
> > Getting a consistent message would be nice, but it seems impossible.
> > 
> 

In which case you CLEARLY didn't read the cover message to that two
patch series.

Again, YOU are giving contradictory information.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 10/11/2022 15:23, Russell King (Oracle) wrote:
>>>
>>> Make up your bloody minds and stop pissing me about. This is why I've
>>> given up trying to get this in.
>>
>> I don't think it is constructive to discuss this with you anymore.
>>
>>>
>>> Getting a consistent message would be nice, but it seems impossible.
>>>
>>
> 
> In which case you CLEARLY didn't read the cover message to that two
> patch series.

I have every day 100-200 new patches, so not every cover letter gets
enough attention. Your did not say this is part of MFD, just mentioned
some build dependency.

Anyway, job of submitter is to make the patch and its context readable
to the reviewers.

There is literally nothing about including in parent MFD node in DT.
Just take a look:

https://lore.kernel.org/all/Y1q5jW8ff0aUdjPd@shell.armlinux.org.uk/

Or maybe you refer to this:
"previously posted before the last merge window."
Yes, so I will go through hundreds of emails from a week or months to
satisfy you?

"this driver is dependent on the
Apple SMC driver in order to be buildable and usable."

The only hint... it's about driver, not MFD. Make it obvious for
readers, not obvious for you.

"It is expected
that this Apple SMC driver will be merged via Lee's MFD tree."

Maybe there is dependency, maybe not, who the heck knows.

> Again, YOU are giving contradictory information.

Best regards,
Krzysztof

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Nov 10, 2022 at 03:36:17PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2022 15:23, Russell King (Oracle) wrote:
> >>>
> >>> Make up your bloody minds and stop pissing me about. This is why I've
> >>> given up trying to get this in.
> >>
> >> I don't think it is constructive to discuss this with you anymore.
> >>
> >>>
> >>> Getting a consistent message would be nice, but it seems impossible.
> >>>
> >>
> > 
> > In which case you CLEARLY didn't read the cover message to that two
> > patch series.
> 
> I have every day 100-200 new patches, so not every cover letter gets
> enough attention. Your did not say this is part of MFD, just mentioned
> some build dependency.
> 
> Anyway, job of submitter is to make the patch and its context readable
> to the reviewers.
> 
> There is literally nothing about including in parent MFD node in DT.
> Just take a look:
> 
> https://lore.kernel.org/all/Y1q5jW8ff0aUdjPd@shell.armlinux.org.uk/
> 
> Or maybe you refer to this:
> "previously posted before the last merge window."
> Yes, so I will go through hundreds of emails from a week or months to
> satisfy you?
> 
> "this driver is dependent on the
> Apple SMC driver in order to be buildable and usable."
> 
> The only hint... it's about driver, not MFD. Make it obvious for
> readers, not obvious for you.
> 
> "It is expected
> that this Apple SMC driver will be merged via Lee's MFD tree."
> 
> Maybe there is dependency, maybe not, who the heck knows.

Everything that needed to be said was said in the cover message.

I'm not going to bother continuing with this thread because there is
nothing more to be said. This is getting utterly idiotic.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 1/7] mfd: Add core Apple Mac SMC driver

[Lee Jones]

On Tue, 08 Nov 2022, Russell King wrote:

> From: Hector Martin <marcan@marcan.st>
> 
> This driver implements support for the SMC (System Management
> Controller) in Apple Macs. In contrast to the existing applesmc driver,
> it uses pluggable backends that allow it to support different SMC
> implementations, and uses the MFD subsystem to expose the core SMC
> functionality so that specific features (gpio, hwmon, battery, etc.) can
> be implemented by separate drivers in their respective downstream
> subsystems.

Could we have Russell's ASCII simplified architecture model here please?

> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/mfd/Kconfig        |   4 +
>  drivers/mfd/Makefile       |   1 +
>  drivers/mfd/macsmc.c       | 239 +++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/macsmc.h | 104 ++++++++++++++++
>  4 files changed, 348 insertions(+)
>  create mode 100644 drivers/mfd/macsmc.c
>  create mode 100644 include/linux/mfd/macsmc.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b93856de432..f73e098b7228 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -224,6 +224,10 @@ config MFD_CROS_EC_DEV
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called cros-ec-dev.
>  
> +config MFD_MACSMC
> +	tristate

Is this selectable?

Worth having a description?

> +	select MFD_CORE

Help section?

Copy / paste from the commit log should be enough.

>  config MFD_MADERA
>  	tristate "Cirrus Logic Madera codecs"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..a5271b578d31 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
>  obj-$(CONFIG_MFD_ENE_KB3930)	+= ene-kb3930.o
>  obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
>  obj-$(CONFIG_MFD_GATEWORKS_GSC)	+= gateworks-gsc.o
> +obj-$(CONFIG_MFD_MACSMC)	+= macsmc.o
>  
>  obj-$(CONFIG_HTC_PASIC3)	+= htc-pasic3.o
>  obj-$(CONFIG_HTC_I2CPLD)	+= htc-i2cpld.o
> diff --git a/drivers/mfd/macsmc.c b/drivers/mfd/macsmc.c
> new file mode 100644
> index 000000000000..e5c3957efea4
> --- /dev/null
> +++ b/drivers/mfd/macsmc.c
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SMC core framework

"SMC (System Management Controller)"

Tiny nit: '\n'

> + * Copyright The Asahi Linux Contributors

Missing (C)

Would you like an Author(s) line here?

> + */
> +
> +#include <linux/device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/macsmc.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +

Would you be kind enough to add a header here to describe the
attributes please.  Some of them are non-standard.

> +struct apple_smc {
> +	struct device *dev;
> +
> +	void *be_cookie;
> +	const struct apple_smc_backend_ops *be;
> +
> +	struct mutex mutex;
> +
> +	u32 key_count;
> +	smc_key first_key;
> +	smc_key last_key;
> +
> +	struct blocking_notifier_head event_handlers;
> +};
> +
> +static const struct mfd_cell apple_smc_devs[] = {
> +	MFD_CELL_OF("macsmc-gpio", NULL, NULL, 0, 0, "apple,smc-gpio"),
> +	MFD_CELL_NAME("macsmc-hid"),
> +	MFD_CELL_NAME("macsmc-power"),
> +	MFD_CELL_NAME("macsmc-reboot"),
> +	MFD_CELL_OF("macsmc-rtc", NULL, NULL, 0, 0, "apple,smc-rtc"),
> +};
> +
> +int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->read_key(smc->be_cookie, key, buf, size);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_read);
> +
> +int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->write_key(smc->be_cookie, key, buf, size);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_write);
> +
> +int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size)
> +{
> +	int ret;
> +
> +	/*
> +	 * Will fail if SMC is busy. This is only used by SMC reboot/poweroff
> +	 * final calls, so it doesn't really matter at that point.
> +	 */
> +	if (!mutex_trylock(&smc->mutex))
> +		return -EBUSY;
> +
> +	ret = smc->be->write_key_atomic(smc->be_cookie, key, buf, size);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_write_atomic);
> +
> +int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
> +		 void *rbuf, size_t rsize)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->rw_key(smc->be_cookie, key, wbuf, wsize, rbuf, rsize);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_rw);
> +
> +int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->get_key_by_index(smc->be_cookie, index, key);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_get_key_by_index);
> +
> +int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->get_key_info(smc->be_cookie, key, info);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_get_key_info);
> +
> +int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key)
> +{
> +	int start = 0, count = smc->key_count;
> +	int ret;
> +
> +	if (key <= smc->first_key)
> +		return 0;
> +	if (key > smc->last_key)
> +		return smc->key_count;
> +
> +	while (count > 1) {
> +		int pivot = start + ((count - 1) >> 1);
> +		smc_key pkey;
> +
> +		ret = apple_smc_get_key_by_index(smc, pivot, &pkey);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (pkey == key)
> +			return pivot;
> +
> +		pivot++;
> +
> +		if (pkey < key) {
> +			count -= pivot - start;
> +			start = pivot;
> +		} else {
> +			count = pivot - start;
> +		}
> +	}
> +
> +	return start;
> +}

Maybe a 1 or 2 line comment to provide an overview of what's happening
in here please.

> +EXPORT_SYMBOL(apple_smc_find_first_key_index);
> +
> +int apple_smc_get_key_count(struct apple_smc *smc)
> +{
> +	return smc->key_count;
> +}
> +EXPORT_SYMBOL(apple_smc_get_key_count);
> +
> +void apple_smc_event_received(struct apple_smc *smc, uint32_t event)
> +{
> +	dev_dbg(smc->dev, "Event: 0x%08x\n", event);
> +	blocking_notifier_call_chain(&smc->event_handlers, event, NULL);
> +}
> +EXPORT_SYMBOL(apple_smc_event_received);
> +
> +int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n)
> +{
> +	return blocking_notifier_chain_register(&smc->event_handlers, n);
> +}
> +EXPORT_SYMBOL(apple_smc_register_notifier);
> +
> +int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n)
> +{
> +	return blocking_notifier_chain_unregister(&smc->event_handlers, n);
> +}
> +EXPORT_SYMBOL(apple_smc_unregister_notifier);
> +
> +void *apple_smc_get_cookie(struct apple_smc *smc)
> +{
> +	return smc->be_cookie;
> +}
> +EXPORT_SYMBOL(apple_smc_get_cookie);

These parts seem like abstraction for the sake of abstraction.

Any reason why the caller can't use the blocking_notifier_* API and
look into the apple_smc for themselves.

> +struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, void *cookie)
> +{
> +	struct apple_smc *smc;
> +	u32 count;
> +	int ret;
> +
> +	smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL);
> +	if (!smc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	smc->dev = dev;
> +	smc->be_cookie = cookie;
> +	smc->be = ops;
> +	mutex_init(&smc->mutex);
> +	BLOCKING_INIT_NOTIFIER_HEAD(&smc->event_handlers);
> +
> +	ret = apple_smc_read_u32(smc, SMC_KEY(#KEY), &count);
> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get key count"));
> +	smc->key_count = be32_to_cpu(count);
> +
> +	ret = apple_smc_get_key_by_index(smc, 0, &smc->first_key);
> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get first key"));
> +
> +	ret = apple_smc_get_key_by_index(smc, smc->key_count - 1, &smc->last_key);
> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get last key"));
> +
> +	/* Enable notifications */
> +	apple_smc_write_flag(smc, SMC_KEY(NTAP), 1);
> +
> +	dev_info(dev, "Initialized (%d keys %p4ch..%p4ch)\n",
> +		 smc->key_count, &smc->first_key, &smc->last_key);
> +
> +	dev_set_drvdata(dev, smc);
> +
> +	ret = mfd_add_devices(dev, -1, apple_smc_devs, ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL);

Please replace the -1 with the defines provided.

> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Subdevice initialization failed"));

"Failed to register sub-devices"

> +	return smc;
> +}
> +EXPORT_SYMBOL(apple_smc_probe);
> +
> +int apple_smc_remove(struct apple_smc *smc)
> +{
> +	mfd_remove_devices(smc->dev);

devm_*?

> +	/* Disable notifications */
> +	apple_smc_write_flag(smc, SMC_KEY(NTAP), 1);

The same command enables and disables notifications?

> +	return 0;
> +}
> +EXPORT_SYMBOL(apple_smc_remove);
> +
> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_DESCRIPTION("Apple SMC core");

SMC (System Management Controller)

> diff --git a/include/linux/mfd/macsmc.h b/include/linux/mfd/macsmc.h
> new file mode 100644
> index 000000000000..99cfa23f27bd
> --- /dev/null
> +++ b/include/linux/mfd/macsmc.h
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SMC core definitions

SMC (System Management Controller)

> + * Copyright (C) The Asahi Linux Contributors
> + */
> +
> +#ifndef _LINUX_MFD_MACSMC_H
> +#define _LINUX_MFD_MACSMC_H
> +
> +struct apple_smc;

You can move the definition into here and omit this line.

> +typedef u32 smc_key;
> +
> +#define SMC_KEY(s) (smc_key)(_SMC_KEY(#s))
> +#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
> +
> +#define APPLE_SMC_READABLE BIT(7)
> +#define APPLE_SMC_WRITABLE BIT(6)
> +#define APPLE_SMC_FUNCTION BIT(4)
> +
> +struct apple_smc_key_info {
> +	u8 size;
> +	u32 type_code;
> +	u8 flags;
> +};
> +
> +int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size);
> +int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size);
> +int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size);
> +int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
> +		 void *rbuf, size_t rsize);
> +
> +int apple_smc_get_key_count(struct apple_smc *smc);
> +int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key);
> +int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key);
> +int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info);
> +
> +static inline bool apple_smc_key_exists(struct apple_smc *smc, smc_key key)
> +{
> +	return apple_smc_get_key_info(smc, key, NULL) >= 0;
> +}
> +
> +#define APPLE_SMC_TYPE_OPS(type) \
> +	static inline int apple_smc_read_##type(struct apple_smc *smc, smc_key key, type *p) \
> +	{ \
> +		int ret = apple_smc_read(smc, key, p, sizeof(*p)); \
> +		return (ret < 0) ? ret : ((ret != sizeof(*p)) ? -EINVAL : 0); \
> +	} \
> +	static inline int apple_smc_write_##type(struct apple_smc *smc, smc_key key, type p) \
> +	{ \
> +		return apple_smc_write(smc, key, &p, sizeof(p)); \
> +	} \
> +	static inline int apple_smc_write_##type##_atomic(struct apple_smc *smc, smc_key key, type p) \
> +	{ \
> +		return apple_smc_write_atomic(smc, key, &p, sizeof(p)); \
> +	} \
> +	static inline int apple_smc_rw_##type(struct apple_smc *smc, smc_key key, \
> +					      type w, type *r) \
> +	{ \
> +		int ret = apple_smc_rw(smc, key, &w, sizeof(w), r, sizeof(*r)); \
> +		return (ret < 0) ? ret : ((ret != sizeof(*r)) ? -EINVAL : 0); \
> +	}
> +
> +APPLE_SMC_TYPE_OPS(u64)
> +APPLE_SMC_TYPE_OPS(u32)
> +APPLE_SMC_TYPE_OPS(u16)
> +APPLE_SMC_TYPE_OPS(u8)
> +APPLE_SMC_TYPE_OPS(s64)
> +APPLE_SMC_TYPE_OPS(s32)
> +APPLE_SMC_TYPE_OPS(s16)
> +APPLE_SMC_TYPE_OPS(s8)
> +
> +static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key)
> +{
> +	u8 val;
> +	int ret = apple_smc_read_u8(smc, key, &val);

Nit: Please separate the declaration and assignment via function call
with a line break in between.

> +	if (ret < 0)
> +		return ret;
> +	return val ? 1 : 0;
> +}
> +#define apple_smc_write_flag apple_smc_write_u8
> +
> +int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n);
> +int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n);
> +
> +/* backend interface */
> +
> +struct apple_smc_backend_ops {
> +	int (*read_key)(void *cookie, smc_key key, void *buf, size_t size);
> +	int (*write_key)(void *cookie, smc_key key, void *buf, size_t size);
> +	int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size);
> +	int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize,
> +		      void *rbuf, size_t rsize);
> +	int (*get_key_by_index)(void *cookie, int index, smc_key *key);
> +	int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info);
> +};
> +
> +struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops,
> +				  void *cookie);
> +void *apple_smc_get_cookie(struct apple_smc *smc);
> +int apple_smc_remove(struct apple_smc *smc);
> +void apple_smc_event_received(struct apple_smc *smc, uint32_t event);
> +
> +#endif

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Lee Jones]

On Wed, 09 Nov 2022, Rob Herring wrote:

> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> > On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> > > On 08/11/2022 17:33, Russell King (Oracle) wrote:
> > > > Add a DT binding for the Apple Mac System Management Controller.
> > > 
> > > Drop the second, redundant "binding" from subject. It's already in prefix.
> > 
> > Yet another thing that's been there from the start... how many more
> > things are you going to pick up in subsequent versions of the patch?
> > When does this stop?
> > 
> > In any case, taking your comment literally,
> > 
> > "dt-bindings: mfd: add for Apple Mac System Management Controller"
> > 
> > makes no sense, so presumably you want something more than that.
> > 
> > In any case, I see several recent cases already merged which follow
> > the pattern that I've used and that you've reviewed.
> > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > new file mode 100644
> > > > index 000000000000..014eba5a1bbc
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > @@ -0,0 +1,67 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Apple Mac System Management Controller
> > > > +
> > > > +maintainers:
> > > > +  - Hector Martin <marcan@marcan.st>
> > > > +
> > > > +description:
> > > > +  Apple Mac System Management Controller implements various functions
> > > > +  such as GPIO, RTC, power, reboot.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - enum:
> > > > +          - apple,t6000-smc
> > > > +          - apple,t8103-smc
> > > > +          - apple,t8112-smc
> > > > +      - const: apple,smc
> > > > +
> > > > +  reg:
> > > > +    items:
> > > > +      - description: SMC area
> > > > +      - description: SRAM area
> > > > +
> > > > +  reg-names:
> > > > +    items:
> > > > +      - const: smc
> > > > +      - const: sram
> > > > +
> > > > +  mboxes:
> > > > +    maxItems: 1
> > > > +
> > > > +  gpio:
> > > > +    $ref: /schemas/gpio/gpio-macsmc.yaml
> > > 
> > > So this depends on other patch, so:
> > > 1. You need mention the dependency in cover letter (nothing there),
> > > 2. Re-order patches.
> > > 
> > > The GPIO cannot go separate tree and this must be explicitly communicated.
> > 
> > Sigh, getting an order that is sensible is really bloody difficult.
> 
> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> when the sub-devices put the parent MFD in their example. The solution 
> there is don't do that. Just 1 complete example in the MFD schema and no 
> examples in the sub-devices.
> 
> > I'm quite sure Lee is only going to want to apply the mfd bits. 
> 
> Indeed. I can't seem to make Lee care... All the schemas should really 
> be applied together.

I care about drivers.  Happy to take the set as a whole if it helps.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 1/7] mfd: Add core Apple Mac SMC driver

[Andy Shevchenko]

On Tue, Nov 08, 2022 at 04:33:17PM +0000, Russell King wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> This driver implements support for the SMC (System Management
> Controller) in Apple Macs. In contrast to the existing applesmc driver,
> it uses pluggable backends that allow it to support different SMC
> implementations, and uses the MFD subsystem to expose the core SMC
> functionality so that specific features (gpio, hwmon, battery, etc.) can
> be implemented by separate drivers in their respective downstream
> subsystems.

...

> +	ret = mfd_add_devices(dev, -1, apple_smc_devs, ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL);

PLATFORM_DEVID_NONE

> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Subdevice initialization failed"));

...

> +EXPORT_SYMBOL(apple_smc_probe);

Namespace?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Petr Mladek]

On Tue 2022-11-08 16:33:22, Russell King wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> it's useful to be able to print generic 4-character codes formatted as
> an integer. Extend it to add format specifiers for printing generic
> 32-bit FOURCCs with various endian semantics:
> 
> %p4ch   Host-endian
> %p4cl	Little-endian
> %p4cb	Big-endian
> %p4cr	Reverse-endian
> 
> The endianness determines how bytes are interpreted as a u32, and the
> FOURCC is then always printed MSByte-first (this is the opposite of
> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> allow printing LSByte-first FOURCCs stored in host endian order
> (other than the hex form being in character order, not the integer
> value).
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Petr Mladek <pmladek@suse.com>

See one nit below.

> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1762,27 +1762,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
>  	char output[sizeof("0123 little-endian (0x01234567)")];
>  	char *p = output;
>  	unsigned int i;
> +	bool pixel_fmt = false;
>  	u32 orig, val;
>  
> -	if (fmt[1] != 'c' || fmt[2] != 'c')
> +	if (fmt[1] != 'c')
>  		return error_string(buf, end, "(%p4?)", spec);
>  
>  	if (check_pointer(&buf, end, fourcc, spec))
>  		return buf;
>  
>  	orig = get_unaligned(fourcc);
> -	val = orig & ~BIT(31);
> +	switch (fmt[2]) {
> +	case 'h':
> +		val = orig;
> +		break;
> +	case 'r':
> +		val = orig = swab32(orig);

I do not like much these multi assignments. I think that the result
was not even defined in some older C standards. Though, I can't find
it now. And even make W=3 does not warn about it.

> +		break;
> +	case 'l':
> +		val = orig = le32_to_cpu(orig);
> +		break;
> +	case 'b':
> +		val = orig = be32_to_cpu(orig);
> +		break;

Best Regards,
Petr

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Andy Shevchenko]

On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> On Tue 2022-11-08 16:33:22, Russell King wrote:

...

> >  	orig = get_unaligned(fourcc);
> > -	val = orig & ~BIT(31);
> > +	switch (fmt[2]) {
> > +	case 'h':
> > +		val = orig;
> > +		break;
> > +	case 'r':
> > +		val = orig = swab32(orig);
> 
> I do not like much these multi assignments. I think that the result
> was not even defined in some older C standards. Though, I can't find
> it now. And even make W=3 does not warn about it.
> 
> > +		break;
> > +	case 'l':
> > +		val = orig = le32_to_cpu(orig);
> > +		break;
> > +	case 'b':
> > +		val = orig = be32_to_cpu(orig);
> > +		break;

Isn't easy to fix? Something like below?

	switch (fmt[2]) {
	case 'h':
		break;
	case 'r':
		orig = swab32(orig);
		break;
	case 'l':
		orig = le32_to_cpu(orig);
		break;
	case 'b':
		orig = be32_to_cpu(orig);
		break;

		...
	}
	val = orig;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Russell King (Oracle)]

On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> On Tue 2022-11-08 16:33:22, Russell King wrote:
> > From: Hector Martin <marcan@marcan.st>
> > 
> > %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> > it's useful to be able to print generic 4-character codes formatted as
> > an integer. Extend it to add format specifiers for printing generic
> > 32-bit FOURCCs with various endian semantics:
> > 
> > %p4ch   Host-endian
> > %p4cl	Little-endian
> > %p4cb	Big-endian
> > %p4cr	Reverse-endian
> > 
> > The endianness determines how bytes are interpreted as a u32, and the
> > FOURCC is then always printed MSByte-first (this is the opposite of
> > V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> > allow printing LSByte-first FOURCCs stored in host endian order
> > (other than the hex form being in character order, not the integer
> > value).
> > 
> > Signed-off-by: Hector Martin <marcan@marcan.st>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> See one nit below.
> 
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1762,27 +1762,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> >  	char output[sizeof("0123 little-endian (0x01234567)")];
> >  	char *p = output;
> >  	unsigned int i;
> > +	bool pixel_fmt = false;
> >  	u32 orig, val;
> >  
> > -	if (fmt[1] != 'c' || fmt[2] != 'c')
> > +	if (fmt[1] != 'c')
> >  		return error_string(buf, end, "(%p4?)", spec);
> >  
> >  	if (check_pointer(&buf, end, fourcc, spec))
> >  		return buf;
> >  
> >  	orig = get_unaligned(fourcc);
> > -	val = orig & ~BIT(31);
> > +	switch (fmt[2]) {
> > +	case 'h':
> > +		val = orig;
> > +		break;
> > +	case 'r':
> > +		val = orig = swab32(orig);
> 
> I do not like much these multi assignments. I think that the result
> was not even defined in some older C standards. Though, I can't find
> it now. And even make W=3 does not warn about it.

Err.

It's been supported for decades. I learnt about it back in 1992 when
I was introduced to C by another experienced C programmer. It's been
supported in ANSI C compilers. The Norcroft C compiler (which is
strict ANSI) on Acorn platforms back in the late 1980s/1990s even
supported it.

I think you're a bit out of date.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Petr Mladek]

On Mon 2022-11-14 17:46:28, Andy Shevchenko wrote:
> On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> > On Tue 2022-11-08 16:33:22, Russell King wrote:
> 
> ...
> 
> > >  	orig = get_unaligned(fourcc);
> > > -	val = orig & ~BIT(31);
> > > +	switch (fmt[2]) {
> > > +	case 'h':
> > > +		val = orig;
> > > +		break;
> > > +	case 'r':
> > > +		val = orig = swab32(orig);
> > 
> > I do not like much these multi assignments. I think that the result
> > was not even defined in some older C standards. Though, I can't find
> > it now. And even make W=3 does not warn about it.
> > 
> > > +		break;
> > > +	case 'l':
> > > +		val = orig = le32_to_cpu(orig);
> > > +		break;
> > > +	case 'b':
> > > +		val = orig = be32_to_cpu(orig);
> > > +		break;
> 
> Isn't easy to fix? Something like below?
> 
> 	switch (fmt[2]) {
> 	case 'h':
> 		break;
> 	case 'r':
> 		orig = swab32(orig);
> 		break;
> 	case 'l':
> 		orig = le32_to_cpu(orig);
> 		break;
> 	case 'b':
> 		orig = be32_to_cpu(orig);
> 		break;
> 
> 		...
> 	}
> 	val = orig;

I though the same. Unfortunately, this is not valid for the "case c:"
path where "orig" stays untouched:

	case 'c':
		/* Pixel formats are printed LSB-first */
		val = swab32(orig & ~BIT(31));
		pixel_fmt = true;
		break;

It is pity that "orig" is handled differently for the pixel and the generic
formats.

But I am afraid that there is no good solution. The code will
always be a mess when it tries to implement a messy definition.

It would be nice if the the FourCC format was used consistently
in all subsystems in the first place.


IMPORTANT: This brings the questions.

	   Is there actually a standard how to print the original
	   number in FourCC?

	   Do we really want to modify "orig" in the generic
	   implementation?

Best Regards,
Petr

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Russell King (Oracle)]

On Mon, Nov 14, 2022 at 04:15:50PM +0000, Russell King (Oracle) wrote:
> On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> > >  	orig = get_unaligned(fourcc);
> > > -	val = orig & ~BIT(31);
> > > +	switch (fmt[2]) {
> > > +	case 'h':
> > > +		val = orig;
> > > +		break;
> > > +	case 'r':
> > > +		val = orig = swab32(orig);
> > 
> > I do not like much these multi assignments. I think that the result
> > was not even defined in some older C standards. Though, I can't find
> > it now. And even make W=3 does not warn about it.
> 
> Err.
> 
> It's been supported for decades. I learnt about it back in 1992 when
> I was introduced to C by another experienced C programmer. It's been
> supported in ANSI C compilers. The Norcroft C compiler (which is
> strict ANSI) on Acorn platforms back in the late 1980s/1990s even
> supported it.
> 
> I think you're a bit out of date.

Oh, and it's not like there isn't precedent for doing this in
lib/vsprintf.c:

841a915d20c7 vsprintf: Do not have bprintf dereference pointers

+                                       len = copy = strlen(args);

If you grep lib/, there's many more examples. So, what is in Hectors
patch is in no way any different from lots of other examples already
merged into the kernel code.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Petr Mladek]

On Mon 2022-11-14 16:15:50, Russell King (Oracle) wrote:
> On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> > On Tue 2022-11-08 16:33:22, Russell King wrote:
> > > From: Hector Martin <marcan@marcan.st>
> > > 
> > > %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> > > it's useful to be able to print generic 4-character codes formatted as
> > > an integer. Extend it to add format specifiers for printing generic
> > > 32-bit FOURCCs with various endian semantics:
> > > 
> > > %p4ch   Host-endian
> > > %p4cl	Little-endian
> > > %p4cb	Big-endian
> > > %p4cr	Reverse-endian
> > > 
> > > The endianness determines how bytes are interpreted as a u32, and the
> > > FOURCC is then always printed MSByte-first (this is the opposite of
> > > V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> > > allow printing LSByte-first FOURCCs stored in host endian order
> > > (other than the hex form being in character order, not the integer
> > > value).
> > > 
> > > Signed-off-by: Hector Martin <marcan@marcan.st>
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > 
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > 
> > See one nit below.
> > 
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -1762,27 +1762,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > >  	char output[sizeof("0123 little-endian (0x01234567)")];
> > >  	char *p = output;
> > >  	unsigned int i;
> > > +	bool pixel_fmt = false;
> > >  	u32 orig, val;
> > >  
> > > -	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > +	if (fmt[1] != 'c')
> > >  		return error_string(buf, end, "(%p4?)", spec);
> > >  
> > >  	if (check_pointer(&buf, end, fourcc, spec))
> > >  		return buf;
> > >  
> > >  	orig = get_unaligned(fourcc);
> > > -	val = orig & ~BIT(31);
> > > +	switch (fmt[2]) {
> > > +	case 'h':
> > > +		val = orig;
> > > +		break;
> > > +	case 'r':
> > > +		val = orig = swab32(orig);
> > 
> > I do not like much these multi assignments. I think that the result
> > was not even defined in some older C standards. Though, I can't find
> > it now. And even make W=3 does not warn about it.
> 
> Err.
> 
> It's been supported for decades. I learnt about it back in 1992 when
> I was introduced to C by another experienced C programmer. It's been
> supported in ANSI C compilers. The Norcroft C compiler (which is
> strict ANSI) on Acorn platforms back in the late 1980s/1990s even
> supported it.

Ah, the problem probably was with a more complicated assignment.
For example, the result of the following code is not obvious:

	  a = b = a++;

Best Regards,
Petr

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Petr Mladek]

On Tue 2022-11-08 16:33:22, Russell King wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> it's useful to be able to print generic 4-character codes formatted as
> an integer. Extend it to add format specifiers for printing generic
> 32-bit FOURCCs with various endian semantics:
> 
> %p4ch   Host-endian
> %p4cl	Little-endian
> %p4cb	Big-endian
> %p4cr	Reverse-endian
> 
> The endianness determines how bytes are interpreted as a u32, and the
> FOURCC is then always printed MSByte-first (this is the opposite of
> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> allow printing LSByte-first FOURCCs stored in host endian order
> (other than the hex form being in character order, not the integer
> value).
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++
>  lib/test_printf.c                         | 39 +++++++++++++++++++----
>  lib/vsprintf.c                            | 35 ++++++++++++++++----
>  3 files changed, 93 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index dbe1aacc79d0..92a488884cf8 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -625,6 +625,38 @@ Passed by reference.
>  	%p4cc	Y10  little-endian (0x20303159)
>  	%p4cc	NV12 big-endian (0xb231564e)
>  
> +Generic FourCC code
> +-------------------
> +
> +::
> +	%p4c[hrbl]	gP00 (0x67503030)
> +
> +Print a generic FourCC code, as both ASCII characters and its numerical
> +value as hexadecimal.
> +
> +The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify
> +host, reversed, big or little endian order data respectively. Host endian
> +order means the data is interpreted as a 32-bit integer and the most
> +significant byte is printed first; that is, the character code as printed
> +matches the byte order stored in memory on big-endian systems, and is reversed
> +on little-endian systems.

I though a bit more about the semantic and got a bit confused.
It might be because I am not familiar with FourCC. Anyway,
the description in the commit message provided some more clues.

The following documentation looks be more clear to me:

<proposal>
Generic FourCC code
-------------------

::
	%p4c[hrbl]	gP00 (0x67503030)

Print a generic FourCC code, as both ASCII characters and its numerical
value as hexadecimal.

The generic FourCC code is always printed in the the big-endian format,
the most significant byte first. This is the opposite of V4L/DRM
FOURCCs.

The additional ``h``, ``r``, ``b``, and ``l`` specifiers define what
endianes is used to load the stored value as 32-bit integer. The value
might be stored as host-endian, reverse-host-endian, big-endian,
or little endian.

Examples for a little-endian machine, host native load &(u32)0x67503030::

	%p4ch	gP00 (0x67503030)
	%p4cr	00Pg (0x30305067)
	%p4cb	00Pg (0x30305067)
	%p4cl	gP00 (0x67503030)

Examples for a big-endian machine, host native load &(u32)0x67503030::

	%p4ch	gP00 (0x67503030)
	%p4cr	00Pg (0x30305067)
	%p4cb	gP00 (0x67503030)
	%p4cl	00Pg (0x30305067)
</proposal>

Best Regards,
Petr