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
Attachment:
signature.asc
Description: PGP signature
