Re: [PATCH v3 06/17] media: i2c: os05b10: Add H/V flip support

[Tarang Raval]

Hi Jai, Mehdi,

Thanks for the review.

> > +       case V4L2_CID_HFLIP:
> > +       case V4L2_CID_VFLIP:
> > +               ret = cci_update_bits(os05b10->cci, OS05B10_REG_FORMAT1,
> > +                                     OS05B10_MIRROR | OS05B10_FLIP,
> > +                                     (!os05b10->hflip->val ?
> > +                                      OS05B10_MIRROR : 0) |
> > +                                     (os05b10->vflip->val ?
> > +                                      OS05B10_FLIP : 0), NULL);
> > +
> > +               ret |= cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
> > +                                (os05b10->vflip->val == 1) ?
> > +                                OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
> > +                                NULL);
> > +               break;
> >         default:
> >                 ret = -EINVAL;
> >                 break;
>
> Please update the active state format on HFLIP/VFLIP, so the subsequent
> G_FMT calls show the correct format.
>
> Many drivers miss this today, I assume because libcamera usually calls
> S_FMT on streaming using the output of ENUM_FMT.

Good point. I'll update the active state so subsequent G_FMT calls report
the correct format.

...

> > +
> > +             ret |= cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
> > +                              (os05b10->vflip->val == 1) ?
> > +                              OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
> > +                              NULL);
>
> I don't know about the ret |= assignment
>
> You can use the last parameter of the cci_*() helpers. That would make
> this cleaner and not combine two error codes to produce something weird.
>
> If you pass &ret to cci_write() the helper will check if an error
> already occured and return it.

Makes sense. I'll update this in v4.

Best Regards,
Tarang