Re: [PATCH v2 2/6] media: i2c: mt9p031: Rewrite a bitwise mask

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Sat, May 02, 2026 at 09:56:19AM +0200, Ricardo Ribalda wrote:
> On Fri, 1 May 2026 at 22:19, Laurent Pinchart wrote:
> > On Fri, May 01, 2026 at 11:32:47AM +0000, Ricardo Ribalda wrote:
> > > The current code makes smatch a bit uncomfortable:
> > > drivers/media/i2c/mt9p031.c:799 mt9p031_s_ctrl() warn: assigning (-1952) to unsigned variable 'data'
> > >
> > > Probably because smatch is not clever enough (yet). Do a simple rewrite
> > > to make sure that smatch understands what we are doing here.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/i2c/mt9p031.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> > > index ea5d43d925ff..5c9dff030b4d 100644
> > > --- a/drivers/media/i2c/mt9p031.c
> > > +++ b/drivers/media/i2c/mt9p031.c
> > > @@ -795,7 +795,7 @@ static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
> > >                       ctrl->val &= ~1;
> > >                       data = (1 << 6) | (ctrl->val >> 1);
> > >               } else {
> > > -                     ctrl->val &= ~7;
> > > +                     ctrl->val -= ctrl->val % 8;
> > >                       data = ((ctrl->val - 64) << 5) | (1 << 6) | 32;
> >
> > I'd still like to keep the ~7 (and, while at it, making the register
> > computation easier to read). I previously proposed
> >
> >                         ctrl->val &= ~7;
> >                         data = (ctrl->val - 64) >> 3;
> >                         data = (data << 8) | (1 << 6) | 32;
> >
> > which didn't quite appease smatch. We could use an explicit mask:
> >
> >                         ctrl->val &= ~7;
> >                         data = ((ctrl->val - 64) >> 3) & 0xff;
> 
> Why 0xff and not 0x7f?
> 
> If I understand it correctly the max is 1024 and (1024-64) >> 3 is < 127.

You're right. This won't matter in practice given that the control value
is bound by the minimum and maximum gains specified when creating the
control.

> Anyway... following the mask idea what about:
> 
> ctrl->val &= ~7;
> data = (((ctrl->val - 64) & 0x3ff) << 5) | (3 << 5);
> 
> ?

The digital gain is a 7-bit value stored in bits [14:8]. As the value
has to be divided by 8, the driver shifts left by 5. This is correct,
but I find it confusing for the reader (including myself, I had to pause
when reading this patch to understand the code). Hence the proposal to
improve readability.

Separating the (1 << 6) and 32 is also meant to make the code readable.
Bit 6 is the 1-bit second analog gain stage, and bits [5:0] store the
first stage analog gain.

> >                         data = (data << 8) | (1 << 6) | 32;
> >
> > >               }
> > >
> > >

-- 
Regards,

Laurent Pinchart