Re: [PATCH v2 2/6] dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type

On Mon 20 Nov 2023 at 10:55, neil.armstrong@linaro.org wrote:

Hi Jerome,

On 20/11/2023 10:18, Jerome Brunet wrote:

quoted

On Mon 20 Nov 2023 at 09:27, Neil Armstrong [off-list ref]
wrote:

quoted

Hi Rob,

On 19/11/2023 17:05, Rob Herring wrote:

quoted

On Fri, 17 Nov 2023 13:59:12 +0100, Jerome Brunet wrote:

quoted

Add a new compatible for the pwm found in the meson8 to sm1 Amlogic SoCs.

The previous clock bindings for these SoCs described the driver and not the
HW itself. The clock provided was used to set the parent of the input clock
mux among the possible parents hard-coded in the driver.

The new bindings allows to describe the actual clock inputs of the PWM in
DT, like most bindings do, instead of relying of hard-coded data.

The new bindings make the old one deprecated.

There is enough experience on this HW to know that the PWM is exactly the
same all the supported SoCs. There is no need for a per-SoC compatible.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
   .../devicetree/bindings/pwm/pwm-amlogic.yaml  | 36 +++++++++++++++++--
   1 file changed, 34 insertions(+), 2 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

I'm puzzled, isn't it recommended to have a per-soc compatible now ?

I have specifically addressed this matter in the description,
haven't I ? What good would it do in this case ?

Yes you did but I was asked for the last year+ that all new compatible
should be soc specific (while imprecise, in our care soc family should be ok),
with a possible semi-generic callback with an IP version or a first soc
implementing the IP.

quoted

Plus the definition of a SoC is very vague. One could argue that
the content of the list bellow are vaguely defined families. Should we
add meson8b, gxl, gxm, sm1 ? ... or even the actual SoC reference ?
This list gets huge for no reason.

I think in our case soc family is reasonable since they share same silicon
design.

quoted

We know all existing PWM of this type are the same. We have been using
them for years. It is not a new support we know nothing about.

quoted

I thought something like:
- items:
     - enum:
         - amlogic,gxbb-pwm
         - amlogic,axg-pwm
         - amlogic,g12a-pwm
     - const: amlogic,pwm-v1

I'm not sure I understand what you are suggesting here.
Adding a "amlogic,pwm-v1" for the obsolete compatible ? No amlogic DT
has that and I'm working to remove this type, so I don't get the point.

quoted

should be preferred instead of a single amlogic,meson8-pwm-v2 ?

This is named after the first SoC supporting the type.
Naming it amlogic,pwm-v2 would feel weird with the s4 coming after.
Plus the doc specifically advise against this type of names.

The -v2 refers to a pure software/dt implementation versioning and not
an HW version, so I'm puzzled and I requires DT maintainers advice here.

Yes meson8b is the first "known" platform, even if I'm pretty sure meson6 has

This is not my point. I picked this name because I have to pick a
specific device based one. Not because it is actually the first or
not. I don't see a problem with meson6 being compatible with
meson8-pwm-v2, if that ever comes along.

I think the binding here satisfy the rule that it should be specific,
and the intent that goes with it:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/writing-bindings.rst?h=v6.7-rc2#n42

the same pwm architecture, this is why "amlogic,pwm-v1" as fallback seems more
reasonable and s4 and later pwm could use the "amlogic,pwm-v2"
fallback.

That is not how understand this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/writing-bindings.rst?h=v6.7-rc2#n82

Neil

quoted

quoted

Neil