[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y8fhIVRbgEok29t1@aspen.lan>
Date: Wed, 18 Jan 2023 12:08:01 +0000
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Jianhua Lu <lujianhua000@...il.com>
Cc: Lee Jones <lee@...nel.org>, Jingoo Han <jingoohan1@...il.com>,
Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Helge Deller <deller@....de>, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-leds@...r.kernel.org,
devicetree@...r.kernel.org, linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v5 2/2] backlight: ktz8866: Add support for Kinetic
KTZ8866 backlight
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@...il.com>
> ---
> 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@...lescrag.net>
> S: Maintained
> F: tools/testing/ktest
>
> +KTZ8866 BACKLIGHT DRIVER
> +M: Jianhua Lu <lujianhua000@...il.com>
> +S: Maintained
> +F: Documentation/devicetree/bindings/leds/backlight/kinetic,ktz8866.yaml
> +F: drivers/video/backlight/ktz8866.c
> +
> L3MDEV
> M: David Ahern <dsahern@...nel.org>
> L: netdev@...r.kernel.org
> 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)
> +++ 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.
> + }
> +
> + 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!
Daniel.
Powered by blists - more mailing lists