Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
From:   Flavio Suligoi <f.suligoi@asem.it>
To:     Daniel Thompson <daniel.thompson@linaro.org>
CC:     Lee Jones <lee@kernel.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>,
        Rob Herring <robh@kernel.org>
Subject: RE: [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C
Thread-Topic: [PATCH v3 1/2] dt-bindings: backlight: Add MPS MP3309C
Thread-Index: AQHZ76uO1Fb/EPNT/k67j1TrFzld37AtE7aAgAmVcBCAAT5MAIAAXgAggAGFVUCAAW0hAIABTgZg
Date:   Fri, 6 Oct 2023 08:56:15 +0000
Message-ID: <AM8PR01MB804533DE0885FFC6A46A7C97F9C9A@AM8PR01MB8045.eurprd01.prod.exchangelabs.com>
References: <20230925122609.78849-1-f.suligoi@asem.it>
 <20230926130113.GB4356@aspen.lan>
 <DU2PR01MB8034F448F6E66BAC3823CDB1F9C4A@DU2PR01MB8034.eurprd01.prod.exchangelabs.com>
 <20231003102138.GB69443@aspen.lan>
 <DU2PR01MB80349344FB8CB1415BFA917DF9C4A@DU2PR01MB8034.eurprd01.prod.exchangelabs.com>
 <DU2PR01MB8034F17A9C81FE198CCAC5B7F9CBA@DU2PR01MB8034.eurprd01.prod.exchangelabs.com>
 <20231005125824.GC4874@aspen.lan>
In-Reply-To: <20231005125824.GC4874@aspen.lan>
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?jGLun+9wF+XQCHgv3sy+xDHcRCT2IHGP6yBCC6EjK7HRETPpIHDz1B3TkNsd?=
 =?us-ascii?Q?zaRBoASUtDsBWieYQyrlVpzt04O7gXih2M4QRs+2A/FDRurS3cLvoTSkHtng?=
 =?us-ascii?Q?0YR0RJa8p88AGqlQisz2e6yOna58i7q0HVY1UtS30pmXY4JXFAe+CHLWa5EF?=
 =?us-ascii?Q?7De2QL9drDiwPldtshf12cB8S0YMRJEjP07UmGY+ONPlA1aG+Go3XXY0eNgE?=
 =?us-ascii?Q?jrHXYtHs9lnDIwvUnhqzyl/BHIvqyyuQQZjWZExXBchBnwDGu0s+3pNaggbU?=
 =?us-ascii?Q?fSNq7N5yF+v8bJTHId4u0HXDtcJG15RORFthdfBompZ6nQ1gHSw2VGeCOdYu?=
 =?us-ascii?Q?iaAoVFE47yxYi/NDuTf3QLsfwQ/w9eWUbfI2P4GKWyeyh2FGn/hz5NCeVJLI?=
 =?us-ascii?Q?t2UaNk3kUj8vNXpcC+2R7FUu6OXM8U/+dbK4ZUS38A2DhjBl9dwZ/hI2//jC?=
 =?us-ascii?Q?RH9aTMrhDVrE57PJiX65B+O1Ant0bN5hXklQQ4eOBESSuh03395uz+k4UxMA?=
 =?us-ascii?Q?UHbUxWmb99Cqh9N5+vZs7nt7Yz4RyL3CXVIguCJKbI2PnSmDLuCjHJdVf2yK?=
 =?us-ascii?Q?o3dmDo8TbYe8mvIIXVGRa6B7m3H4OsHVRYVMhNpmqAdmauqYy/RAcwx5nNaB?=
 =?us-ascii?Q?t7ohUsdt/syrMAv81x6R8han57Pn0PQhjNOAKjWoc8pguO3OCCiSF6dv/TfF?=
 =?us-ascii?Q?8OEflh5upMVsVfqgVky46s4n2ee5QjOJkusiAOoQ1JOT21xyds2VVorgEsNz?=
 =?us-ascii?Q?uZZ0kkKkmLuNIh2T+yNJoUEI3HTRtZI1rhEh7XJMWldcF5/Ym9v8Zi+kFOT4?=
 =?us-ascii?Q?92r8L6Rg8ut1MMCJB9QBWGsyYGHbaqFHZ30HNPbrzddeGApDKoSwOdJL1Mbi?=
 =?us-ascii?Q?psPkiYS7SaJB7bzsLCiFu2T2FkgDO3JwCujSHxND+HzOcYufFD0fj0X19rvS?=
 =?us-ascii?Q?U15Pxvw6mj2V6JXwmazBU39LaI3xkvtJfb/opLwDDM7s1yJvnFrQMDcx0oPp?=
 =?us-ascii?Q?l+FeFAzW+/BNhXx+VTXIXqHdbUxpO/v0McBVfqQor8nBZzsdn/gE9kaK00Hn?=
 =?us-ascii?Q?6mgdtMEcgaIDrmWCecASslxOgOvo/RzGb0QUGZu7rukKcoHsEmy0ipbjEz7y?=
 =?us-ascii?Q?WcHJDt8/7UdMNl87jAOp0w9Lz8Q2jcmmD2AQVqesqyrFAv6mKcMMXiSIri0v?=
 =?us-ascii?Q?VMKKB5/SUYYgWLTQpB/1tBmsO2iOkvolwXRf2ONxpZxuzKMRPMbNqoJpgHWS?=
 =?us-ascii?Q?tfF5Y1NC/sNv6uoWzWimai6kz8q59of3wdcUTEHSPCNijZlmpjuE2nC7xwSt?=
 =?us-ascii?Q?7zSn1Fn6udkTAtuoj6rra2bGmgShRRkhFpV8pjVk/fl1d1bTsgv+/60FIGJC?=
 =?us-ascii?Q?YQ6/0CdGPmsz5lJz6MLmDL/6NFkHiC3N6YRY4Cny3hJnuI7sVvlEM9SJpsqI?=
 =?us-ascii?Q?vSU+5dYYg/S08O3+qz5anKIsErtnvW4sPD6dTXM8Ungud5szILRxfdBwPF/8?=
 =?us-ascii?Q?pTo9+79CciwZKm68nMY2hXe/bcQvi5X9IpqJgOB8T0QUpcdE/KBg1W4LgyOb?=
 =?us-ascii?Q?tFB0clkwDhdh3ny0vg4=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: AM8PR01MB8045.eurprd01.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5dc2ef25-7521-4c00-8352-08dbc64a190c
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Oct 2023 08:56:15.2788
 (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: nd98O/bV4pkKsT3PD643C4NwNHYaJfbBLg0heNIxUU/uv4I3EENeZVgwUey7PuZeVBEbYvP1LAM6J0T69jGv9A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PR3PR01MB7162
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 fry.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 (fry.vger.email [0.0.0.0]); Fri, 06 Oct 2023 01:56:31 -0700 (PDT)

HI Daniel,

...
> > ...
> > > ...
> > > > > > > +required:
> > > > > > > +  - compatible
> > > > > > > +  - reg
> > > > > > > +  - max-brightness
> > > > > >
> > > > > > Why is this mandatory?
> > > > > >
> > > > > > There's no point in setting max-brightness when running in I2C
> > > > > > mode
> > > > > > (max- brightness should default to 31 in that case).
> > > > > >
> > > > > >
> > > > > > > +  - default-brightness
> > > > > >
> > > > > > Again. I'm not clear why this needs to be mandatory.
> > > > > >
> > > > > >
> > > > >
> > > > > Ok, you are right, I'll remove max-brightness and
> > > > > default-brightness from required properties list. I think to
> > > > > change these properties, for the pwm dimming, into a clearer:
> > > > >
> > > > > - brightness-levels (uint32)
> > > > > - default-brightness-levels (uint32).
> > > > >
> > > > > For example:
> > > > >
> > > > >   brightness-levels:
> > > > >     description:
> > > > >       Number of brightness levels. The actual brightness
> > > > >       level (PWM duty cycle) will be interpolated from 0 to this =
value.
> > > > >       0 means a  0% duty cycle (darkest/off), while the
> > > > > brightness-levels
> > > > represents
> > > > >       a 100% duty cycle (brightest).
> > > > >     $ref: /schemas/types.yaml#/definitions/uint32
> > > > >
> > > > >   default-brightness-level:
> > > > >     description:
> > > > >       The default brightness level (from 0 to brightness-levels)
> > > > >     $ref: /schemas/types.yaml#/definitions/uint32
> > > > >
> > > > > Example:
> > > > > brightness-levels =3D <10>;
> > > > > default-brightness-level =3D <6>;
> > > > >
> > > > > What do you think about this solution?
> > > >
> > > > If you want to introduce a brightness-levels property then I would
> > > > expect it to be defined with the same meaning as pwm-backlight
> > > > (it's not relevant to the bindings but ideally it would be
> > > > implemented by refactoring and reusing the code from pwm_bl.c).
> > >
> > > ok, I'll use the brightness-levels property as used in pwm-backlight
> > >
> > > >
> > > > Same with default-brightness-level although I'm not sure why one
> > > > wouldn't just use default-brightness for new bindings (doesn't
> > > > default-brightness-level simply do exactly the same thing as
> > > > default-
> > > brightness).
> > >
> > > ok for default-brightness instead of default-brightness-level
> >
> > Just a question: default-brightness-level is the index into the brightn=
ess-
> levels array.
> > But, if I use default-brightness instead of default-brightness-level,
> > should I consider default-brightness also as an index into brightness-l=
evels
> array?
>=20
> Yes.
>=20
>=20
> > Or, in this case, have the default-brightness to be equal to one of
> > the values inside the brightness-levels array?
>=20
> When there is a brightness array (and there is no interpolation) then it =
is
> indexed by brightness. The values in the array are not brightness (e.g. t=
he
> controlable value describing the output of the hardware). The values in t=
he
> table are merely the PWM duty cycle...

ok

>=20
> Main difference is, with a correct table the brightness can use an approp=
riate
> logarithmic power scale (which matches how humans perceive
> brightness) instead of the linear scale provided by the PWM duty cycle.
>=20
>=20
> Daniel.
>=20
>=20
> Brightness and "index into the brightness-levels array" should be one and=
 the
> same thing

ok, I'll use default-brightness, thanks for the explanations!

> >
> > >
> > > >
> > > >
> > > > Daniel.
> > >
> > > Thanks an best regards,
> > > Flavio
> >
> > Thanks,
> >
> > Flavio

Flavio