Re: [PATCH v2 2/2] backlight: aw99706: Add support for Awinic AW99706 backlight

[Daniel Thompson <daniel@riscstar.com>]

On Mon, Nov 03, 2025 at 07:06:48PM +0800, Junjie Cao wrote:
> From: Pengyu Luo <mitltlatltl@gmail.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@gmail.com>
> Signed-off-by: Junjie Cao <caojunjie650@gmail.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.