[PATCH 0/8] soc: amlogic: switch bindings to yaml and adjust some dtbs's

[Uwe Kleine-König]

Hello,

On Tue, Jan 24, 2023 at 08:16:45AM +0100, Neil Armstrong wrote:
> Le 23/01/2023 à 22:22, Heiner Kallweit a écrit :
> > At first adjust some existing dtbs's so that they pass dtbs_check
> > after switching bindings to yaml.
> 
> Thanks for this patchset, but please drop patches 1, 3 & 4, and take
> in account the existing compatible usage in your new bindings like
> I did in my conversion patchset.
> 
> While we did remove some bad compatibles we introduced a few years ago,
> now the GXBB, GXL & GXM are now stable a aew LTS releases now and
> a few other projects uses them as-is (U-Boot, BSDs, ...) so changing
> the compatibles isn't an option anymore... and we can't know which
> one they use and how the implementation behaves we must document
> the existing usage without breaking any potential users (including linux).

I only looked into patch #1, and I support dropping it for stronger
reasons than not breaking things which maybe started to rely on the
existing contents.

In patch #1 you write:

| amlogic,meson-gx-pwm isn't a valid compatible string, so remove it.
| See drivers/pwm/pwm-meson.c.
| 
| Signed-off-by: Heiner Kallweit <hkallweit1 at gmail.com>
| ---
|  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 8 ++++----
|  1 file changed, 4 insertions(+), 4 deletions(-)
| 
| diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
| index a79a35e84..75d35dcfe 100644
| --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
| +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
| @@ -328,14 +328,14 @@ i2c_A: i2c at 8500 {
|                         };
| 
|                         pwm_ab: pwm at 8550 {
| -                               compatible = "amlogic,meson-gx-pwm", "amlogic,meson-gxbb-pwm";
| +                               compatible = "amlogic,meson-gxbb-pwm";

There are two issues:

a) drivers/pwm/pwm-meson.c isn't the reference. The driver doesn't
   justify which compatibles should be used. You should refer to the
   binding document instead.

b) Having the SoC name as an additional compatible (i.e. the status quo
   before your patch) is an advantage. While it doesn't hurt (apart from
   making the dtb a tad bigger) it makes it possible to adapt the driver
   if in the future someone discovers that the PWM component on GX is a
   tad different from the GXBB one. In that case you can add a check in
   the driver à la 

   	if (of_device_is_compatible(np, amlogic,meson-gx-pwm))
		do_the_special_gx_handling()

   without having to adapt the device trees then (or use some ugly
   code that somehow detects if it's running on GX).

So the driver not handling amlogic,meson-gx-pwm today is fine. I expect
the fix to be: Include that compatible in the binding.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20230126/85878aa0/attachment.sig>