[PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Andy Shevchenko]

This is to cater the need for non-ACPI system whereby
a platform device has to be created in order to bind
with the Denverton pinctrl platform driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-denverton.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-denverton.c b/drivers/pinctrl/intel/pinctrl-denverton.c
index 0c4694cfa594..a1a7242e0451 100644
--- a/drivers/pinctrl/intel/pinctrl-denverton.c
+++ b/drivers/pinctrl/intel/pinctrl-denverton.c
@@ -257,6 +257,11 @@ static const struct acpi_device_id dnv_pinctrl_acpi_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, dnv_pinctrl_acpi_match);
 
+static const struct platform_device_id dnv_pinctrl_platform_ids[] = {
+	{ "denverton-pinctrl", (kernel_ulong_t)&dnv_soc_data },
+	{ }
+};
+
 static struct platform_driver dnv_pinctrl_driver = {
 	.probe = intel_pinctrl_probe_by_hid,
 	.driver = {
@@ -264,6 +269,7 @@ static struct platform_driver dnv_pinctrl_driver = {
 		.acpi_match_table = dnv_pinctrl_acpi_match,
 		.pm = &dnv_pinctrl_pm_ops,
 	},
+	.id_table = dnv_pinctrl_platform_ids,
 };
 
 static int __init dnv_pinctrl_init(void)
@@ -281,4 +287,5 @@ module_exit(dnv_pinctrl_exit);
 MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
 MODULE_DESCRIPTION("Intel Denverton SoC pinctrl/GPIO driver");
 MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:denverton-pinctrl");
 MODULE_IMPORT_NS(PINCTRL_INTEL);
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Mika Westerberg]

On Tue, Sep 26, 2023 at 10:08:18PM +0300, Andy Shevchenko wrote:
> This is to cater the need for non-ACPI system whereby
> a platform device has to be created in order to bind
> with the Denverton pinctrl platform driver.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Andy Shevchenko]

On Wed, Sep 27, 2023 at 10:43:55AM +0300, Mika Westerberg wrote:
> On Tue, Sep 26, 2023 at 10:08:18PM +0300, Andy Shevchenko wrote:
> > This is to cater the need for non-ACPI system whereby
> > a platform device has to be created in order to bind
> > with the Denverton pinctrl platform driver.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Krzysztof Kozlowski]

On 26/09/2023 21:08, Andy Shevchenko wrote:
> This is to cater the need for non-ACPI system whereby
> a platform device has to be created in order to bind
> with the Denverton pinctrl platform driver.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-denverton.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-denverton.c b/drivers/pinctrl/intel/pinctrl-denverton.c
> index 0c4694cfa594..a1a7242e0451 100644
> --- a/drivers/pinctrl/intel/pinctrl-denverton.c
> +++ b/drivers/pinctrl/intel/pinctrl-denverton.c
> @@ -257,6 +257,11 @@ static const struct acpi_device_id dnv_pinctrl_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, dnv_pinctrl_acpi_match);
>  
> +static const struct platform_device_id dnv_pinctrl_platform_ids[] = {
> +	{ "denverton-pinctrl", (kernel_ulong_t)&dnv_soc_data },
> +	{ }
> +};
> +
>  static struct platform_driver dnv_pinctrl_driver = {
>  	.probe = intel_pinctrl_probe_by_hid,
>  	.driver = {
> @@ -264,6 +269,7 @@ static struct platform_driver dnv_pinctrl_driver = {
>  		.acpi_match_table = dnv_pinctrl_acpi_match,
>  		.pm = &dnv_pinctrl_pm_ops,
>  	},
> +	.id_table = dnv_pinctrl_platform_ids,
>  };
>  
>  static int __init dnv_pinctrl_init(void)
> @@ -281,4 +287,5 @@ module_exit(dnv_pinctrl_exit);
>  MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>");
>  MODULE_DESCRIPTION("Intel Denverton SoC pinctrl/GPIO driver");
>  MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:denverton-pinctrl");

Why do you need the alias? It's the same as ID table. You most likely
miss MODULE_DEVICE_TABLE() or your table is just wrong.

Best regards,
Krzysztof

Re: [PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Andy Shevchenko]

On Wed, Oct 4, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 26/09/2023 21:08, Andy Shevchenko wrote:

...

> > +MODULE_ALIAS("platform:denverton-pinctrl");
>
> Why do you need the alias? It's the same as ID table. You most likely
> miss MODULE_DEVICE_TABLE() or your table is just wrong.

This is cargo cult from pinctrl-broxton.c. If we want to fix, we need
to fix both.
Care to send patches?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Krzysztof Kozlowski]

On 04/10/2023 15:09, Andy Shevchenko wrote:
> On Wed, Oct 4, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 26/09/2023 21:08, Andy Shevchenko wrote:
> 
> ...
> 
>>> +MODULE_ALIAS("platform:denverton-pinctrl");
>>
>> Why do you need the alias? It's the same as ID table. You most likely
>> miss MODULE_DEVICE_TABLE() or your table is just wrong.
> 
> This is cargo cult from pinctrl-broxton.c. If we want to fix, we need
> to fix both.
> Care to send patches?

I don't understand how some other file affects this. Why do you exactly
need module alias here? Which use-case does not work without it (after
adding proper MODULE_DEVICE_TABLE())?

Best regards,
Krzysztof

Re: [PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Andy Shevchenko]

On Wed, Oct 4, 2023 at 4:20 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 04/10/2023 15:09, Andy Shevchenko wrote:
> > On Wed, Oct 4, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> On 26/09/2023 21:08, Andy Shevchenko wrote:

