| 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: <aQjY5_uEaTv4_L2s@aspen.lan>
Date: Mon, 3 Nov 2025 16:31:35 +0000
From: Daniel Thompson <daniel@...cstar.com>
To: Junjie Cao <caojunjie650@...il.com>
Cc: Lee Jones <lee@...nel.org>, Daniel Thompson <danielt@...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 v2 2/2] backlight: aw99706: Add support for Awinic
AW99706 backlight
On Mon, Nov 03, 2025 at 07:06:48PM +0800, Junjie Cao wrote:
> From: Pengyu Luo <mitltlatltl@...il.com>
>
> 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>
> ---
> Changes in v2:
> - add handler for max-brightness and default-brightness
> - use proper units for properties (Krzysztof)
> - drop non-fixed properties (Krzysztof)
> - include default values in the aw99706_dt_props table (Daniel)
> - warn when a property value from DT is invalid (Daniel)
> - drop warning when optional properties are missing (Daniel)
> - add a function pointer into the aw99706_dt_props table to handle lookup (Daniel)
> - use a lookup function instead of hardcoding the formula for the iLED max (Daniel)
> - move BL enalbe handler into aw99706_update_brightness (Daniel)
> - Link to v1: https://lore.kernel.org/linux-leds/20251026123923.1531727-3-caojunjie650@gmail.com
Thanks for the changes.
I'm afraid I don't like encoding the `shift` in the DT properties table.
Caching something that is so easy to recalculate makes no sense to me.
See below:
> +struct aw99706_dt_prop {
> + const char * const name;
> + int (*lookup)(const struct aw99706_dt_prop *prop, u32 dt_val, u8 *val);
> + const u32 * const lookup_tbl;
> + u8 tbl_size;
> + u8 reg;
> + u8 mask;
> + u8 shift;
There should bee no need to record `shift` here. It's just a
duplicating information already held in `mask`.
> + u32 def_val;
> +};
> +
> +static int aw99706_dt_property_lookup(const struct aw99706_dt_prop *prop,
> + u32 dt_val, u8 *val)
> +{
> + int i;
> +
> + if (!prop->lookup_tbl) {
> + *val = dt_val;
> + return 0;
> + }
> +
> + for (i = 0; i < prop->tbl_size; i++)
> + if (prop->lookup_tbl[i] == dt_val)
> + break;
> +
> + *val = i;
> +
> + return i == prop->tbl_size ? -1 : 0;
> +}
> +
> +#define MIN_ILED_MAX 5000
> +#define MAX_ILED_MAX 50000
> +#define STEP_ILED_MAX 500
> +
> +static int
> +aw99706_dt_property_iled_max_convert(const struct aw99706_dt_prop *prop,
> + u32 dt_val, u8 *val)
> +{
> + if (dt_val > MAX_ILED_MAX || dt_val < MIN_ILED_MAX)
> + return -1;
> +
> + *val = (dt_val - MIN_ILED_MAX) / STEP_ILED_MAX;
> +
> + return (dt_val - MIN_ILED_MAX) % STEP_ILED_MAX;
> +}
> +
> +static const struct aw99706_dt_prop aw99706_dt_props[] = {
> + {
> + "awinic,dim-mode", aw99706_dt_property_lookup,
> + NULL, 0,
> + AW99706_CFG0_REG,
> + AW99706_DIM_MODE_MASK, __builtin_ctz(AW99706_DIM_MODE_MASK),
These __builtin_ctz() calls shouldn't be in the lookup table (if they
are not in the lookup table then can never be inconsistant with the
mask).
> + 1,
> + },
<snip>
> + {
> + "awinic,ramp-ctl", aw99706_dt_property_lookup,
> + NULL, 0,
> + AW99706_CFG6_REG,
> + AW99706_RAMP_CTL_MASK, __builtin_ctz(AW99706_RAMP_CTL_MASK),
> + 2,
> + },
> +};
> +
> +struct reg_init_data {
> + u8 reg;
> + u8 mask;
> + u8 val;
> +};
> +
> +static struct reg_init_data reg_init_tbl[ARRAY_SIZE(aw99706_dt_props)];
> +
> +static void aw99706_dt_parse(struct aw99706_device *aw,
> + struct backlight_properties *bl_props)
> +{
> + const struct aw99706_dt_prop *prop;
> + u32 dt_val;
> + int ret, i;
> + u8 val;
> +
> + for (i = 0; i < ARRAY_SIZE(aw99706_dt_props); i++) {
> + prop = &aw99706_dt_props[i];
> + ret = device_property_read_u32(aw->dev, prop->name, &dt_val);
> + if (ret < 0)
> + dt_val = prop->def_val;
> +
> + if (prop->lookup(prop, dt_val, &val)) {
> + dev_warn(aw->dev, "invalid value %d for property %s, using default value %d\n",
> + dt_val, prop->name, prop->def_val);
> +
> + prop->lookup(prop, prop->def_val, &val);
> + }
> +
> + reg_init_tbl[i].reg = prop->reg;
> + reg_init_tbl[i].mask = prop->mask;
> + reg_init_tbl[i].val = val << prop->shift;
Can't you just use FIELD_PREP() to set val (either here or at the point
the init table is consumed)? That why there's no ffs() or clz() at all.
> + }
> +
> + aw->init_tbl = reg_init_tbl;
> + aw->init_tbl_size = ARRAY_SIZE(reg_init_tbl);
Copying a pointer to a single instance static data buffer into a
dynamically allocated data structure isn't right.
You should include the init table as part of `struct aw99706_device`.
> +
> + bl_props->brightness = AW99706_MAX_BRT_LVL >> 1;
> + bl_props->max_brightness = AW99706_MAX_BRT_LVL;
> + device_property_read_u32(aw->dev, "default-brightness",
> + &bl_props->brightness);
> + device_property_read_u32(aw->dev, "max-brightness",
> + &bl_props->max_brightness);
> +
> + if (bl_props->max_brightness > AW99706_MAX_BRT_LVL)
> + bl_props->max_brightness = AW99706_MAX_BRT_LVL;
> +
> + if (bl_props->brightness > bl_props->max_brightness)
> + bl_props->brightness = bl_props->max_brightness;
> +}
> +
> +static int aw99706_hw_init(struct aw99706_device *aw)
> +{
> + int ret, i;
> +
> + gpiod_set_value_cansleep(aw->hwen_gpio, 1);
> +
> + for (i = 0; i < aw->init_tbl_size; i++) {
> + ret = aw99706_i2c_update_bits(aw, aw->init_tbl[i].reg,
> + aw->init_tbl[i].mask,
> + aw->init_tbl[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);
This should use FIELD_PREP() not FIELD_MODIFY();
> + 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;
> +}
Daniel.
Powered by blists - more mailing lists