[net-next] dt-bindings: net: convert sff,sfp to dtschema

Convert the sff,sfp.txt bindings to the DT schema format.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 .../devicetree/bindings/net/sff,sfp.txt       |  85 ------------
 .../devicetree/bindings/net/sff,sfp.yaml      | 130 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 3 files changed, 131 insertions(+), 85 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/net/sff,sfp.txt
 create mode 100644 Documentation/devicetree/bindings/net/sff,sfp.yaml

On Wed, Mar 16, 2022 at 09:23:45AM +0100, Krzysztof Kozlowski wrote:
> On 15/03/2022 20:07, Ioana Ciornei wrote:
> > On Tue, Mar 15, 2022 at 07:21:59PM +0100, Krzysztof Kozlowski wrote:
> >> On 15/03/2022 13:33, Ioana Ciornei wrote:
> >>> Convert the sff,sfp.txt bindings to the DT schema format.
> >>>
> >>> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> >>> ---
> > 
> > (..)
> > 
> >>> +maintainers:
> >>> +  - Russell King <linux@armlinux.org.uk>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - sff,sfp  # for SFP modules
> >>> +      - sff,sff  # for soldered down SFF modules
> >>> +
> >>> +  i2c-bus:
> >>
> >> Thanks for the conversion.
> >>
> >> You need here a type because this does not look like standard property.
> > 
> > Ok.
> > 
> >>
> >>> +    description:
> >>> +      phandle of an I2C bus controller for the SFP two wire serial
> >>> +
> >>> +  maximum-power-milliwatt:
> >>> +    maxItems: 1
> >>> +    description:
> >>> +      Maximum module power consumption Specifies the maximum power consumption
> >>> +      allowable by a module in the slot, in milli-Watts. Presently, modules can
> >>> +      be up to 1W, 1.5W or 2W.
> >>> +
> >>> +patternProperties:
> >>> +  "mod-def0-gpio(s)?":
> >>
> >> This should be just "mod-def0-gpios", no need for pattern. The same in
> >> all other places.
> >>
> > 
> > The GPIO subsystem accepts both suffixes: "gpio" and "gpios", see
> > gpio_suffixes[]. If I just use "mod-def0-gpios" multiple DT files will
> > fail the check because they are using the "gpio" suffix.
> > 
> > Why isn't this pattern acceptable?
> 
> Because original bindings required gpios, so DTS are wrong, and the
> pattern makes it difficult to grep and read such simple property.
> 
> The DTSes which do not follow bindings should be corrected.
> 

Russell, do you have any thoughts on this?
I am asking this because you were the one that added the "-gpios" suffix
in the dtbinding and the "-gpio" usage in the DT files so I wouldn't
want this to diverge from your thinking.

Do you have a preference?

