Re: [PATCH 1/2] dt-bindings: mfd: Add Realtek switch

[Chris Packham]

Hi Krzysztof,

On 9/09/24 18:38, Krzysztof Kozlowski wrote:
> On Mon, Sep 09, 2024 at 01:47:06PM +1200, Chris Packham wrote:
> > Add device tree schema for the Realtek switch. Currently the only
> > supported feature is the syscon-reboot which is needed to be able to
> > reboot the board.
> >
> > Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   .../bindings/mfd/realtek,switch.yaml          | 50 +++++++++++++++++++
> >   1 file changed, 50 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/mfd/realtek,switch.yaml
> Use compatible as filename.

My hope was eventually that this would support multiple Realtek 
switches. But sure for now at least I can name it after the one in front 
of me.

> > diff --git a/Documentation/devicetree/bindings/mfd/realtek,switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml
> > new file mode 100644
> > index 000000000000..84b57f87bd3a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/realtek,switch.yaml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://scanmail.trustwave.com/?c=20988&d=55fe5gyquxahZ_dJqiHMxmkDG8M1MWjoNtZN70yrng&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fmfd%2frealtek%2cswitch%2eyaml%23
> > +$schema: http://scanmail.trustwave.com/?c=20988&d=55fe5gyquxahZ_dJqiHMxmkDG8M1MWjoNoNFvkz8nA&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
> > +
> > +title: Realtek Switch with Internal CPU
> What sort of Switch? Like network switch? Then this should be placed in
> respective net (or deeper, e.g. net/dsa/) directory.
Yes network switch. But this is one of those all in one chips that has a 
CPU, network switch and various peripherals. MFD seemed appropriate.
> Maintainers go here. See example-schema.

Ack.

> > +
> > +description:
> > +  The RTL9302 ethernet switch has an internal CPU. The switch is a multi-port
> > +  networking switch that supports many interfaces. Additionally, the device can
> > +  support MDIO, SPI and I2C busses.
> I don't get why syscon node is called switch. This looks incomplete or
> you used description from some other device.

Yes I did take a lot of inspiration from the mscc,ocelot. I am working 
on more support for the switch and some of the other peripherals so I 
figured I'd word it towards that end goal. If you prefer I could word 
this more towards the one function (reboot) that is supported right now.

> But if this is DSA, then you miss dsa ref and dsa-related properties.

So far I'm resisting DSA. The usage of the RTL9300 as a SoC+Switch 
doesn't really lend itself to the DSA architecture (there is a external 
CPU mode that would). I think eventually we'd end up with something like 
the mscc,oscelot where both switchdev and DSA usage is supported. There 
would be some properties (e.g. port/phy arrangement) that apply to both 
uses.

I have got a (kind of) working proof of concept switchdev driver which 
has some of the support you've mentioned. It's not really ready so I 
didn't include the dt-binding for that stuff in this patch.

> > +
> > +maintainers:
> > +  - Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - realtek,rtl9302c-switch
> > +      - const: syscon
> > +      - const: simple-mfd
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  reboot:
> > +    $ref: /schemas/power/reset/syscon-reboot.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reboot
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    switch0: ethernet-switch@1b000000 {
> Drop unused label.
Ack.
> Best regards,
> Krzysztof