| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAK6c68h3Mc0=JbbbVAmo_cYeOR_T-_rRy5EacgYQh7HgQZOPBg@mail.gmail.com>
Date: Wed, 29 Oct 2025 19:49:35 +0800
From: Junjie Cao <caojunjie650@...il.com>
To: Daniel Thompson <danielt@...nel.org>
Cc: Lee Jones <lee@...nel.org>, Jingoo Han <jingoohan1@...il.com>,
Pavel Machek <pavel@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Helge Deller <deller@....de>,
dri-devel@...ts.freedesktop.org, linux-leds@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-fbdev@...r.kernel.org, Pengyu Luo <mitltlatltl@...il.com>
Subject: Re: [PATCH 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight
On Tue, Oct 28, 2025 at 9:21 PM Daniel Thompson <danielt@...nel.org> wrote:
>
> On Sun, Oct 26, 2025 at 08:39:23PM +0800, Junjie Cao wrote:
> > Add support for Awinic AW99706 backlight, which can be found in
> > tablet and notebook backlight, one case is the Lenovo Legion Y700
> > Gen4. This driver refers to the official datasheets and android
> > driver, they can be found in [1].
> >
> > [1] https://www.awinic.com/en/productDetail/AW99706QNR
> >
> > Signed-off-by: Pengyu Luo <mitltlatltl@...il.com>
> > Signed-off-by: Junjie Cao <caojunjie650@...il.com>
> > ---
> > diff --git a/drivers/video/backlight/aw99706.c b/drivers/video/backlight/aw99706.c
> > new file mode 100644
> > index 000000000..8dafdea45
> > --- /dev/null
> > +++ b/drivers/video/backlight/aw99706.c
> > @@ -0,0 +1,503 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * aw99706 - Backlight driver for the AWINIC AW99706
> > + *
> > + * Copyright (C) 2025 Junjie Cao <caojunjie650@...il.com>
> > + * Copyright (C) 2025 Pengyu Luo <mitltlatltl@...il.com>
> > + *
> > + * Based on vendor driver:
> > + * Copyright (c) 2023 AWINIC Technology CO., LTD
> > + */
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#define AW99706_MAX_BRT_LVL 4095
> > +#define AW99706_REG_MAX 0x1F
> > +#define AW99706_ID 0x07
> > +
> > +/* registers list */
> > +#define AW99706_CFG0_REG 0x00
> > +#define AW99706_DIM_MODE_MASK GENMASK(1, 0)
> > +
> > +#define AW99706_CFG1_REG 0x01
> > +#define AW99706_SW_FREQ_MASK GENMASK(3, 0)
> > +#define AW99706_SW_ILMT_MASK GENMASK(5, 4)
> > +
> > +#define AW99706_CFG2_REG 0x02
> > +#define AW99706_ILED_MAX_MASK GENMASK(6, 0)
> > +#define AW99706_UVLOSEL_MASK BIT(7)
> > +
> > +#define AW99706_CFG3_REG 0x03
> > +#define AW99706_CFG4_REG 0x04
> > +#define AW99706_BRT_MSB_MASK GENMASK(3, 0)
> > +
> > +#define AW99706_CFG5_REG 0x05
> > +#define AW99706_BRT_LSB_MASK GENMASK(7, 0)
> > +
> > +#define AW99706_CFG6_REG 0x06
> > +#define AW99706_FADE_TIME_MASK GENMASK(2, 0)
> > +#define AW99706_SLOPE_TIME_MASK GENMASK(5, 3)
> > +#define AW99706_RAMP_CTL_MASK GENMASK(7, 6)
> > +
> > +#define AW99706_CFG7_REG 0x07
> > +#define AW99706_BRT_MODE_MASK GENMASK(1, 0)
> > +
> > +#define AW99706_CFG8_REG 0x08
> > +#define AW99706_ONOFF_TIME_MASK GENMASK(2, 0)
> > +
> > +#define AW99706_CFG9_REG 0x09
> > +#define AW99706_CFGA_REG 0x0A
> > +#define AW99706_CFGB_REG 0x0B
> > +#define AW99706_CFGC_REG 0x0C
> > +#define AW99706_CFGD_REG 0x0D
> > +#define AW99706_FLAG_REG 0x10
> > +#define AW99706_BACKLIGHT_EN_MASK BIT(7)
> > +
> > +#define AW99706_CHIPID_REG 0x11
> > +#define AW99706_LED_OPEN_FLAG_REG 0x12
> > +#define AW99706_LED_SHORT_FLAG_REG 0x13
> > +#define AW99706_MTPLDOSEL_REG 0x1E
> > +#define AW99706_MTPRUN_REG 0x1F
> > +
> > +#define RESV 0
> > +
> > +/* Boost switching frequency table, in kHz */
> > +static const u32 aw99706_sw_freq_tbl[] = {
> > + RESV, RESV, RESV, RESV, 300, 400, 500, 600,
> > + 660, 750, 850, 1000, 1200, 1330, 1500, 1700
> > +};
> > +
> > +/* Switching current limitation table, in mA */
> > +static const u32 aw99706_sw_ilmt_tbl[] = {
> > + 1500, 2000, 2500, 3000
> > +};
> > +
> > +/* ULVO threshold table, in mV */
> > +static const u32 aw99706_ulvo_thres_tbl[] = {
> > + 2200, 5000
> > +};
> > +
> > +/* Fade In/Out time table, in us */
> > +static const u32 aw99706_fade_time_tbl[] = {
> > + 8, 16, 32, 64, 128, 256, 512, 1024
> > +};
> > +
> > +/* Slope time table, in ms */
> > +static const u32 aw99706_slopetime_tbl[] = {
> > + 8, 24, 48, 96, 200, 300, 400, 500
> > +};
> > +
> > +/* Turn on/off time table, in ns */
> > +static const u32 aw99706_onoff_time_tbl[] = {
> > + RESV, 250, 500, 1000, 2000, 4000, 8000, 16000
> > +};
> > +
> > +struct aw99706_device {
> > + struct i2c_client *client;
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct backlight_device *bl_dev;
> > + struct gpio_desc *hwen_gpio;
> > + bool bl_enable;
> > +};
> > +
> > +enum reg_access {
> > + REG_NONE_ACCESS = 0,
> > + REG_RD_ACCESS = 1,
> > + REG_WR_ACCESS = 2,
> > +};
> > +
> > +struct aw99706_reg {
> > + u8 defval;
> > + u8 access;
> > +};
> > +
> > +const struct aw99706_reg aw99706_regs[AW99706_REG_MAX + 1] = {
> > + [AW99706_CFG0_REG] = {0x65, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG1_REG] = {0x39, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG2_REG] = {0x1e, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG3_REG] = {0x04, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG4_REG] = {0x00, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG5_REG] = {0x00, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG6_REG] = {0xa9, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG7_REG] = {0x04, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG8_REG] = {0x0c, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFG9_REG] = {0x4b, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFGA_REG] = {0x72, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFGB_REG] = {0x01, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFGC_REG] = {0x6c, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_CFGD_REG] = {0xfe, REG_RD_ACCESS | REG_WR_ACCESS},
> > + [AW99706_FLAG_REG] = {0x00, REG_RD_ACCESS},
> > + [AW99706_CHIPID_REG] = {AW99706_ID, REG_RD_ACCESS},
> > + [AW99706_LED_OPEN_FLAG_REG] = {0x00, REG_RD_ACCESS},
> > + [AW99706_LED_SHORT_FLAG_REG] = {0x00, REG_RD_ACCESS},
> > +
> > + /*
> > + * Write bit is dropped here, writing BIT(0) to MTPLDOSEL will unlock
> > + * Multi-time Programmable (MTP).
> > + */
> > + [AW99706_MTPLDOSEL_REG] = {0x00, REG_RD_ACCESS},
> > + [AW99706_MTPRUN_REG] = {0x00, REG_NONE_ACCESS},
> > +};
> > +
> > +static bool aw99706_readable_reg(struct device *dev, unsigned int reg)
> > +{
> > + return aw99706_regs[reg].access & REG_RD_ACCESS;
> > +}
> > +
> > +static bool aw99706_writeable_reg(struct device *dev, unsigned int reg)
> > +{
> > + return aw99706_regs[reg].access & REG_WR_ACCESS;
> > +}
> > +
> > +static inline int aw99706_i2c_read(struct aw99706_device *aw, u8 reg,
> > + unsigned int *val)
> > +{
> > + return regmap_read(aw->regmap, reg, val);
> > +}
> > +
> > +static inline int aw99706_i2c_write(struct aw99706_device *aw, u8 reg, u8 val)
> > +{
> > + return regmap_write(aw->regmap, reg, val);
> > +}
> > +
> > +static inline int aw99706_i2c_update_bits(struct aw99706_device *aw, u8 reg,
> > + u8 mask, u8 val)
> > +{
> > + return regmap_update_bits(aw->regmap, reg, mask, val);
> > +}
> > +
> > +struct aw99706_dt_prop {
> > + const char * const name;
> > + const u32 * const lookup_tbl;
> > + u8 tbl_size;
> > + u8 reg;
> > + u8 mask;
> > + u8 val;
> > + u32 raw_val;
> > +};
> > +
> > +static struct aw99706_dt_prop aw99706_dt_props[] = {
> > + {
> > + "awinic,dim-mode", NULL,
> > + 0,
> > + AW99706_CFG0_REG, AW99706_DIM_MODE_MASK
> > + },
> > + {
> > + "awinic,sw-freq", aw99706_sw_freq_tbl,
> > + ARRAY_SIZE(aw99706_sw_freq_tbl),
> > + AW99706_CFG1_REG, AW99706_SW_FREQ_MASK
> > + },
> > + {
> > + "awinic,sw-ilmt", aw99706_sw_ilmt_tbl,
> > + ARRAY_SIZE(aw99706_sw_ilmt_tbl),
> > + AW99706_CFG1_REG, AW99706_SW_ILMT_MASK
> > + },
> > + {
> > + "awinic,iled-max", NULL,
> > + 0,
> > + AW99706_CFG2_REG, AW99706_ILED_MAX_MASK
> > +
> > + },
> > + {
> > + "awinic,uvlo-thres", aw99706_ulvo_thres_tbl,
> > + ARRAY_SIZE(aw99706_ulvo_thres_tbl),
> > + AW99706_CFG2_REG, AW99706_UVLOSEL_MASK
> > + },
> > + {
> > + "awinic,fade-time", aw99706_fade_time_tbl,
> > + ARRAY_SIZE(aw99706_fade_time_tbl),
> > + AW99706_CFG6_REG, AW99706_FADE_TIME_MASK
> > + },
> > + {
> > + "awinic,slope-time", aw99706_slopetime_tbl,
> > + ARRAY_SIZE(aw99706_slopetime_tbl),
> > + AW99706_CFG6_REG, AW99706_SLOPE_TIME_MASK
> > + },
> > + {
> > + "awinic,ramp-ctl", NULL,
> > + 0,
> > + AW99706_CFG6_REG, AW99706_RAMP_CTL_MASK
> > + },
> > + {
> > + "awinic,brt-mode", NULL,
> > + 0,
> > + AW99706_CFG7_REG, AW99706_BRT_MODE_MASK
> > + },
> > + {
> > + "awinic,onoff-time", aw99706_onoff_time_tbl,
> > + ARRAY_SIZE(aw99706_onoff_time_tbl),
> > + AW99706_CFG8_REG, AW99706_ONOFF_TIME_MASK
> > + },
> > +};
> > +
> > +static int aw99706_lookup(const u32 * const tbl, int size, u32 val)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < size; i++)
> > + if (tbl[i] == val)
> > + return i;
> > +
> > + return -1;
> > +}
> > +
> > +static inline void aw99706_prop_set_default(struct aw99706_dt_prop *prop)
> > +{
> > + prop->val = prop->mask & aw99706_regs[prop->reg].defval;
>
> Why included the default value in the register descriptions?
>
> defval is only used to provide values for missing DT properties so using
> the raw register values is cryptic and hard to read.
>
> Including a default value in the aw99706_dt_props table instead would be
> much more readable (because the defaults could use the same units at the
> device tree).
>
Agree, I will include the default values in the aw99706_dt_props table.
>
> > +}
> > +
> > +static void aw99706_dt_property_convert(struct aw99706_dt_prop *prop)
> > +{
> > + unsigned int val, shift;
> > +
> > + if (prop->lookup_tbl) {
> > + val = aw99706_lookup(prop->lookup_tbl, prop->tbl_size,
> > + prop->raw_val);
> > + if (val < 0) {
> > + aw99706_prop_set_default(prop);
>
> This should not happen silently.
>
> If the DT has provided an invalid value then we be issuing *at minimum*
> a message at warning level or above. Many drivers will simply refuse to
> probe when the DT is broken.
>
Indeed, I missed it.
>
> > + return;
> > + }
> > +
> > + } else {
> > + val = prop->raw_val;
> > + }
> > +
> > + shift = ffs(prop->mask) - 1;
> > + val <<= shift;
> > + prop->val = prop->mask & val;
> > +}
> > +
> > +static void aw99706_dt_parse(struct aw99706_device *aw)
> > +{
> > + struct aw99706_dt_prop *prop;
> > + int ret, i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> > + prop = &aw99706_dt_props[i];
> > + ret = device_property_read_u32(aw->dev, prop->name,
> > + &prop->raw_val);
> > + if (ret < 0) {
> > + dev_warn(aw->dev, "Missing property %s: %d\n",
> > + prop->name, ret);
>
> Why is there a warning when an optional property is not present. A DT
> not including an optional property needs no message at all.
>
They are mandatory in the downstream, and providing all properties is
difficult sometimes, so I set a default value if one is missing. But
one device may use a configuration different from the component
vendor's. These default values may be not optimal, so I issue a
warning for property missing. (I forgot to address it)
>
> > +
> > + aw99706_prop_set_default(prop);
> > + } else {
> > + aw99706_dt_property_convert(prop);
> > + }
> > + }
> > +
> > + /* This property requires a long linear array, using formula for now */
> > + aw99706_dt_props[3].val = (aw99706_dt_props[3].raw_val - 5000) / 500;
>
> Using a formula is fine, but I don't like doing it retrospectively.
> Hard coding the 3 makes maintenance difficult and we end up making the
> whole of aw99706_dt_props writeable just so we can store raw_val once!
>
> Much better, IMHO, to embed a function pointer into the table and make
> the whole table const. The function pointer can be
> aw99706_dt_property_convert() in most cases (although rename it
> `aw99706_dt_property_lookup_from_table() ) and can implement any
> formula you need.
>
Helpful opinion. I will do this in next version.
>
> > +}
> > +
> > +static int aw99706_hw_init(struct aw99706_device *aw)
> > +{
> > + int ret, i;
> > +
> > + gpiod_set_value_cansleep(aw->hwen_gpio, 1);
> > +
> > + for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> > + ret = aw99706_i2c_update_bits(aw, aw99706_dt_props[i].reg,
> > + aw99706_dt_props[i].mask,
> > + aw99706_dt_props[i].val);
> > + if (ret < 0) {
> > + dev_err(aw->dev, "Failed to write init data %d\n", ret);
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int aw99706_bl_enable(struct aw99706_device *aw, bool en)
> > +{
> > + int ret;
> > + u8 val;
> > +
> > + FIELD_MODIFY(AW99706_BACKLIGHT_EN_MASK, &val, en);
> > + ret = aw99706_i2c_update_bits(aw, AW99706_CFGD_REG,
> > + AW99706_BACKLIGHT_EN_MASK, val);
> > + if (ret)
> > + dev_err(aw->dev, "Failed to enable backlight!\n");
> > +
> > + return ret;
> > +}
> > +
> > +static int aw99706_backlight_switch(struct aw99706_device *aw, u32 brt_lvl)
> > +{
> > + bool bl_enable_now = !!brt_lvl;
> > + int ret = 0;
> > +
> > + if (aw->bl_enable != bl_enable_now) {
> > + aw->bl_enable = bl_enable_now;
> > + ret = aw99706_bl_enable(aw, bl_enable_now);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int aw99706_update_brightness(struct aw99706_device *aw, u32 brt_lvl)
> > +{
> > + int ret;
> > +
> > + ret = aw99706_i2c_write(aw, AW99706_CFG4_REG,
> > + (brt_lvl >> 8) & AW99706_BRT_MSB_MASK);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = aw99706_i2c_write(aw, AW99706_CFG5_REG,
> > + brt_lvl & AW99706_BRT_LSB_MASK);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return aw99706_backlight_switch(aw, brt_lvl);
>
> I'm not sure there is much benefit pushing this out into a seperate
> function. Merge this inline.
>
> > +}
>
I see.
Regards,
Junjie
Powered by blists - more mailing lists