| Message ID | 20170321191310.32957-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
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, > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, > > > >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
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,
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. 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(-)