| Message ID | 20241018-en7581-pinctrl-v8-3-b676b966a1d1@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add mfd, pinctrl and pwm support to EN7581 SoC | expand |
Il 18/10/24 15:19, Lorenzo Bianconi ha scritto: > Introduce device-tree binding documentation for Airoha EN7581 pwm > controller. > > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > .../devicetree/bindings/pwm/airoha,en7581-pwm.yaml | 61 ++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..fb68c10b037b840a571a2ceee57f13cbae78da66 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Airoha EN7581 PWM Controller > + > +maintainers: > + - Lorenzo Bianconi <lorenzo@kernel.org> > + > +allOf: > + - $ref: pwm.yaml# > + > +properties: > + compatible: > + const: airoha,en7581-pwm > + > + "#pwm-cells": > + const: 3 > + > + airoha,74hc595-mode: > + description: Set the PWM to handle attached shift register chip 74HC595. > + I think that you can either indent your description or you just don't; this means that - if you don't - you shouldn't add extra blank lines. In any case, that's left for the bindings maintainer to check - as for the missing new properties I complained about in the previous version, you can get my Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cheers, Angelo > + With this disabled, PWM assume a 74HC164 chip attached. > + > + The main difference between the 2 chip is the presence of a latch pin > + that needs to triggered to apply the configuration and PWM needs to > + account for that. > + type: boolean > + > + airoha,sipo-clock-divisor: > + description: Declare Shift Register chip clock divisor (clock source is > + from SoC APB Clock) > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 32 > + enum: [4, 8, 16, 32] > + > + airoha,sipo-clock-delay: > + description: Declare Serial GPIO Clock delay. > + This can be needed to permit the attached shift register to correctly > + setup and apply settings. Value must NOT be greater than > + "airoha,sipo-clock-divisor" / 2 > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 1 > + minimum: 1 > + maximum: 16 > + > +required: > + - compatible > + - "#pwm-cells" > + > +additionalProperties: false > + > +examples: > + - | > + pwm { > + compatible = "airoha,en7581-pwm"; > + > + #pwm-cells = <3>; > + }; >
On Fri, Oct 18, 2024 at 03:19:04PM +0200, Lorenzo Bianconi wrote: > Introduce device-tree binding documentation for Airoha EN7581 pwm > controller. > > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > .../devicetree/bindings/pwm/airoha,en7581-pwm.yaml | 61 ++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..fb68c10b037b840a571a2ceee57f13cbae78da66 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Airoha EN7581 PWM Controller > + > +maintainers: > + - Lorenzo Bianconi <lorenzo@kernel.org> > + > +allOf: > + - $ref: pwm.yaml# > + > +properties: > + compatible: > + const: airoha,en7581-pwm > + > + "#pwm-cells": > + const: 3 > + > + airoha,74hc595-mode: > + description: Set the PWM to handle attached shift register chip 74HC595. > + > + With this disabled, PWM assume a 74HC164 chip attached. > + > + The main difference between the 2 chip is the presence of a latch pin > + that needs to triggered to apply the configuration and PWM needs to > + account for that. > + type: boolean > + > + airoha,sipo-clock-divisor: > + description: Declare Shift Register chip clock divisor (clock source is > + from SoC APB Clock) Where is the clock source defined? You can specify the PWM frequency in PWM cells and should be able to get the APB Clock frequency. Then you can calculate the divider. > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 32 > + enum: [4, 8, 16, 32] > + > + airoha,sipo-clock-delay: > + description: Declare Serial GPIO Clock delay. > + This can be needed to permit the attached shift register to correctly > + setup and apply settings. Value must NOT be greater than > + "airoha,sipo-clock-divisor" / 2 > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 1 > + minimum: 1 > + maximum: 16 > + > +required: > + - compatible > + - "#pwm-cells" > + > +additionalProperties: false > + > +examples: > + - | > + pwm { > + compatible = "airoha,en7581-pwm"; > + > + #pwm-cells = <3>; > + }; > > -- > 2.47.0 >
On Mon, Oct 21, 2024 at 02:00:53PM -0500, Rob Herring wrote: > On Fri, Oct 18, 2024 at 03:19:04PM +0200, Lorenzo Bianconi wrote: > > Introduce device-tree binding documentation for Airoha EN7581 pwm > > controller. > > > > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > .../devicetree/bindings/pwm/airoha,en7581-pwm.yaml | 61 ++++++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > > new file mode 100644 > > index 0000000000000000000000000000000000000000..fb68c10b037b840a571a2ceee57f13cbae78da66 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > > @@ -0,0 +1,61 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Airoha EN7581 PWM Controller > > + > > +maintainers: > > + - Lorenzo Bianconi <lorenzo@kernel.org> > > + > > +allOf: > > + - $ref: pwm.yaml# > > + > > +properties: > > + compatible: > > + const: airoha,en7581-pwm > > + > > + "#pwm-cells": > > + const: 3 > > + > > + airoha,74hc595-mode: > > + description: Set the PWM to handle attached shift register chip 74HC595. > > + > > + With this disabled, PWM assume a 74HC164 chip attached. > > + > > + The main difference between the 2 chip is the presence of a latch pin > > + that needs to triggered to apply the configuration and PWM needs to > > + account for that. > > + type: boolean > > + > > + airoha,sipo-clock-divisor: > > + description: Declare Shift Register chip clock divisor (clock source is > > + from SoC APB Clock) > > Where is the clock source defined? > > You can specify the PWM frequency in PWM cells and should be able to get > the APB Clock frequency. Then you can calculate the divider. > Hi Rob, this property is related to the Shift Register chip and is not related to the clock of the PWM. It's really to configure the clock that will be feed to Shift Register chip if for whatever reason one OEM mount a different kind (but still register compatible) and requires to run at higher clock rate. We can consider hardcoding it if really needed but considering the case with 2 different kind of shift register supported, I assume configuring this might be needed on some corner case Devices. For the clock we are not 100% but we might have an idea of what is the source, but still it will be just referenced and enabled in the driver (it's always enabled). Hope I can get some hint by you on how to proceed. Is it ok with: - Defining the attached clock - Keep the property ? > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + default: 32 > > + enum: [4, 8, 16, 32] > > + > > + airoha,sipo-clock-delay: > > + description: Declare Serial GPIO Clock delay. > > + This can be needed to permit the attached shift register to correctly > > + setup and apply settings. Value must NOT be greater than > > + "airoha,sipo-clock-divisor" / 2 > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + default: 1 > > + minimum: 1 > > + maximum: 16 > > + > > +required: > > + - compatible > > + - "#pwm-cells" > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + pwm { > > + compatible = "airoha,en7581-pwm"; > > + > > + #pwm-cells = <3>; > > + }; > > > > -- > > 2.47.0 > >
On 21/10/2024 21:00, Rob Herring wrote: >> + airoha,sipo-clock-divisor: >> + description: Declare Shift Register chip clock divisor (clock source is >> + from SoC APB Clock) > Where is the clock source defined? > > You can specify the PWM frequency in PWM cells and should be able to get > the APB Clock frequency. Then you can calculate the divider. Hi this controls SRCLK for shift registers. https://www.ti.com/lit/ds/symlink/sn74hc595.pdf page 8 has the limits for this chip. I am fine with removal of this option. The main use of this pwm driver is for leds and for that purpose the default is ok. But I will try to get a value for the clock source, I guess with that value the option can stay in. MvH Benjamin Larsson
On 21/10/2024 21:00, Rob Herring wrote: >> + airoha,sipo-clock-divisor: >> + description: Declare Shift Register chip clock divisor (clock source is >> + from SoC APB Clock) > Where is the clock source defined? > By measurement the clock was found to be 125MHz. MvH Benjamin Larsson
On Tue, Oct 22, 2024 at 10:02:05PM +0200, Benjamin Larsson wrote: > On 21/10/2024 21:00, Rob Herring wrote: > > > + airoha,sipo-clock-divisor: > > > + description: Declare Shift Register chip clock divisor (clock source is > > > + from SoC APB Clock) > > Where is the clock source defined? > > > By measurement the clock was found to be 125MHz. What I mean is the clock input should be a 'clocks' property. Assuming this is a clock input to the PWM which I'm not so sure about given the other replies. Rob
On Tue, Oct 22, 2024 at 04:08:58PM -0500, Rob Herring wrote: > On Tue, Oct 22, 2024 at 10:02:05PM +0200, Benjamin Larsson wrote: > > On 21/10/2024 21:00, Rob Herring wrote: > > > > + airoha,sipo-clock-divisor: > > > > + description: Declare Shift Register chip clock divisor (clock source is > > > > + from SoC APB Clock) > > > Where is the clock source defined? > > > > > By measurement the clock was found to be 125MHz. > > What I mean is the clock input should be a 'clocks' property. Assuming > this is a clock input to the PWM which I'm not so sure about given the > other replies. > Yep it's not, we are just dropping this property and hardcoding it to a sensible value in the driver. Sorry for the confusion and thanks for the clarification.
On Tue, Oct 22, 2024 at 06:06:06PM +0200, Christian Marangi wrote: > On Mon, Oct 21, 2024 at 02:00:53PM -0500, Rob Herring wrote: > > On Fri, Oct 18, 2024 at 03:19:04PM +0200, Lorenzo Bianconi wrote: > > > Introduce device-tree binding documentation for Airoha EN7581 pwm > > > controller. > > > > > > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > --- > > > .../devicetree/bindings/pwm/airoha,en7581-pwm.yaml | 61 ++++++++++++++++++++++ > > > 1 file changed, 61 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > > > new file mode 100644 > > > index 0000000000000000000000000000000000000000..fb68c10b037b840a571a2ceee57f13cbae78da66 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > > > @@ -0,0 +1,61 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Airoha EN7581 PWM Controller > > > + > > > +maintainers: > > > + - Lorenzo Bianconi <lorenzo@kernel.org> > > > + > > > +allOf: > > > + - $ref: pwm.yaml# > > > + > > > +properties: > > > + compatible: > > > + const: airoha,en7581-pwm > > > + > > > + "#pwm-cells": > > > + const: 3 > > > + > > > + airoha,74hc595-mode: > > > + description: Set the PWM to handle attached shift register chip 74HC595. > > > + > > > + With this disabled, PWM assume a 74HC164 chip attached. > > > + > > > + The main difference between the 2 chip is the presence of a latch pin > > > + that needs to triggered to apply the configuration and PWM needs to > > > + account for that. > > > + type: boolean > > > + > > > + airoha,sipo-clock-divisor: > > > + description: Declare Shift Register chip clock divisor (clock source is > > > + from SoC APB Clock) > > > > Where is the clock source defined? > > > > You can specify the PWM frequency in PWM cells and should be able to get > > the APB Clock frequency. Then you can calculate the divider. > > > > Hi Rob, > > this property is related to the Shift Register chip and is not related > to the clock of the PWM. > > It's really to configure the clock that will be feed to Shift Register > chip if for whatever reason one OEM mount a different kind (but still > register compatible) and requires to run at higher clock rate. > > We can consider hardcoding it if really needed but considering the case > with 2 different kind of shift register supported, I assume configuring > this might be needed on some corner case Devices. > > For the clock we are not 100% but we might have an idea of what is the > source, but still it will be just referenced and enabled in the driver > (it's always enabled). > > Hope I can get some hint by you on how to proceed. > > Is it ok with: > > - Defining the attached clock > - Keep the property > > ? So how is the PWM involved? I'm going to need a picture. If this external shift register chip can be attached to any PWM and clock providers, then perhaps it needs to be its own node with a 'pwms' property and clock source. I would suggest you go back to the version without these properties and that I already reviewed, then discuss adding them separately. Rob
On 22/10/2024 23:08, Rob Herring wrote: > On Tue, Oct 22, 2024 at 10:02:05PM +0200, Benjamin Larsson wrote: >> On 21/10/2024 21:00, Rob Herring wrote: >>>> + airoha,sipo-clock-divisor: >>>> + description: Declare Shift Register chip clock divisor (clock source is >>>> + from SoC APB Clock) >>> Where is the clock source defined? >>> >> By measurement the clock was found to be 125MHz. > What I mean is the clock input should be a 'clocks' property. Assuming > this is a clock input to the PWM which I'm not so sure about given the > other replies. > > Rob Hi, the SoC has 8 pwm generators. The pwm signals from the generators can be muxed out almost everywhere. One place is on the pins of a shift register chip. This setting configures the clock signal this shift register chip is fed by the SoC. The main scenario for the shift register is to drive leds. For that the default value is perfectly fine and the property can be dropped. MvH Benjamin Larsson
On 22/10/2024 23:17, Rob Herring wrote: > So how is the PWM involved? I'm going to need a picture. > > If this external shift register chip can be attached to any PWM and > clock providers, then perhaps it needs to be its own node with a 'pwms' > property and clock source. > > I would suggest you go back to the version without these properties > and that I already reviewed, then discuss adding them separately. > > Rob Hi, to answer your question. The shift register is not attached to any PWM output, there is a specific pin function (mux) that the shift register needs to be connected to for the (pwm capable) gpio hardware to be able to drive it. If that is done the shift register pins can output pwm signals. So if you are low on gpio pins for leds you can connect a shift register and serialize the state of leds connected to the shift register. You can even connect several shift registers chips in a chain with a total of 17 outputs. The kernel already has support to drive shift register leds but with pwm you can do fancy stuff like dimming and color mixing. MvH Benjamin Larsson
diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml new file mode 100644 index 0000000000000000000000000000000000000000..fb68c10b037b840a571a2ceee57f13cbae78da66 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Airoha EN7581 PWM Controller + +maintainers: + - Lorenzo Bianconi <lorenzo@kernel.org> + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + const: airoha,en7581-pwm + + "#pwm-cells": + const: 3 + + airoha,74hc595-mode: + description: Set the PWM to handle attached shift register chip 74HC595. + + With this disabled, PWM assume a 74HC164 chip attached. + + The main difference between the 2 chip is the presence of a latch pin + that needs to triggered to apply the configuration and PWM needs to + account for that. + type: boolean + + airoha,sipo-clock-divisor: + description: Declare Shift Register chip clock divisor (clock source is + from SoC APB Clock) + $ref: /schemas/types.yaml#/definitions/uint32 + default: 32 + enum: [4, 8, 16, 32] + + airoha,sipo-clock-delay: + description: Declare Serial GPIO Clock delay. + This can be needed to permit the attached shift register to correctly + setup and apply settings. Value must NOT be greater than + "airoha,sipo-clock-divisor" / 2 + $ref: /schemas/types.yaml#/definitions/uint32 + default: 1 + minimum: 1 + maximum: 16 + +required: + - compatible + - "#pwm-cells" + +additionalProperties: false + +examples: + - | + pwm { + compatible = "airoha,en7581-pwm"; + + #pwm-cells = <3>; + };