Re: [PATCH v3 2/2] backlight: Add new lm3509 backlight driver

[Sam Ravnborg <sam@ravnborg.org>]

Hi Patrick,

a few comments in the following.

	Sam

On Sat, Mar 09, 2024 at 02:24:56PM +0100, Patrick Gansterer wrote:
> This is a general driver for LM3509 backlight chip of TI.
> LM3509 is High Efficiency Boost for White LEDs and/or OLED Displays with
> Dual Current Sinks. This driver supports OLED/White LED select, brightness
> control and sub/main control.
> The datasheet can be found at http://www.ti.com/product/lm3509.
> 
> Signed-off-by: Patrick Gansterer <paroga@paroga.com>
> ---
>  drivers/video/backlight/Kconfig     |   7 +
>  drivers/video/backlight/Makefile    |   1 +
>  drivers/video/backlight/lm3509_bl.c | 340 ++++++++++++++++++++++++++++
>  3 files changed, 348 insertions(+)
>  create mode 100644 drivers/video/backlight/lm3509_bl.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index ea2d0d69bd8c..96ad5dc584b6 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -366,6 +366,13 @@ config BACKLIGHT_AAT2870
>  	  If you have a AnalogicTech AAT2870 say Y to enable the
>  	  backlight driver.
>  
> +config BACKLIGHT_LM3509
> +	tristate "Backlight Driver for LM3509"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  This supports TI LM3509 Backlight Driver
> +
>  config BACKLIGHT_LM3630A
>  	tristate "Backlight Driver for LM3630A"
>  	depends on I2C && PWM
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 06966cb20459..51a4ac5d0530 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -35,6 +35,7 @@ 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_KTZ8866)		+= ktz8866.o
> +obj-$(CONFIG_BACKLIGHT_LM3509)		+= lm3509_bl.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/lm3509_bl.c b/drivers/video/backlight/lm3509_bl.c
> new file mode 100644
> index 000000000000..bfad0aaffa0d
> --- /dev/null
> +++ b/drivers/video/backlight/lm3509_bl.c
> @@ -0,0 +1,340 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define LM3509_NAME "lm3509_bl"
> +
> +#define LM3509_SINK_MAIN 0
> +#define LM3509_SINK_SUB 1
> +#define LM3509_NUM_SINKS 2
> +
> +#define LM3509_DEF_BRIGHTNESS 0x12
> +#define LM3509_MAX_BRIGHTNESS 0x1F
> +
> +#define REG_GP 0x10
> +#define REG_BMAIN 0xA0
> +#define REG_BSUB 0xB0
> +#define REG_MAX 0xFF
> +
> +enum {
> +	REG_GP_ENM_BIT = 0,
> +	REG_GP_ENS_BIT,
> +	REG_GP_UNI_BIT,
> +	REG_GP_RMP0_BIT,
> +	REG_GP_RMP1_BIT,
> +	REG_GP_OLED_BIT,
> +};
> +
> +struct lm3509_bl {
> +	struct regmap *regmap;
> +	struct backlight_device *bl_main;
> +	struct backlight_device *bl_sub;
> +	struct gpio_desc *reset_gpio;
> +};
> +
> +struct lm3509_bl_led_pdata {
> +	const char *label;
> +	int led_sources;
> +	u32 brightness;
> +	u32 max_brightness;
> +};
> +
> +static void lm3509_reset(struct lm3509_bl *data)
> +{
> +	if (data->reset_gpio) {
> +		gpiod_set_value(data->reset_gpio, 1);
> +		udelay(1);
> +		gpiod_set_value(data->reset_gpio, 0);
> +		udelay(10);
> +	}
> +}
> +
> +static int lm3509_update_status(struct backlight_device *bl,
> +				unsigned int en_mask, unsigned int br_reg)
> +{
> +	struct lm3509_bl *data = bl_get_data(bl);
> +	int ret;
> +	bool en;
> +
> +	ret = regmap_write(data->regmap, br_reg, bl->props.brightness);

Here you can use backlight_get_brightness() thus avoiding direct access
to backlight internal properties.

> +	if (ret < 0)
> +		return ret;
> +
> +	en = bl->props.power <= FB_BLANK_NORMAL;
Use backlight_is_blank() here.

	Sam



> +	return regmap_update_bits(data->regmap, REG_GP, en_mask,
> +				  en ? en_mask : 0);
> +}
> +
> +static int lm3509_main_update_status(struct backlight_device *bl)
> +{
> +	return lm3509_update_status(bl, BIT(REG_GP_ENM_BIT), REG_BMAIN);
> +}
> +
> +static const struct backlight_ops lm3509_main_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,
> +	.update_status = lm3509_main_update_status,
> +};
> +
> +static int lm3509_sub_update_status(struct backlight_device *bl)
> +{
> +	return lm3509_update_status(bl, BIT(REG_GP_ENS_BIT), REG_BSUB);
> +}
> +
> +static const struct backlight_ops lm3509_sub_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,
> +	.update_status = lm3509_sub_update_status,
> +};
> +
> +static struct backlight_device *
> +lm3509_backlight_register(struct device *dev, const char *name_suffix,
> +			  struct lm3509_bl *data,
> +			  const struct backlight_ops *ops,
> +			  const struct lm3509_bl_led_pdata *pdata)
> +
> +{
> +	struct backlight_device *bd;
> +	struct backlight_properties props;
> +	const char *label = pdata->label;
> +	char name[64];
> +
> +	memset(&props, 0, sizeof(props));
> +	props.type = BACKLIGHT_RAW;
> +	props.brightness = pdata->brightness;
> +	props.max_brightness = pdata->max_brightness;
> +	props.power = pdata->brightness > 0 ? FB_BLANK_UNBLANK :
> +					      FB_BLANK_POWERDOWN;
props.power is not supposed to be set by the user - is is maintained by
the backlight core.


	Sam