Re: [PATCH 2/2 v1] backlight: Add Kinetic KTD253 backlight driver

Hi Linus.

On Mon, Jul 20, 2020 at 10:35:06PM +0200, Linus Walleij wrote:

The Kinetic KTD253 backlight driver is controlled with a single GPIO line, but still supports a range of brightness settings by sending fast pulses on the line.

This is based off the source code release for the Samsung GT-S7710 mobile phone.

Signed-off-by: Linus Walleij linus.walleij@linaro.org

In -next there is a few updated to backlight stuff that this driver could benefit from. My comments in the following assumes you have the latest -next.

Sam

---

MAINTAINERS | 6 + drivers/video/backlight/Kconfig | 8 + drivers/video/backlight/Makefile | 1 + drivers/video/backlight/ktd253-backlight.c | 254 +++++++++++++++++++++ 4 files changed, 269 insertions(+) create mode 100644 drivers/video/backlight/ktd253-backlight.c

diff --git a/MAINTAINERS b/MAINTAINERS index b4a43a9e7fbc..ea6fcc5bb79e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9610,6 +9610,12 @@ F: Documentation/admin-guide/auxdisplay/ks0108.rst F: drivers/auxdisplay/ks0108.c F: include/linux/ks0108.h

+KTD253 BACKLIGHT DRIVER +M: Linus Walleij linus.walleij@linaro.org +S: Maintained +F: Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml +F: drivers/video/backlight/ktd253-backlight.c

L3MDEV M: David Ahern dsahern@kernel.org L: netdev@vger.kernel.org diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig index 7d22d7377606..6a74c60707b4 100644 --- a/drivers/video/backlight/Kconfig +++ b/drivers/video/backlight/Kconfig @@ -190,6 +190,14 @@ config BACKLIGHT_IPAQ_MICRO computers. Say yes if you have one of the h3100/h3600/h3700 machines.

+config BACKLIGHT_KTD253

• tristate "Backlight Driver for Kinetic KTD253"
• depends on GPIOLIB || COMPILE_TEST
• help

•  Say y to enabled the backlight driver for the Kinetic KTD253
  

•  which is a 1-wire GPIO-controlled backlight found in some mobile
  

•  phones.
  

config BACKLIGHT_LM3533 tristate "Backlight Driver for LM3533" depends on MFD_LM3533 diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile index 0c1a1524627a..d50cd12574ae 100644 --- a/drivers/video/backlight/Makefile +++ b/drivers/video/backlight/Makefile @@ -36,6 +36,7 @@ obj-$(CONFIG_BACKLIGHT_GPIO) += gpio_backlight.o 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_LM3533) += lm3533_bl.o obj-$(CONFIG_BACKLIGHT_LM3630A) += lm3630a_bl.o obj-$(CONFIG_BACKLIGHT_LM3639) += lm3639_bl.o diff --git a/drivers/video/backlight/ktd253-backlight.c b/drivers/video/backlight/ktd253-backlight.c new file mode 100644 index 000000000000..d460d1fef329 --- /dev/null +++ b/drivers/video/backlight/ktd253-backlight.c @@ -0,0 +1,254 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Backlight driver for the Kinetic KTD253
• • Based on code and know-how from the Samsung GT-S7710
• • Gareth Phillips gareth.phillips@samsung.com
• */

+#include <linux/backlight.h> +#include <linux/err.h> +#include <linux/fb.h> +#include <linux/gpio/consumer.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/limits.h>

In drm land these needs to be sorted. I do not think backlight demands it.

+/* Current ratio is n/32 from 1/32 to 32/32 */ +#define KTD253_MIN_RATIO 1 +#define KTD253_MAX_RATIO 32 +#define KTD253_DEFAULT_RATIO 13

+/* With the table we use this is 24/32 current ratio actually */ +#define KTD253_MAX_BRIGHTNESS 255 +#define KTD253_DEFAULT_BRIGHTNESS 160

+#define KTD253_T_LOW_NS (200 + 10) /* Additional 10ns as safety factor */ +#define KTD253_T_HIGH_NS (200 + 10) /* Additional 10ns as safety factor */ +#define KTD253_T_OFF_MS 3

+struct ktd253_backlight {

• struct device *dev;
• struct gpio_desc *gpiod;
• u16 ratio;

• unsigned int brightness;

brightness is not used - delete.

+};

