Re: [PATCH v3 18/29] media: bcm2835-unicam: Remove frame descriptor workaround

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hej Linus,

On Fri, Sep 04, 2026 at 12:43:29PM +0200, Linus Walleij wrote:
> Hi Sakari,
> 
> thanks for your patch!
> 
> On Mon, Aug 24, 2026 at 2:14 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> 
> > As the v4l2_subdev_get_frame_desc() either returns a frame descriptor or
> > returns an error, remove the workaround for drivers that didn't implement
> > the get_frame_desc() pad op. Also handle v4l2_subdev_get_frame_desc()
> > errors.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> (...)
> 
> > @@ -933,7 +933,11 @@ static void unicam_start_rx(struct unicam_device *unicam,
> >         fmtinfo = unicam_find_format_by_code(fmt->code,
> >                                              UNICAM_SD_PAD_SOURCE_IMAGE);
> >         if (WARN_ON(!fmtinfo))
> > -               return;
> > +               return -EINVAL;
> > +
> > +       ret = unicam_get_image_vc_dt(unicam, state, &vc, &dt);
> > +       if (ret)
> > +               return ret;
> 
> vs
> 
> > -       ret = unicam_get_image_vc_dt(unicam, state, &vc, &dt);
> > -       if (ret) {
> > -               /*
> > -                * If the source doesn't support frame descriptors, default to
> > -                * VC 0 and use the DT corresponding to the format.
> > -                */
> > -               vc = 0;
> > -               dt = fmtinfo->csi_dt;
> > -       }
> 
> I *think* what Frank is pointing out is that vc=0 and dt=0
> needs to be in this patch so we see that
> the new code works when using CCP2 data?

Indeed, this is one of the few devices that actually support CCP2. The
other one I know is omap3isp. I think then dropping this patch makes sense
instead of adding support for CCP2 in frame descriptors.

> 
> Like here:
> 
> static void unicam_cfg_image_id(struct unicam_device *unicam, u8 vc, u8 dt)
> {
>         if (unicam->bus_type == V4L2_MBUS_CSI2_DPHY) {
>                 /* CSI2 mode  */
>                 unicam_reg_write(unicam, UNICAM_IDI0, (vc << 6) | dt);
>         } else {
>                 /* CCP2 mode */
>                 unicam_reg_write(unicam, UNICAM_IDI0, 0x80 | dt);
>         }
> }
> 
> Other than that it LGTM (OK may not be worth much, I'm a bit
> lost in the details here):
> Reviewed-by: Linus Walleij <linusw@kernel.org>
> 
> Yours,
> Linus Walleij

-- 
Sakari Ailus