> > 
> >>> +
> >>> +  "rate-select1-gpio(s)?":
> >>> +    maxItems: 1
> >>> +    description:
> >>> +      GPIO phandle and a specifier of the Tx Signaling Rate Select (AKA RS1)
> >>> +      output gpio signal (SFP+ only), low - low Tx rate, high - high Tx rate. Must
> >>> +      not be present for SFF modules
> >>
> >> This and other cases should have a "allOf: if: ...." with a
> >> "rate-select1-gpios: false", to disallow this property on SFF modules.
> >>
> > 
> > Ok, didn't know that's possible.
> > 
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - i2c-bus
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - | # Direct serdes to SFP connection
> >>> +    #include <dt-bindings/gpio/gpio.h>
> >>> +
> >>> +    sfp_eth3: sfp-eth3 {
> >>
> >> Generic node name please, so maybe "transceiver"? or just "sfp"?
> >>
> > 
> > Ok, I can do just "sfp".
> > 
> >>> +      compatible = "sff,sfp";
> >>> +      i2c-bus = <&sfp_1g_i2c>;
> >>> +      los-gpios = <&cpm_gpio2 22 GPIO_ACTIVE_HIGH>;
> >>> +      mod-def0-gpios = <&cpm_gpio2 21 GPIO_ACTIVE_LOW>;
> >>> +      maximum-power-milliwatt = <1000>;
> >>> +      pinctrl-names = "default";
> >>> +      pinctrl-0 = <&cpm_sfp_1g_pins &cps_sfp_1g_pins>;
> >>> +      tx-disable-gpios = <&cps_gpio1 24 GPIO_ACTIVE_HIGH>;
> >>> +      tx-fault-gpios = <&cpm_gpio2 19 GPIO_ACTIVE_HIGH>;
> >>> +    };
> >>> +
> >>> +    cps_emac3 {
> >>
> >> This is not related, so please remove.
> > 
> > It's related since it shows a generic usage pattern of the sfp node.
> > I wouldn't just remove it since it's just adds context to the example
> > not doing any harm.
> 
> Usage (consumer) is not related to these bindings. The bindings for this
> phy/mac should show the usage of sfp, but not the provider bindings.
> 
> The bindings of each clock provider do not contain examples for clock
> consumer. Same for regulator, pinctrl, power domains, interconnect and
> every other component. It would be a lot of code duplication to include
> consumers in each provider. Instead, we out the example of consumer in
> the consumer bindings.
> 
> The harm is - duplicated code and one more example which can be done
> wrong (like here node name not conforming to DT spec).

I suppose you refer to "sfp-eth3" which you suggested here to be just
"sfp". In an example, that's totally acceptable but on boards there can
be multiple SFPs which would mean that there would be multiple sfp
nodes. We have to discern somehow between them. Adding a unit name would
not be optimal since there is no "reg" property to go with it.

So "sfp-eth3" or variants I think are necessary even though not
conforming to the DT spec.

> 
> If you insist to keep it, please share why these bindings are special,
> different than all other bindings I mentioned above.

If it's that unheard of to have a somewhat complete example why are
there multiple dtschema files submitted even by yourself with this same
setup?
As an example for a consumer device being listed in the provider yaml
file is 'gpio-pca95xx.yaml' and for the reverse (provider described in
the consumer) I can list 'samsung,s5pv210-clock.yaml',
'samsung,exynos5260-clock.yaml' etc.

Ioana

On 16/03/2022 11:18, Ioana Ciornei wrote:
>>>
>>> It's related since it shows a generic usage pattern of the sfp node.
>>> I wouldn't just remove it since it's just adds context to the example
>>> not doing any harm.
>>
>> Usage (consumer) is not related to these bindings. The bindings for this
>> phy/mac should show the usage of sfp, but not the provider bindings.
>>
>> The bindings of each clock provider do not contain examples for clock
>> consumer. Same for regulator, pinctrl, power domains, interconnect and
>> every other component. It would be a lot of code duplication to include
>> consumers in each provider. Instead, we out the example of consumer in
>> the consumer bindings.
>>
>> The harm is - duplicated code and one more example which can be done
>> wrong (like here node name not conforming to DT spec).
> 
> I suppose you refer to "sfp-eth3" which you suggested here to be just
> "sfp". 

I refer now to "cps_emac3" which uses specific name instead of generic
and underscore instead of hyphen (although underscore is actually listed
as allowed in DT spec, dtc will complain about it).

>In an example, that's totally acceptable but on boards there can
> be multiple SFPs which would mean that there would be multiple sfp
> nodes. We have to discern somehow between them. Adding a unit name would
> not be optimal since there is no "reg" property to go with it.

The common practice is adding a numbering suffix.

> 
> So "sfp-eth3" or variants I think are necessary even though not
> conforming to the DT spec.
> 
>>
>> If you insist to keep it, please share why these bindings are special,
>> different than all other bindings I mentioned above.
> 
> If it's that unheard of to have a somewhat complete example why are
> there multiple dtschema files submitted even by yourself with this same
> setup?

I am also learning and I wished many of my mistakes of early DT bindings
conversion were spotted. Especially my early bindings... but even now I
keep making mistakes. Human thing. :)

I converted quite a lot of bindings, so can you point to such examples
of my schema which includes consumer example in a provider bindings? If
you find such, please send a patch removing trivial code.


> As an example for a consumer device being listed in the provider yaml
> file is 'gpio-pca95xx.yaml'

Indeed, this is trivial and useless code which I kept from conversion,
should be removed.

>
 and for the reverse (provider described in
> the consumer) I can list 'samsung,s5pv210-clock.yaml',
> 'samsung,exynos5260-clock.yaml' etc.

These are different. This is an example how to model the input clock to
the device being described in the bindings. This is not an example how
to use the clock provider, like you created here. The input clock
sometimes is defined in Exynos clock controller, sometimes outside. The
example there shows the second case - when it has to come outside. It's
not showing the usage of clocks provided by this device, but I agree
that it also might be trivial and obvious. If you think it is obvious,
feel free to comment/send a patch.

Best regards,
Krzysztof