I had expected to see a backlight pointer in the above structure. Like we do in most drivers.

+/*

• • The following table is used to convert brightness level to the LED
• • Current Ratio expressed as (full current) /(n * 32).
• • i.e. 1 = 1/32 full current. Zero indicates LED is powered off.
• • The table is intended to allow the brightness level to be "tuned"
• • to compensate for non-linearity of brightness relative to current.
• */

+static const u16 ktd253_brightness_to_current_ratio[] = {

• 0, /* (0/32) KTD253_BACKLIGHT_OFF */
• 39, /* (1/32) KTD253_MIN_RATIO */
• 58, /* (2/32) */
• 67, /* (3/32) */
• 76, /* (4/32) */
• 85, /* (5/32) */
• 94, /* (6/32) */
• 104, /* (7/32) */
• 113, /* (8/32) */
• 122, /* (9/32) */
• 131, /* (10/32) */
• 145, /* (11/32) */
• 159, /* (12/32) */
• 169, /* (13/32) */
• 179, /* (14/32) */
• 189, /* (15/32) */
• 196, /* (16/32) */
• 203, /* (17/32) */
• 210, /* (18/32) */
• 217, /* (19/32) */
• 224, /* (20/32) */
• 231, /* (21/32) */
• 238, /* (22/32) */
• 245, /* (23/32) */
• 255, /* (24/32) */
• 300, /* (25/32) */
• 300, /* (26/32) */
• 300, /* (27/32) */
• 300, /* (28/32) */
• 300, /* (29/32) */
• 300, /* (30/32) */
• 300, /* (31/32) */
• 300 /* (32/32) KTD253_MAX_RATIO */

+};