...

> >>> +MODULE_ALIAS("platform:denverton-pinctrl");
> >>
> >> Why do you need the alias? It's the same as ID table. You most likely
> >> miss MODULE_DEVICE_TABLE() or your table is just wrong.
> >
> > This is cargo cult from pinctrl-broxton.c. If we want to fix, we need
> > to fix both.
> > Care to send patches?
>
> I don't understand how some other file affects this. Why do you exactly
> need module alias here? Which use-case does not work without it (after
> adding proper MODULE_DEVICE_TABLE())?

We try to keep these drivers uniform. So, if something should be done
differently, it should be done for all affected drivers.
Since patch is applied, now we have two drivers to improve.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Krzysztof Kozlowski]

On 04/10/2023 21:42, Andy Shevchenko wrote:
> On Wed, Oct 4, 2023 at 4:20 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 04/10/2023 15:09, Andy Shevchenko wrote:
>>> On Wed, Oct 4, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> On 26/09/2023 21:08, Andy Shevchenko wrote:
> 
> ...
> 
>>>>> +MODULE_ALIAS("platform:denverton-pinctrl");
>>>>
>>>> Why do you need the alias? It's the same as ID table. You most likely
>>>> miss MODULE_DEVICE_TABLE() or your table is just wrong.
>>>
>>> This is cargo cult from pinctrl-broxton.c. If we want to fix, we need
>>> to fix both.
>>> Care to send patches?
>>
>> I don't understand how some other file affects this. Why do you exactly
>> need module alias here? Which use-case does not work without it (after
>> adding proper MODULE_DEVICE_TABLE())?
> 
> We try to keep these drivers uniform. So, if something should be done
> differently, it should be done for all affected drivers.
> Since patch is applied, now we have two drivers to improve.

Awesome reason. So if you add accidentally bug to one driver, it should
be copied to other file as well.

Best regards,
Krzysztof

Re: [PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Andy Shevchenko]

On Thu, Oct 05, 2023 at 09:53:51AM +0200, Krzysztof Kozlowski wrote:
> On 04/10/2023 21:42, Andy Shevchenko wrote:
> > On Wed, Oct 4, 2023 at 4:20 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> On 04/10/2023 15:09, Andy Shevchenko wrote:
> >>> On Wed, Oct 4, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>>> On 26/09/2023 21:08, Andy Shevchenko wrote:

...

> >>>>> +MODULE_ALIAS("platform:denverton-pinctrl");
> >>>>
> >>>> Why do you need the alias? It's the same as ID table. You most likely
> >>>> miss MODULE_DEVICE_TABLE() or your table is just wrong.
> >>>
> >>> This is cargo cult from pinctrl-broxton.c. If we want to fix, we need
> >>> to fix both.
> >>> Care to send patches?
> >>
> >> I don't understand how some other file affects this. Why do you exactly
> >> need module alias here? Which use-case does not work without it (after
> >> adding proper MODULE_DEVICE_TABLE())?
> > 
> > We try to keep these drivers uniform. So, if something should be done
> > differently, it should be done for all affected drivers.
> > Since patch is applied, now we have two drivers to improve.
> 
> Awesome reason. So if you add accidentally bug to one driver, it should
> be copied to other file as well.

I'm not sure where it's a bug, it works. The problem is implementation
and I believe Mika and certainly me agree with your points that
MODULE_DEVICE_TABLE() is a right way to go.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] pinctrl: denverton: Enable platform device in the absence of ACPI enumeration

[Andy Shevchenko]

On Thu, Oct 05, 2023 at 11:38:38AM +0300, Andy Shevchenko wrote:
> On Thu, Oct 05, 2023 at 09:53:51AM +0200, Krzysztof Kozlowski wrote:
> > On 04/10/2023 21:42, Andy Shevchenko wrote:
> > > On Wed, Oct 4, 2023 at 4:20 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >> On 04/10/2023 15:09, Andy Shevchenko wrote:
> > >>> On Wed, Oct 4, 2023 at 11:18 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >>>> On 26/09/2023 21:08, Andy Shevchenko wrote:

...

> > >>>>> +MODULE_ALIAS("platform:denverton-pinctrl");
> > >>>>
> > >>>> Why do you need the alias? It's the same as ID table. You most likely
> > >>>> miss MODULE_DEVICE_TABLE() or your table is just wrong.
> > >>>
> > >>> This is cargo cult from pinctrl-broxton.c. If we want to fix, we need
> > >>> to fix both.
> > >>> Care to send patches?
> > >>
> > >> I don't understand how some other file affects this. Why do you exactly
> > >> need module alias here? Which use-case does not work without it (after
> > >> adding proper MODULE_DEVICE_TABLE())?
> > > 
> > > We try to keep these drivers uniform. So, if something should be done
> > > differently, it should be done for all affected drivers.
> > > Since patch is applied, now we have two drivers to improve.
> > 
> > Awesome reason. So if you add accidentally bug to one driver, it should
> > be copied to other file as well.
> 
> I'm not sure where it's a bug, it works. The problem is implementation
> and I believe Mika and certainly me agree with your points that
> MODULE_DEVICE_TABLE() is a right way to go.

The initial implementation was done by 0c3013bbe1d8 ("pinctrl/broxton:
enable platform device in the absence of ACPI enumeration"). It was
ACKed by the driver maintainer as this one. But again, we agree that
your way is better.

-- 
With Best Regards,
Andy Shevchenko