Hi Laurent, On Mon, May 27, 2024 at 11:31:33AM +0300, Laurent Pinchart wrote: > On Mon, May 27, 2024 at 08:14:00AM +0000, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Mon, May 27, 2024 at 09:56:00AM +0200, Jacopo Mondi wrote: > > > > > +#include <linux/media/raspberrypi/pisp_be_config.h> > > > > > > > > Where is the header included from? If it's just this driver, then I'd put > > > > it in the driver's directory. > > > > > > It's the uAPI header file. Or did I miss your question ? > > > > If it's uapi, then you should have uapi in its header path. I.e. > > > > #include <uapi/linux...> > > > > > > > + /* Everything else is as supplied by the user. */ > > > > > + begin = offsetof(struct pisp_be_config, global.bayer_order) > > > > > + / sizeof(u32); > > > > > > > > The slash should be on the previous line. Same elsewhere. > > > > > > > > > > Please, this is highly subjective and other people (like Laurent) often > > > ask for the contrary. Without any polemic intent, I encourage reviewers > > > (myself included) in considering how much time we spend (and > > > demand) on such subjective issues. Even more when other reviewers might have > > > different opinions, with the end result of pulling contributors in > > > different directions. > > > > Having binary operators at the beginning of a statement split on multiple > > lines is simply uncommon, perhaps around 10 % of the cases in the media > > tree based on a quick look. Keeping the coding style consistent is > > beneficial for us all. > > I've been slowly but steadily working on increasing that number :-) I > think the style above is the most readable, and I would leave it to > driver authors (as long as they're consistent within a driver). This has been one of the differences between what's commonly (albeit not explicitly I guess) used in Linux compared to the GNU coding standards which is explicit about it. I prefer to keep it at the end of the line which apparently is the preference of a largish majority. ... > > > > > + /* Hardware initialisation */ > > > > > + pm_runtime_set_autosuspend_delay(pispbe->dev, 200); > > > > > + pm_runtime_use_autosuspend(pispbe->dev); > > > > > + pm_runtime_enable(pispbe->dev); > > > > > + > > > > > + ret = pm_runtime_resume_and_get(pispbe->dev); > > > > > > > > You'll need to call the driver's resume function manually instead. The > > > > above depends on CONFIG_PM. > > > > > > The driver selects CONFIG_PM, doesn't it ? > > > > It depends on PM. > > > > It'd be trivial to remove that dependency. > > For drivers such as sensor drivers that need to work on a wide variety > of platforms, with varying configurations, I agree that not depending on > CONFIG_PM is a good thing (I reserve the right to change my mind though > :-)). For this driver, I don't think the dependency is an issue. People tend to copy these from one driver to another so there is value in doing it properly even if the benefits for this driver might be minor. -- Regards, Sakari Ailus
