Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   Flavio Suligoi <f.suligoi@asem.it>
To:     Conor Dooley <conor@kernel.org>
CC:     Lee Jones <lee@kernel.org>,
        Daniel Thompson <daniel.thompson@linaro.org>,
        Jingoo Han <jingoohan1@gmail.com>,
        Helge Deller <deller@gmx.de>, Pavel Machek <pavel@ucw.cz>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-leds@vger.kernel.org" <linux-leds@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-fbdev@vger.kernel.org" <linux-fbdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/1] dt-bindings: backlight: mp3309c: remove two required
 properties
Thread-Topic: [PATCH 1/1] dt-bindings: backlight: mp3309c: remove two required
 properties
Thread-Index: AQHaA10IhwE5ZkDArkS4RoPMFlMBm7BS1g+AgARFCACAAHpeAIAA+0AQ
Date:   Tue, 24 Oct 2023 07:53:38 +0000
Message-ID: <DU2PR01MB8034CF8EE4358B9446809AA2F9DFA@DU2PR01MB8034.eurprd01.prod.exchangelabs.com>
References: <20231020135434.2598578-1-f.suligoi@asem.it>
 <20231020135434.2598578-2-f.suligoi@asem.it>
 <20231020-moonrise-senate-86d0edb2d404@spud>
 <DU2PR01MB803498DFD93E82DD3947D72DF9D8A@DU2PR01MB8034.eurprd01.prod.exchangelabs.com>
 <20231023-anybody-silver-4548023f8f26@spud>
