| Message ID | 20210607123317.3242031-1-robert.marko@sartura.hr |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6,1/6] mfd: simple-mfd-i2c: Add Delta TN48M CPLD support | expand |
On Mon, Jun 7, 2021 at 2:33 PM Robert Marko <robert.marko@sartura.hr> wrote: > > Add binding documents for the Delta TN48M CPLD drivers. > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > --- > Changes in v3: > * Include bindings for reset driver > > Changes in v2: > * Implement MFD as a simple I2C MFD > * Add GPIO bindings as separate > > .../bindings/gpio/delta,tn48m-gpio.yaml | 42 +++++++++ > .../bindings/mfd/delta,tn48m-cpld.yaml | 90 +++++++++++++++++++ > .../bindings/reset/delta,tn48m-reset.yaml | 35 ++++++++ > 3 files changed, 167 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > create mode 100644 Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > new file mode 100644 > index 000000000000..aca646aecb12 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > @@ -0,0 +1,42 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Delta Networks TN48M CPLD GPIO controller > + > +maintainers: > + - Robert Marko <robert.marko@sartura.hr> > + > +description: | > + This module is part of the Delta TN48M multi-function device. For more > + details see ../mfd/delta,tn48m-cpld.yaml. > + > + GPIO controller module provides GPIO-s for the SFP slots. > + It is split into 3 controllers, one output only for the SFP TX disable > + pins, one input only for the SFP present pins and one input only for > + the SFP LOS pins. > + > +properties: > + compatible: > + enum: > + - delta,tn48m-gpio-sfp-tx-disable > + - delta,tn48m-gpio-sfp-present > + - delta,tn48m-gpio-sfp-los > + > + reg: > + maxItems: 1 > + > + "#gpio-cells": > + const: 2 > + > + gpio-controller: true > + > +required: > + - compatible > + - reg > + - "#gpio-cells" > + - gpio-controller > + > +additionalProperties: false > diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > new file mode 100644 > index 000000000000..2c6e2adf73ca > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > @@ -0,0 +1,90 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Delta Networks TN48M CPLD controller > + > +maintainers: > + - Robert Marko <robert.marko@sartura.hr> > + > +description: | > + Lattice CPLD onboard the TN48M switches is used for system > + management. > + > + It provides information about the hardware model, revision, > + PSU status etc. > + > + It is also being used as a GPIO expander for the SFP slots and > + reset controller for the switch MAC-s and other peripherals. > + > +properties: > + compatible: > + const: delta,tn48m-cpld > + > + reg: > + description: > + I2C device address. > + maxItems: 1 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > +required: > + - compatible > + - reg > + - "#address-cells" > + - "#size-cells" > + > +patternProperties: > + "^gpio(@[0-9a-f]+)?$": > + $ref: ../gpio/delta,tn48m-gpio.yaml > + > + "^reset-controller?$": > + $ref: ../reset/delta,tn48m-reset.yaml > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + cpld@41 { > + compatible = "delta,tn48m-cpld"; > + reg = <0x41>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + gpio@31 { > + compatible = "delta,tn48m-gpio-sfp-tx-disable"; > + reg = <0x31>; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + > + gpio@3a { > + compatible = "delta,tn48m-gpio-sfp-present"; > + reg = <0x3a>; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + > + gpio@40 { > + compatible = "delta,tn48m-gpio-sfp-los"; > + reg = <0x40>; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + > + reset-controller { > + compatible = "delta,tn48m-reset"; > + #reset-cells = <1>; > + }; > + }; > + }; > diff --git a/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > new file mode 100644 > index 000000000000..0e5ee8decc0d > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > @@ -0,0 +1,35 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/reset/delta,tn48m-reset.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Delta Networks TN48M CPLD reset controller > + > +maintainers: > + - Robert Marko <robert.marko@sartura.hr> > + > +description: | > + This module is part of the Delta TN48M multi-function device. For more > + details see ../mfd/delta,tn48m-cpld.yaml. > + > + Reset controller modules provides resets for the following: > + * 88F7040 SoC > + * 88F6820 SoC > + * 98DX3265 switch MAC-s > + * 88E1680 PHY-s > + * 88E1512 PHY > + * PoE PSE controller > + > +properties: > + compatible: > + const: delta,tn48m-reset > + > + "#reset-cells": > + const: 1 > + > +required: > + - compatible > + - "#reset-cells" > + > +additionalProperties: false > -- > 2.31.1 > Are there any issues with the bindings? The patch series is depending on this as the rest has been reviewed. Regards, Robert
On Fri, Jun 25, 2021 at 01:46:08PM +0200, Robert Marko wrote: > On Mon, Jun 7, 2021 at 2:33 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > > Add binding documents for the Delta TN48M CPLD drivers. > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > --- > > Changes in v3: > > * Include bindings for reset driver > > > > Changes in v2: > > * Implement MFD as a simple I2C MFD > > * Add GPIO bindings as separate > > > > .../bindings/gpio/delta,tn48m-gpio.yaml | 42 +++++++++ > > .../bindings/mfd/delta,tn48m-cpld.yaml | 90 +++++++++++++++++++ > > .../bindings/reset/delta,tn48m-reset.yaml | 35 ++++++++ > > 3 files changed, 167 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > create mode 100644 Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > > diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > new file mode 100644 > > index 000000000000..aca646aecb12 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > @@ -0,0 +1,42 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Delta Networks TN48M CPLD GPIO controller > > + > > +maintainers: > > + - Robert Marko <robert.marko@sartura.hr> > > + > > +description: | > > + This module is part of the Delta TN48M multi-function device. For more > > + details see ../mfd/delta,tn48m-cpld.yaml. > > + > > + GPIO controller module provides GPIO-s for the SFP slots. > > + It is split into 3 controllers, one output only for the SFP TX disable > > + pins, one input only for the SFP present pins and one input only for > > + the SFP LOS pins. > > + > > +properties: > > + compatible: > > + enum: > > + - delta,tn48m-gpio-sfp-tx-disable > > + - delta,tn48m-gpio-sfp-present > > + - delta,tn48m-gpio-sfp-los > > + > > + reg: > > + maxItems: 1 > > + > > + "#gpio-cells": > > + const: 2 > > + > > + gpio-controller: true > > + > > +required: > > + - compatible > > + - reg > > + - "#gpio-cells" > > + - gpio-controller > > + > > +additionalProperties: false > > diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > new file mode 100644 > > index 000000000000..2c6e2adf73ca > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > @@ -0,0 +1,90 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Delta Networks TN48M CPLD controller > > + > > +maintainers: > > + - Robert Marko <robert.marko@sartura.hr> > > + > > +description: | > > + Lattice CPLD onboard the TN48M switches is used for system > > + management. > > + > > + It provides information about the hardware model, revision, > > + PSU status etc. > > + > > + It is also being used as a GPIO expander for the SFP slots and > > + reset controller for the switch MAC-s and other peripherals. > > + > > +properties: > > + compatible: > > + const: delta,tn48m-cpld > > + > > + reg: > > + description: > > + I2C device address. > > + maxItems: 1 > > + > > + "#address-cells": > > + const: 1 > > + > > + "#size-cells": > > + const: 0 > > + > > +required: > > + - compatible > > + - reg > > + - "#address-cells" > > + - "#size-cells" > > + > > +patternProperties: > > + "^gpio(@[0-9a-f]+)?$": > > + $ref: ../gpio/delta,tn48m-gpio.yaml > > + > > + "^reset-controller?$": > > + $ref: ../reset/delta,tn48m-reset.yaml > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + cpld@41 { > > + compatible = "delta,tn48m-cpld"; > > + reg = <0x41>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + gpio@31 { > > + compatible = "delta,tn48m-gpio-sfp-tx-disable"; > > + reg = <0x31>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > + > > + gpio@3a { > > + compatible = "delta,tn48m-gpio-sfp-present"; > > + reg = <0x3a>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > + > > + gpio@40 { > > + compatible = "delta,tn48m-gpio-sfp-los"; > > + reg = <0x40>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + }; > > + > > + reset-controller { > > + compatible = "delta,tn48m-reset"; > > + #reset-cells = <1>; > > + }; > > + }; > > + }; > > diff --git a/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > new file mode 100644 > > index 000000000000..0e5ee8decc0d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > @@ -0,0 +1,35 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/reset/delta,tn48m-reset.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Delta Networks TN48M CPLD reset controller > > + > > +maintainers: > > + - Robert Marko <robert.marko@sartura.hr> > > + > > +description: | > > + This module is part of the Delta TN48M multi-function device. For more > > + details see ../mfd/delta,tn48m-cpld.yaml. > > + > > + Reset controller modules provides resets for the following: > > + * 88F7040 SoC > > + * 88F6820 SoC > > + * 98DX3265 switch MAC-s > > + * 88E1680 PHY-s > > + * 88E1512 PHY > > + * PoE PSE controller > > + > > +properties: > > + compatible: > > + const: delta,tn48m-reset > > + > > + "#reset-cells": > > + const: 1 > > + > > +required: > > + - compatible > > + - "#reset-cells" > > + > > +additionalProperties: false > > -- > > 2.31.1 > > > > Are there any issues with the bindings? Yes. Primarily the GPIO function being part of the compatible. I'm surprised Linus W is okay with that. > The patch series is depending on this as the rest has been reviewed. The bindings have been reviewed too, you just didn't like my comments... Rob
On Wed, Jul 14, 2021 at 12:25 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Jun 25, 2021 at 01:46:08PM +0200, Robert Marko wrote: > > On Mon, Jun 7, 2021 at 2:33 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > > > > Add binding documents for the Delta TN48M CPLD drivers. > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > > --- > > > Changes in v3: > > > * Include bindings for reset driver > > > > > > Changes in v2: > > > * Implement MFD as a simple I2C MFD > > > * Add GPIO bindings as separate > > > > > > .../bindings/gpio/delta,tn48m-gpio.yaml | 42 +++++++++ > > > .../bindings/mfd/delta,tn48m-cpld.yaml | 90 +++++++++++++++++++ > > > .../bindings/reset/delta,tn48m-reset.yaml | 35 ++++++++ > > > 3 files changed, 167 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > > create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > > create mode 100644 Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > > new file mode 100644 > > > index 000000000000..aca646aecb12 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > > @@ -0,0 +1,42 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Delta Networks TN48M CPLD GPIO controller > > > + > > > +maintainers: > > > + - Robert Marko <robert.marko@sartura.hr> > > > + > > > +description: | > > > + This module is part of the Delta TN48M multi-function device. For more > > > + details see ../mfd/delta,tn48m-cpld.yaml. > > > + > > > + GPIO controller module provides GPIO-s for the SFP slots. > > > + It is split into 3 controllers, one output only for the SFP TX disable > > > + pins, one input only for the SFP present pins and one input only for > > > + the SFP LOS pins. > > > + > > > +properties: > > > + compatible: > > > + enum: > > > + - delta,tn48m-gpio-sfp-tx-disable > > > + - delta,tn48m-gpio-sfp-present > > > + - delta,tn48m-gpio-sfp-los > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + "#gpio-cells": > > > + const: 2 > > > + > > > + gpio-controller: true > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - "#gpio-cells" > > > + - gpio-controller > > > + > > > +additionalProperties: false > > > diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > > new file mode 100644 > > > index 000000000000..2c6e2adf73ca > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > > @@ -0,0 +1,90 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Delta Networks TN48M CPLD controller > > > + > > > +maintainers: > > > + - Robert Marko <robert.marko@sartura.hr> > > > + > > > +description: | > > > + Lattice CPLD onboard the TN48M switches is used for system > > > + management. > > > + > > > + It provides information about the hardware model, revision, > > > + PSU status etc. > > > + > > > + It is also being used as a GPIO expander for the SFP slots and > > > + reset controller for the switch MAC-s and other peripherals. > > > + > > > +properties: > > > + compatible: > > > + const: delta,tn48m-cpld > > > + > > > + reg: > > > + description: > > > + I2C device address. > > > + maxItems: 1 > > > + > > > + "#address-cells": > > > + const: 1 > > > + > > > + "#size-cells": > > > + const: 0 > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - "#address-cells" > > > + - "#size-cells" > > > + > > > +patternProperties: > > > + "^gpio(@[0-9a-f]+)?$": > > > + $ref: ../gpio/delta,tn48m-gpio.yaml > > > + > > > + "^reset-controller?$": > > > + $ref: ../reset/delta,tn48m-reset.yaml > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + i2c { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + cpld@41 { > > > + compatible = "delta,tn48m-cpld"; > > > + reg = <0x41>; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + gpio@31 { > > > + compatible = "delta,tn48m-gpio-sfp-tx-disable"; > > > + reg = <0x31>; > > > + gpio-controller; > > > + #gpio-cells = <2>; > > > + }; > > > + > > > + gpio@3a { > > > + compatible = "delta,tn48m-gpio-sfp-present"; > > > + reg = <0x3a>; > > > + gpio-controller; > > > + #gpio-cells = <2>; > > > + }; > > > + > > > + gpio@40 { > > > + compatible = "delta,tn48m-gpio-sfp-los"; > > > + reg = <0x40>; > > > + gpio-controller; > > > + #gpio-cells = <2>; > > > + }; > > > + > > > + reset-controller { > > > + compatible = "delta,tn48m-reset"; > > > + #reset-cells = <1>; > > > + }; > > > + }; > > > + }; > > > diff --git a/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > new file mode 100644 > > > index 000000000000..0e5ee8decc0d > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > @@ -0,0 +1,35 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/reset/delta,tn48m-reset.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Delta Networks TN48M CPLD reset controller > > > + > > > +maintainers: > > > + - Robert Marko <robert.marko@sartura.hr> > > > + > > > +description: | > > > + This module is part of the Delta TN48M multi-function device. For more > > > + details see ../mfd/delta,tn48m-cpld.yaml. > > > + > > > + Reset controller modules provides resets for the following: > > > + * 88F7040 SoC > > > + * 88F6820 SoC > > > + * 98DX3265 switch MAC-s > > > + * 88E1680 PHY-s > > > + * 88E1512 PHY > > > + * PoE PSE controller > > > + > > > +properties: > > > + compatible: > > > + const: delta,tn48m-reset > > > + > > > + "#reset-cells": > > > + const: 1 > > > + > > > +required: > > > + - compatible > > > + - "#reset-cells" > > > + > > > +additionalProperties: false > > > -- > > > 2.31.1 > > > > > > > Are there any issues with the bindings? > > Yes. Primarily the GPIO function being part of the compatible. I'm > surprised Linus W is okay with that. I think I already explained this before, having a single compatible won't work here. Then there would not be anything to know whether its input or output only as the pins have specific purpose. And knowing the capabilites is a requirment of the GPIO regmap driver and the GPIO core itself as it exposes that information in a generic manner and driver like for the SFP bus use that. Maybe Linus W can chime in here as well. > > > The patch series is depending on this as the rest has been reviewed. > > The bindings have been reviewed too, you just didn't like my comments... Sorry, I did not pick that up as after my replies there was no further discussion. I am really hoping that we can find a middle ground here and get this merged as the driver code itself has been revied and ACK-ed. Regards, Robert > > Rob -- Robert Marko Staff Embedded Linux Engineer Sartura Ltd. Lendavska ulica 16a 10000 Zagreb, Croatia Email: robert.marko@sartura.hr Web: www.sartura.hr
On Sun, 18 Jul 2021, Robert Marko wrote: > On Wed, Jul 14, 2021 at 12:25 AM Rob Herring <robh@kernel.org> wrote: > > > > On Fri, Jun 25, 2021 at 01:46:08PM +0200, Robert Marko wrote: > > > On Mon, Jun 7, 2021 at 2:33 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > > > > > > Add binding documents for the Delta TN48M CPLD drivers. > > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > > > --- > > > > Changes in v3: > > > > * Include bindings for reset driver > > > > > > > > Changes in v2: > > > > * Implement MFD as a simple I2C MFD > > > > * Add GPIO bindings as separate > > > > > > > > .../bindings/gpio/delta,tn48m-gpio.yaml | 42 +++++++++ > > > > .../bindings/mfd/delta,tn48m-cpld.yaml | 90 +++++++++++++++++++ > > > > .../bindings/reset/delta,tn48m-reset.yaml | 35 ++++++++ > > > > 3 files changed, 167 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > > > create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > > > create mode 100644 Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > > > new file mode 100644 > > > > index 000000000000..aca646aecb12 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > > > @@ -0,0 +1,42 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Delta Networks TN48M CPLD GPIO controller > > > > + > > > > +maintainers: > > > > + - Robert Marko <robert.marko@sartura.hr> > > > > + > > > > +description: | > > > > + This module is part of the Delta TN48M multi-function device. For more > > > > + details see ../mfd/delta,tn48m-cpld.yaml. > > > > + > > > > + GPIO controller module provides GPIO-s for the SFP slots. > > > > + It is split into 3 controllers, one output only for the SFP TX disable > > > > + pins, one input only for the SFP present pins and one input only for > > > > + the SFP LOS pins. > > > > + > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - delta,tn48m-gpio-sfp-tx-disable > > > > + - delta,tn48m-gpio-sfp-present > > > > + - delta,tn48m-gpio-sfp-los > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + "#gpio-cells": > > > > + const: 2 > > > > + > > > > + gpio-controller: true > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - "#gpio-cells" > > > > + - gpio-controller > > > > + > > > > +additionalProperties: false > > > > diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > > > new file mode 100644 > > > > index 000000000000..2c6e2adf73ca > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > > > @@ -0,0 +1,90 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Delta Networks TN48M CPLD controller > > > > + > > > > +maintainers: > > > > + - Robert Marko <robert.marko@sartura.hr> > > > > + > > > > +description: | > > > > + Lattice CPLD onboard the TN48M switches is used for system > > > > + management. > > > > + > > > > + It provides information about the hardware model, revision, > > > > + PSU status etc. > > > > + > > > > + It is also being used as a GPIO expander for the SFP slots and > > > > + reset controller for the switch MAC-s and other peripherals. > > > > + > > > > +properties: > > > > + compatible: > > > > + const: delta,tn48m-cpld > > > > + > > > > + reg: > > > > + description: > > > > + I2C device address. > > > > + maxItems: 1 > > > > + > > > > + "#address-cells": > > > > + const: 1 > > > > + > > > > + "#size-cells": > > > > + const: 0 > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - "#address-cells" > > > > + - "#size-cells" > > > > + > > > > +patternProperties: > > > > + "^gpio(@[0-9a-f]+)?$": > > > > + $ref: ../gpio/delta,tn48m-gpio.yaml > > > > + > > > > + "^reset-controller?$": > > > > + $ref: ../reset/delta,tn48m-reset.yaml > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + i2c { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + cpld@41 { > > > > + compatible = "delta,tn48m-cpld"; > > > > + reg = <0x41>; > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + gpio@31 { > > > > + compatible = "delta,tn48m-gpio-sfp-tx-disable"; > > > > + reg = <0x31>; > > > > + gpio-controller; > > > > + #gpio-cells = <2>; > > > > + }; > > > > + > > > > + gpio@3a { > > > > + compatible = "delta,tn48m-gpio-sfp-present"; > > > > + reg = <0x3a>; > > > > + gpio-controller; > > > > + #gpio-cells = <2>; > > > > + }; > > > > + > > > > + gpio@40 { > > > > + compatible = "delta,tn48m-gpio-sfp-los"; > > > > + reg = <0x40>; > > > > + gpio-controller; > > > > + #gpio-cells = <2>; > > > > + }; > > > > + > > > > + reset-controller { > > > > + compatible = "delta,tn48m-reset"; > > > > + #reset-cells = <1>; > > > > + }; > > > > + }; > > > > + }; > > > > diff --git a/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > > new file mode 100644 > > > > index 000000000000..0e5ee8decc0d > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > > @@ -0,0 +1,35 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/reset/delta,tn48m-reset.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Delta Networks TN48M CPLD reset controller > > > > + > > > > +maintainers: > > > > + - Robert Marko <robert.marko@sartura.hr> > > > > + > > > > +description: | > > > > + This module is part of the Delta TN48M multi-function device. For more > > > > + details see ../mfd/delta,tn48m-cpld.yaml. > > > > + > > > > + Reset controller modules provides resets for the following: > > > > + * 88F7040 SoC > > > > + * 88F6820 SoC > > > > + * 98DX3265 switch MAC-s > > > > + * 88E1680 PHY-s > > > > + * 88E1512 PHY > > > > + * PoE PSE controller > > > > + > > > > +properties: > > > > + compatible: > > > > + const: delta,tn48m-reset > > > > + > > > > + "#reset-cells": > > > > + const: 1 > > > > + > > > > +required: > > > > + - compatible > > > > + - "#reset-cells" > > > > + > > > > +additionalProperties: false > > > > > > > > > > Are there any issues with the bindings? > > > > Yes. Primarily the GPIO function being part of the compatible. I'm > > surprised Linus W is okay with that. > > I think I already explained this before, having a single compatible > won't work here. > Then there would not be anything to know whether its input or output > only as the pins have specific purpose. Properties? > And knowing the capabilites is a requirment of the GPIO regmap driver > and the GPIO > core itself as it exposes that information in a generic manner and > driver like for the > SFP bus use that. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
On Sun, Jul 18, 2021 at 11:15:38AM +0200, Robert Marko wrote: > On Wed, Jul 14, 2021 at 12:25 AM Rob Herring <robh@kernel.org> wrote: > > > > On Fri, Jun 25, 2021 at 01:46:08PM +0200, Robert Marko wrote: > > > On Mon, Jun 7, 2021 at 2:33 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > > > > > > Add binding documents for the Delta TN48M CPLD drivers. > > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr> > > > > --- > > > > Changes in v3: > > > > * Include bindings for reset driver > > > > > > > > Changes in v2: > > > > * Implement MFD as a simple I2C MFD > > > > * Add GPIO bindings as separate > > > > > > > > .../bindings/gpio/delta,tn48m-gpio.yaml | 42 +++++++++ > > > > .../bindings/mfd/delta,tn48m-cpld.yaml | 90 +++++++++++++++++++ > > > > .../bindings/reset/delta,tn48m-reset.yaml | 35 ++++++++ > > > > 3 files changed, 167 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > > > create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > > > create mode 100644 Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > > > new file mode 100644 > > > > index 000000000000..aca646aecb12 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml > > > > @@ -0,0 +1,42 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Delta Networks TN48M CPLD GPIO controller > > > > + > > > > +maintainers: > > > > + - Robert Marko <robert.marko@sartura.hr> > > > > + > > > > +description: | > > > > + This module is part of the Delta TN48M multi-function device. For more > > > > + details see ../mfd/delta,tn48m-cpld.yaml. > > > > + > > > > + GPIO controller module provides GPIO-s for the SFP slots. > > > > + It is split into 3 controllers, one output only for the SFP TX disable > > > > + pins, one input only for the SFP present pins and one input only for > > > > + the SFP LOS pins. > > > > + > > > > +properties: > > > > + compatible: > > > > + enum: > > > > + - delta,tn48m-gpio-sfp-tx-disable > > > > + - delta,tn48m-gpio-sfp-present > > > > + - delta,tn48m-gpio-sfp-los > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + "#gpio-cells": > > > > + const: 2 > > > > + > > > > + gpio-controller: true > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - "#gpio-cells" > > > > + - gpio-controller > > > > + > > > > +additionalProperties: false > > > > diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > > > new file mode 100644 > > > > index 000000000000..2c6e2adf73ca > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml > > > > @@ -0,0 +1,90 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Delta Networks TN48M CPLD controller > > > > + > > > > +maintainers: > > > > + - Robert Marko <robert.marko@sartura.hr> > > > > + > > > > +description: | > > > > + Lattice CPLD onboard the TN48M switches is used for system > > > > + management. > > > > + > > > > + It provides information about the hardware model, revision, > > > > + PSU status etc. > > > > + > > > > + It is also being used as a GPIO expander for the SFP slots and > > > > + reset controller for the switch MAC-s and other peripherals. > > > > + > > > > +properties: > > > > + compatible: > > > > + const: delta,tn48m-cpld > > > > + > > > > + reg: > > > > + description: > > > > + I2C device address. > > > > + maxItems: 1 > > > > + > > > > + "#address-cells": > > > > + const: 1 > > > > + > > > > + "#size-cells": > > > > + const: 0 > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + - "#address-cells" > > > > + - "#size-cells" > > > > + > > > > +patternProperties: > > > > + "^gpio(@[0-9a-f]+)?$": > > > > + $ref: ../gpio/delta,tn48m-gpio.yaml > > > > + > > > > + "^reset-controller?$": > > > > + $ref: ../reset/delta,tn48m-reset.yaml > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + i2c { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + cpld@41 { > > > > + compatible = "delta,tn48m-cpld"; > > > > + reg = <0x41>; > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + gpio@31 { > > > > + compatible = "delta,tn48m-gpio-sfp-tx-disable"; > > > > + reg = <0x31>; > > > > + gpio-controller; > > > > + #gpio-cells = <2>; > > > > + }; > > > > + > > > > + gpio@3a { > > > > + compatible = "delta,tn48m-gpio-sfp-present"; > > > > + reg = <0x3a>; > > > > + gpio-controller; > > > > + #gpio-cells = <2>; > > > > + }; > > > > + > > > > + gpio@40 { > > > > + compatible = "delta,tn48m-gpio-sfp-los"; > > > > + reg = <0x40>; > > > > + gpio-controller; > > > > + #gpio-cells = <2>; > > > > + }; > > > > + > > > > + reset-controller { > > > > + compatible = "delta,tn48m-reset"; > > > > + #reset-cells = <1>; > > > > + }; > > > > + }; > > > > + }; > > > > diff --git a/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > > new file mode 100644 > > > > index 000000000000..0e5ee8decc0d > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/reset/delta,tn48m-reset.yaml > > > > @@ -0,0 +1,35 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/reset/delta,tn48m-reset.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Delta Networks TN48M CPLD reset controller > > > > + > > > > +maintainers: > > > > + - Robert Marko <robert.marko@sartura.hr> > > > > + > > > > +description: | > > > > + This module is part of the Delta TN48M multi-function device. For more > > > > + details see ../mfd/delta,tn48m-cpld.yaml. > > > > + > > > > + Reset controller modules provides resets for the following: > > > > + * 88F7040 SoC > > > > + * 88F6820 SoC > > > > + * 98DX3265 switch MAC-s > > > > + * 88E1680 PHY-s > > > > + * 88E1512 PHY > > > > + * PoE PSE controller > > > > + > > > > +properties: > > > > + compatible: > > > > + const: delta,tn48m-reset > > > > + > > > > + "#reset-cells": > > > > + const: 1 > > > > + > > > > +required: > > > > + - compatible > > > > + - "#reset-cells" > > > > + > > > > +additionalProperties: false > > > > -- > > > > 2.31.1 > > > > > > > > > > Are there any issues with the bindings? > > > > Yes. Primarily the GPIO function being part of the compatible. I'm > > surprised Linus W is okay with that. > > I think I already explained this before, having a single compatible > won't work here. > Then there would not be anything to know whether its input or output > only as the pins > have specific purpose. The client side should tell the direction. Are you using the SFP binding?: Documentation/devicetree/bindings/net/sff,sfp.txt Specific purpose IOs are not general purpose IOs. Repeating Linus W here. Maybe his opinion has evolved... If the programming model of each instance is different, then different compatibles are justified. But describe what the difference is, not the connection. > And knowing the capabilites is a requirment of the GPIO regmap driver > and the GPIO > core itself as it exposes that information in a generic manner and > driver like for the > SFP bus use that. Driver details aren't justification for bindings. > Maybe Linus W can chime in here as well. > > > > > > The patch series is depending on this as the rest has been reviewed. > > > > The bindings have been reviewed too, you just didn't like my comments... > > Sorry, I did not pick that up as after my replies there was no further > discussion. > > I am really hoping that we can find a middle ground here and get this > merged as the driver code itself has been revied and ACK-ed. > > Regards, > Robert > > > > Rob > > > > -- > Robert Marko > Staff Embedded Linux Engineer > Sartura Ltd. > Lendavska ulica 16a > 10000 Zagreb, Croatia > Email: robert.marko@sartura.hr > Web: www.sartura.hr >
On Tue, Jul 20, 2021 at 12:59 AM Rob Herring <robh@kernel.org> wrote: > > > > Are there any issues with the bindings? > > > > > > Yes. Primarily the GPIO function being part of the compatible. I'm > > > surprised Linus W is okay with that. > > > > I think I already explained this before, having a single compatible > > won't work here. > > Then there would not be anything to know whether its input or output > > only as the pins > > have specific purpose. > > The client side should tell the direction. Are you using the SFP > binding?: Documentation/devicetree/bindings/net/sff,sfp.txt > > Specific purpose IOs are not general purpose IOs. Repeating Linus W > here. Maybe his opinion has evolved... Nah. I think at one time or two I was convinced to let something special purpose slip through as "GPIO". Typical case: LED control lines that were in practice used for other things, such as controlling motors. Here there is a pin named "SFP TX disable" which is suspicious. Why isn't whatever is now managing SFP just read/write this bit without going through the GPIO abstraction to disable TX? If it is a regmap in Linux then that is fine, just pass the regmap around inside the kernel, OK finished. But really that is an OS detail. If the pin is in practice used for other things, say connected to a LED, I would soften up and accept it as a GPIO compatible. > If the programming model of each instance is different, then different > compatibles are justified. But describe what the difference is, not the > connection. IIRC that is the case as the instances are different. So those differences should be described for each compatible in the bindings. So there is this: > + GPIO controller module provides GPIO-s for the SFP slots. > + It is split into 3 controllers, one output only for the SFP TX disable > + pins, one input only for the SFP present pins and one input only for > + the SFP LOS pins. This should read "the hardware instances are different in such way that the first can only (by hardware restrictions) be used as output..." etc, so that it is crystal clear what this means. But if the lines are special purpose not general purpose, they should not be GPIOs to begin with. Yours, Linus Walleij
On Wed, Jul 21, 2021 at 4:17 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jul 20, 2021 at 12:59 AM Rob Herring <robh@kernel.org> wrote: > > > > > > Are there any issues with the bindings? > > > > > > > > Yes. Primarily the GPIO function being part of the compatible. I'm > > > > surprised Linus W is okay with that. > > > > > > I think I already explained this before, having a single compatible > > > won't work here. > > > Then there would not be anything to know whether its input or output > > > only as the pins > > > have specific purpose. > > > > The client side should tell the direction. Are you using the SFP > > binding?: Documentation/devicetree/bindings/net/sff,sfp.txt > > > > Specific purpose IOs are not general purpose IOs. Repeating Linus W > > here. Maybe his opinion has evolved... > > Nah. I think at one time or two I was convinced to let something > special purpose slip through as "GPIO". > > Typical case: LED control lines that were in practice used for other > things, such as controlling motors. > > Here there is a pin named "SFP TX disable" which is suspicious. > Why isn't whatever is now managing SFP just read/write this bit > without going through the GPIO abstraction to disable TX? Hi Linus, The pins that this driver wants to expose are used for SFP-s only, they are provided by the Lattice CPLD which also does other things. Linux has a generic SFP driver which is used to manage these SFP ports, but it only supports GPIO-s, it has no concept of anything else. Since the driver is fully generic, I have no idea how could one extend it to effectively handle these pins internally, especially since I have more switches that use the CPLD for SFP-s as well, even for 48 ports and 192 pins for them. GPIO regmap works perfectly for this as its generic enough to cover all of these cases. CPLD also provides pins to test the port LED-s per color as well, but I have chosen not to expose them so far. > > If it is a regmap in Linux then that is fine, just pass the regmap > around inside the kernel, OK finished. But really that is an OS > detail. Yes, its regmap but I cant really pass it to the SFP driver as I don't have special driver handling the SFP but rather the generic kernel one. It only knows how to handle GPIO-s. > > If the pin is in practice used for other things, say connected > to a LED, I would soften up and accept it as a GPIO compatible. > > > If the programming model of each instance is different, then different > > compatibles are justified. But describe what the difference is, not the > > connection. > > IIRC that is the case as the instances are different. > > So those differences should be described for each compatible in the > bindings. Ok, makes sense, I will make it clear in the bindings. > > So there is this: > > > + GPIO controller module provides GPIO-s for the SFP slots. > > + It is split into 3 controllers, one output only for the SFP TX disable > > + pins, one input only for the SFP present pins and one input only for > > + the SFP LOS pins. > > This should read "the hardware instances are different in such way > that the first can only (by hardware restrictions) be used as output..." > etc, so that it is crystal clear what this means. Ok, makes sense, I will make it clear in the bindings. > > But if the lines are special purpose not general purpose, they > should not be GPIOs to begin with. > > Yours, > Linus Walleij Looking forward to your reply. Regards, Robert. -- Robert Marko Staff Embedded Linux Engineer Sartura Ltd. Lendavska ulica 16a 10000 Zagreb, Croatia Email: robert.marko@sartura.hr Web: www.sartura.hr
On Tue, Aug 3, 2021 at 9:23 PM Robert Marko <robert.marko@sartura.hr> wrote: > The pins that this driver wants to expose are used for SFP-s only, > they are provided by the Lattice CPLD which also does other things. > > Linux has a generic SFP driver which is used to manage these SFP > ports, but it only supports GPIO-s, it has no concept of anything else. > Since the driver is fully generic, I have no idea how could one extend it > to effectively handle these pins internally, especially since I have more > switches that use the CPLD for SFP-s as well, even for 48 ports and 192 > pins for them. Which file is this driver in so I can look? Maybe it is not a good idea to look for generic code just because it is convenient? I have had this problem before with GPIO, along the lines "lemme just do this dirty thing this one time because it is so convenient for me" (more or less) and the answer is still "no". Can you either augment the driver to handle a regmap with bit indices instead or write a new similar driver for that or refactor it some other way? It is not a simple solution to your problem, but it might be the right solution even if it means some more work. > GPIO regmap works perfectly for this as its generic enough to cover all of > these cases. Yeah but it might be the wrong thing to do even if it is simple to use and works. > CPLD also provides pins to test the port LED-s per color as well, > but I have chosen not to expose them so far. Have you considered Documentation/devicetree/bindings/leds/register-bit-led.txt > > If it is a regmap in Linux then that is fine, just pass the regmap > > around inside the kernel, OK finished. But really that is an OS > > detail. > > Yes, its regmap but I cant really pass it to the SFP driver as I don't have > special driver handling the SFP but rather the generic kernel one. > It only knows how to handle GPIO-s. Of course you have to program it. If I know which driver it is it is easier to provide architecture ideas. Yours, Linus Walleij
On Wed, Aug 11, 2021 at 2:17 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Aug 3, 2021 at 9:23 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > The pins that this driver wants to expose are used for SFP-s only, > > they are provided by the Lattice CPLD which also does other things. > > > > Linux has a generic SFP driver which is used to manage these SFP > > ports, but it only supports GPIO-s, it has no concept of anything else. > > Since the driver is fully generic, I have no idea how could one extend it > > to effectively handle these pins internally, especially since I have more > > switches that use the CPLD for SFP-s as well, even for 48 ports and 192 > > pins for them. > > Which file is this driver in so I can look? Hi Linus, Sorry for the late reply. Sure, here is the generic Linux driver that is used for SFP handling: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/sfp.c?h=v5.14-rc7 > > Maybe it is not a good idea to look for generic code just because > it is convenient? I have had this problem before with GPIO, along > the lines "lemme just do this dirty thing this one time because it > is so convenient for me" (more or less) and the answer is still > "no". > > Can you either augment the driver to handle a regmap with bit indices > instead or write a new similar driver for that or refactor it some other > way? > > It is not a simple solution to your problem, but it might be the right > solution even if it means some more work. I understand your position, believe me, I spend some time looking at what would be the logical way for these switches. But I see no way how could the SFP driver be extended in a generic way that would allow supporting different register layouts when it comes to pins. > > > GPIO regmap works perfectly for this as its generic enough to cover all of > > these cases. > > Yeah but it might be the wrong thing to do even if it is simple > to use and works. > > > CPLD also provides pins to test the port LED-s per color as well, > > but I have chosen not to expose them so far. > > Have you considered > Documentation/devicetree/bindings/leds/register-bit-led.txt Yeah, but unfortunately in this case it wont work as the LED-s are for debugging/test purposes only and you first need to switch the CPLD out of it interpreting the LED state with a BIT flip. Regards, Robert > > > > If it is a regmap in Linux then that is fine, just pass the regmap > > > around inside the kernel, OK finished. But really that is an OS > > > detail. > > > > Yes, its regmap but I cant really pass it to the SFP driver as I don't have > > special driver handling the SFP but rather the generic kernel one. > > It only knows how to handle GPIO-s. > > Of course you have to program it. If I know which driver it > is it is easier to provide architecture ideas. > > Yours, > Linus Walleij -- Robert Marko Staff Embedded Linux Engineer Sartura Ltd. Lendavska ulica 16a 10000 Zagreb, Croatia Email: robert.marko@sartura.hr Web: www.sartura.hr
On Tue, Aug 24, 2021 at 10:03 AM Robert Marko <robert.marko@sartura.hr> wrote: > > On Wed, Aug 11, 2021 at 2:17 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Tue, Aug 3, 2021 at 9:23 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > > > The pins that this driver wants to expose are used for SFP-s only, > > > they are provided by the Lattice CPLD which also does other things. > > > > > > Linux has a generic SFP driver which is used to manage these SFP > > > ports, but it only supports GPIO-s, it has no concept of anything else. > > > Since the driver is fully generic, I have no idea how could one extend it > > > to effectively handle these pins internally, especially since I have more > > > switches that use the CPLD for SFP-s as well, even for 48 ports and 192 > > > pins for them. > > > > Which file is this driver in so I can look? > > Hi Linus, > Sorry for the late reply. > > Sure, here is the generic Linux driver that is used for SFP handling: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/sfp.c?h=v5.14-rc7 > > > > > Maybe it is not a good idea to look for generic code just because > > it is convenient? I have had this problem before with GPIO, along > > the lines "lemme just do this dirty thing this one time because it > > is so convenient for me" (more or less) and the answer is still > > "no". > > > > Can you either augment the driver to handle a regmap with bit indices > > instead or write a new similar driver for that or refactor it some other > > way? > > > > It is not a simple solution to your problem, but it might be the right > > solution even if it means some more work. > > I understand your position, believe me, I spend some time looking at > what would be the logical way for these switches. > But I see no way how could the SFP driver be extended in a generic way > that would allow supporting different register layouts when it comes to pins. > > > > > > GPIO regmap works perfectly for this as its generic enough to cover all of > > > these cases. > > > > Yeah but it might be the wrong thing to do even if it is simple > > to use and works. > > > > > CPLD also provides pins to test the port LED-s per color as well, > > > but I have chosen not to expose them so far. > > > > Have you considered > > Documentation/devicetree/bindings/leds/register-bit-led.txt > > Yeah, but unfortunately in this case it wont work as the LED-s > are for debugging/test purposes only and you first need to switch > the CPLD out of it interpreting the LED state with a BIT flip. > > Regards, > Robert > > > > > > If it is a regmap in Linux then that is fine, just pass the regmap > > > > around inside the kernel, OK finished. But really that is an OS > > > > detail. > > > > > > Yes, its regmap but I cant really pass it to the SFP driver as I don't have > > > special driver handling the SFP but rather the generic kernel one. > > > It only knows how to handle GPIO-s. > > > > Of course you have to program it. If I know which driver it > > is it is easier to provide architecture ideas. > > > > Yours, > > Linus Walleij Linus, can I offer some further explanation? Regards, Robert > > > > -- > Robert Marko > Staff Embedded Linux Engineer > Sartura Ltd. > Lendavska ulica 16a > 10000 Zagreb, Croatia > Email: robert.marko@sartura.hr > Web: www.sartura.hr -- Robert Marko Staff Embedded Linux Engineer Sartura Ltd. Lendavska ulica 16a 10000 Zagreb, Croatia Email: robert.marko@sartura.hr Web: www.sartura.hr
On Tue, Sep 7, 2021 at 11:02 PM Robert Marko <robert.marko@sartura.hr> wrote: > > On Tue, Aug 24, 2021 at 10:03 AM Robert Marko <robert.marko@sartura.hr> wrote: > > > > On Wed, Aug 11, 2021 at 2:17 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > On Tue, Aug 3, 2021 at 9:23 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > > > > > The pins that this driver wants to expose are used for SFP-s only, > > > > they are provided by the Lattice CPLD which also does other things. > > > > > > > > Linux has a generic SFP driver which is used to manage these SFP > > > > ports, but it only supports GPIO-s, it has no concept of anything else. > > > > Since the driver is fully generic, I have no idea how could one extend it > > > > to effectively handle these pins internally, especially since I have more > > > > switches that use the CPLD for SFP-s as well, even for 48 ports and 192 > > > > pins for them. > > > > > > Which file is this driver in so I can look? > > > > Hi Linus, > > Sorry for the late reply. > > > > Sure, here is the generic Linux driver that is used for SFP handling: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/sfp.c?h=v5.14-rc7 > > > > > > > > Maybe it is not a good idea to look for generic code just because > > > it is convenient? I have had this problem before with GPIO, along > > > the lines "lemme just do this dirty thing this one time because it > > > is so convenient for me" (more or less) and the answer is still > > > "no". > > > > > > Can you either augment the driver to handle a regmap with bit indices > > > instead or write a new similar driver for that or refactor it some other > > > way? > > > > > > It is not a simple solution to your problem, but it might be the right > > > solution even if it means some more work. > > > > I understand your position, believe me, I spend some time looking at > > what would be the logical way for these switches. > > But I see no way how could the SFP driver be extended in a generic way > > that would allow supporting different register layouts when it comes to pins. > > > > > > > > > GPIO regmap works perfectly for this as its generic enough to cover all of > > > > these cases. > > > > > > Yeah but it might be the wrong thing to do even if it is simple > > > to use and works. > > > > > > > CPLD also provides pins to test the port LED-s per color as well, > > > > but I have chosen not to expose them so far. > > > > > > Have you considered > > > Documentation/devicetree/bindings/leds/register-bit-led.txt > > > > Yeah, but unfortunately in this case it wont work as the LED-s > > are for debugging/test purposes only and you first need to switch > > the CPLD out of it interpreting the LED state with a BIT flip. > > > > Regards, > > Robert > > > > > > > > If it is a regmap in Linux then that is fine, just pass the regmap > > > > > around inside the kernel, OK finished. But really that is an OS > > > > > detail. > > > > > > > > Yes, its regmap but I cant really pass it to the SFP driver as I don't have > > > > special driver handling the SFP but rather the generic kernel one. > > > > It only knows how to handle GPIO-s. > > > > > > Of course you have to program it. If I know which driver it > > > is it is easier to provide architecture ideas. > > > > > > Yours, > > > Linus Walleij > > Linus, > > can I offer some further explanation? > > Regards, > Robert Hi Linus, I would really like to move forward with this somehow. I have multiple switches depending on the outcome of this series. Regards, Robert > > > > > > > > -- > > Robert Marko > > Staff Embedded Linux Engineer > > Sartura Ltd. > > Lendavska ulica 16a > > 10000 Zagreb, Croatia > > Email: robert.marko@sartura.hr > > Web: www.sartura.hr > > > > -- > Robert Marko > Staff Embedded Linux Engineer > Sartura Ltd. > Lendavska ulica 16a > 10000 Zagreb, Croatia > Email: robert.marko@sartura.hr > Web: www.sartura.hr -- Robert Marko Staff Embedded Linux Engineer Sartura Ltd. Lendavska ulica 16a 10000 Zagreb, Croatia Email: robert.marko@sartura.hr Web: www.sartura.hr
Hi Robert, sorry for slow reply, I am a bit busy. On Tue, Aug 24, 2021 at 10:03 AM Robert Marko <robert.marko@sartura.hr> wrote: > On Wed, Aug 11, 2021 at 2:17 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Tue, Aug 3, 2021 at 9:23 PM Robert Marko <robert.marko@sartura.hr> wrote: > > > > > The pins that this driver wants to expose are used for SFP-s only, > > > they are provided by the Lattice CPLD which also does other things. > > > > > > Linux has a generic SFP driver which is used to manage these SFP > > > ports, but it only supports GPIO-s, it has no concept of anything else. > > > Since the driver is fully generic, I have no idea how could one extend it > > > to effectively handle these pins internally, especially since I have more > > > switches that use the CPLD for SFP-s as well, even for 48 ports and 192 > > > pins for them. > > > > Which file is this driver in so I can look? > > Hi Linus, > Sorry for the late reply. > > Sure, here is the generic Linux driver that is used for SFP handling: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/sfp.c?h=v5.14-rc7 So this has this: enum { GPIO_MODDEF0, GPIO_LOS, GPIO_TX_FAULT, GPIO_TX_DISABLE, GPIO_RATE_SELECT, GPIO_MAX, SFP_F_PRESENT = BIT(GPIO_MODDEF0), SFP_F_LOS = BIT(GPIO_LOS), SFP_F_TX_FAULT = BIT(GPIO_TX_FAULT), SFP_F_TX_DISABLE = BIT(GPIO_TX_DISABLE), SFP_F_RATE_SELECT = BIT(GPIO_RATE_SELECT), SFP_E_INSERT = 0, SFP_E_REMOVE, This does not look general purpose to me at all? It's just some hardware engineer that thougt "GPIO" was a nice thing to call this. > > Maybe it is not a good idea to look for generic code just because > > it is convenient? I have had this problem before with GPIO, along > > the lines "lemme just do this dirty thing this one time because it > > is so convenient for me" (more or less) and the answer is still > > "no". > > > > Can you either augment the driver to handle a regmap with bit indices > > instead or write a new similar driver for that or refactor it some other > > way? > > > > It is not a simple solution to your problem, but it might be the right > > solution even if it means some more work. > > I understand your position, believe me, I spend some time looking at > what would be the logical way for these switches. > But I see no way how could the SFP driver be extended in a generic way > that would allow supporting different register layouts when it comes to pins. Why do you think you have to use the GPIO abstraction and bindings? Just invent something that satisfy your needs, the bindings are just strings. Why does the consumer have to use the GPIO binding? They can just use phandle named anything. Some "sfp-foo-resource = <&...> or so. For example I created this: Documentation/devicetree/bindings/firmware/intel,ixp4xx-network-processing-engine.yaml It's handling out a resource using a phandle. Nothing different than GPIO, regulator, clock etc. Just invent something for SFP? Yours, Linus Walleij
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index b74efa469e90..733c2f9adb15 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -297,6 +297,16 @@ config MFD_ASIC3 This driver supports the ASIC3 multifunction chip found on many PDAs (mainly iPAQ and HTC based ones) +config MFD_TN48M_CPLD + tristate "Delta Networks TN48M switch CPLD driver" + depends on I2C + select MFD_SIMPLE_MFD_I2C + help + Select this option to enable support for Delta Networks TN48M switch + CPLD. It consists of reset and GPIO drivers. CPLD provides GPIOS-s + for the SFP slots as well as power supply related information. + SFP support depends on the GPIO driver being selected. + config PMIC_DA903X bool "Dialog Semiconductor DA9030/DA9034 PMIC Support" depends on I2C=y diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c index 87f684cff9a1..af8e91781417 100644 --- a/drivers/mfd/simple-mfd-i2c.c +++ b/drivers/mfd/simple-mfd-i2c.c @@ -39,6 +39,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c) static const struct of_device_id simple_mfd_i2c_of_match[] = { { .compatible = "kontron,sl28cpld" }, + { .compatible = "delta,tn48m-cpld" }, {} }; MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);
Delta TN48M switches have a Lattice CPLD that serves multiple purposes including being a GPIO expander. So, lets use the simple I2C MFD driver to provide the MFD core. Also add a virtual symbol which pulls in the simple-mfd-i2c driver and provide a common symbol on which the subdevice drivers can depend on. Signed-off-by: Robert Marko <robert.marko@sartura.hr> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org> --- Changes in v2: * Drop the custom MFD driver and header * Use simple I2C MFD driver drivers/mfd/Kconfig | 10 ++++++++++ drivers/mfd/simple-mfd-i2c.c | 1 + 2 files changed, 11 insertions(+)