Re: [PATCH v7 7/8] media: raspberrypi: Add support for PiSP BE

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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