| Message ID | 20240120-ktd2801-v3-3-fe2cbafffb21@skole.hr |
|---|---|
| State | New |
| Headers | show |
| Series | Kinetic ExpressWire library and KTD2801 backlight driver | expand |
On Sat, Jan 20, 2024 at 10:27 PM Duje Mihanović <duje.mihanovic@skole.hr> 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> Very slim after abstracting out the library and the library has the change I requested so: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
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. > 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.
On Monday, January 22, 2024 11:28:05 AM CET Daniel Thompson wrote: > On Sat, Jan 20, 2024 at 10:26:45PM +0100, Duje Mihanović wrote: > > 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 > > <snip> > > +/* 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. Does this apply for ktd2692 as well? Regards, -- Duje
On Mon, Jan 22, 2024 at 05:24:56PM +0100, Duje Mihanović wrote: > On Monday, January 22, 2024 11:28:05 AM CET Daniel Thompson wrote: > > On Sat, Jan 20, 2024 at 10:26:45PM +0100, Duje Mihanović wrote: > > > 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 > > > <snip> > > > +/* 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. > > Does this apply for ktd2692 as well? I think so, yes... but I won't get in the way if you (or anyone else) decides otherwise. Daniel.
diff --git a/MAINTAINERS b/MAINTAINERS index 87b12d2448a0..dddffbd8d2a0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11891,6 +11891,12 @@ S: Maintained F: Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml F: drivers/video/backlight/ktd253-backlight.c +KTD2801 BACKLIGHT DRIVER +M: Duje Mihanović <duje.mihanovic@skole.hr> +S: Maintained +F: Documentation/devicetree/bindings/leds/backlight/kinetic,ktd2801.yaml +F: drivers/video/backlight/ktd2801-backlight.c + KTEST M: Steven Rostedt <rostedt@goodmis.org> M: John Hawley <warthog9@eaglescrag.net> 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 + 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/Makefile b/drivers/video/backlight/Makefile index f72e1c3c59e9..b33b647f31ca 100644 --- a/drivers/video/backlight/Makefile +++ b/drivers/video/backlight/Makefile @@ -35,6 +35,7 @@ obj-$(CONFIG_BACKLIGHT_HP680) += hp680_bl.o obj-$(CONFIG_BACKLIGHT_HP700) += jornada720_bl.o obj-$(CONFIG_BACKLIGHT_IPAQ_MICRO) += ipaq_micro_bl.o obj-$(CONFIG_BACKLIGHT_KTD253) += ktd253-backlight.o +obj-$(CONFIG_BACKLIGHT_KTD2801) += ktd2801-backlight.o obj-$(CONFIG_BACKLIGHT_KTZ8866) += ktz8866.o obj-$(CONFIG_BACKLIGHT_LM3533) += lm3533_bl.o obj-$(CONFIG_BACKLIGHT_LM3630A) += lm3630a_bl.o 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 + +#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))); + + expresswire_end(&ktd2801->props); + return 0; +} + +static const struct backlight_ops ktd2801_backlight_ops = { + .update_status = ktd2801_update_status, +}; + +static int ktd2801_backlight_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct backlight_device *bd; + struct ktd2801_backlight *ktd2801; + u32 brightness, max_brightness; + int ret; + + ktd2801 = devm_kzalloc(dev, sizeof(*ktd2801), GFP_KERNEL); + if (!ktd2801) + return -ENOMEM; + ktd2801->was_on = true; + ktd2801->props.timing = ktd2801_timing; + + ret = device_property_read_u32(dev, "max-brightness", &max_brightness); + if (ret) + max_brightness = KTD2801_MAX_BRIGHTNESS; + if (max_brightness > KTD2801_MAX_BRIGHTNESS) { + dev_err(dev, "illegal max brightness specified\n"); + max_brightness = KTD2801_MAX_BRIGHTNESS; + } + + ret = device_property_read_u32(dev, "default-brightness", &brightness); + if (ret) + brightness = KTD2801_DEFAULT_BRIGHTNESS; + if (brightness > max_brightness) { + dev_err(dev, "default brightness exceeds max\n"); + brightness = max_brightness; + } + + ktd2801->props.ctrl_gpio = devm_gpiod_get(dev, "ctrl", GPIOD_OUT_HIGH); + if (IS_ERR(ktd2801->props.ctrl_gpio)) + return dev_err_probe(dev, PTR_ERR(ktd2801->props.ctrl_gpio), + "failed to get backlight GPIO"); + gpiod_set_consumer_name(ktd2801->props.ctrl_gpio, dev_name(dev)); + + bd = devm_backlight_device_register(dev, dev_name(dev), dev, ktd2801, + &ktd2801_backlight_ops, NULL); + if (IS_ERR(bd)) + return dev_err_probe(dev, PTR_ERR(bd), + "failed to register backlight"); + + bd->props.max_brightness = max_brightness; + bd->props.brightness = brightness; + + ktd2801->bd = bd; + platform_set_drvdata(pdev, bd); + backlight_update_status(bd); + + return 0; +} + +static const struct of_device_id ktd2801_of_match[] = { + { .compatible = "kinetic,ktd2801" }, + { } +}; +MODULE_DEVICE_TABLE(of, ktd2801_of_match); + +static struct platform_driver ktd2801_backlight_driver = { + .driver = { + .name = "ktd2801-backlight", + .of_match_table = ktd2801_of_match, + }, + .probe = ktd2801_backlight_probe, +}; +module_platform_driver(ktd2801_backlight_driver); + +MODULE_IMPORT_NS(EXPRESSWIRE); +MODULE_AUTHOR("Duje Mihanović <duje.mihanovic@skole.hr>"); +MODULE_DESCRIPTION("Kinetic KTD2801 Backlight Driver"); +MODULE_LICENSE("GPL");
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> --- MAINTAINERS | 6 ++ drivers/video/backlight/Kconfig | 8 ++ drivers/video/backlight/Makefile | 1 + drivers/video/backlight/ktd2801-backlight.c | 143 ++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+)