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
