netdev - Re: [RFC PATCH 0/2] MIIM regmap and RTL8231 GPIO expander support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d73a44809c96abd0397474c63219a41e28f78235.camel@svanheule.net>
Date:   Fri, 09 Apr 2021 07:42:32 +0200
From:   Sander Vanheule <sander@...nheule.net>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-gpio@...r.kernel.org, Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>, bert@...t.com
Subject: Re: [RFC PATCH 0/2] MIIM regmap and RTL8231 GPIO expander support

Hi Andrew,

Thank you for the feedback. You can find a (leaked) datasheet at:
https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_1.2.pdf

On Fri, 2021-04-09 at 00:18 +0200, Andrew Lunn wrote:
> > - Providing no compatible for an MDIO child node is considered to
> > be equivalent
> >   to a c22 ethernet phy, so one must be provided. However, this
> > node is then
> >   not automatically probed.
> 
> It cannot be automatically probed, since register 2 and 3 do not
> contain an ID, which PHYs do. So you need to explicitly list in on
> the
> MDIO bus, and when the of_mdiobus_register() is called, the device
> will be instantiated.
> 
> Is it okay to provide a binding without a driver?
> >   If some code is required, where should this be put?
> >   Current devicetree structure:
> >     mdio-bus {
> >         compatible = "vendor,mdio";
> >         ...
> > 
> >         expander0: expander@0 {
> >             /*
> >              * Provide compatible for working registration of mdio
> > device.
> >              * Device probing happens in gpio1 node.
> >              */
> >             compatible = "realtek,rtl8231-expander";
> >             reg = <0>;
> >         };
> > 
> >     };
> >     gpio1 : ext_gpio {
> >         compatible = "realtek,rtl8231-mdio";
> >         gpio-controller;
> >         ...
> >     };
> 
> I don't understand this split. Why not
> 
>      mdio-bus {
>          compatible = "vendor,mdio";
>          ...
>  
>          expander0: expander@0 {
>              /*
>               * Provide compatible for working registration of mdio
> device.
>               * Device probing happens in gpio1 node.
>               */
>              compatible = "realtek,rtl8231-expander";
>              reg = <0>;
>              gpio-controller;
>          };
>      };
> 
> You can list whatever properties you need in the node. Ethernet
> switches have interrupt-controller, embedded MDIO busses with PHYs on
> them etc.

This is what I tried initially, but it doesn't seem to work. The node
is probably still added as an MDIO device, but rtl8231_gpio_probe()
doesn't appear to get called at all. I do agree it would be preferable
over the split specification.

Having another look, I see mdio_device_id is used for ethernet phys,
but like you said this requires and ID in registers 2 & 3. These
registers contain pin configuration on the RTL8231, so this can't be
used.
Registering as a phy_driver appears to have the same issue, although it
looks like I could use a custom match_phy_device(). I do feel like this
would be stretching the meaning of what a PHY is.


> > - MFD driver:
> >   The RTL8231 is not just a GPIO expander, but also a pin
> > controller and LED
> >   matrix controller. Regmap initialisation could probably be moved
> > to a parent
> >   MFD, with gpio, led, and pinctrl cells. Is this a hard
> > requirement if only a
> >   GPIO controller is provided?
> 
> You need to think about forward/backwards compatibility. You are
> defining a binding now, which you need to keep. Do you see how an MFD
> could be added without breaking backwards compatibility?

There are pin-/gpio-controllers that have the gpio and pinctrl nodes in
the device's root node. So I think adding pinctrl later shouldn't be an
issue. The LED matrix description would probably need a dedicated sub-
node. I'll see if I can write some preliminary bindings later today or
this weekend.

Best,
Sander