Hi Sakari, On Mon, May 27, 2024 at 08:45:22AM +0000, Sakari Ailus wrote: > 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. For most ISP drivers I don't really see much value in supporting !CONFIG_PM. I don't think it's worth doing so here, especially given that it will never be tested. -- Regards, Laurent Pinchart