In-Reply-To: <20231023-anybody-silver-4548023f8f26@spud>
Accept-Language: it-IT, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?eTdYAe98Ptx3BfIQCc5GgZBJ0ElOdhCrWJ9HGFE+wdGBUvUe9ll6LjYNRBax?=
 =?us-ascii?Q?iYqObZlGpaa8L9OeBcbc419TBnEkO5upi34lsCDQXEx6McrFz/TFil0lcYue?=
 =?us-ascii?Q?WT8UUg7M80fx+/hII7c+divAK3ln23TuWdN068rgKwPGqY+0LLeHZWVrRa6a?=
 =?us-ascii?Q?7x3bccuE5T6S8MUnt+XEgcoq/0/poI8eTSYCYRop8aIdzq8+5V0CmF1BpGrs?=
 =?us-ascii?Q?zrFsUVII99nf3imAG0wZz2PDOrGLN7V+WS89VEjgoWM5uUrGutMhRY1dScqS?=
 =?us-ascii?Q?M4H7tyn8A9ypHkG8SBq0Iz1kWkx4OYG7tNHTQ1qHHMNX9I6uRgGRFt2z64N0?=
 =?us-ascii?Q?Toq/2QmHegrI160EVkATu/gyv4f5N/vPMfBMxOg0nq5EIC9t6oDl9MJumJu5?=
 =?us-ascii?Q?aIZJ2jkpAIlQg3Cdp9J1ioJxSlL+LuRqUpOndvaJeq8q2VEHN2xsHu5NpHCO?=
 =?us-ascii?Q?CdjIJnsw+gywtrFEcdJIQldb89jQWCv2QbAiaZB3fjV7GJGnbj32g8wTOtGk?=
 =?us-ascii?Q?RjcD7n9TAYKDFoYIXFz/6GTLkaRlI3Imj9fBZUpwmoe8+00YSbmum4NKa+Uj?=
 =?us-ascii?Q?ycoxXWCauF6cPd+G1CFc3Y23l+iFGZGCkmRAmrJ5/GLmq7TWYKGTV15OOSba?=
 =?us-ascii?Q?PtUaJHGCxjFy+mPfZkYC0FEyc4b6xy1KqvgMDibSus00zBE/YepS0piCp40J?=
 =?us-ascii?Q?aIZNlb14XO1zcEuDoCCtu/RhaM1p/WvKhmQUe1tBjTz2gw4ij+kZvpgGwTHt?=
 =?us-ascii?Q?/xI1xWjFynX5Rc3aAJYB9y57sZrnRT0w4CGGweA/YF/r4FuOWwmg38i8KHJf?=
 =?us-ascii?Q?Pwhv45fMqb38f8QpzqfTYHub0J3fZl1WdJKahck/PsJwn+B10Hr6VyP7YkFc?=
 =?us-ascii?Q?rO5CeDqePgDzSHsKMEulv8aRedI8vXEW0pVa5Z3y4iRJ1w0ckCuNDFwLbqzx?=
 =?us-ascii?Q?EeIwz3ooOSAP2edgU1/CZ4YlhGV05Ezi43gRVxXIL2At0KGUIrJIFz9m+BKm?=
 =?us-ascii?Q?LBWK55MnBPwo4shVN/afRIVT0xkbR55NHE4vcBrjgvA5M8h5uEE8VXxlDvii?=
 =?us-ascii?Q?zEBZcVr3i5JtGn5Y+n29pBKlCUSWWuuukCS4AvA1hyfMcwN/4vp0QVabXupM?=
 =?us-ascii?Q?1o7X++eKwzXG46D8eYWFioVhS0RCDuoahjzcMRSYZG7O/GObtgIHFC6JuUHF?=
 =?us-ascii?Q?3+NRKqfGpWQQYWxTLXKQL0GtoIOXcImTsb/iBPBFQuFJN3lGhHrXBni26znw?=
 =?us-ascii?Q?08KPtbn1N7T6/w4hyBTQmsNo+OvPXfEdbfc36S5yGBd7NdYHg0zwt/5aVx5m?=
 =?us-ascii?Q?cuGPxIA38gD4pAvrSfoarTL/d+JfuTSmdPXS9fqkyo0DnJ4hFnOVTGuLlIQq?=
 =?us-ascii?Q?PqdJHnbfxn31lmpf6f6JJDDEA0LKV2ud++wGkNZIZhyEuzSGeJPFo41S06r2?=
 =?us-ascii?Q?lpueSYeZmM4QY0zsrUXMp8KjmK/jWe24YbShRNsIHWR5mg7KehAtLnXBmKqD?=
 =?us-ascii?Q?XOGz3WDoTPm9HHmcUrlajGik89M7QSJZfMTb+02sXOn2k+m6PwVKyiBkJV/q?=
 =?us-ascii?Q?gIBJs6M7F6a8wxps9Do=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: asem.it
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DU2PR01MB8034.eurprd01.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 61f1a778-2b52-4e1e-a847-08dbd4665584
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Oct 2023 07:53:38.9415
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: d0a766c6-7992-4344-a4a2-a467a7bb1ed2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Zn7Ihj5jUvFsPcsKbU7uKFAhPS3VFhYMJ8qWNIS34BkQ++RuFBLZV4k2jVZaCriKZwHuxGNxZ2o/c2bZMLb1gA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU5PR01MB10437
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Tue, 24 Oct 2023 00:54:01 -0700 (PDT)

Hi Conor,

...

> On Mon, Oct 23, 2023 at 09:28:03AM +0000, Flavio Suligoi wrote:
> > > On Fri, Oct 20, 2023 at 03:54:33PM +0200, Flavio Suligoi wrote:
> > > > The two properties:
> > > >
> > > > - max-brightness
> > > > - default brightness
> > > >
> > > > are not really required, so they can be removed from the "required"
> > > > section.
> > >
> > > Why are they not required? You need to provide an explanation.
> >
> > The "max-brightness" is not more used now in the driver (I used it in
> > the first version of the driver).
>=20
> If it is not used any more, what happens when someone passes an old
> devicetree to the kernel, that contains max-brightness, but not any of yo=
ur
> new properties?

This is not a problem, because the device driver has not yet been included =
in any kernel.
My patch for the device driver is still being analyzed by the maintainers.
Only this dt-binding yaml file is already included in the "for-backlight-ne=
xt" branch
of the "backlight" kernel repository.
At the moment, this driver is used only in a i.MX8MM board produced in my c=
ompany,
under my full control. No other developer is using it now.

