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

[Krzysztof Kozlowski]

On 09/09/2024 22:36, Chris Packham wrote:
> 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.

This might never happen, so unless you document more models now, I
strongly suggest using compatible as filename.

> 
>>
>>> 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.

There is no such device as MFD. MFD is a Linux framework.

>>
>> 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.

Your commit msg is not explaining here much. And "Currently the only
supported" feels like a driver description. We expect bindings to be
complete. It's fine to bring partial description of hardware, but this
should be explained in the commit msg and entire binding should be still
created to accommodate that full description.

However such complex devices like Ocelot should be described fully so we
can easily see how you organize entire binding.

> 
>> 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.

This feels ok, although you really should create complete binding here.

> 
> 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.


Best regards,
Krzysztof