Hi Paul, Hi Michael Thank you for the review! On Wed, Oct 25, 2023 at 11:48:08AM +0200, Paul Kocialkowski wrote: > 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! > Yes, I will change it ro cif_write() > > 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? I am going with cif_*() > > 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". Yes "remote" sounds right in this situation -- Kind Regards Mehdi Djait
