linux-kernel - Re: [PATCH v8 2/3] media: rockchip: Add a driver for Rockhip's camera interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZTjkWIbhV0OsvepD@aptenodytes>
Date:   Wed, 25 Oct 2023 11:48:08 +0200
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Michael Riesch <michael.riesch@...fvision.net>
Cc:     Mehdi Djait <mehdi.djait@...tlin.com>, mchehab@...nel.org,
        heiko@...ech.de, hverkuil-cisco@...all.nl,
        krzysztof.kozlowski+dt@...aro.org, robh+dt@...nel.org,
        conor+dt@...nel.org, ezequiel@...guardiasur.com.ar,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
        alexandre.belloni@...tlin.com, maxime.chevallier@...tlin.com
Subject: Re: [PATCH v8 2/3] media: rockchip: Add a driver for Rockhip's
 camera interface

Michael,

On Wed 25 Oct 23, 11:38, Michael Riesch wrote:
> Hi Paul,
> 
> On 10/25/23 10:49, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Mon 23 Oct 23, 15:28, Michael Riesch wrote:
> >> Typo in the subject: "Rockhip's" -> "Rockchip's"
> >> I think this typo has been in there for a while now ;-)
> > 
> > Great hips make for great dancing!
> 
> ...to rock music, obviously.

:)

> > [...]
> >>> +#define write_vip_reg(base, addr, val)  writel(val, (addr) + (base))
> >>> +#define read_vip_reg(base, addr) readl((addr) + (base))
> >>
> >> Please provide those helpers as proper inline functions. As to the
> >> naming, the "_reg" suffix seems unnecessary.
> >>
> >> Alternatively, you could consider converting the driver to use regmap.
> > 
> > Come to think of it, I feel like it would make more sense to have an inline
> > function which is given a struct rk_vip_device instead of having to dereference
> > it every time in the caller to access the base address.
> 
> Indeed. Either using regmap, e.g.,
> 
> int regmap_write(struct regmap *map, unsigned int reg, unsigned int val);
> 
> or something equivalant
> 
> static inline int cif_write(struct cif_device *device, unsigned int reg,
> unsigned int val);

Looks good to me!

> Not sure what you agreed on in terms of a method prefix. The Rockchip
> RGA driver uses "rga_something", the Rockchip ISP driver uses
> "rkisp1_something". This would mean either "cif_something" or
> "rkcif_something", right?

Yeah I don't really have strong opinions on this so I'll let Mehdi decide
(as long as it's consistent everywhere in the code).

I guess there is a slight readability advantage in using "cif_" instead of
"rkcif_".

> > [...]
> >>> +	struct rk_vip_sensor_info	sensor;
> >>
> >> Using "sensor" as name does not seem correct. As pointed out above it
> >> could be a video decoder just as well. Something with "subdevice" maybe?
> > 
> > Agreed. I suggest renaming the struct "rk_vip_sensor_info" -> "rk_cif_remote"
> > and just calling the member "remote".
> 
> "remote" sounds reasonable. Prefix to be bikeshedded, see comment above.
> 
> In the future, we may add an array with mipi_remotes that represents the
> subdevices attached wia MIPI CSI-2.

Sounds good!

Thanks,

Paul

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)