Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback

[Dmitry Baryshkov]

On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:
> Hi,
> 
> Thanks a for you reviewing my patch.
> 
> 
> On 2024/4/23 21:28, Andy Shevchenko wrote:
> > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:
> > > Because the software node backend of the fwnode API framework lacks an
> > > implementation for the .device_get_match_data function callback. This
> > > makes it difficult to use(and/or test) a few drivers that originates
> > Missing space before opening parenthesis.
> 
> OK, will be fixed at the next version.
> 
> 
> > > from DT world on the non-DT platform.
> > > 
> > > Implement the .device_get_match_data fwnode callback, device drivers or
> > > platform setup codes are expected to provide a string property, named as
> > > "compatible", the value of this software node string property is used to
> > > match against the compatible entries in the of_device_id table.
> > Yep and again, how is this related? If you want to test a driver originating
> > from DT, you would probably want to have a DT (overlay) to be provided.
> 
> There are a few reasons, please fixed me if I'm wrong.
> 
> DT (overlay) can be possible solution, but DT (overlay) still depend on DT.
> For example, one of my x86 computer with Ubuntu 22.04 Linux/x86 6.5.0-28-generic
> kernel configuration do not has the DT enabled. This means that the default kernel
> configuration is decided by the downstream OS distribution. It is not decided by
> usual programmers. This means that out-of-tree device drivers can never utilize
> DT or DT overlay, right?

No, this is not fully correct. The drivers anyway have to adopted for
the platforms they are used with. It is perfectly fine to have a driver
that supports both DT and ACPI at the same time.

> 
> I means that Linux kernel is intended to be used by both in-tree drivers and out-of-tree drivers.
> Out-of-tree device drivers don't have a chance to alter kernel config, they can only managed to
> get their source code compiled against the Linux kernel the host in-using.
> 
> Some out-of-tree device drivers using DKMS to get their source code compiled,
> with the kernel configuration already *fixed*. So they don't have a opportunity
> to use DT overlay.
> 
> Relying on DT overlay is *still* *DT* *dependent*, and I not seeing matured solution
> get merged into upstream kernel yet. However, software node has *already* been merged
> into Linux kernel. It can be used on both DT systems and non-DT systems. Software node
> has the least requirement, it is *handy* for interact with drivers who only need a small
> set properties.
> 
> In short, I still think my patch maybe useful for some peoples. DT overlay support on
> X86 is not matured yet, need some extra work. For out-of-tree kernel module on
> downstream kernel. Select DT and DT overlay on X86 is out-of-control. And I don't want
> to restrict the freedom of developers.

I don't think upstream developers care about the downstream kernels.

But let me throw an argument why this patch (or something similar) looks
to be necessary.

Both on DT and non-DT systems the kernel allows using the non-OF based
matching. For the platform devices there is platform_device_id-based
matching.

Currently handling the data coming from such device_ids requires using
special bits of code, e.g. platform_get_device_id(pdev)->driver_data to
get the data from the platform_device_id. Having such codepaths goes
against the goal of unifying DT and non-DT paths via generic property /
fwnode code.

As such, I support Sui's idea of being able to use device_get_match_data
for non-DT, non-ACPI platform devices.

Sui, if that fits your purpose, please make sure that with your patch
(or the next iteration of it) you can get driver_data from the matched
platform_device_id.

> 
> 
> > > This also helps to keep the three backends of the fwnode API aligned as
> > > much as possible, which is a fundamential step to make device driver
> > > OF-independent truely possible.
> > > 
> > > Fixes: ffb42e64561e ("drm/tiny/repaper: Make driver OF-independent")
> > > Fixes: 5703d6ae9573 ("drm/tiny/st7735r: Make driver OF-independent")
> > How is it a fix?
> 
> 
> Because the drm/tiny/repaper driver and drm/tiny/st7735r driver requires extra
> device properties. We can not make them OF-independent simply by switching to
> device_get_match_data(). As the device_get_match_data() is a *no-op* on non-DT
> environment.

This doesn't constitute a fix. It's not that there is a bug that you are
fixing. You are adding new feature ('support for non-DT platforms').

> 
> Hence, before my patch is applied, the two "Make driver OF-independent" patch
> have no effect. Using device_get_match_data() itself is exactly *same* with
> using of_device_get_match_data() as long as the .device_get_match_data hook is
> not implemented.
> 
> 
> See my analysis below:
> 
> When the .device_get_match_data hook is not implemented:
> 
> 1) On DT systems, device_get_match_data() just redirect to of_fwnode_device_get_match_data(),
>    which is just a wrapper of of_device_get_match_data().
> 
> 2) On Non-DT system, device_get_match_data() has *ZERO* effect, it just return NULL.
> 
> 
> Therefore, device_get_match_data() adds *ZERO* benefits to the mentioned drivers if
> the .device_get_match_data is not implemented.
> 
> Only when the .device_get_match_data hook get implemented, device_get_match_data()
> can redirect tosoftware_node_get_match_data() function in this patch.
> Therefore, the two driver has a way to get a proper driver match data on
> non-DT environment. Beside, the users of those two driver can provide
> additional software node property at platform setup code. as long as at
> somewhere before the driver is probed.
> 
> So the two driver really became OF-independent after applied my patch.
> 
> 
> > > Closes: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
> > Yes, and then Reported-by, which is missing here.
> > 
> > > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > Cc: Daniel Scally <djrscally@xxxxxxxxx>
> > > Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > Please, move these after the cutter '---' line (note you may have that line in
> > your local repo).
> > 
> > ...
> > 
> 
> OK, thanks a lot for teaching me.
> 
> 
> > > +static const void *
> > > +software_node_get_match_data(const struct fwnode_handle *fwnode,
> > > +			     const struct device *dev)
> > > +{
> > > +	struct swnode *swnode = to_swnode(fwnode);
> > > +	const struct of_device_id *matches = dev->driver->of_match_table;
> > > +	const char *val = NULL;
> > > +	int ret;
> > > +	ret = property_entry_read_string_array(swnode->node->properties,
> > > +					       "compatible", &val, 1);
> > And if there are more than one compatible provided?
> 
> Nope, I think this is kind of limitation of the software node,
> platform setup code generally could provide a compatible property.
> No duplicate name is allowed. But we the best explanation would be
> platform setup code should provide the "best" or "default" compatible
> property.

The implementation is still incorrect. The swnode code shouldn't look
into the OF data. Please use non-DT match IDs.

> 
> 
> > > +	if (ret < 0 || !val)
> > > +		return NULL;
> > > +	while (matches && matches->compatible[0]) {
> > First part of the conditional is invariant to the loop. Can be simply
> 
> 
> Right, thanks.
> 
> 
> > 	matches = dev->driver->of_match_table;
> > 	if (!matches)
> > 		return NULL;
> > 
> > 	while (...)
> > 
> > > +		if (!strcmp(matches->compatible, val))
> > > +			return matches->data;
> > > +
> > > +		matches++;
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> 
> -- 
> Best regards,
> Sui
> 

-- 
With best wishes
Dmitry