Re: [PATCH v3 3/3] backlight: Add Kinetic KTD2801 backlight support
From: Daniel Thompson <hidden>
Date: 2024-01-22 10:28:09
Also in:
dri-devel, linux-devicetree, linux-leds, lkml, phone-devel
On Sat, Jan 20, 2024 at 10:26:45PM +0100, Duje Mihanović wrote:
KTD2801 is a LED backlight driver IC found in samsung,coreprimevelte. The brightness can be set using PWM or the ExpressWire protocol. Add support for the KTD2801. Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
As Linus W. said, this is looking really nice now. Thanks! Just a couple of nits below.
quoted hunk ↗ jump to hunk
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig index 51387b1ef012..585a5a713759 100644 --- a/drivers/video/backlight/Kconfig +++ b/drivers/video/backlight/Kconfig@@ -183,6 +183,14 @@ config BACKLIGHT_KTD253 which is a 1-wire GPIO-controlled backlight found in some mobile phones. +config BACKLIGHT_KTD2801 + tristate "Backlight Driver for Kinetic KTD2801" + depends on GPIOLIB || COMPILE_TEST
As patch 1 feedback, seems odd for the client to be responsible for this. It should be managed in LEDS_EXPRESSWIRE.
quoted hunk ↗ jump to hunk
+ select LEDS_EXPRESSWIRE + help + Say Y to enable the backlight driver for the Kinetic KTD2801 1-wire + GPIO-controlled backlight found in Samsung Galaxy Core Prime VE LTE. + config BACKLIGHT_KTZ8866 tristate "Backlight Driver for Kinetic KTZ8866" depends on I2Cdiff --git a/drivers/video/backlight/ktd2801-backlight.c b/drivers/video/backlight/ktd2801-backlight.c new file mode 100644 index 000000000000..7b9d1a93aa71 --- /dev/null +++ b/drivers/video/backlight/ktd2801-backlight.c@@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Datasheet: + * https://www.kinet-ic.com/uploads/web/KTD2801/KTD2801-04b.pdf + */ +#include <linux/backlight.h> +#include <linux/bits.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/leds-expresswire.h> +#include <linux/platform_device.h> +#include <linux/property.h> + +/* These values have been extracted from Samsung's driver. */ +#define KTD2801_EXPRESSWIRE_DETECT_DELAY_US 150 +#define KTD2801_EXPRESSWIRE_DETECT_US 270 +#define KTD2801_SHORT_BITSET_US 5 +#define KTD2801_LONG_BITSET_US (3 * KTD2801_SHORT_BITSET_US) +#define KTD2801_DATA_START_US 5 +#define KTD2801_END_OF_DATA_LOW_US 10 +#define KTD2801_END_OF_DATA_HIGH_US 350 +#define KTD2801_PWR_DOWN_DELAY_US 2600
These are a little pointless now. They are all single use constants and have little documentary value. The lack of documentary value is because, for example, KTD2801_EXPRESSWIRE_DETECT_DELAY_US, is assigned to a structure field called detect_delay_us. Likewise I doubt that explicitly stating that long_bitset_us is 3x bigger than short_bitset_us is important for future driver maintainance.
+
+#define KTD2801_DEFAULT_BRIGHTNESS 100
+#define KTD2801_MAX_BRIGHTNESS 255
+
+const struct expresswire_timing ktd2801_timing = {
+ .poweroff_us = KTD2801_PWR_DOWN_DELAY_US,
+ .detect_delay_us = KTD2801_EXPRESSWIRE_DETECT_DELAY_US,
+ .detect_us = KTD2801_EXPRESSWIRE_DETECT_US,
+ .data_start_us = KTD2801_DATA_START_US,
+ .short_bitset_us = KTD2801_SHORT_BITSET_US,
+ .long_bitset_us = KTD2801_LONG_BITSET_US,
+ .end_of_data_low_us = KTD2801_END_OF_DATA_LOW_US,
+ .end_of_data_high_us = KTD2801_END_OF_DATA_HIGH_US
+};
+
+struct ktd2801_backlight {
+ struct expresswire_common_props props;
+ struct backlight_device *bd;
+ bool was_on;
+};
+
+static int ktd2801_update_status(struct backlight_device *bd)
+{
+ struct ktd2801_backlight *ktd2801 = bl_get_data(bd);
+ u8 brightness = (u8) backlight_get_brightness(bd);
+
+ if (backlight_is_blank(bd)) {
+ expresswire_power_off(&ktd2801->props);
+ ktd2801->was_on = false;
+ return 0;
+ }
+
+ if (!ktd2801->was_on) {
+ expresswire_enable(&ktd2801->props);
+ ktd2801->was_on = true;
+ }
+
+ expresswire_start(&ktd2801->props);
+
+ for (int i = 7; i >= 0; i--)
+ expresswire_set_bit(&ktd2801->props, !!(brightness & BIT(i)));The !! is redundant... but, as previous feedback, I think writing a u8 should be in the library code anyway.
+ expresswire_end(&ktd2801->props); + return 0; +} + <snip>
Daniel.