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

2024-04-25 Thread Andy Shevchenko
On Thu, Apr 25, 2024 at 09:42:53PM +0800, Sui Jingfeng wrote:
> On 2024/4/25 00:44, Andy Shevchenko wrote:
> > On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote:
> > > > > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko
> > > > >  wrote:
> > > > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote:
> > > > > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:
> > > > > > > > On 2024/4/23 21:28, Andy Shevchenko wrote:
> > > > > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:

...

> > > > > > > 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.
> > > > > > I'm not sure I buy this. We have a special helpers based on the bus 
> > > > > > type to
> > > > > > combine device_get_match_data() with the respective ID table 
> > > > > > crawling, see
> > > > > > the SPI and I²C cases as the examples.
> > > > > I was thinking that we might be able to deprecate these helpers and
> > > > > always use device_get_match_data().
> > > > True, but that is orthogonal to swnode match_data support, right?
> > > > There even was (still is?) a patch series to do something like a new
> > > > member to struct device_driver (? don't remember) to achieve that.
> > > Maybe the scenario was not properly described in the commit message, or
> > > maybe I missed something. The usecase that I understood from the commit
> > > message was to use instatiated i2c / spi devices, which means
> > > i2c_device_id / spi_device_id. The commit message should describe why
> > > the usecase requires using 'compatible' property and swnode. Ideally it
> > > should describe how these devices are instantiated at the first place.
> > Yep. I also do not clearly understand the use case and why we need to have
> > a board file, because the swnodes all are about board files that we must not
> > use for the new platforms.
> 
> Would you like to tell us what's the 'board file'?
> 
> I am asking because I can not understand those two words at all.
> I'm really don't know what's the meanings of 'board file'.

Hmm... This is very well established term meaning the hard coded platform
description (you may consider that as "device tree" written in C inside
the Linux kernel). There are plenty of legacy platforms still exist in
the Linux kernel source tree, you may find examples, like (first comes
to mind) arch/arm/mach-pxa/spitz.c.

> Do you means that board file is something like the dts, or
> somethings describe the stuff on the motherboard but outside
> the CPU?
> 
> Does the hardware IP core belong to the "board file"?
> 
> Can we using more concrete vocabulary instead of the vague
> vocabulary to communicate?

Most of (I though 100% before this message) the Linux kernel developers
_know_ this term, sorry that you maybe young enough :-)

-- 
With Best Regards,
Andy Shevchenko




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

2024-04-25 Thread Sui Jingfeng



On 2024/4/25 00:44, Andy Shevchenko wrote:

On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote:

On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote:

On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote:

On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko
 wrote:

On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote:

On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:

On 2024/4/23 21:28, Andy Shevchenko wrote:

On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:

...


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.

I'm not sure I buy this. We have a special helpers based on the bus type to
combine device_get_match_data() with the respective ID table crawling, see
the SPI and I²C cases as the examples.

I was thinking that we might be able to deprecate these helpers and
always use device_get_match_data().

