Hi Laurent,
On Sat, Nov 11, 2023 at 12:53:35AM +0200, Laurent Pinchart wrote:
> > > > @@ -3653,7 +3649,7 @@ static int ccs_module_init(void)
> > > > ccs_limit_offsets[l + 1].lim =
> > > > ALIGN(ccs_limit_offsets[l].lim +
> > > > ccs_limits[i].size,
> > > > - ccs_reg_width(ccs_limits[i + 1].reg));
> > > > + max(CCI_REG_WIDTH_BYTES(ccs_limits[i + 1].reg), 1UL));
> > >
> > > What's the reason for the max() here ?
> >
> > The guardian entry of the ccs_limits array contains zero in the reg field,
> > as one would expect, and CCI_REG_WIDTH_BYTES() returns, as one would
> > expect, zero. And aligning by zero is generally shunned by experienced
> > kernel developers.
> >
> > ccs_reg_width() used flags for 16- and 32-bit registers so a register that
> > had neither of the flags was considered an 8-bit one.
>
> Could you instead process the sentinel outside of the loop ? It sounds
> less error-prone.
I'd need to make it a special case within the loop. I prefer to keep it
as-is. But I've added a comment on this there.
There's a check of the integrity of the data structure and that fails when
accessing the information if the last limit somehow gets mangled.
> > > > int ccs_write_addr(struct ccs_sensor *sensor, u32 reg, u32 val)
> > > > {
> > > > + unsigned int retries = 10;
> > >
> > > This is really not nice, but unrelated to this patch.
Some writes occasionally fail right after power-on (or reset) on some
sensors. This could be handled in power-on and one could hope it doesn't
happen (often) elsewhere. Outside the scope of this set though.
> > >
> > > > int rval;
> > > >
> > > > rval = ccs_call_quirk(sensor, reg_access, true, ®, &val);
> > > > @@ -364,7 +218,12 @@ int ccs_write_addr(struct ccs_sensor *sensor, u32 reg, u32 val)
> > > > if (rval < 0)
> > > > return rval;
> > >
> > > Here you test rval < 0, but below cci_write() will consider any positive
> > > value as an error too. There may not be an actual bug if the function
> > > call above doesn't return positive values, but it's error-prone
> > > nonetheless.
> >
> > Good point. The quirk function is supposed to return either an error or
> > zero, but I'll assign it to zero here just in case.
>
> You could also change the check to
>
> if (rval)
> return rval;
I'd like to return either an error or 0 here, not a positive value. They
often are related, as you noted, to a bug.
--
Regards,
Sakari Ailus
