Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x"

Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x"

[Peter Rosin]

On 2017-03-21 20:13, Andy Shevchenko wrote:
> In ACPI world any ID should be carefully chosen and registered
> officially. The commit bbf9d262a147 seems did a wrong assumption because
> PCA is the registered PNP ID for "PHILIPS BU ADD ON CARD". I'm pretty
> sure this prefix has nothing to do with the driver in question.

[Cc: leds people, in case they know any details]

Hmmm, a couple of questions about that "pretty sure"...

Philips and NXP are pretty much just different faces of the same coin, IIUC.
But, what do I know? Also, what about the leds drivers for NXP PCA955x and
NXP PCA963x? Do they suffer from the same crap? And if not, why is that
any different?

>From drivers/leds/leds-pca955x.c

static const struct acpi_device_id pca955x_acpi_ids[] = {
        { "PCA9550",  pca9550 },
        { "PCA9551",  pca9551 },
        { "PCA9552",  pca9552 },
        { "PCA9553",  pca9553 },
        { }
};
MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);

and from drivers/leds/leds-pca963x.c

static const struct acpi_device_id pca963x_acpi_ids[] = {
        { "PCA9632", pca9633 },
        { "PCA9633", pca9633 },
        { "PCA9634", pca9634 },
        { "PCA9635", pca9635 },
        { }
};
MODULE_DEVICE_TABLE(acpi, pca963x_acpi_ids);

But maybe I'm full of it and these led chips really are part of "PHILIPS
BU ADD ON CARD", while the muxer chips are not? I doubt it though...
But again, what do I know?

Cheers,
peda

