Re: [PATCH v5 2/2] backlight: ktz8866: Add support for Kinetic KTZ8866 backlight

[Jianhua Lu]

On Wed, Jan 18, 2023 at 12:08:01PM +0000, Daniel Thompson wrote:
> On Tue, Jan 17, 2023 at 11:44:08PM +0800, Jianhua Lu wrote:
> > Add support for Kinetic KTZ8866 backlight, which is used in
> > Xiaomi tablet, Mi Pad 5 series. This driver lightly based on
> > downstream implementation [1].
> > [1] https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/elish-r-oss/drivers/video/backlight/ktz8866.c
> >
> > Signed-off-by: Jianhua Lu <lujianhua000@xxxxxxxxx>
> > ---
> > Changes in v2:
> >   - Add missing staitc modifier to ktz8866_write function.
> >
> > Changes in v3:
> >   - Add 2022 to Copyright line.
> >   - Sort headers.
> >   - Remove meaningless comment.
> >   - Use definitions instead of hardcoding.
> >   - Add missing maintainer info.
> >
> > Changes in v4:
> >   - Change 2022 to 2023.
> >   - Remove useless macro and enum.
> >   - Describe settings by devicetree.
> >   - Move header file to C file.
> > Changes in v5:
> >   - Change "2023" to "2022, 2023" in Copyright line.
> >   - Set scale property for backlight.
> >
> >  MAINTAINERS                       |   6 +
> >  drivers/video/backlight/Kconfig   |   8 ++
> >  drivers/video/backlight/Makefile  |   1 +
> >  drivers/video/backlight/ktz8866.c | 201 ++++++++++++++++++++++++++++++
> >  4 files changed, 216 insertions(+)
> >  create mode 100644 drivers/video/backlight/ktz8866.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 42fc47c6edfd..2084e74e1b58 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11674,6 +11674,12 @@ M:	John Hawley <warthog9@xxxxxxxxxxxxxx>
> >  S:	Maintained
> >  F:	tools/testing/ktest
> >
> > +KTZ8866 BACKLIGHT DRIVER
> > +M:	Jianhua Lu <lujianhua000@xxxxxxxxx>
> > +S:	Maintained
> > +F:	Documentation/devicetree/bindings/leds/backlight/kinetic,ktz8866.yaml
> > +F:	drivers/video/backlight/ktz8866.c
> > +
> >  L3MDEV
> >  M:	David Ahern <dsahern@xxxxxxxxxx>
> >  L:	netdev@xxxxxxxxxxxxxxx
> > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> > index 936ba1e4d35e..2845fd7e33ad 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> > @@ -190,6 +190,14 @@ config BACKLIGHT_KTD253
> >  	  which is a 1-wire GPIO-controlled backlight found in some mobile
> >  	  phones.
> >
> > +config BACKLIGHT_KTZ8866
> > +	tristate "Backlight Driver for Kinetic KTZ8866"
> > +	depends on I2C
> > +	select REGMAP_I2C
> > +	help
> > +		Say Y to enabled the backlight driver for the Kinetic KTZ8866
> > +		found in Xiaomi Mi Pad 5 series.
> 
> s/enabled/enable/ (and sorry for spotting this one so late)
Get it.
> 
> 
> > +++ b/drivers/video/backlight/ktz8866.c
> > @@ -0,0 +1,201 @@
> > [...]
> > +static void ktz8866_init(struct ktz8866 *ktz)
> > +{
> > +	unsigned int val;
> > +
> > +	if(of_property_read_u32(ktz->client->dev.of_node, "current-num-sinks", &val))
> > +		ktz8866_write(ktz, BL_EN, BIT(val) - 1);
> > +	else
> > +		/* Enable all 6 current sinks if the number of current sinks isn't specifed. */
> > +		ktz8866_write(ktz, BL_EN, BIT(6) - 1);
> > +
> > +	if(of_property_read_u32(ktz->client->dev.of_node, "current-ramping-time-us", &val)) {
> > +		if(val <= 128) {
> > +			ktz8866_write(ktz, BL_CFG2, BIT(7) | (ilog2(val) << 3) | PWM_HYST);
> > +		} else {
> > +			ktz8866_write(ktz, BL_CFG2, BIT(7) | ((5 + val / 64) << 3) | PWM_HYST);
> > +		}
> 
> This code is interpreting current-ramping-time-us as milliseconds rather
> than microseconds!
> 
> I know I used microseconds in the example I proposed in the feedback for
> v4 DT bindings but "something like" means I am merely providing an
> example (mostly I was intending to show that the units should be
> included both in the property name *and* description).
> 
> It is up to you whether you fix the mismatch by changing the DT bindings
> document to current-ramping-time-ms or change this code to accept
> values in microseconds.
This is my mistake, sorry.
> 
> 
> > +	}
> > +
> > +	if(of_property_read_u32(ktz->client->dev.of_node, "led-ramping-time-us", &val)) {
> > +		unsigned int ramp_off_time = ilog2(val) + 1;
> > +		unsigned int ramp_on_time = ramp_off_time << 4;
> > +		ktz8866_write(ktz, BL_DIMMING, ramp_on_time | ramp_off_time);
> > +	}
> 
> Similarly, this code has not adopted the units specified in the
> bindings documentation.
> 
> In this case 0 would map to 512us so if you decided to use milliseconds
> you will need to add comment in the description saying that 0 will map
> to 512us because the hardware cannot ramp faster than this!
Get it, thank you very much.
> 
> 
> Daniel.