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]
Date:   Wed, 25 Oct 2023 11:38:25 +0200
From:   Michael Riesch <>
To:     Paul Kocialkowski <>
Cc:     Mehdi Djait <>,,,,,,,,,,,,,
Subject: Re: [PATCH v8 2/3] media: rockchip: Add a driver for Rockhip's camera

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! 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);

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?

> [...]
>>> +	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.

Best regards,

> Cheers,
> Paul

Powered by blists - more mailing lists