Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support

[Pratyush Yadav]

On 31/03/21 09:06AM, Tomi Valkeinen wrote:
> Hi,
> 
> On 30/03/2021 20:33, Pratyush Yadav wrote:
> > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > capture over a CSI-2 bus.
> > 
> > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and
> > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge
> > called the SHIM layer. It takes in data from stream 0, repacks it, and
> > sends it to memory over PSI-L DMA.
> > 
> > This driver acts as the "front end" to V4L2 client applications. It
> > implements the required ioctls and buffer operations, passes the
> > necessary calls on to the bridge, programs the SHIM layer, and performs
> > DMA via the dmaengine API to finally return the data to a buffer
> > supplied by the application.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@xxxxxx>
> > ---
> >   MAINTAINERS                               |   7 +
> >   drivers/media/platform/Kconfig            |  11 +
> >   drivers/media/platform/ti-vpe/Makefile    |   1 +
> >   drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++++++++++++++++++++++
> >   4 files changed, 983 insertions(+)
> >   create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c
> 
> Some quick comments:
> 
> "ti-vpe" directory is not correct, this has nothing to do with VPE. That
> said, the directory has already been abused by having CAL driver there,
> perhaps we should rename the directory just to "ti". But if we do that, I
> think we should have subdirs for cal, vpe and this new one.

Right. I thought about doing this but then figured "let's just follow 
what CAL does". Will move them into separate subdirectories.

> 
> "ti-csi2rx" is rather generic name. TI has had CSI-2 RX IPs before (CAL) and
> probably will also have new ones in the future. If there's no clear model
> name for the IP, as I think is the case here, it's probably best to just use
> the SoC model in the name. E.g. the DSS on J7 is "ti,j721e-dss".

Ok.

> 
> This driver implements the legacy video API. I think it would be better (and
> easier to maintain) to only implement the media-controller API, unless you
> specifically need to support the legacy API for existing userspace.

:-(

I'm afraid the documentation has let me down here. There is no clear 
mention about the fact that media controller API replaces the "legacy" 
API. In fact, [0] seems to suggest that the media controller API is 
optional.

  Bridge drivers traditionally expose one or multiple video nodes to 
  userspace, and control subdevices through the v4l2_subdev_ops 
  operations in response to video node operations. This hides the 
  complexity of the underlying hardware from applications. For complex 
  devices, finer-grained control of the device than what the video nodes 
  offer may be required. In those cases, bridge drivers that implement 
  the media controller API may opt for making the subdevice operations 
  directly accessible from userpace.

Anyway, I don't think supporting the legacy API makes much sense since 
this is a new driver. Will convert it to use the MC API. We can always 
add the legacy API support if some application demands it.

[0] https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-subdev.html#v4l2-sub-device-userspace-api

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.