Received-SPF: pass (google.com: domain of linux-kernel+bounces-157614-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240422164658.217037-1-sui.jingfeng@linux.dev>
 <Zie3ebHOEpWHj1qV@smile.fi.intel.com> <d5bc1e73-a553-451e-ab74-f5f0ca259c6b@linux.dev>
 <hcltp2bbxxg2t7ibmzgiib7mgbwgmvzwnnq6mochdh7c4h76r3@bxj6yk5zq64e>
 <ZikE4qOVO7rgIs9a@smile.fi.intel.com> <CAA8EJpr1FSjizAh6Dp5Bmux3NrGYh=BfHFL4D1fa87Og4ymY0w@mail.gmail.com>
 <Zikck2FJb4-PgXX0@smile.fi.intel.com> <kucey3jsydjye3ndflns3cqolcpizhnxgzz3fkmcatlnpbnwrk@22ulsqfh2vf6>
 <Zik3AjiWkytSVn-1@smile.fi.intel.com>
In-Reply-To: <Zik3AjiWkytSVn-1@smile.fi.intel.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Wed, 24 Apr 2024 22:32:33 +0300
Message-ID: <CAA8EJpr_zAmsVMeOvMjmnxZpcoDsLk3X9fTtQjn5aDEt=hQPPQ@mail.gmail.com>
Subject: Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Sui Jingfeng <sui.jingfeng@linux.dev>, dri-devel@lists.freedesktop.org, 
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Daniel Scally <djrscally@gmail.com>, Heikki Krogerus <heikki.krogerus@linux.intel.com>, 
	Sakari Ailus <sakari.ailus@linux.intel.com>, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, 24 Apr 2024 at 19:45, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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
> > > > <andriy.shevchenko@linux.intel.com> 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 wrot=
e:
>
> ...
>
> > > > > > But let me throw an argument why this patch (or something simil=
ar) looks
> > > > > > to be necessary.
> > > > > >
> > > > > > Both on DT and non-DT systems the kernel allows using the non-O=
F based
> > > > > > matching. For the platform devices there is platform_device_id-=
based
> > > > > > matching.
> > > > > >
> > > > > > Currently handling the data coming from such device_ids require=
s 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 pr=
operty /
> > > > > > fwnode code.
> > > > > >
> > > > > > As such, I support Sui's idea of being able to use device_get_m=
atch_data
> > > > > > for non-DT, non-ACPI platform devices.
> > > > >
> > > > > I'm not sure I buy this. We have a special helpers based on the b=
us type to
> > > > > combine device_get_match_data() with the respective ID table craw=
ling, see
> > > > > the SPI and I=C2=B2C 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 hav=
e
> 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] =3D
PROPERTY_ENTRY_STRING("compatible", "snps,designware-i2c");
drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c: nodes->sfp_props[0] =3D
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).

--=20
With best wishes
Dmitry