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

[Andy Shevchenko]

On Tue, Oct 31, 2023 at 10:45:32AM +0000, Sakari Ailus wrote:
> On Mon, Oct 30, 2023 at 12:42:41PM +0200, Laurent Pinchart wrote:
> > 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/of_device.h>

I believe this shouldn't (mustn't?) be used in a new code.
Rob have been doing a big job of replacing some OF-specific
APIs by generic ones.

...

> > > > > > > uapi/linux/thp7321.h ?

Why does the driver even have that?! Does it allow direct IOCTLs? Some
other hardware information that should be supplied via "abstract"
(presumably existing IOCTL)?

...

> > > > > > > > +	sensor->dev->parent = dev;
> > > > > > > > +	sensor->dev->of_node = of_node_get(sensor->of_node);

This should be device_set_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.

Besides ACPI it may be other ways of instantiating the driver.
And we, in general, asking for creating OF-independent drivers as long
as there is no strong evidence that the platform itself and the particular
hardware never ever will have anything than OF. And it almost always
not true for discrete (outside the SoC) components.

> > > > > 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.

I agree with Sakari. Let's reduce the scope of ACPI/OF/etc-specific functions
in the drivers. There are really little that have no generic counterparts.
And most of the usages are special cases.

> 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.

-- 
With Best Regards,
Andy Shevchenko