Re: [PATCH v8 3/6] dt-bindings: pwm: airoha: Add EN7581 pwm

[Christian Marangi <ansuelsmth@gmail.com>]

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

-- 
	Ansuel