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: <YG+BObnBEOZnoJ1K@lunn.ch>
Date:   Fri, 9 Apr 2021 00:18:33 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Sander Vanheule <sander@...nheule.net>
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

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

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

      Andrew