Message-ID: <Zf3IWyrW8IZLTg4x@smile.fi.intel.com>
Date: Fri, 22 Mar 2024 20:05:15 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Sui Jingfeng <sui.jingfeng@...ux.dev>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>,
	Daniel Scally <djrscally@...il.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-acpi@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org,
	Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH] software node: Implement device_get_match_data fwnode
 callback

On Sat, Mar 23, 2024 at 01:43:56AM +0800, Sui Jingfeng wrote:
> On 2024/3/23 00:14, Andy Shevchenko wrote:
> > On Fri, Mar 22, 2024 at 05:00:05PM +0800, Sui Jingfeng wrote:
> > > On 2024/3/21 04:28, Andy Shevchenko wrote:

..

> > > > > > > By replacing it with device_get_match_data() and creating a software
> > > > > > > graph that mimics the OF graph, everything else works fine, except that
> > > > > > > there isn't an out-of-box replacement for the of_device_get_match_data()
> > > > > > > function. Because the software node backend of the fwnode framework lacks
> > > > > > > an implementation for the device_get_match_data callback.
> > > > > > .device_get_match_data
> > > > > > 
> > > > > > > Implement device_get_match_data fwnode callback fwnode callback to fill
> > > > > > .device_get_match_data
> > > > > OK, thanks a lot.
> > > > > 
> > > > > > > this gap. Device drivers or platform setup codes are expected to provide
> > > > > > > a "compatible" string property. The value of this string property is used
> > > > > > > to match against the compatible entries in the of_device_id table. Which
> > > > > > > is consistent with the original usage style.
> > > > > > Why do you need to implement the graph in the board file?
> > > > > It can be inside the chip, there is no clear cut.\
> > > > Which chip? Flash memory / ROM or you meant something like FPGA here?
> > > > For the latter there is another discussion on how to use DT overlays
> > > > in ACPI-enabled environments for the FPGA configurations.
> > > There are some hardware resource or software entity is created on the
> > > driver runtime. But DT or DT overlays are compiled before device driver
> > > get loaded. GPIO-emulated-I2C is just an example, this is kind of driver
> > > level knowledge on the runtime. With the GPIO or programmable some
> > > hardware IP unit, device driver authors can change the connection relationship
> > > at their will at the runtime. While with DT, every thing has to be sure
> > > before the compile time.
> > > 
> > > DT overlays can be a alternative solution, but this doesn't conflict with
> > > this patch. This patch won't assume how device drives go to use it, and
> > > allow device driver creating device instead enumerating by DT. In one
> > > word: "flexibility".
> > Software nodes in general for the device driver / platform quirks.
> 
> The real problem is that we probably shouldn't make an assumption
> how does the user is going to use the infrastructure, right?
> 
> You could say it is *mostly* for quirks or whatever, Like the
> ./drivers/i2c/busses/i2c-cht-wc.c. But software nodes *can* also
> be something else.
> 
> Can we stop restricting its usage by limited understanding or someone
> personal judgement?

Please, try to research the topic before calling it 'personal judgement'.

59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node framework")

(Read the first paragraph carefully.)

Let's say it's not personal, it's by design. Extending this to cover more needs
a good justification. I do not see a such.

> A workaround or quirk may be enough for some corner usage. Vladimir is also
> encounter similar problem, right?

> > They are not designed for what you are talking about here.
> 
> I have never hint anything about any real applications, the materials
> and/or talk given here is just for example purpose.
> 
> What we are doing here is to keep the three back-ends aligned.
> 
> 
> > Consider using SSDT / DT overlays instead.
> > 
> NAK,
> 
> When developers are doing task 'A' , reviewers ask them to do task 'B'.
> And when developers doing task 'B', reviewers then recommend that the tool
> 'C'  is a better alternative.
> ...
> ...
> 
> This is not good.
> 
> 
> As I have read the lengthy thread in link [1] as you pointed to me.
> 
> The boring coding review is just as the following scheme:
> 
> 1) Asking details about what they do with software nodes impolitely.
> 2) Wasting time to talk about irreverent things by brute force.
> 3) Tell everybody that software nodes are not designed for what you application.
> 4) Recommending DT overlays or something else.
> 
> Again, this is non-technical discussion, the time being wasting is not worthwhile.
> And the judgements being given is irrelevant to the *patch itself*.

The patch tries to tight the driver data to the device description provided by
a software node, which is 100% equivalent to the legacy board files which we
do NOT want to have. Besides that, the kernel project rule is "we do not add
the dead (unused) code".

I believe these two is quite enough to NAK patch.

You may come with a better explanation AND a user of this in the same series.
People at least can see your use case.

> [1] https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists