[PATCH v3 0/3] Kinetic ExpressWire library and KTD2801 backlight driver

[PATCH v3 0/3] Kinetic ExpressWire library and KTD2801 backlight driver

[Duje Mihanović]

Hello,

This series adds support for the Kinetic KTD2801 LED backlight driver
IC found in samsung,coreprimevelte.

Support is already upstream for the somewhat similar KTD2692 flash
driver, and this series since v3 also moves its ExpressWire code into a
separate library and converts the KTD2692 driver to use that library.

Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
Changes in v3:
- Split ExpressWire code into library (and convert KTD2692 to use this
  library)
- Rewrite commit messages
- Add link to datasheet
- Drop of.h include in ktd2801
- Use _cansleep and usleep_range when powering off
- Clean up bitwise operation in update_status
- Link to v2: https://lore.kernel.org/r/20240118-ktd2801-v2-0-425cf32e0769@skole.hr

Changes in v2:
- Address maintainer comments:
  - Drop MODULE_ALIAS
  - Rename enable-gpios to ctrl-gpios
  - Rename ktd2801_backlight->desc to ktd2801_backlight->gpiod
  - Give time constants more descriptive names and note their origins in
    Samsung driver
  - Convert to GPIO_ACTIVE_HIGH
- Update trailers
- Link to v1: https://lore.kernel.org/r/20231005-ktd2801-v1-0-43cd85b0629a@skole.hr

---
Duje Mihanović (3):
      leds: ktd2692: move ExpressWire code to library
      dt-bindings: backlight: add Kinetic KTD2801 binding
      backlight: Add Kinetic KTD2801 backlight support

 .../bindings/leds/backlight/kinetic,ktd2801.yaml   |  46 +++++++
 MAINTAINERS                                        |  13 ++
 drivers/leds/Kconfig                               |   3 +
 drivers/leds/Makefile                              |   3 +
 drivers/leds/flash/Kconfig                         |   1 +
 drivers/leds/flash/leds-ktd2692.c                  |  91 ++++---------
 drivers/leds/leds-expresswire.c                    |  59 +++++++++
 drivers/video/backlight/Kconfig                    |   8 ++
 drivers/video/backlight/Makefile                   |   1 +
 drivers/video/backlight/ktd2801-backlight.c        | 143 +++++++++++++++++++++
 include/linux/leds-expresswire.h                   |  35 +++++
 11 files changed, 336 insertions(+), 67 deletions(-)
---
base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
change-id: 20231004-ktd2801-0f3883cb59d0

Best regards,
-- 
Duje Mihanović <duje.mihanovic@skole.hr>

[PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Duje Mihanović]

The ExpressWire protocol is shared between at least KTD2692 and KTD2801
with slight differences such as timings and the former not having a
defined set of pulses for enabling the protocol (possibly because it
does not support PWM unlike KTD2801). Despite these differences the
ExpressWire handling code can be shared between the two, so move it into
a library in preparation for adding KTD2801 support.

Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
 MAINTAINERS                       |  7 +++
 drivers/leds/Kconfig              |  3 ++
 drivers/leds/Makefile             |  3 ++
 drivers/leds/flash/Kconfig        |  1 +
 drivers/leds/flash/leds-ktd2692.c | 91 +++++++++++----------------------------
 drivers/leds/leds-expresswire.c   | 59 +++++++++++++++++++++++++
 include/linux/leds-expresswire.h  | 35 +++++++++++++++
 7 files changed, 132 insertions(+), 67 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index a7c4cf8201e0..87b12d2448a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7902,6 +7902,13 @@ S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git
 F:	fs/exfat/
 
+EXPRESSWIRE PROTOCOL LIBRARY
+M:	Duje Mihanović <duje.mihanovic@skole.hr>
+L:	linux-leds@vger.kernel.org
+S:	Maintained
+F:	drivers/leds/leds-expresswire.c
+F:	include/linux/leds-expresswire.h
+
 EXT2 FILE SYSTEM
 M:	Jan Kara <jack@suse.com>
 L:	linux-ext4@vger.kernel.org
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6292fddcc55c..d29b6823e7d1 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -181,6 +181,9 @@ config LEDS_EL15203000
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-el15203000.
 
+config LEDS_EXPRESSWIRE
+	bool
+
 config LEDS_TURRIS_OMNIA
 	tristate "LED support for CZ.NIC's Turris Omnia"
 	depends on LEDS_CLASS_MULTICOLOR
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d7348e8bc019..a63a07d01c6f 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -89,6 +89,9 @@ obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
 obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
 
+# Kinetic ExpressWire Protocol
+obj-$(CONFIG_LEDS_EXPRESSWIRE)		+= leds-expresswire.o
+
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
index 4e08dbc05709..526c1d063cd8 100644
--- a/drivers/leds/flash/Kconfig
+++ b/drivers/leds/flash/Kconfig
@@ -24,6 +24,7 @@ config LEDS_KTD2692
 	tristate "LED support for Kinetic KTD2692 flash LED controller"
 	depends on OF
 	depends on GPIOLIB || COMPILE_TEST
+	select LEDS_EXPRESSWIRE
 	help
 	  This option enables support for Kinetic KTD2692 LED flash connected
 	  through ExpressWire interface.
diff --git a/drivers/leds/flash/leds-ktd2692.c b/drivers/leds/flash/leds-ktd2692.c
index 598eee5daa52..8c17de3d621f 100644
--- a/drivers/leds/flash/leds-ktd2692.c
+++ b/drivers/leds/flash/leds-ktd2692.c
@@ -9,6 +9,7 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
+#include <linux/leds-expresswire.h>
 #include <linux/led-class-flash.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -48,11 +49,6 @@
 /* KTD2692 default length of name */
 #define KTD2692_NAME_LENGTH			20
 
-enum ktd2692_bitset {
-	KTD2692_LOW = 0,
-	KTD2692_HIGH,
-};
-
 /* Movie / Flash Mode Control */
 enum ktd2692_led_mode {
 	KTD2692_MODE_DISABLE = 0,	/* default */
@@ -71,7 +67,19 @@ struct ktd2692_led_config_data {
 	enum led_brightness max_brightness;
 };
 
+const struct expresswire_timing ktd2692_timing = {
+	.poweroff_us = KTD2692_TIME_RESET_US,
+	.data_start_us = KTD2692_TIME_DATA_START_TIME_US,
+	.end_of_data_low_us = KTD2692_TIME_LOW_END_OF_DATA_US,
+	.end_of_data_high_us = KTD2692_TIME_HIGH_END_OF_DATA_US,
+	.short_bitset_us = KTD2692_TIME_SHORT_BITSET_US,
+	.long_bitset_us = KTD2692_TIME_LONG_BITSET_US
+};
+
 struct ktd2692_context {
+	/* Common ExpressWire properties (ctrl GPIO and timing) */
+	struct expresswire_common_props props;
+
 	/* Related LED Flash class device */
 	struct led_classdev_flash fled_cdev;
 
@@ -80,7 +88,6 @@ struct ktd2692_context {
 	struct regulator *regulator;
 
 	struct gpio_desc *aux_gpio;
-	struct gpio_desc *ctrl_gpio;
 
 	enum ktd2692_led_mode mode;
 	enum led_brightness torch_brightness;
@@ -92,65 +99,14 @@ static struct ktd2692_context *fled_cdev_to_led(
 	return container_of(fled_cdev, struct ktd2692_context, fled_cdev);
 }
 
-static void ktd2692_expresswire_start(struct ktd2692_context *led)
-{
-	gpiod_direction_output(led->ctrl_gpio, KTD2692_HIGH);
-	udelay(KTD2692_TIME_DATA_START_TIME_US);
-}
-
-static void ktd2692_expresswire_reset(struct ktd2692_context *led)
-{
-	gpiod_direction_output(led->ctrl_gpio, KTD2692_LOW);
-	udelay(KTD2692_TIME_RESET_US);
-}
-
-static void ktd2692_expresswire_end(struct ktd2692_context *led)
-{
-	gpiod_direction_output(led->ctrl_gpio, KTD2692_LOW);
-	udelay(KTD2692_TIME_LOW_END_OF_DATA_US);
-	gpiod_direction_output(led->ctrl_gpio, KTD2692_HIGH);
-	udelay(KTD2692_TIME_HIGH_END_OF_DATA_US);
-}
-
-static void ktd2692_expresswire_set_bit(struct ktd2692_context *led, bool bit)
-{
-	/*
-	 * The Low Bit(0) and High Bit(1) is based on a time detection
-	 * algorithm between time low and time high
-	 * Time_(L_LB) : Low time of the Low Bit(0)
-	 * Time_(H_LB) : High time of the LOW Bit(0)
-	 * Time_(L_HB) : Low time of the High Bit(1)
-	 * Time_(H_HB) : High time of the High Bit(1)
-	 *
-	 * It can be simplified to:
-	 * Low Bit(0) : 2 * Time_(H_LB) < Time_(L_LB)
-	 * High Bit(1) : 2 * Time_(L_HB) < Time_(H_HB)
-	 * HIGH  ___           ____    _..     _________    ___
-	 *          |_________|    |_..  |____|         |__|
-	 * LOW        <L_LB>  <H_LB>     <L_HB>  <H_HB>
-	 *          [  Low Bit (0) ]     [  High Bit(1) ]
-	 */
-	if (bit) {
-		gpiod_direction_output(led->ctrl_gpio, KTD2692_LOW);
-		udelay(KTD2692_TIME_SHORT_BITSET_US);
-		gpiod_direction_output(led->ctrl_gpio, KTD2692_HIGH);
-		udelay(KTD2692_TIME_LONG_BITSET_US);
-	} else {
-		gpiod_direction_output(led->ctrl_gpio, KTD2692_LOW);
-		udelay(KTD2692_TIME_LONG_BITSET_US);
-		gpiod_direction_output(led->ctrl_gpio, KTD2692_HIGH);
-		udelay(KTD2692_TIME_SHORT_BITSET_US);
-	}
-}
-
 static void ktd2692_expresswire_write(struct ktd2692_context *led, u8 value)
 {
 	int i;
 
-	ktd2692_expresswire_start(led);
+	expresswire_start(&led->props);
 	for (i = 7; i >= 0; i--)
-		ktd2692_expresswire_set_bit(led, value & BIT(i));
-	ktd2692_expresswire_end(led);
+		expresswire_set_bit(&led->props, value & BIT(i));
+	expresswire_end(&led->props);
 }
 
 static int ktd2692_led_brightness_set(struct led_classdev *led_cdev,
@@ -163,7 +119,7 @@ static int ktd2692_led_brightness_set(struct led_classdev *led_cdev,
 
 	if (brightness == LED_OFF) {
 		led->mode = KTD2692_MODE_DISABLE;
-		gpiod_direction_output(led->aux_gpio, KTD2692_LOW);
+		gpiod_direction_output(led->aux_gpio, 0);
 	} else {
 		ktd2692_expresswire_write(led, brightness |
 					KTD2692_REG_MOVIE_CURRENT_BASE);
@@ -191,10 +147,10 @@ static int ktd2692_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
 				| KTD2692_REG_FLASH_TIMEOUT_BASE);
 
 		led->mode = KTD2692_MODE_FLASH;
-		gpiod_direction_output(led->aux_gpio, KTD2692_HIGH);
+		gpiod_direction_output(led->aux_gpio, 1);
 	} else {
 		led->mode = KTD2692_MODE_DISABLE;
-		gpiod_direction_output(led->aux_gpio, KTD2692_LOW);
+		gpiod_direction_output(led->aux_gpio, 0);
 	}
 
 	ktd2692_expresswire_write(led, led->mode | KTD2692_REG_MODE_BASE);
@@ -247,8 +203,8 @@ static void ktd2692_init_flash_timeout(struct led_classdev_flash *fled_cdev,
 static void ktd2692_setup(struct ktd2692_context *led)
 {
 	led->mode = KTD2692_MODE_DISABLE;
-	ktd2692_expresswire_reset(led);
-	gpiod_direction_output(led->aux_gpio, KTD2692_LOW);
+	expresswire_power_off(&led->props);
+	gpiod_direction_output(led->aux_gpio, 0);
 
 	ktd2692_expresswire_write(led, (KTD2692_MM_MIN_CURR_THRESHOLD_SCALE - 1)
 				 | KTD2692_REG_MM_MIN_CURR_THRESHOLD_BASE);
@@ -277,8 +233,8 @@ static int ktd2692_parse_dt(struct ktd2692_context *led, struct device *dev,
 	if (!np)
 		return -ENXIO;
 
-	led->ctrl_gpio = devm_gpiod_get(dev, "ctrl", GPIOD_ASIS);
-	ret = PTR_ERR_OR_ZERO(led->ctrl_gpio);
+	led->props.ctrl_gpio = devm_gpiod_get(dev, "ctrl", GPIOD_ASIS);
+	ret = PTR_ERR_OR_ZERO(led->props.ctrl_gpio);
 	if (ret)
 		return dev_err_probe(dev, ret, "cannot get ctrl-gpios\n");
 
@@ -412,6 +368,7 @@ static struct platform_driver ktd2692_driver = {
 
 module_platform_driver(ktd2692_driver);
 
+MODULE_IMPORT_NS(EXPRESSWIRE);
 MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>");
 MODULE_DESCRIPTION("Kinetic KTD2692 LED driver");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/leds/leds-expresswire.c b/drivers/leds/leds-expresswire.c
new file mode 100644
index 000000000000..88e76c968cf0
--- /dev/null
+++ b/drivers/leds/leds-expresswire.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Shared library for Kinetic's ExpressWire protocol.
+ * This protocol works by pulsing the ExpressWire IC's control GPIO.
+ * ktd2692 and ktd2801 are known to use this protocol.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/leds-expresswire.h>
+
+void expresswire_power_off(struct expresswire_common_props *props)
+{
+	gpiod_set_value_cansleep(props->ctrl_gpio, 0);
+	usleep_range(props->timing.poweroff_us, props->timing.poweroff_us * 2);
+}
+EXPORT_SYMBOL_NS_GPL(expresswire_power_off, EXPRESSWIRE);
+
+void expresswire_enable(struct expresswire_common_props *props)
+{
+	gpiod_set_value(props->ctrl_gpio, 1);
+	udelay(props->timing.detect_delay_us);
+	gpiod_set_value(props->ctrl_gpio, 0);
+	udelay(props->timing.detect_us);
+	gpiod_set_value(props->ctrl_gpio, 1);
+}
+EXPORT_SYMBOL_NS_GPL(expresswire_enable, EXPRESSWIRE);
+
+void expresswire_start(struct expresswire_common_props *props)
+{
+	gpiod_set_value(props->ctrl_gpio, 1);
+	udelay(props->timing.data_start_us);
+}
+EXPORT_SYMBOL_NS_GPL(expresswire_start, EXPRESSWIRE);
+
+void expresswire_end(struct expresswire_common_props *props)
+{
+	gpiod_set_value(props->ctrl_gpio, 0);
+	udelay(props->timing.end_of_data_low_us);
+	gpiod_set_value(props->ctrl_gpio, 1);
+	udelay(props->timing.end_of_data_high_us);
+}
+EXPORT_SYMBOL_NS_GPL(expresswire_end, EXPRESSWIRE);
+
+void expresswire_set_bit(struct expresswire_common_props *props, bool bit)
+{
+	if (bit) {
+		gpiod_set_value(props->ctrl_gpio, 0);
+		udelay(props->timing.short_bitset_us);
+		gpiod_set_value(props->ctrl_gpio, 1);
+		udelay(props->timing.long_bitset_us);
+	} else {
+		gpiod_set_value(props->ctrl_gpio, 0);
+		udelay(props->timing.long_bitset_us);
+		gpiod_set_value(props->ctrl_gpio, 1);
+		udelay(props->timing.short_bitset_us);
+	}
+}
+EXPORT_SYMBOL_NS_GPL(expresswire_set_bit, EXPRESSWIRE);
diff --git a/include/linux/leds-expresswire.h b/include/linux/leds-expresswire.h
new file mode 100644
index 000000000000..b5aad0556cb8
--- /dev/null
+++ b/include/linux/leds-expresswire.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Shared library for Kinetic's ExpressWire protocol.
+ * This protocol works by pulsing the ExpressWire IC's control GPIO.
+ * ktd2692 and ktd2801 are known to use this protocol.
+ */
+
+#ifndef _LEDS_EXPRESSWIRE_H
+#define _LEDS_EXPRESSWIRE_H
+
+#include <linux/gpio/consumer.h>
+
+struct expresswire_timing {
+	unsigned long poweroff_us;
+	unsigned long detect_delay_us;
+	unsigned long detect_us;
+	unsigned long data_start_us;
+	unsigned long end_of_data_low_us;
+	unsigned long end_of_data_high_us;
+	unsigned long short_bitset_us;
+	unsigned long long_bitset_us;
+};
+
+struct expresswire_common_props {
+	struct gpio_desc *ctrl_gpio;
+	struct expresswire_timing timing;
+};
+
+extern void expresswire_power_off(struct expresswire_common_props *props);
+extern void expresswire_enable(struct expresswire_common_props *props);
+extern void expresswire_start(struct expresswire_common_props *props);
+extern void expresswire_end(struct expresswire_common_props *props);
+extern void expresswire_set_bit(struct expresswire_common_props *props, bool bit);
+
+#endif /* _LEDS_EXPRESSWIRE_H */

-- 
2.43.0

[PATCH v3 2/3] dt-bindings: backlight: add Kinetic KTD2801 binding

[Duje Mihanović]

KTD2801 is a LED backlight driver IC found in samsung,coreprimevelte.
The brightness can be set using PWM or the ExpressWire protocol. Add
a DT binding for the KTD2801.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
 .../bindings/leds/backlight/kinetic,ktd2801.yaml   | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd2801.yaml b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd2801.yaml
new file mode 100644
index 000000000000..b005065e0f48
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd2801.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/kinetic,ktd2801.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Kinetic Technologies KTD2801 one-wire backlight
+
+maintainers:
+  - Duje Mihanović <duje.mihanovic@skole.hr>
+
+description: |
+  The Kinetic Technologies KTD2801 is a LED backlight driver controlled
+  by a single GPIO line. The driver can be controlled with a PWM signal
+  or by pulsing the GPIO line to set the backlight level. This is called
+  "ExpressWire".
+
+allOf:
+  - $ref: common.yaml#
+
+properties:
+  compatible:
+    const: kinetic,ktd2801
+
+  ctrl-gpios:
+    maxItems: 1
+
+  default-brightness: true
+  max-brightness: true
+
+required:
+  - compatible
+  - ctrl-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    backlight {
+        compatible = "kinetic,ktd2801";
+        ctrl-gpios = <&gpio 97 GPIO_ACTIVE_HIGH>;
+        max-brightness = <210>;
+        default-brightness = <100>;
+    };

-- 
2.43.0

[PATCH v3 3/3] backlight: Add Kinetic KTD2801 backlight support

[Duje Mihanović]

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(+)

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");

-- 
2.43.0

Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Linus Walleij]

On Sat, Jan 20, 2024 at 10:27 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:

> The ExpressWire protocol is shared between at least KTD2692 and KTD2801
> with slight differences such as timings and the former not having a
> defined set of pulses for enabling the protocol (possibly because it
> does not support PWM unlike KTD2801). Despite these differences the
> ExpressWire handling code can be shared between the two, so move it into
> a library in preparation for adding KTD2801 support.
>
> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>

This is great stuff.
I looked at my KTD253 driver but AFAICT it uses a different method:
instead of transferring a numeral, it increases/decreases a counter, so
it can't use the library.

> +extern void expresswire_power_off(struct expresswire_common_props *props);
> +extern void expresswire_enable(struct expresswire_common_props *props);
> +extern void expresswire_start(struct expresswire_common_props *props);
> +extern void expresswire_end(struct expresswire_common_props *props);
> +extern void expresswire_set_bit(struct expresswire_common_props *props, bool bit);

I would skip the keyword "extern" since it is default I think even
checkpatch complains about it these days?

Anyway, no big deal:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3 3/3] backlight: Add Kinetic KTD2801 backlight support

[Linus Walleij]

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

Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Duje Mihanović]

On Sunday, January 21, 2024 3:35:46 PM CET Linus Walleij wrote:
> > +extern void expresswire_power_off(struct expresswire_common_props 
*props);
> > +extern void expresswire_enable(struct expresswire_common_props *props);
> > +extern void expresswire_start(struct expresswire_common_props *props);
> > +extern void expresswire_end(struct expresswire_common_props *props);
> > +extern void expresswire_set_bit(struct expresswire_common_props *props,
> > bool bit);
> I would skip the keyword "extern" since it is default I think even
> checkpatch complains about it these days?

Doesn't seem to, I tried it myself:

$ git format-patch HEAD~3
0001-leds-ktd2692-move-ExpressWire-code-to-library.patch
0002-dt-bindings-backlight-add-Kinetic-KTD2801-binding.patch
0003-backlight-Add-Kinetic-KTD2801-backlight-support.patch
$ ./scripts/checkpatch.pl 0001-leds-ktd2692-move-ExpressWire-code-to-
library.patch            
total: 0 errors, 0 warnings, 291 lines checked

0001-leds-ktd2692-move-ExpressWire-code-to-library.patch has no obvious style 
problems and is ready for submission.

I'll keep that in mind if a v4 is needed though.

Regards,
--
Duje

Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Daniel Thompson]

On Sat, Jan 20, 2024 at 10:26:43PM +0100, Duje Mihanović wrote:
> The ExpressWire protocol is shared between at least KTD2692 and KTD2801
> with slight differences such as timings and the former not having a
> defined set of pulses for enabling the protocol (possibly because it
> does not support PWM unlike KTD2801). Despite these differences the
> ExpressWire handling code can be shared between the two, so move it into
> a library in preparation for adding KTD2801 support.
>
> Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---
>  MAINTAINERS                       |  7 +++
>  drivers/leds/Kconfig              |  3 ++
>  drivers/leds/Makefile             |  3 ++
>  drivers/leds/flash/Kconfig        |  1 +
>  drivers/leds/flash/leds-ktd2692.c | 91 +++++++++++----------------------------
>  drivers/leds/leds-expresswire.c   | 59 +++++++++++++++++++++++++
>  include/linux/leds-expresswire.h  | 35 +++++++++++++++
>  7 files changed, 132 insertions(+), 67 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a7c4cf8201e0..87b12d2448a0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7902,6 +7902,13 @@ S:	Maintained
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat.git
>  F:	fs/exfat/
>
> +EXPRESSWIRE PROTOCOL LIBRARY
> +M:	Duje Mihanović <duje.mihanovic@skole.hr>
> +L:	linux-leds@vger.kernel.org
> +S:	Maintained
> +F:	drivers/leds/leds-expresswire.c
> +F:	include/linux/leds-expresswire.h
> +
>  EXT2 FILE SYSTEM
>  M:	Jan Kara <jack@suse.com>
>  L:	linux-ext4@vger.kernel.org
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6292fddcc55c..d29b6823e7d1 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -181,6 +181,9 @@ config LEDS_EL15203000
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called leds-el15203000.
>
> +config LEDS_EXPRESSWIRE
> +	bool
> +

Shouldn't there be a "select GPIOLIB" here? It seems odd to make the
clients responsible for the dependencies.

BTW there seems to be very little consistency across the kernel between
"depends on GPIOLIB" and "select GPIOLIB".. but select is marginally
more popular (283 vs. 219 in the kernel I checked).


> diff --git a/drivers/leds/flash/leds-ktd2692.c b/drivers/leds/flash/leds-ktd2692.c
> index 598eee5daa52..8c17de3d621f 100644
> --- a/drivers/leds/flash/leds-ktd2692.c
> +++ b/drivers/leds/flash/leds-ktd2692.c
>  <snip>
>  static void ktd2692_expresswire_write(struct ktd2692_context *led, u8 value)
>  {
>  	int i;
>
> -	ktd2692_expresswire_start(led);
> +	expresswire_start(&led->props);
>  	for (i = 7; i >= 0; i--)
> -		ktd2692_expresswire_set_bit(led, value & BIT(i));
> -	ktd2692_expresswire_end(led);
> +		expresswire_set_bit(&led->props, value & BIT(i));
> +	expresswire_end(&led->props);
>  }

Is there any reason not to have an expresswire_write_u8() method in the
library code? It is a concept that appears in both drivers.


Daniel.

Re: [PATCH v3 3/3] backlight: Add Kinetic KTD2801 backlight support

[Daniel Thompson]

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.

Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Duje Mihanović]

On Monday, January 22, 2024 11:19:26 AM CET Daniel Thompson wrote:
> On Sat, Jan 20, 2024 at 10:26:43PM +0100, Duje Mihanović wrote:
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 6292fddcc55c..d29b6823e7d1 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -181,6 +181,9 @@ config LEDS_EL15203000
> > 
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called leds-el15203000.
> > 
> > +config LEDS_EXPRESSWIRE
> > +	bool
> > +
> 
> Shouldn't there be a "select GPIOLIB" here? It seems odd to make the
> clients responsible for the dependencies.
> 
> BTW there seems to be very little consistency across the kernel between
> "depends on GPIOLIB" and "select GPIOLIB".. but select is marginally
> more popular (283 vs. 219 in the kernel I checked).

I believe a "select" would be more appropriate here unless these backlights 
should be hidden if GPIOLIB is disabled. The catch with "select" is that there 
seems to be no way to throw in the "|| COMPILE_TEST" other GPIO-based 
backlights have and I'm not sure what to do about that.

> > diff --git a/drivers/leds/flash/leds-ktd2692.c
> > b/drivers/leds/flash/leds-ktd2692.c index 598eee5daa52..8c17de3d621f 
100644
> > --- a/drivers/leds/flash/leds-ktd2692.c
> > +++ b/drivers/leds/flash/leds-ktd2692.c
> > 
> >  <snip>
> >  static void ktd2692_expresswire_write(struct ktd2692_context *led, u8
> >  value)
> >  {
> >  
> >  	int i;
> > 
> > -	ktd2692_expresswire_start(led);
> > +	expresswire_start(&led->props);
> > 
> >  	for (i = 7; i >= 0; i--)
> > 
> > -		ktd2692_expresswire_set_bit(led, value & BIT(i));
> > -	ktd2692_expresswire_end(led);
> > +		expresswire_set_bit(&led->props, value & BIT(i));
> > +	expresswire_end(&led->props);
> > 
> >  }
> 
> Is there any reason not to have an expresswire_write_u8() method in the
> library code? It is a concept that appears in both drivers.

Not really, I'll add it in v4.

Regards,
--
Duje

Re: [PATCH v3 3/3] backlight: Add Kinetic KTD2801 backlight support

[Duje Mihanović]

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

Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Daniel Thompson]

On Mon, Jan 22, 2024 at 05:24:51PM +0100, Duje Mihanović wrote:
> On Monday, January 22, 2024 11:19:26 AM CET Daniel Thompson wrote:
> > On Sat, Jan 20, 2024 at 10:26:43PM +0100, Duje Mihanović wrote:
> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > index 6292fddcc55c..d29b6823e7d1 100644
> > > --- a/drivers/leds/Kconfig
> > > +++ b/drivers/leds/Kconfig
> > > @@ -181,6 +181,9 @@ config LEDS_EL15203000
> > >
> > >  	  To compile this driver as a module, choose M here: the module
> > >  	  will be called leds-el15203000.
> > >
> > > +config LEDS_EXPRESSWIRE
> > > +	bool
> > > +
> >
> > Shouldn't there be a "select GPIOLIB" here? It seems odd to make the
> > clients responsible for the dependencies.
> >
> > BTW there seems to be very little consistency across the kernel between
> > "depends on GPIOLIB" and "select GPIOLIB".. but select is marginally
> > more popular (283 vs. 219 in the kernel I checked).
>
> I believe a "select" would be more appropriate here unless these backlights
> should be hidden if GPIOLIB is disabled. The catch with "select" is that there
> seems to be no way to throw in the "|| COMPILE_TEST" other GPIO-based
> backlights have and I'm not sure what to do about that.

I think the "|| COMPILE_TEST" might just be a copy 'n paste'ism (in fact I
may even have been guilty off propagating it in reviews when checking
for inconsistencies).

AFAICT nothing will inhibit setting GPIOLIB so allyes- and allmodconfig
builds will always end up with GPIOLIB enabled. If we are happy to
select it then I think that is enough!


Daniel.

Re: [PATCH v3 3/3] backlight: Add Kinetic KTD2801 backlight support

[Daniel Thompson]

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.

Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Duje Mihanović]

On Monday, January 22, 2024 5:50:11 PM CET Daniel Thompson wrote:
> On Mon, Jan 22, 2024 at 05:24:51PM +0100, Duje Mihanović wrote:
> > I believe a "select" would be more appropriate here unless these 
backlights
> > should be hidden if GPIOLIB is disabled. The catch with "select" is that
> > there seems to be no way to throw in the "|| COMPILE_TEST" other GPIO-
based
> > backlights have and I'm not sure what to do about that.
> 
> I think the "|| COMPILE_TEST" might just be a copy 'n paste'ism (in fact I
> may even have been guilty off propagating it in reviews when checking
> for inconsistencies).
> 
> AFAICT nothing will inhibit setting GPIOLIB so allyes- and allmodconfig
> builds will always end up with GPIOLIB enabled. If we are happy to
> select it then I think that is enough!

In that case I guess I'll just make it select GPIOLIB.

Regards,
--
Duje

Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Duje Mihanović]

On Monday, January 22, 2024 5:57:53 PM CET Duje Mihanović wrote:
> On Monday, January 22, 2024 5:50:11 PM CET Daniel Thompson wrote:
> > AFAICT nothing will inhibit setting GPIOLIB so allyes- and allmodconfig
> > builds will always end up with GPIOLIB enabled. If we are happy to
> > select it then I think that is enough!
> 
> In that case I guess I'll just make it select GPIOLIB.

Nevermind that, it'll have to be 'depends on' after all:

drivers/gpio/Kconfig:6:error: recursive dependency detected!
drivers/gpio/Kconfig:6: symbol GPIOLIB is selected by LEDS_EXPRESSWIRE
drivers/leds/Kconfig:184:       symbol LEDS_EXPRESSWIRE depends on NEW_LEDS
drivers/leds/Kconfig:9: symbol NEW_LEDS is selected by SENSORS_APPLESMC
drivers/hwmon/Kconfig:348:      symbol SENSORS_APPLESMC depends on HWMON
drivers/hwmon/Kconfig:6:        symbol HWMON is selected by EEEPC_LAPTOP
drivers/platform/x86/Kconfig:325:       symbol EEEPC_LAPTOP depends on 
ACPI_VIDEO
drivers/acpi/Kconfig:209:       symbol ACPI_VIDEO depends on 
BACKLIGHT_CLASS_DEVICE
drivers/video/backlight/Kconfig:136:    symbol BACKLIGHT_CLASS_DEVICE is 
selected by FB_BACKLIGHT
drivers/video/fbdev/core/Kconfig:180:   symbol FB_BACKLIGHT is selected by 
FB_SSD1307
drivers/video/fbdev/Kconfig:1934:       symbol FB_SSD1307 depends on GPIOLIB
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"

Regards,
--
Duje

Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Daniel Thompson]

On Mon, Jan 22, 2024 at 06:26:04PM +0100, Duje Mihanović wrote:
> On Monday, January 22, 2024 5:57:53 PM CET Duje Mihanović wrote:
> > On Monday, January 22, 2024 5:50:11 PM CET Daniel Thompson wrote:
> > > AFAICT nothing will inhibit setting GPIOLIB so allyes- and allmodconfig
> > > builds will always end up with GPIOLIB enabled. If we are happy to
> > > select it then I think that is enough!
> >
> > In that case I guess I'll just make it select GPIOLIB.
>
> Nevermind that, it'll have to be 'depends on' after all:
>
> drivers/gpio/Kconfig:6:error: recursive dependency detected!
> drivers/gpio/Kconfig:6: symbol GPIOLIB is selected by LEDS_EXPRESSWIRE
> drivers/leds/Kconfig:184:       symbol LEDS_EXPRESSWIRE depends on NEW_LEDS

Can this dependency could be broken by declaring LEDS_EXPRESSWIRE at
the top (or bottom) of the KConfig file (it's an option without a UI
and does not need to be within the if NEW_LEDS block).

I'm aware this kind of change could provoke an argument about which
sub-system the expresswire code should live in... but I think it's
a worthwhile change anyway! We shouldn't need NEW_LEDS for this.