True, but that is orthogonal to swnode match_data support, right?
There even was (still is?) a patch series to do something like a new
member to struct device_driver (? don't remember) to achieve that.

Maybe the scenario was not properly described in the commit message, or
maybe I missed something. The usecase that I understood from the commit
message was to use instatiated i2c / spi devices, which means
i2c_device_id / spi_device_id. The commit message should describe why
the usecase requires using 'compatible' property and swnode. Ideally it
should describe how these devices are instantiated at the first place.

Yep. I also do not clearly understand the use case and why we need to have
a board file, because the swnodes all are about board files that we must not
use for the new platforms.



Would you like to tell us what's the 'board file'?

I am asking because I can not understand those two words at all.
I'm really don't know what's the meanings of 'board file'.

Do you means that board file is something like the dts, or
somethings describe the stuff on the motherboard but outside
the CPU?

Does the hardware IP core belong to the "board file"?

Can we using more concrete vocabulary instead of the vague
vocabulary to communicate?


--
Best regards,
Sui



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

2024-04-24 Thread Dmitry Baryshkov
On Wed, 24 Apr 2024 at 19:45, Andy Shevchenko
 wrote:
>
> On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote:
> > On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote:
> > > On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote:
> > > > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko
> > > >  wrote:
> > > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote:
> > > > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:
> > > > > > > On 2024/4/23 21:28, Andy Shevchenko wrote:
> > > > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:
>
> ...
>
> > > > > > 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.
> > > > >
> > > > > I'm not sure I buy this. We have a special helpers based on the bus 
> > > > > type to
> > > > > combine device_get_match_data() with the respective ID table 
> > > > > crawling, see
> > > > > the SPI and I²C cases as the examples.
> > > >
> > > > I was thinking that we might be able to deprecate these helpers and
> > > > always use device_get_match_data().
> > >
> > > True, but that is orthogonal to swnode match_data support, right?
> > > There even was (still is?) a patch series to do something like a new
> > > member to struct device_driver (? don't remember) to achieve that.
> >
> > Maybe the scenario was not properly described in the commit message, or
> > maybe I missed something. The usecase that I understood from the commit
> > message was to use instatiated i2c / spi devices, which means
> > i2c_device_id / spi_device_id. The commit message should describe why
> > the usecase requires using 'compatible' property and swnode. Ideally it
> > should describe how these devices are instantiated at the first place.
>
> Yep. I also do not clearly understand the use case and why we need to have
> a board file, because the swnodes all are about board files that we must not
> use for the new platforms.

Not necessarily board files. In some cases it also allows creating
device nodes to patch devices, e.g. when the ACPI description is
incomplete. But my main concern is about using the "compatible"
property here. I suppose that it should be avoided if not absolutely
required, instead the driver should use native foo_device_id matching.

Here is a list of existing "compatible" properties and their usecases.

arch/arm/mach-omap1/board-nokia770.c:
PROPERTY_ENTRY_STRING("compatible", "ti,ads7846"),

This one looks like a way to overcome shortcomings of the ads7846
driver. The driver should add spi_device_id, use
spi_get_device_match_data(), then the property can be dropped.

drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c: nodes->i2c_props[0] =
PROPERTY_ENTRY_STRING("compatible", "snps,designware-i2c");
drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c: nodes->sfp_props[0] =
PROPERTY_ENTRY_STRING("compatible", "sff,sfp");

I think these two can also be dropped if the corresponding drivers get
platform_device_id entries.

drivers/platform/x86/x86-android-tablets/asus.c:
PROPERTY_ENTRY_STRING("compatible", "simple-battery"),
drivers/platform/x86/x86-android-tablets/shared-psy-info.c:
PROPERTY_ENTRY_STRING("compatible", "simple-battery"),

These are required by the power_supply core to identify the "battery
stub" case. There is no corresponding device being created and/or
matched.

drivers/platform/chrome/chromeos_laptop.c:
PROPERTY_ENTRY_STRING("compatible", "atmel,maxtouch"),
drivers/platform/chrome/chromeos_laptop.c:
PROPERTY_ENTRY_STRING("compatible", "atmel,maxtouch"),
drivers/platform/chrome/chromeos_laptop.c:
PROPERTY_ENTRY_STRING("compatible", "atmel,maxtouch"),
drivers/platform/x86/x86-android-tablets/asus.c:
PROPERTY_ENTRY_STRING("compatible", "atmel,atmel_mxt_ts"),

The driver checks for the presence of the "compatible" property to
check that the device has properties at all. I think it was added as a
safegap against treating incomplete trackpad nodes (which should have
linux,gpio-keymap) as touchscreens (which have none).

-- 
With best wishes
Dmitry


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

2024-04-24 Thread Sui Jingfeng

Hi,


On 2024/4/25 00:34, Dmitry Baryshkov wrote:

On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote:

On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote:

On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko
 wrote:

On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote:

On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:

On 2024/4/23 21:28, Andy Shevchenko wrote:

On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:

...


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.

I'm not sure I buy this. We have a special helpers based on the bus type to
combine device_get_match_data() with the respective ID table crawling, see
the SPI and I²C cases as the examples.

I was thinking that we might be able to deprecate these helpers and
always use device_get_match_data().

True, but that is orthogonal to swnode match_data support, right?
There even was (still is?) a patch series to do something like a new
member to struct device_driver (? don't remember) to achieve that.

Maybe the scenario was not properly described in the commit message,


No thecommit message is very clear, just you don't clear. Can't you see that 
only you are out of scope here and complaining with wrong hypothesis?



or
maybe I missed something.



No, you miss and mess everything.


The usecase that I understood from the commit
message was to use instatiated i2c / spi devices, which means
i2c_device_id / spi_device_id.


It can also be platform device with manually assigned software node.


The commit message should describe why
the usecase requires using 'compatible' property and swnode. Ideally it
should describe how these devices are instantiated at the first place.


Yes, I admit it, its not the first time you do such a thing.  I know you might
good at debating and directing blindly. But those skills are not really helpful.
As it brings ZERO benefits to the developers and end user of Linux kernel. What
the end user need is a good DRM driver and infrastructure like i915, amdgpu,
radeon, vc4, ast, X server or wayland etc.

Its fine to keep quite if you don't understand something very well, especially,
the for the things that not directly maintained by you. As you don't have to
responsible for it and you don't need to pay for the obligation. You might
deceive yourself to believe that you are reviewing, actually, you are just
wasting your time as well as other people's time.
 
If the the commit message is really matters to you, then do you thing about

the following series [1][2][3] ?

[1] https://patchwork.freedesktop.org/series/123812/
[2] https://patchwork.freedesktop.org/patch/579730/?series=130021&rev=2
[3] https://patchwork.freedesktop.org/series/123737/

How about the witting of its commit message, very well, right?
Think before you type, and type with the brain not with the emotion.

--
Best regards,
Sui



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

2024-04-24 Thread Andy Shevchenko
On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote:
> On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote:
> > On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko
> > >  wrote:
> > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote:
> > > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:
> > > > > > On 2024/4/23 21:28, Andy Shevchenko wrote:
> > > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:

...

> > > > > 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.
> > > >
> > > > I'm not sure I buy this. We have a special helpers based on the bus 
> > > > type to
> > > > combine device_get_match_data() with the respective ID table crawling, 
> > > > see
> > > > the SPI and I²C cases as the examples.
> > > 
> > > I was thinking that we might be able to deprecate these helpers and
> > > always use device_get_match_data().
> > 
> > True, but that is orthogonal to swnode match_data support, right?
> > There even was (still is?) a patch series to do something like a new
> > member to struct device_driver (? don't remember) to achieve that.
> 
> Maybe the scenario was not properly described in the commit message, or
> maybe I missed something. The usecase that I understood from the commit
> message was to use instatiated i2c / spi devices, which means
> i2c_device_id / spi_device_id. The commit message should describe why
> the usecase requires using 'compatible' property and swnode. Ideally it
> should describe how these devices are instantiated at the first place.

Yep. I also do not clearly understand the use case and why we need to have
a board file, because the swnodes all are about board files that we must not
use for the new platforms.

-- 
With Best Regards,
Andy Shevchenko




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

2024-04-24 Thread Dmitry Baryshkov
On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote:
> > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko
> >  wrote:
> > >
> > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote:
> > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:
> > > > > On 2024/4/23 21:28, Andy Shevchenko wrote:
> > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:
> 
> ...
> 
> > > > 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.
> > >
> > > I'm not sure I buy this. We have a special helpers based on the bus type 
> > > to
> > > combine device_get_match_data() with the respective ID table crawling, see
> > > the SPI and I²C cases as the examples.
> > 
> > I was thinking that we might be able to deprecate these helpers and
> > always use device_get_match_data().
> 
> True, but that is orthogonal to swnode match_data support, right?
> There even was (still is?) a patch series to do something like a new
> member to struct device_driver (? don't remember) to achieve that.

Maybe the scenario was not properly described in the commit message, or
maybe I missed something. The usecase that I understood from the commit
message was to use instatiated i2c / spi devices, which means
i2c_device_id / spi_device_id. The commit message should describe why
the usecase requires using 'compatible' property and swnode. Ideally it
should describe how these devices are instantiated at the first place.

-- 
With best wishes
Dmitry


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

2024-04-24 Thread Andy Shevchenko
On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote:
> On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko
>  wrote:
> >
> > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote:
> > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:
> > > > On 2024/4/23 21:28, Andy Shevchenko wrote:
> > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:

...

> > > 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.
> >
> > I'm not sure I buy this. We have a special helpers based on the bus type to
> > combine device_get_match_data() with the respective ID table crawling, see
> > the SPI and I²C cases as the examples.
> 
> I was thinking that we might be able to deprecate these helpers and
> always use device_get_match_data().

True, but that is orthogonal to swnode match_data support, right?
There even was (still is?) a patch series to do something like a new
member to struct device_driver (? don't remember) to achieve that.

-- 
With Best Regards,
Andy Shevchenko




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

2024-04-24 Thread Dmitry Baryshkov
On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko
 wrote:
>
> On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote:
> > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:
> > > On 2024/4/23 21:28, Andy Shevchenko wrote:
> > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:
>
> ...
>
> > 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.
>
> I'm not sure I buy this. We have a special helpers based on the bus type to
> combine device_get_match_data() with the respective ID table crawling, see
> the SPI and I²C cases as the examples.

I was thinking that we might be able to deprecate these helpers and
always use device_get_match_data().

-- 
With best wishes
Dmitry


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

2024-04-24 Thread Andy Shevchenko
On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote:
> On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote:
> > On 2024/4/23 21:28, Andy Shevchenko wrote:
> > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:

...

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

I'm not sure I buy this. We have a special helpers based on the bus type to
combine device_get_match_data() with the respective ID table crawling, see
the SPI and I²C cases as the examples.

-- 
With Best Regards,
Andy Shevchenko




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

2024-04-24 Thread Sui Jingfeng

Hi,


On 2024/4/24 16:39, Dmitry Baryshkov wrote:

Sui, if that fits your purpose,

That doesn't fits my purpose, please stop the recommendation, thanks.



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

No, that's a another problem.

The 'platform_get_device_id(pdev)->driver_data' you mentioned is completely
off the domain of fwnode API framework. You are completely deviate what we
are currently talking about.

What we are talking about is something within the fwnode API framework.

You can hack the device_get_match_data() function to call 
platform_get_device_id()
as a fallback code path when the fwnode subsystem couldn't return a match data 
to
you. But this is another problem.

No. I was using this as a pointer for having non-DT driver data.


Whatever.

Whatever how does it going to be used by you, or whatever data the pointer you 
use to point to.
Just remember one thing, it is not relevant to my patch itself.



As I
wrote several paragraphs above, other subsystems use their own
driver-specific match structures.



Fine, but on the DT systems, they mostly probed via DT.
Thus, the so-called driver-specific match structures won't be used.



Reworking subsystems one-by-one to
be able to use generic codepath sounds to me like a way to go.


Fine, you are encouraged to do whatever you like, you don't have to told me.



Adding
"compatible" property doesn't.




As I have told you several times, software node is kind of complement to ACPI, 
it's
definitely need to follow the style of ACPI counterpart. Software node can be 
secondary
node of the primary ACPI device node. With this understood, please read the 
implementation
of acpi_of_match_device() before express objection. Or you can read several 
relevant commit
such as 4222f38ca3b7a ('ACPI / bus: Do not traverse through non-existed device 
table')
for knowing some extra background.

Beside, users of the software node backend itself can do whatever they like.
The value of the "compatible" property is *just* string, programmers
are free to name their string property in their driver. It is not you to say no 
though.

No offensive here, I means that both of us are not good at this domain. 
Especially me,
but at the very least, I'm willing to listen to what expects in ACPI/swnode 
domain say.
One day, you become the top maintainer of specific domain, and when I go to 
contribute
then, I will also read to you reviews message carefully.

Back to the technical itself, the "compatible" property is a standard property
of ACPI _DSD object. This is written into the ACPI Spec. The value of the 
"compatible"
property is just string, store it at 
'platform_get_device_id(pdev)->driver_data' or in
'of_device_id->compatible' or some other places doesn't really changes much in 
semantic.

A device driver can be platform probed, DT probed, ACPI probed, I2C probed, SPI 
probed...
Take the driver of I2C slave display bridge as an example, it can platform 
probed, DT probed,
I2C probed, in the future, there may have programmers want to add 
acpi_device_id, then, it will
gain the 'ACPI probed'.

If each of them introduce a driver-specific match structure. Then, that going 
to introduce
very heavy maintain overhead to the maintainers, not to mention to achieve your 
cheap slogan:
"Unifying DT and non-DT paths via generic property / fwnode code".


--
Best regards,
Sui



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

2024-04-24 Thread Sui Jingfeng

Hi,


On 2024/4/24 16:39, Dmitry Baryshkov wrote:

Ok, what is the_bug_  that is being fixed by this patch?


I didn't have a way to use that driver under non DT environment,
this is the bug. Note: I demanding full features.

Both st7735r.c and repaper.c requires additional device property.

 - For st7735r.c: device_property_read_u32(dev, "rotation", &rotation);
 - For repaper.c: device_property_read_string(dev, "pervasive,thermal-zone", 
&thermal_zone)


Under non DT environment, those device properties can not be read.
Software node backend provided a way. Otherwise, the net results
of the patch doesn't meet the description in the commit message.


  If you check
the 'submitting-patches.rst', you'll find this phrase as a description
of the Fixes: tag.


I have readthe 'submitting-patches.rst', the first sentence tell us that
"A Fixes: tag indicates that the patch fixes an issue in a previous commit."

So what's the problem?



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.

As far as I can see, repaper correctly handles the case by falling
back to the spi_id. So does st7735r.c.



Yeah, this explicitly is the issue.

Falling back to other means is robust design, but it explicitly says
that the freshly addeddevice_get_match_data() don't works at all
under non DT environment. This is the bug, so what's you concern?
 
According to the commit message, the purpose of the introduction of
thedevice_get_match_data() is to achieve OF independent. But as you said, 
it will  fall back, then how does the goal "Make driver

OF-independent" can be achieved by the patch itself?


--
Best regards,
Sui



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

2024-04-24 Thread Dmitry Baryshkov
On Wed, 24 Apr 2024 at 08:09, Sui Jingfeng  wrote:
>
> Hi,
>
>
> On 2024/4/24 05:37, Dmitry Baryshkov wrote:
> > 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.
>
>
> Theupstream kernels are facing the same problem,by default 
> drm-misc-x86_defconfigdon't has the CONFIG_OF and CONFIG_OF_OVERLAY  selected.
> See [1] for an example.
>
> [1] 
> https://cgit.freedesktop.org/drm/drm-tip/tree/drm-misc-x86_defconfig?h=rerere-cache
>
>
> > But let me throw an argument why this patch (or something similar) looks
> > to be necessary.
>
> Agreed till to here.
>
>
> > 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.
>
>
> Yeah, still sounds good.
>
>
> > Currently handling the data coming from such device_ids requires using
> > special bits of code,
>
>
> It get started to deviate from here, as you are going to rash onto a narrow 
> way.
> Because you made the wrong assumption, it can be platform devices, it can 
> *also*
> be of platform device created by the of_platform_device_create(). The patch 
> itself
> won't put strong restrictions about its users.

Devices created via of_platform_device_create() have associated
device_node, so they won't have such an issue.

>
>
> > e.g. platform_get_device_id(pdev)->driver_data to
> > get the data from the platform_device_id.
>
> Right, but you run into a narrow area and stuck yourself.
> The so called non-DT, non-ACPI platform devices are all you basis of you 
> argument, right?
>
> There have plenty i2c device and SPI device associated with software note 
> properties.
> After applied this patch, it means that device_get_match_data() can also 
> works for
> those device.
>
> And the approach you provide already generate a lot of *boilerplate*...

Ok, so here you are making an assumption

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

2024-04-23 Thread Sui Jingfeng

Hi,


On 2024/4/24 05:37, Dmitry Baryshkov wrote:

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.



Theupstream kernels are facing the same problem,by default 
drm-misc-x86_defconfigdon't has the CONFIG_OF and CONFIG_OF_OVERLAY  selected.
See [1] for an example.
 
[1] https://cgit.freedesktop.org/drm/drm-tip/tree/drm-misc-x86_defconfig?h=rerere-cache




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


Agreed till to here.



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.



Yeah, still sounds good.



Currently handling the data coming from such device_ids requires using
special bits of code,



It get started to deviate from here, as you are going to rash onto a narrow way.
Because you made the wrong assumption, it can be platform devices, it can *also*
be of platform device created by the of_platform_device_create(). The patch 
itself
won't put strong restrictions about its users.



e.g. platform_get_device_id(pdev)->driver_data to
get the data from the platform_device_id.


Right, but you run into a narrow area and stuck yourself.
The so called non-DT, non-ACPI platform devices are all you basis of you 
argument, right?

There have plenty i2c device and SPI device associated with software note 
properties.
After applied this patch, it means that device_get_match_data() can also works 
for
those device.

And the approach you provide already generate a lot of *boilerplate*...


Having such codepaths goes
against the goal of unifying DT and non-DT paths via generic property /
fwnode code.



Who's goal? your goal or community's goal? is it documented somewhere?

Andy's goal is just to make those two drivers truely DT independent,
and I agree with Andy. I'm going to cooperate with Andy to achieve this
small goal.

However, apparently, our goal is *different* with your goal, your goal
is a big goal. If you have such a ambitious goal, you can definitely do
something on behalf of yourself.

For example, improving DT overlay support for the FPGA device, Or making
the of_device_id stuff truly platform neutral before telling pe

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

2024-04-23 Thread 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_mat

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

2024-04-23 Thread Sui Jingfeng

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?

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.
 




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.

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 
Cc: Daniel Scally 
Cc: Heikki Krogerus 
Cc: Sakari Ailus 
Cc: Greg Kroah-Hartman 
Cc: "Rafael J. Wysocki" 

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 =

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

2024-04-23 Thread Andy Shevchenko
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.

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

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

> Closes: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/

Yes, and then Reported-by, which is missing here.

> Cc: Andy Shevchenko 
> Cc: Daniel Scally 
> Cc: Heikki Krogerus 
> Cc: Sakari Ailus 
> Cc: Greg Kroah-Hartman 
> Cc: "Rafael J. Wysocki" 

Please, move these after the cutter '---' line (note you may have that line in
your local repo).

...

> +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?

> + if (ret < 0 || !val)
> + return NULL;

> + while (matches && matches->compatible[0]) {

First part of the conditional is invariant to the loop. Can be simply

matches = dev->driver->of_match_table;
if (!matches)
return NULL;

while (...)

> + if (!strcmp(matches->compatible, val))
> + return matches->data;
> +
> + matches++;
> + }
> +
> + return NULL;
> +}

-- 
With Best Regards,
Andy Shevchenko




[PATCH v2] software node: Implement device_get_match_data fwnode callback

2024-04-22 Thread Sui Jingfeng
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
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.

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")
Closes: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
Cc: Andy Shevchenko 
Cc: Daniel Scally 
Cc: Heikki Krogerus 
Cc: Sakari Ailus 
Cc: Greg Kroah-Hartman 
Cc: "Rafael J. Wysocki" 
Signed-off-by: Sui Jingfeng 
---
 V2: Update commit message
 drivers/base/swnode.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index eb6eb25b343b..48d18a90b97b 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -390,6 +391,30 @@ static void software_node_put(struct fwnode_handle *fwnode)
kobject_put(&swnode->kobj);
 }
 
+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);
+   if (ret < 0 || !val)
+   return NULL;
+
+   while (matches && matches->compatible[0]) {
+   if (!strcmp(matches->compatible, val))
+   return matches->data;
+
+   matches++;
+   }
+
+   return NULL;
+}
+
 static bool software_node_property_present(const struct fwnode_handle *fwnode,
   const char *propname)
 {
@@ -676,6 +701,7 @@ software_node_graph_parse_endpoint(const struct 
fwnode_handle *fwnode,
 static const struct fwnode_operations software_node_ops = {
.get = software_node_get,
.put = software_node_put,
+   .device_get_match_data = software_node_get_match_data,
.property_present = software_node_property_present,
.property_read_int_array = software_node_read_int_array,
.property_read_string_array = software_node_read_string_array,
-- 
2.34.1