+/* Inspired by gpio_bl.c */ +static int ktd253_backlight_get_next_brightness(struct backlight_device *bl) +{

• int brightness = bl->props.brightness;
• if (bl->props.power != FB_BLANK_UNBLANK ||

•    bl->props.fb_blank != FB_BLANK_UNBLANK ||
  

•    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
  

• brightness = 0;
  

• return brightness;

+}

+static int ktd253_backlight_update_status(struct backlight_device *bl) +{

• struct ktd253_backlight *ktd253 = bl_get_data(bl);
• int brightness = ktd253_backlight_get_next_brightness(bl);

Use backligt_get_brightness() to get the brightness. Then you can delete ktd253_backlight_get_next_brightness()

• u16 target_ratio;
• u16 current_ratio = ktd253->ratio;
• unsigned long flags;
• dev_dbg(ktd253->dev, "new brightness: %d\n", brightness);
• /* Look up the current ratio */
• for (target_ratio = KTD253_MAX_RATIO; target_ratio > 0; target_ratio--) {

• if (brightness > ktd253_brightness_to_current_ratio[target_ratio - 1])
  

• 	break;
  

• }
• dev_dbg(ktd253->dev, "new ratio: %d/32\n", target_ratio);

Maybe only one print with both brightness and ratio?

• if (target_ratio == current_ratio)

• /* This is already right */
  

• return 0;
  

• if (target_ratio == 0) {

• gpiod_set_value_cansleep(ktd253->gpiod, 0);
  

• /*
  

•  * We need to keep the GPIO low for at least this long
  

•  * to actually switch the KTD253 off.
  

•  */
  

• msleep(KTD253_T_OFF_MS);
  

• ktd253->ratio = 0;
  

• return 0;
  

• }
• if (current_ratio == 0) {

• gpiod_set_value_cansleep(ktd253->gpiod, 1);
  

• ndelay(KTD253_T_HIGH_NS);
  

• /* We always fall back to this when we power on */
  

• current_ratio = KTD253_MAX_RATIO;
  

• }
• /*

• * WARNING:
  

• * The loop to set the correct current level is performed
  

• * with interrupts disabled as it is timing critical.
  

• * The maximum number of cycles of the loop is 32
  

• * so the time taken will be (T_LOW_NS + T_HIGH_NS + loop_time) * 32,
  

• */
  

• local_irq_save(flags);
• while (current_ratio != target_ratio) {

• /*
  

•  * These GPIO operations absolutely can NOT sleep so no
  

•  * _cansleep suffixes, and no using GPIO expanders on
  

•  * slow buses for this!
  

•  */
  

• gpiod_set_value(ktd253->gpiod, 0);
  

• ndelay(KTD253_T_LOW_NS);
  

• gpiod_set_value(ktd253->gpiod, 1);
  

• ndelay(KTD253_T_HIGH_NS);
  

• /* After 1/32 we loop back to 32/32 */
  

• if (current_ratio == KTD253_MIN_RATIO)
  

• 	current_ratio = KTD253_MAX_RATIO;
  

• else
  

• 	current_ratio--;
  

• }
• local_irq_restore(flags);
• ktd253->ratio = current_ratio;
• dev_dbg(ktd253->dev, "new ratio set to %d/32\n", target_ratio);
• return 0;

+}

+static const struct backlight_ops ktd253_backlight_ops = {

• .options = BL_CORE_SUSPENDRESUME,
• .update_status = ktd253_backlight_update_status,

+};

+static int ktd253_backlight_probe(struct platform_device *pdev) +{

• struct device *dev = &pdev->dev;
• struct backlight_device *bl;
• struct ktd253_backlight *ktd253;
• u32 max_brightness;
• u32 brightness;
• int ret;
• ktd253 = devm_kzalloc(dev, sizeof(*ktd253), GFP_KERNEL);
• if (!ktd253)

• return -ENOMEM;
  

• ktd253->dev = dev;
• ret = device_property_read_u32(dev, "max-brightness", &max_brightness);
• if (ret)

• max_brightness = KTD253_MAX_BRIGHTNESS;
  

• ret = device_property_read_u32(dev, "default-brightness", &brightness);
• if (ret)

• brightness = KTD253_DEFAULT_BRIGHTNESS;
  

• if (brightness)

• /* This will be the default ratio when the KTD253 is enabled */
  

• ktd253->ratio = KTD253_MAX_RATIO;
  

• else

• ktd253->ratio = 0;
  

• ktd253->gpiod = devm_gpiod_get(dev, NULL,

• 		       brightness ? GPIOD_OUT_HIGH :
  

• 		       GPIOD_OUT_LOW);
  

• if (IS_ERR(ktd253->gpiod)) {

• ret = PTR_ERR(ktd253->gpiod);
  

• if (ret != -EPROBE_DEFER)
  

• 	dev_err(dev, "gpio line missing or invalid.\n");
  

• return ret;
  

• }
• gpiod_set_consumer_name(ktd253->gpiod, dev_name(dev));
• bl = devm_backlight_device_register(dev, dev_name(dev), dev, ktd253,

• 			    &ktd253_backlight_ops, NULL);
  

• if (IS_ERR(bl)) {

• dev_err(dev, "failed to register backlight\n");
  

• return PTR_ERR(bl);
  

• }
• bl->props.max_brightness = max_brightness;
• /* When we just enable the GPIO line we set max brightness */
• if (brightness) {

• bl->props.brightness = brightness;
  

• bl->props.power = FB_BLANK_UNBLANK;
  

• ktd253_backlight_update_status(bl);
  

• } else {

• bl->props.brightness = 0;
  

• bl->props.power = FB_BLANK_POWERDOWN;
  

• }

Pass a backlight_properties to devm_backlight_device_register. So this is correct at init time.

FB_BLANK_* are constant used by the fb_blank icotl - and should not be used here. Do not assign props.power - as there is no change in power state to report.

In other words: Init backlight_properties with: - max_brightness - brightness - Type (RAW) Call devm_backlight_device_register()

Then unconditionally call backlight_update_status() (Not the local variant, go via backlight core)

The above is my understandig - but let the backlight people chime in too.

I would love if we could make it simpler to register a backlight device...

Sam

• platform_set_drvdata(pdev, bl);
• return 0;

+}

+static const struct of_device_id ktd253_backlight_of_match[] = {

• { .compatible = "kinetic,ktd253" },
• { /* sentinel */ }

+}; +MODULE_DEVICE_TABLE(of, ktd253_backlight_of_match);

+static struct platform_driver ktd253_backlight_driver = {

• .driver = {

• .name = "ktd253-backlight",
  

• .of_match_table = ktd253_backlight_of_match,
  

• },
• .probe = ktd253_backlight_probe,

+}; +module_platform_driver(ktd253_backlight_driver);

+MODULE_AUTHOR("Linus Walleij linus.walleij@linaro.org"); +MODULE_DESCRIPTION("Kinetic KTD253 Backlight Driver"); +MODULE_LICENSE("GPL");

+MODULE_ALIAS("platform:ktd253-backlight");

2.26.2

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel