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

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

On Tue, Oct 31, 2023 at 03:41:25PM +0200, Andy Shevchenko wrote:
> 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)?

Custome V4L2 controls.

> ...
> 
> > > > > > > > > +	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'm fine making drivers OF-independent even without any clear evidence
that the device will be used on a non-OF platform when doing so does not
incur an unreasonable extra development cost.

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

Sakar has submitted a patch to add one missing fwnode function. If it
gets accepted, I'll try it out and see if I can convert this driver. I
will still not do a partial conversion if I hit any other blocker.

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

-- 
Regards,

Laurent Pinchart