Re: [PATCH v4 3/3] media: i2c: Add driver for THine THP7312

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Laurent,

On Mon, Oct 30, 2023 at 12:42:41PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Mon, Oct 30, 2023 at 08:09:36AM +0000, Sakari Ailus wrote:
> > On Sat, Oct 28, 2023 at 06:18:58PM +0300, Laurent Pinchart wrote:
> > > On Fri, Oct 27, 2023 at 02:50:09PM +0000, Sakari Ailus wrote:
> > > > On Fri, Oct 27, 2023 at 03:45:29PM +0300, Laurent Pinchart wrote:
> > > > 
> > > > ...
> > > > 
> > > > > > > +#include <linux/clk.h>
> > > > > > > +#include <linux/delay.h>
> > > > > > > +#include <linux/device.h>
> > > > > > > +#include <linux/firmware.h>
> > > > > > > +#include <linux/gpio/consumer.h>
> > > > > > > +#include <linux/i2c.h>
> > > > > > > +#include <linux/init.h>
> > > > > > > +#include <linux/iopoll.h>
> > > > > > > +#include <linux/kernel.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/mtd/spi-nor.h>
> > > > > > > +#include <linux/of_device.h>
> > > > > > > +#include <linux/pm_runtime.h>
> > > > > > > +#include <linux/regulator/consumer.h>
> > > > > > > +#include <linux/slab.h>
> > > > > > > +#include <linux/thp7312.h>
> > > > > > 
> > > > > > uapi/linux/thp7321.h ?
> > > > > 
> > > > > Is that needed ?
> > > > 
> > > > It's a UAPI header. Wouldn't it be reasonable to include it that way
> > > > (instead of relying on searching include/uapi as well)?
> > > 
> > > There are some occurences of '#include <uapi/' in drivers/ (I counted
> > > 338), but why is that better ?
> > 
> > I'd presume that at some point the -Iinclude/uapi will be cleaned up and
> > then the only option remains to include it from there directly. Why not to
> > do it already now?
> 
> Will it be ? I've never heard of such a plan, but I may have missed it.
> I thought it was a feature meant to stay, and the recommended way to
> include headers in the uapi/ directory.

I'm not sure if anyone is cleaning that up actively but it seems like a
fairly obvious candidate for cleanup.

...

> > > > > > > +	/*
> > > > > > > +	 * Register a device for the sensor, to support usage of the regulator
> > > > > > > +	 * API.
> > > > > > > +	 */
> > > > > > > +	sensor->dev = kzalloc(sizeof(*sensor->dev), GFP_KERNEL);
> > > > > > > +	if (!sensor->dev)
> > > > > > > +		return -ENOMEM;
> > > > > > > +
> > > > > > > +	sensor->dev->parent = dev;
> > > > > > > +	sensor->dev->of_node = of_node_get(sensor->of_node);
> > > > > > 
> > > > > > This device could well find its way to a non-OF system. Could you use the
> > > > > > fwnode property API instead?
> > > > > 
> > > > > I'm pretty sure there will be problems if someone was using this driver
> > > > > on an ACPI-based system, so trying to pretend it's supported without
> > > > > being able to test it may not be the best use of development time. I'll
> > > > > try, but if I hit any issue, I'll keep using the OF-specific functions
> > > > > in the next version.
> > > > 
> > > > I'd suggest to use OF functions if there's no corresponding fwnode function
> > > > available. The intention is they cover the same scope, so it is likely
> > > > something that's missing will be added sooner or later.
> > > 
> > > I understand, but if the conversion is not complete, it's not very
> > > valuable. I have no objection against using the fwnode API in the
> > > driver, but I'll let someone else handle it when and if needed.
> > 
> > If you leave it using OF-only API now in a driver that is not bound to OF
> > in any way, someone moving it to fwnode later may not be able to test it on
> > OF, increasing the likelihood something breaks. So use fwnode API where you
> > can now, and we'll address that one call later on.
> 
> Sorry, this is extra work for very little gain (if any) now, so I don't
> plan to do so if I can't implement a full conversion.

I don't see why would you leave this for someone else to clean up later.
It's called "technical debt". Similarly, we have no ACPI-only sensor
drivers that would use ACPI specific functions that would not be available
on non-ACPI systems --- they've all used the fwnode API, missing just
regulators, clocks and GPIOs.

If you like, I think we could have an fwnode version of the same function,
to be used with DT binding compliant format for the device in ACPI DSDT.
Plain ACPI would have no need for the function.

Cc Andy, too.

-- 
Regards,

Sakari Ailus