Re: [PATCH v6 4/5] media: platform: mediatek: isp_30: add mediatek ISP3.0 camsv

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

Hi CK,

On Wed, Aug 07, 2024 at 01:31:57AM +0000, CK Hu (胡俊光) wrote:
> On Wed, 2024-07-31 at 11:29 +0300, Laurent Pinchart wrote:
> > On Wed, Jul 31, 2024 at 02:59:51AM +0000, CK Hu (胡俊光) wrote:
> > > On Mon, 2024-07-29 at 16:48 +0200, Julien Stephan wrote:
> > > >  From: Phi-bang Nguyen <pnguyen@baylibre.com>
> > > >
> > > > This driver provides a path to bypass the SoC ISP so that image data
> > > > coming from the SENINF can go directly into memory without any image
> > > > processing. This allows the use of an external ISP.
> > > >
> > > > Signed-off-by: Phi-bang Nguyen <pnguyen@baylibre.com>
> > > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > > > [Paul Elder fix irq locking]
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Co-developed-by: Julien Stephan <jstephan@baylibre.com>
> > > > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > +static void mtk_cam_cmos_vf_enable(struct mtk_cam_dev *cam_dev,
> > > > +   bool enable, bool pak_en)
> > > > +{
> > > > +struct device *dev = cam_dev->dev;
> > > > +
> > > > +if (pm_runtime_get_sync(dev) < 0) {
> > > > +dev_err(dev, "failed to get pm_runtime\n");
> > > > +goto out;
> > > > +}
> > > > +
> > > > +if (enable)
> > > > +cam_dev->hw_functions->mtk_cam_cmos_vf_hw_enable(cam_dev);
> > >
> > > Directly call mtk_camsv30_cmos_vf_hw_enable().
> >
> > The goal, when this was developed, was to support multiple generations
> > of hardware with a single driver. I think it's a worthwhile goal, but at
> > the same time, I'm not sure that will ever happen as I'm not aware of
> > plans to upstream Genio 350 and 500 support (which is a bad sad, as it's
> > more or less working out-of-tree). I'm thus fine either way, and if we
> > think the most likely outcome is that this driver will only support
> > Genio 300, I'm fine dropping the abstraction layer.
> 
> I know this goal.
> For the mtk_camsv_30_setup(), in new SoC, if only one line in this function is different,
> should we duplicate the whole function and modify only one line?
> I think we don't know what would happen in future,
> so we should not design for something which we have no any information.

For future platforms, I fully agree with you. For Genio 350 and 500 we
have already identified some common elements. However, as there's no
point to upstream those at the moment, and as we can't review an
abstraction layer properly if support for only a single platform is
available, I'm fine dropping the abstraction.

> > > > +else
> > > > +cam_dev->hw_functions->mtk_cam_cmos_vf_hw_disable(cam_dev);
> > >
> > > Directly call mtk_camsv30_cmos_vf_hw_disable().
> > >
> > > > +
> > > > +out:
> > > > +pm_runtime_put_autosuspend(dev);
> > > > +}
> > > > +

-- 
Regards,

Laurent Pinchart