Daniel.

Re: [PATCH v3 1/3] leds: ktd2692: move ExpressWire code to library

[Duje Mihanović]

On Monday, January 22, 2024 6:50:31 PM CET Daniel Thompson wrote:
> On Mon, Jan 22, 2024 at 06:26:04PM +0100, Duje Mihanović wrote:
> > On Monday, January 22, 2024 5:57:53 PM CET Duje Mihanović wrote:
> > > On Monday, January 22, 2024 5:50:11 PM CET Daniel Thompson wrote:
> > > > AFAICT nothing will inhibit setting GPIOLIB so allyes- and 
allmodconfig
> > > > builds will always end up with GPIOLIB enabled. If we are happy to
> > > > select it then I think that is enough!
> > > 
> > > In that case I guess I'll just make it select GPIOLIB.
> > 
> > Nevermind that, it'll have to be 'depends on' after all:
> > 
> > drivers/gpio/Kconfig:6:error: recursive dependency detected!
> > drivers/gpio/Kconfig:6: symbol GPIOLIB is selected by LEDS_EXPRESSWIRE
> > drivers/leds/Kconfig:184:       symbol LEDS_EXPRESSWIRE depends on 
NEW_LEDS
> 
> Can this dependency could be broken by declaring LEDS_EXPRESSWIRE at
> the top (or bottom) of the KConfig file (it's an option without a UI
> and does not need to be within the if NEW_LEDS block).

Nope, the only change is that the dependency graph is somewhat shorter:

drivers/gpio/Kconfig:6:error: recursive dependency detected!
drivers/gpio/Kconfig:6: symbol GPIOLIB is selected by LEDS_EXPRESSWIRE
drivers/leds/Kconfig:9: symbol LEDS_EXPRESSWIRE is selected by 
BACKLIGHT_KTD2801
drivers/video/backlight/Kconfig:186:    symbol BACKLIGHT_KTD2801 depends on 
BACKLIGHT_CLASS_DEVICE
drivers/video/backlight/Kconfig:136:    symbol BACKLIGHT_CLASS_DEVICE is 
selected by FB_BACKLIGHT
drivers/video/fbdev/core/Kconfig:180:   symbol FB_BACKLIGHT is selected by 
FB_SSD1307
drivers/video/fbdev/Kconfig:1934:       symbol FB_SSD1307 depends on GPIOLIB
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"

Regards,
--
Duje