> > The "default-brightness", if omitted in the DT, is managed by the
> > device driver, using a default value. This depends on the dimming mode
> used:
>=20
> For default-brightness, has here always been support in the driver for th=
e
> property being omitted, or is this newly added?

In the first version of the driver this property was a "required property",
but nobody has used this driver before, so this should be not a problem.

>=20
> > - for the "analog mode", via I2C commands, this value is fixed by
> > hardware (=3D31)
> > - while in case of pwm mode the default used is the last value of the
> >   brightness-levels array.
> >
> > Also the brightness-levels array is not required; if it is omitted,
> > the driver uses a default array of 0..255 and the "default-brightness" =
is the
> last one, which is "255".
>=20
> Firstly, this is the sort of rationale that needs to be put into your com=
mit
> messages, rather than bullet pointed lists of what you have done.

You are absolutely right, I'll include these details in the next commit mes=
sage.

>=20
> Secondly, what about other operating systems etc, do any of those support
> this platform and depend on presence of these properties?

I used this backlight driver in our i.MX8MM board only, with Linux only.

>=20
> >
> > > > Other changes:
> > > >
> > > > - improve the backlight working mode description, in the "descripti=
on"
> > > >   section
> > >
> > > > - update the example, removing the "max-brightness" and introducing
> the
> > > >   "brightess-levels" property
> > >
> > > Why is this more useful?
> >
> > I introduced the "brightness-levels" instead of "max-brightness" for
> > homogeneity, since the "analog mode" dimming has a brightness-levels ar=
ray
> fixed by hardware (0..31).
> > In this way also the "pwm" mode can use the same concepts of array of
> levels.
>=20
> What I would like is an explanation in the commit message as to why the
> revised example is more helpful than the existing (and
> must-remain-valid) one.

As said before, no one may have ever used this device driver,
so I would leave only this new version of the example.

>=20
> Cheers,
> Conor.

Thanks for your help and best regards,
Flavio.

>=20
> > >
> > > >
> > > > Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> > > > ---
> > > >  .../bindings/leds/backlight/mps,mp3309c.yaml           | 10 ++++--=
----
> > > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git
> > > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > > index 4191e33626f5..527a37368ed7 100644
> > > > ---
> > > a/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > > +++
> > > b/Documentation/devicetree/bindings/leds/backlight/mps,mp3309c.yaml
> > > > @@ -14,8 +14,8 @@ description: |
> > > >    programmable switching frequency to optimize efficiency.
> > > >    It supports two different dimming modes:
> > > >
> > > > -  - analog mode, via I2C commands (default)
> > > > -  - PWM controlled mode.
> > > > +  - analog mode, via I2C commands, as default mode (32 dimming
> > > > + levels)
> > > > +  - PWM controlled mode (optional)
> > > >
> > > >    The datasheet is available at:
> > > >    https://www.monolithicpower.com/en/mp3309c.html
> > > > @@ -50,8 +50,6 @@ properties:
> > > >  required:
> > > >    - compatible
> > > >    - reg
> > > > -  - max-brightness
> > > > -  - default-brightness
> > > >
> > > >  unevaluatedProperties: false
> > > >
> > > > @@ -66,8 +64,8 @@ examples:
> > > >              compatible =3D "mps,mp3309c";
> > > >              reg =3D <0x17>;
> > > >              pwms =3D <&pwm1 0 3333333 0>; /* 300 Hz --> (1/f) * 1*=
10^9 */
> > > > -            max-brightness =3D <100>;
> > > > -            default-brightness =3D <80>;
> > > > +            brightness-levels =3D <0 4 8 16 32 64 128 255>;
> > > > +            default-brightness =3D <6>;
> > > >              mps,overvoltage-protection-microvolt =3D <24000000>;
> > > >          };
> > > >      };
> > > > --
> > > > 2.34.1
> > > >