| 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: <CAK6c68gjzSytxwX5kUYLwbRceFgTHP-mynUSq1tNmUWD8n55Cg@mail.gmail.com>
Date: Tue, 4 Nov 2025 13:23:15 +0800
From: Junjie Cao <caojunjie650@...il.com>
To: Daniel Thompson <daniel@...cstar.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 Tue, Nov 4, 2025 at 12:30 AM Daniel Thompson <daniel@...cstar.com> wrote:
>
> 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.
>
Thanks for it, I tried to find something like FIELD_PREP() to avoid
keeping shift, but I failed to notice it in the bitfield.h. I will drop
the shift field and use it to handle bit operations gracefully.
>
> > + }
> > +
> > + 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`.
>
I see. Multiple instances should be taken into account.
>
> > +
> > + 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();
>
ACK. FIELD_PREP() makes more sense here.
Regards,
Junjie
Powered by blists - more mailing lists