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); 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, Michael > Cheers, > > Paul >
