Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
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>
Subject: RE: [PATCH v3 2/2] backlight: mp3309c: Add support for MPS MP3309C
Thread-Topic: [PATCH v3 2/2] backlight: mp3309c: Add support for MPS MP3309C
Thread-Index: AQHZ76uOJLAmrZru10GD/iMoRWqxtLAtFlmAgArGkbA=
Date:   Tue, 3 Oct 2023 09:45:27 +0000
Message-ID: <DU2PR01MB803463A09B0213D2072F598DF9C4A@DU2PR01MB8034.eurprd01.prod.exchangelabs.com>
References: <20230925122609.78849-1-f.suligoi@asem.it>
 <20230925122609.78849-2-f.suligoi@asem.it> <20230926131039.GC4356@aspen.lan>
In-Reply-To: <20230926131039.GC4356@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?Liys9P61dqRZ0go2NaTn60/SxyJLMVoQN+fKa1yzSr9NEycpfZQ4qp42HmWE?=
 =?us-ascii?Q?bzGGs4H40rxiN1lowpdhkh6YVtjZ3GuzLn8zbTea2f4cYkWoOxlCIVhkgcOP?=
 =?us-ascii?Q?NS7LJLcZFmxofUqlIuHKhFS/HNrQxnIlZyr/fbcvUWjQwwDMv9VZJ9zWprng?=
 =?us-ascii?Q?1X1RTzVqqWTlWVjiVNqbnnQ2IFv+VWlUVJwc6Ps5kYL1dZZJuHueTAniTO2Q?=
 =?us-ascii?Q?as9Z710SoegX6flMc7o/1KBjcPGUcOrp7reXx6QMjRfC4ArX8xKSuvO/Y93/?=
 =?us-ascii?Q?EDRvYX0pGS18dmMzRoijvTK0YTFMhLmNlJsQ283T2ZTO1j8jtjAOhmM9lLE8?=
 =?us-ascii?Q?hqf93JQaR27HKk5B5DUxZyb0+iSw3ajGxzatlqyNnTknjQU7tgL8Tw7IgeGj?=
 =?us-ascii?Q?R0z/B/ES7A9giVId2vEBiGHvXGbHNGl+oi3rGTGXYN+b2pB09ZdSrItQS4XH?=
 =?us-ascii?Q?WzzloP3N1J2IG33gal7mFPD3NVQuT7FGROm/2Qjw0TAK4LV/CLiGHyVYxlo0?=
 =?us-ascii?Q?XalTmnTjjv9SotQWnPGkIrZUq4qZxj0BAv8zli30WupYGdQmgbuJpIG6R158?=
 =?us-ascii?Q?AX7mzoLC26FIAJ/bnee//vUdtBCC2UGD241pAChev0v6YmjmdZeti60HJK8D?=
 =?us-ascii?Q?Gp01If7hiQCqxnTxGCQNeP1lAMz8QNzc8slttiZAlJX/ntcqmYEhMzqjIbOu?=
 =?us-ascii?Q?GDjUNrlqluq08F5/jBWS6lqCojsInetT6gsV6WWkcMDxF3evMT/YlN93dNzm?=
 =?us-ascii?Q?E2r8cN/kGZk5ieOVxFY1kb0uZqF+eJTf5SOjxBQ7Qriqd0cu9d37SAqBvk/c?=
 =?us-ascii?Q?82nfclizTbxkshuGaj3eFQBjYaIpD/QRnJHcapy3D6Yx1628UhvwW0MnKpLn?=
 =?us-ascii?Q?hBXCxBKjcTH7Gwf8IRhdzybvIUj8qW2XLiISuwId4UCTNfsXLmUkXnMpnxS5?=
 =?us-ascii?Q?6g1BiDAC6LPWJ5K5SSWjWUSYu86F2IEWRmwsSmjeFQYadjpXXCd6mKhesmRb?=
 =?us-ascii?Q?TuJyg70gV0HbvFJrUsHqcV6VfnRBN+ICdc15m1AStkbpQmgELfuOq79aR/Kg?=
 =?us-ascii?Q?udx2DK8kqAgNTLBd4Fh9b8lYLqNYK1qg/DhUxNqEL5UpEKtiIgAOrUJPCJdb?=
 =?us-ascii?Q?5qZRAVTYijwNQiymYMLgDM7F0NaqFWbzJkoy1r1kP+NUUKAB/LZcoQEXHtkj?=
 =?us-ascii?Q?RTiIG47ahJYZ41rEjjwAWCwW8CJoG9S8ntnolnqBH7PrqC1ArCDs07HW2bfh?=
 =?us-ascii?Q?3kLIG0qPABcfBxiMh33KAnPVwsGu2gRABjyyukiJ9pWReD7Tys5mbgu+wGdO?=
 =?us-ascii?Q?L/tPi+MImyntoMCIqkOLIARYIWy8NA8nxuRWs1JzzRDS2foD+TdeR/LV09WI?=
 =?us-ascii?Q?MyHAducF4TbiZdjBub3fGfaga5igRUbYOZNTnHYE9iY+JVGvB61/hWzINa/p?=
 =?us-ascii?Q?qHORjbxWSMbhNyW6aofQm77rvME/irATvuWWQNBoOA/mH6ObjmWjoX65Jc9T?=
 =?us-ascii?Q?P5G/CL2zd4xAEKyz/+umfxm++NT0dX0KPEPpubsaUw0gJDucnvCt52Eu4zOw?=
 =?us-ascii?Q?UDgA07dZTEGgp3pQa18=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: 8d909c22-1ac2-4d0d-8f57-08dbc3f57955
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Oct 2023 09:45:27.1536
 (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: fAgZ+scHh83Ke9Id93EhUEIB0Jz+bWPUGCEHVtii8Lk0L2vU+MljoJmjMcSUdiOWPWvfPaV0kCXiASYR5oDVhA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR01MB8830
X-Spam-Status: No, score=-0.9 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 groat.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 (groat.vger.email [0.0.0.0]); Tue, 03 Oct 2023 02:46:05 -0700 (PDT)

Hi Daniel,

...

> > +static int mp3309c_bl_update_status(struct backlight_device *bl) {
> > +	struct mp3309c_chip *chip =3D bl_get_data(bl);
> > +	int brightness =3D backlight_get_brightness(bl);
> > +	struct pwm_state pwmstate;
> > +	unsigned int analog_val, bits_val;
> > +	int i, ret;
> > +
> > +	if (chip->pdata->dimming_mode =3D=3D DIMMING_PWM) {
> > +		/*
> > +		 * PWM dimming mode
> > +		 */
> > +		pwm_get_state(chip->pwmd, &pwmstate);
> > +		pwm_set_relative_duty_cycle(&pwmstate, brightness,
> > +					    chip->pdata->max_brightness);
> > +		pwmstate.enabled =3D true;
> > +		ret =3D pwm_apply_state(chip->pwmd, &pwmstate);
> > +		if (ret)
> > +			return ret;
> > +
> > +		switch (chip->pdata->status) {
> > +		case FIRST_POWER_ON:
> > +		case BACKLIGHT_OFF:
> > +			/*
> > +			 * After 20ms of low pwm signal level, the chip turns
> > +			   off automatically. In this case, before enabling the
> > +			   chip again, we must wait about 10ms for pwm signal
> to
> > +			   stabilize.
> > +			 */
> > +			if (brightness > 0) {
> > +				msleep(10);
> > +				mp3309c_enable_device(chip);
> > +				chip->pdata->status =3D BACKLIGHT_ON;
> > +			} else {
> > +				chip->pdata->status =3D BACKLIGHT_OFF;
> > +			}
> > +			break;
> > +		case BACKLIGHT_ON:
> > +			if (brightness =3D=3D 0)
> > +				chip->pdata->status =3D BACKLIGHT_OFF;
> > +			break;
> > +		}
> > +	} else {
> > +		/*
> > +		 * Analog dimming (by I2C command) dimming mode
> > +		 *
> > +		 * The first time, before setting brightness, we must enable the
> > +		 * device
> > +		 */
> > +		if (chip->pdata->status =3D=3D FIRST_POWER_ON)
> > +			mp3309c_enable_device(chip);
> > +
> > +		/*
> > +		 * Dimming mode I2C command
> > +		 *
> > +		 * The 5 bits of the dimming analog value D4..D0 is allocated
> > +		 * in the I2C register #0, in the following way:
> > +		 *
> > +		 *     +--+--+--+--+--+--+--+--+
> > +		 *     |EN|D0|D1|D2|D3|D4|XX|XX|
> > +		 *     +--+--+--+--+--+--+--+--+
> > +		 */
> > +		analog_val =3D DIV_ROUND_UP(ANALOG_MAX_VAL *
> brightness,
> > +					  chip->pdata->max_brightness);
>=20
> Sorry to only notice after sharing a Reviewed-by:[1] but...
>=20
> Scaling brightness here isn't right. When running in I2C dimming mode the=
n
> max_brightness *must* be 31 or lower, meaning the value in brightness can
> be applied directly to the hardware without scaling.

ok, right; max brightness is 31, fixed

>=20
> Quoting the DT binding docs about how max-brightness should be
> interpretted:
>=20
>   Normally the maximum brightness is determined by the hardware and this
>   property is not required. This property is used to put a software
>   limit on the brightness apart from what the driver says, as it could
>   happen that a LED can be made so bright that it gets damaged or causes
>   damage due to restrictions in a specific system, such as mounting
>   conditions.
>=20

ok

>=20
> Daniel.
>=20
>=20
> [1] I remember checking if this code could overflow the field but I was
>     so distracted by that I ended up missing the obvious!

Thanks and best regards,
Flavio