From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Subject: Re: [PATCH v1 1/2] Revert "i2c: mux: pca954x: Add ACPI support for pca954x" Date: Wed, 22 Mar 2017 15:05:42 +0200 Message-ID: <1490187942.19767.161.camel@linux.intel.com> References: <20170321191310.32957-1-andriy.shevchenko@linux.intel.com> <01b7c79e-c52f-8e87-59a8-2eb17a72d733@axentia.se> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <01b7c79e-c52f-8e87-59a8-2eb17a72d733@axentia.se> Sender: linux-leds-owner@vger.kernel.org To: Peter Rosin , linux-i2c@vger.kernel.org, Wolfram Sang Cc: Tin Huynh , Richard Purdie , Jacek Anaszewski , Pavel Machek , linux-leds@vger.kernel.org List-Id: linux-i2c@vger.kernel.org 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 > > Signed-off-by: Andy Shevchenko > > --- > >  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 > >  #include > >  #include > >  #include > > @@ -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 Intel Finland Oy