> Moreover, newer ACPI specification has a support of _DSD method and
> special device IDs to allow drivers be enumerated via compatible string.
> The slight change to support this kind of enumeration will be added in
> sequential patch against pca954x.c.
> 
> Revert the commit bbf9d262a147 for good.
> 
> Cc: Tin Huynh <tnhuynh@apm.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 28 +---------------------------
>  1 file changed, 1 insertion(+), 27 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index dfc1c0e37c40..333a3918b656 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -35,7 +35,6 @@
>   * warranty of any kind, whether express or implied.
>   */
>  
> -#include <linux/acpi.h>
>  #include <linux/device.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
> @@ -141,21 +140,6 @@ static const struct i2c_device_id pca954x_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
>  
> -#ifdef CONFIG_ACPI
> -static const struct acpi_device_id pca954x_acpi_ids[] = {
> -	{ .id = "PCA9540", .driver_data = pca_9540 },
> -	{ .id = "PCA9542", .driver_data = pca_9542 },
> -	{ .id = "PCA9543", .driver_data = pca_9543 },
> -	{ .id = "PCA9544", .driver_data = pca_9544 },
> -	{ .id = "PCA9545", .driver_data = pca_9545 },
> -	{ .id = "PCA9546", .driver_data = pca_9545 },
> -	{ .id = "PCA9547", .driver_data = pca_9547 },
> -	{ .id = "PCA9548", .driver_data = pca_9548 },
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(acpi, pca954x_acpi_ids);
> -#endif
> -
>  #ifdef CONFIG_OF
>  static const struct of_device_id pca954x_of_match[] = {
>  	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
> @@ -393,17 +377,8 @@ static int pca954x_probe(struct i2c_client *client,
>  	match = of_match_device(of_match_ptr(pca954x_of_match), &client->dev);
>  	if (match)
>  		data->chip = of_device_get_match_data(&client->dev);
> -	else if (id)
> +	else
>  		data->chip = &chips[id->driver_data];
> -	else {
> -		const struct acpi_device_id *acpi_id;
> -
> -		acpi_id = acpi_match_device(ACPI_PTR(pca954x_acpi_ids),
> -						&client->dev);
> -		if (!acpi_id)
> -			return -ENODEV;
> -		data->chip = &chips[acpi_id->driver_data];
> -	}
>  
>  	data->last_chan = 0;		   /* force the first selection */
>  
> @@ -492,7 +467,6 @@ static struct i2c_driver pca954x_driver = {
>  		.name	= "pca954x",
>  		.pm	= &pca954x_pm,
>  		.of_match_table = of_match_ptr(pca954x_of_match),
> -		.acpi_match_table = ACPI_PTR(pca954x_acpi_ids),
>  	},
>  	.probe		= pca954x_probe,
>  	.remove		= pca954x_remove,
> 

Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x"

[Andy Shevchenko]

On Wed, 2017-03-22 at 11:23 +0100, Peter Rosin wrote:
> On 2017-03-21 20:13, Andy Shevchenko wrote:
> > In ACPI world any ID should be carefully chosen and registered
> > officially. The commit bbf9d262a147 seems did a wrong assumption
> > because
> > PCA is the registered PNP ID for "PHILIPS BU ADD ON CARD". I'm
> > pretty
> > sure this prefix has nothing to do with the driver in question.
> 
> [Cc: leds people, in case they know any details]
> 
> Hmmm, a couple of questions about that "pretty sure"...

I didn't neither see the *real* excerpt from DSDT nor hear anything
about official IDs from Phillips.

> Philips and NXP are pretty much just different faces of the same coin,
> IIUC.

Good to know.

While I might be mistaken, I would like to remove a confusion until we
get an official confirmation either in *real* existing product on the
market or letter from Phillips representatives (see above).

> But, what do I know? Also, what about the leds drivers for NXP PCA955x
> and
> NXP PCA963x? Do they suffer from the same crap? And if not, why is
> that
> any different?

They pretty much do.

Yesterday I send a patch to remove LP3952 invented ID which TI didn't
confirm to exists.

My scope now is limited by the ACPI-enabled drivers which are using
*gpiod_get*() functions.

> From drivers/leds/leds-pca955x.c
> 
> static const struct acpi_device_id pca955x_acpi_ids[] = {
>         { "PCA9550",  pca9550 },
>         { "PCA9551",  pca9551 },
>         { "PCA9552",  pca9552 },
>         { "PCA9553",  pca9553 },
>         { }
> };
> MODULE_DEVICE_TABLE(acpi, pca955x_acpi_ids);
> 
> and from drivers/leds/leds-pca963x.c
> 
> static const struct acpi_device_id pca963x_acpi_ids[] = {
>         { "PCA9632", pca9633 },
>         { "PCA9633", pca9633 },
>         { "PCA9634", pca9634 },
>         { "PCA9635", pca9635 },
>         { }
> };
> MODULE_DEVICE_TABLE(acpi, pca963x_acpi_ids);
> 
> But maybe I'm full of it and these led chips really are part of
> "PHILIPS
> BU ADD ON CARD", while the muxer chips are not? I doubt it though...
> But again, what do I know?

Thanks for input to this topic. As I said above I might be mistaken too,
though we can't just wilfully invent ACPI IDs without vendors' approvals
/ confirmations.

> 
> Cheers,
> peda
> 
> > Moreover, newer ACPI specification has a support of _DSD method and
> > special device IDs to allow drivers be enumerated via compatible
> > string.
> > The slight change to support this kind of enumeration will be added
> > in
> > sequential patch against pca954x.c.
> > 
> > Revert the commit bbf9d262a147 for good.
> > 
> > Cc: Tin Huynh <tnhuynh@apm.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/i2c/muxes/i2c-mux-pca954x.c | 28 +-------------------------
> > --
> >  1 file changed, 1 insertion(+), 27 deletions(-)
> > 
> > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > index dfc1c0e37c40..333a3918b656 100644
> > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> > @@ -35,7 +35,6 @@
> >   * warranty of any kind, whether express or implied.
> >   */
> >  
> > -#include <linux/acpi.h>
> >  #include <linux/device.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> > @@ -141,21 +140,6 @@ static const struct i2c_device_id pca954x_id[]
> > = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, pca954x_id);
> >  
> > -#ifdef CONFIG_ACPI
> > -static const struct acpi_device_id pca954x_acpi_ids[] = {
> > -	{ .id = "PCA9540", .driver_data = pca_9540 },
> > -	{ .id = "PCA9542", .driver_data = pca_9542 },
> > -	{ .id = "PCA9543", .driver_data = pca_9543 },
> > -	{ .id = "PCA9544", .driver_data = pca_9544 },
> > -	{ .id = "PCA9545", .driver_data = pca_9545 },
> > -	{ .id = "PCA9546", .driver_data = pca_9545 },
> > -	{ .id = "PCA9547", .driver_data = pca_9547 },
> > -	{ .id = "PCA9548", .driver_data = pca_9548 },
> > -	{ }
> > -};
> > -MODULE_DEVICE_TABLE(acpi, pca954x_acpi_ids);
> > -#endif
> > -
> >  #ifdef CONFIG_OF
> >  static const struct of_device_id pca954x_of_match[] = {
> >  	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
> > @@ -393,17 +377,8 @@ static int pca954x_probe(struct i2c_client
> > *client,
> >  	match = of_match_device(of_match_ptr(pca954x_of_match),
> > &client->dev);
> >  	if (match)
> >  		data->chip = of_device_get_match_data(&client-
> > >dev);
> > -	else if (id)
> > +	else
> >  		data->chip = &chips[id->driver_data];
> > -	else {
> > -		const struct acpi_device_id *acpi_id;
> > -
> > -		acpi_id =
> > acpi_match_device(ACPI_PTR(pca954x_acpi_ids),
> > -						&client->dev);
> > -		if (!acpi_id)
> > -			return -ENODEV;
> > -		data->chip = &chips[acpi_id->driver_data];
> > -	}
> >  
> >  	data->last_chan = 0;		   /* force the first
> > selection */
> >  
> > @@ -492,7 +467,6 @@ static struct i2c_driver pca954x_driver = {
> >  		.name	= "pca954x",
> >  		.pm	= &pca954x_pm,
> >  		.of_match_table = of_match_ptr(pca954x_of_match),
> > -		.acpi_match_table = ACPI_PTR(pca954x_acpi_ids),
> >  	},
> >  	.probe		= pca954x_probe,
> >  	.remove		= pca954x_remove,
> > 
> 
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x"

[Peter Rosin]

On 2017-03-22 14:05, Andy Shevchenko wrote:
> On Wed, 2017-03-22 at 11:23 +0100, Peter Rosin wrote:
>> On 2017-03-21 20:13, Andy Shevchenko wrote:
>>> In ACPI world any ID should be carefully chosen and registered
>>> officially. The commit bbf9d262a147 seems did a wrong assumption
>>> because
>>> PCA is the registered PNP ID for "PHILIPS BU ADD ON CARD". I'm
>>> pretty
>>> sure this prefix has nothing to do with the driver in question.
>>
>> [Cc: leds people, in case they know any details]
>>
>> Hmmm, a couple of questions about that "pretty sure"...
> 
> I didn't neither see the *real* excerpt from DSDT nor hear anything
> about official IDs from Phillips.
> 
>> Philips and NXP are pretty much just different faces of the same coin,
>> IIUC.
> 
> Good to know.
> 
> While I might be mistaken, I would like to remove a confusion until we
> get an official confirmation either in *real* existing product on the
> market or letter from Phillips representatives (see above).

Right, I don't disagree with the revert at all. The IDs were
apparently just grabbed and, as you point out, that is not the ACPI
way.

One more question though, the revert (patch 1/2) should probably be
queued up for current (4.11) and sent to stable as well (4.10 is the
only version affected), but what about patch 2/2? Is that 4.12
material or should it too be "rushed"? I feel a bit like I have
been thrown in at the deep end, and could use some guidance...

*snip*

> Thanks for input to this topic. As I said above I might be mistaken too,
> though we can't just wilfully invent ACPI IDs without vendors' approvals
> / confirmations.

Yes, I (now) fully understand that the ACPI namespace is not in our
hands.

*snip*

Cheers,
peda

Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x"

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]

On Thu 2017-03-23 08:45:58, Peter Rosin wrote:
> On 2017-03-22 14:05, Andy Shevchenko wrote:
> > On Wed, 2017-03-22 at 11:23 +0100, Peter Rosin wrote:
> >> On 2017-03-21 20:13, Andy Shevchenko wrote:
> >>> In ACPI world any ID should be carefully chosen and registered
> >>> officially. The commit bbf9d262a147 seems did a wrong assumption
> >>> because
> >>> PCA is the registered PNP ID for "PHILIPS BU ADD ON CARD". I'm
> >>> pretty
> >>> sure this prefix has nothing to do with the driver in question.
> >>
> >> [Cc: leds people, in case they know any details]
> >>
> >> Hmmm, a couple of questions about that "pretty sure"...
> > 
> > I didn't neither see the *real* excerpt from DSDT nor hear anything
> > about official IDs from Phillips.
> > 
> >> Philips and NXP are pretty much just different faces of the same coin,
> >> IIUC.
> > 
> > Good to know.
> > 
> > While I might be mistaken, I would like to remove a confusion until we
> > get an official confirmation either in *real* existing product on the
> > market or letter from Phillips representatives (see above).
> 
> Right, I don't disagree with the revert at all. The IDs were
> apparently just grabbed and, as you point out, that is not the ACPI
> way.
> 
> One more question though, the revert (patch 1/2) should probably be
> queued up for current (4.11) and sent to stable as well (4.10 is the
> only version affected), but what about patch 2/2? Is that 4.12

Sent to stable? What serious bug it fixes?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x"

[Peter Rosin]

On 2017-03-23 11:04, Pavel Machek wrote:
> On Thu 2017-03-23 08:45:58, Peter Rosin wrote:
>> On 2017-03-22 14:05, Andy Shevchenko wrote:
>>> On Wed, 2017-03-22 at 11:23 +0100, Peter Rosin wrote:
>>>> On 2017-03-21 20:13, Andy Shevchenko wrote:
>>>>> In ACPI world any ID should be carefully chosen and registered
>>>>> officially. The commit bbf9d262a147 seems did a wrong assumption
>>>>> because
>>>>> PCA is the registered PNP ID for "PHILIPS BU ADD ON CARD". I'm
>>>>> pretty
>>>>> sure this prefix has nothing to do with the driver in question.
>>>>
>>>> [Cc: leds people, in case they know any details]
>>>>
>>>> Hmmm, a couple of questions about that "pretty sure"...
>>>
>>> I didn't neither see the *real* excerpt from DSDT nor hear anything
>>> about official IDs from Phillips.
>>>
>>>> Philips and NXP are pretty much just different faces of the same coin,
>>>> IIUC.
>>>
>>> Good to know.
>>>
>>> While I might be mistaken, I would like to remove a confusion until we
>>> get an official confirmation either in *real* existing product on the
>>> market or letter from Phillips representatives (see above).
>>
>> Right, I don't disagree with the revert at all. The IDs were
>> apparently just grabbed and, as you point out, that is not the ACPI
>> way.
>>
>> One more question though, the revert (patch 1/2) should probably be
>> queued up for current (4.11) and sent to stable as well (4.10 is the
>> only version affected), but what about patch 2/2? Is that 4.12
> 
> Sent to stable? What serious bug it fixes?

Hi Pavel,

It obviously fixes the bug that someone might start depending on these
ACPI IDs when they shouldn't, which potentially stops that someone from
later complaining when it no longer works. I thought that this was
probably serious and qualified under "oh, that's not good". Because
regressions are nasty. Now that I look closer on the stable rules I
suppose it also disqualifies as it is on the theoretical side.

Also, the general tone from Andy indicates a certain amount of frustration
with the whole issue, which perhaps has no bearing on the seriousness,
but what do I know?

So, I'm still seeking guidance on how to handle these two patches.

Cheers,
peda

Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x"

[Andy Shevchenko]

On Thu, 2017-03-23 at 12:21 +0100, Peter Rosin wrote:
> On 2017-03-23 11:04, Pavel Machek wrote:
> > On Thu 2017-03-23 08:45:58, Peter Rosin wrote:
> > > On 2017-03-22 14:05, Andy Shevchenko wrote:
> > > > On Wed, 2017-03-22 at 11:23 +0100, Peter Rosin wrote:
> > > > > On 2017-03-21 20:13, Andy Shevchenko wrote:

> Also, the general tone from Andy indicates a certain amount of
> frustration
> with the whole issue,

I'm sorry for my tone, it begins with amount of abuse case of GPIO for
ACPI in kernel followed by wilfully invented ACPI IDs in the drivers.

>  which perhaps has no bearing on the seriousness,
> but what do I know?
> 
> So, I'm still seeking guidance on how to handle these two patches.

I have no strong opinion, but IDs might collide in the future if PCAxxxx
will be assigned to something else and the patch will go stable@ (yeah,
OTOH it's quite unlikely).

So, we may postpone any stable@ back porting up to some real issue
appears. Regarding second one, it's not important at all (could be even
dropped). Consider it as example how to use _DSD and PRP0001 in ACPI
case for DT-enabled drivers.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x"

[Peter Rosin]

On 2017-03-23 13:16, Andy Shevchenko wrote:
> On Thu, 2017-03-23 at 12:21 +0100, Peter Rosin wrote:
>> On 2017-03-23 11:04, Pavel Machek wrote:
>>> On Thu 2017-03-23 08:45:58, Peter Rosin wrote:
>>>> On 2017-03-22 14:05, Andy Shevchenko wrote:
>>>>> On Wed, 2017-03-22 at 11:23 +0100, Peter Rosin wrote:
>>>>>> On 2017-03-21 20:13, Andy Shevchenko wrote:
> 
>> Also, the general tone from Andy indicates a certain amount of
>> frustration
>> with the whole issue,
> 
> I'm sorry for my tone, it begins with amount of abuse case of GPIO for
> ACPI in kernel followed by wilfully invented ACPI IDs in the drivers.

No worries, it's understandable that someone chasing after the same
bleeding issue over and over will get tired of it.

>>  which perhaps has no bearing on the seriousness,
>> but what do I know?
>>
>> So, I'm still seeking guidance on how to handle these two patches.
> 
> I have no strong opinion, but IDs might collide in the future if PCAxxxx
> will be assigned to something else and the patch will go stable@ (yeah,
> OTOH it's quite unlikely).
> 
> So, we may postpone any stable@ back porting up to some real issue
> appears. Regarding second one, it's not important at all (could be even
> dropped). Consider it as example how to use _DSD and PRP0001 in ACPI
> case for DT-enabled drivers.

Ok, I have added 1/2 to my i2c-mux for-current branch and will drop
2/2 unless someone pipes up.

Cheers,
peda

Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x"

[Andy Shevchenko]

On Fri, 2017-03-24 at 11:21 +0100, Peter Rosin wrote:
> On 2017-03-23 13:16, Andy Shevchenko wrote:
> > On Thu, 2017-03-23 at 12:21 +0100, Peter Rosin wrote:

> > > So, I'm still seeking guidance on how to handle these two patches.
> > 
> > I have no strong opinion, but IDs might collide in the future if
> > PCAxxxx
> > will be assigned to something else and the patch will go stable@
> > (yeah,
> > OTOH it's quite unlikely).
> > 
> > So, we may postpone any stable@ back porting up to some real issue
> > appears. Regarding second one, it's not important at all (could be
> > even
> > dropped). Consider it as example how to use _DSD and PRP0001 in ACPI
> > case for DT-enabled drivers.
> 
> Ok, I have added 1/2 to my i2c-mux for-current branch and will drop
> 2/2 unless someone pipes up.

Fair enough.
Thanks!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy