Re: [PATCH v11 2/3] media: rockchip: Add a driver for Rockchip's camera interface

[Paul Kocialkowski]

Hi Michael,

On Wed 22 Nov 23, 13:42, Michael Riesch wrote:
> Hi Tommaso,
> 
> On 11/21/23 19:41, Tommaso Merciai wrote:
> > Hi Mehdi,
> > 
> > On Thu, Nov 16, 2023 at 12:04:39PM +0100, Mehdi Djait wrote:
> >> This introduces a V4L2 driver for the Rockchip CIF video capture controller.
> >>
> >> This controller supports multiple parallel interfaces, but for now only the
> >> BT.656 interface could be tested, hence it's the only one that's supported
> >> in the first version of this driver.
> >>
> >> This controller can be found on RK3066, PX30, RK1808, RK3128 and RK3288,
> >> but for now it's only been tested on the PX30.
> >>
> >> CIF is implemented as a video node-centric driver.
> >>
> >> Most of this driver was written following the BSP driver from rockchip,
> >> removing the parts that either didn't fit correctly the guidelines, or that
> >> couldn't be tested.
> >>
> >> This basic version doesn't support cropping nor scaling and is only
> >> designed with one SDTV video decoder being attached to it at any time.
> >>
> >> This version uses the "pingpong" mode of the controller, which is a
> >> double-buffering mechanism.
> >>
> >> Signed-off-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> >> ---
> >>  MAINTAINERS                                   |    7 +
> >>  drivers/media/platform/rockchip/Kconfig       |    1 +
> >>  drivers/media/platform/rockchip/Makefile      |    1 +
> >>  drivers/media/platform/rockchip/cif/Kconfig   |   13 +
> >>  drivers/media/platform/rockchip/cif/Makefile  |    3 +
> >>  drivers/media/platform/rockchip/cif/capture.c | 1120 +++++++++++++++++
> >>  drivers/media/platform/rockchip/cif/capture.h |   21 +
> >>  drivers/media/platform/rockchip/cif/common.h  |  129 ++
> >>  drivers/media/platform/rockchip/cif/dev.c     |  302 +++++
> >>  drivers/media/platform/rockchip/cif/regs.h    |  127 ++
> >>  10 files changed, 1724 insertions(+)
> >>  create mode 100644 drivers/media/platform/rockchip/cif/Kconfig
> >>  create mode 100644 drivers/media/platform/rockchip/cif/Makefile
> >>  create mode 100644 drivers/media/platform/rockchip/cif/capture.c
> >>  create mode 100644 drivers/media/platform/rockchip/cif/capture.h
> >>  create mode 100644 drivers/media/platform/rockchip/cif/common.h
> >>  create mode 100644 drivers/media/platform/rockchip/cif/dev.c
> >>  create mode 100644 drivers/media/platform/rockchip/cif/regs.h
> > 
> > Just a logigistic comment on my side for now, sorry :)
> > What about use cif-* prefix in front of driver files?
> > 
> > like:
> > 
> > cif-capture.c
> > cif-capture.h
> > cif-common.h
> > cif-dev.c
> > cif-regs.h
> 
> What would be the rationale here?
> 
> IMHO the files are in a folder named cif, so adding this prefix seems
> kind of redundant.
> 
> That said, if there is a good reason I could live with cif-*.{c,h} as
> well, of course. My only request would be to agree on something ASAP.

It's rather common to do that in Linux and one advantage is that it makes it
clear in your text editor which driver you are looking at when only the file
name is shown.

I don't have any strong opinion on this either though.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature