Il 15/07/22 18:29, Daniel Thompson ha scritto:

On Fri, Jul 15, 2022 at 02:38:45PM +0200, AngeloGioacchino Del Regno wrote:

quoted

Il 15/07/22 13:26, ChiaEn Wu ha scritto:

quoted

From: ChiaEn Wu <redacted>

MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger
with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight
driver, display bias voltage supply, one general purpose LDO, and the
USB Type-C & PD controller complies with the latest USB Type-C and PD
standards.

This adds support for MediaTek MT6370 Backlight driver. It's commonly used
to drive the display WLED. There are 4 channels inside, and each channel
supports up to 30mA of current capability with 2048 current steps in
exponential or linear mapping curves.

Signed-off-by: ChiaEn Wu <redacted>

Hello ChiaEn,

I propose to move this one to drivers/leds (or drivers/pwm) and, instead of
registering a backlight device, register a PWM device.

This way you will be able to reuse the generic backlight-pwm driver, as you'd
be feeding the PWM device exposed by this driver to the generic one: this will
most importantly make it easy to chain it with MTK_DISP_PWM (mtk-pwm-disp)
with a devicetree that looks like...

Out of interest, does MT6370 have the same structure for backlights as the prior
systems using mtk-pwm-disp or was mtk-pwm-disp simply a normal(-ish) PWM
that relied on something on the board for all the constant current
driver hardware?

As per my understanding, mtk-pwm-disp is chained to other multimedia features of
the display block of MediaTek SoCs, such as the AAL (adaptive ambient light),
CABC (content adaptive backlight control) etc, other than being a normal(ish)
PWM... that's the reason of my request.

Moreover, in the end, this PMIC's backlight controller is just a "fancy" PWM
controller, with OCP/OVP.

quoted

	pwmleds-disp {
		compatible = "pwm-leds";

		disp_led: disp-pwm {
			label = "backlight-pwm";
			pwms = <&pwm0 0 500000>;
			max-brightness = <1024>;
		};
	};

	backlight_lcd0: backlight {
		compatible = "led-backlight";
		leds = <&disp_led>, <&pmic_bl_led>;
		default-brightness-level = <300>;
	};

I think this proposal has to start with the devicetree bindings rather
than the driver. Instead I think the question is: does this proposal
result in DT bindings that better describe the underlying hardware?

 From how I understand it - yes: we have a fancy PWM (&pwm0) that we use
to control display backlight (backlight-pwm)...

Obviously, here we're not talking about OLEDs, but LCDs, where the backlight
is made of multiple strings of WhiteLED (effectively, a "pwm-leds" controlled
"led-backlight").

Using PWM will also allow for a little more fine-grained board specific
configuration, as I think that this PMIC (and/or variants of it) will be
used in completely different form factors: I think that's going to be both
smartphones and tablets/laptops... and I want to avoid vendor properties
to configure the PWM part in a somehow different way.

This device has lots of backlight centric features (OCP, OVP, single
control with multiple outputs, exponential curves, etc) and its not
clear where they would fit into the "PWM" bindings.

For OCP and OVP, the only bindings that fit would be regulators, but that's
not a regulator... and that's about it - I don't really have arguments for
that.

What I really want to see here is usage of "generic" drivers like led_bl
and/or pwm_bl as to get some "standardization" around with all the benefits
that this carries.

Come to think of it I'm also a little worried also about the whole linear
versus exponential curve thing since I thought LED drivers were required
to use exponential curves.

That probably depends on how the controller interprets the data, I guess,
but I agree with you on this thought.

Regards,
Angelo