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

[Rob Herring <robh@kernel.org>]

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