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.mihano...@skole.hr>
As Linus W. said, this is looking really nice now. Thanks! Just a couple of nits below. > 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. > + 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 I2C > diff --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.
