| Message ID | 20171225155723.6338-3-m.capdeville@no-log.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marc,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.15-rc5 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Marc-CAPDEVILLE/i2c-core-acpi-Add-i2c_acpi_set_connection/20171226-083729
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-c0-12261310 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/iio/light/cm32181.o: In function `cm32181_irq':
>> drivers/iio/light/cm32181.c:336: undefined reference to `i2c_smbus_alert_event'
drivers/iio/light/cm32181.o: In function `cm32181_probe':
>> drivers/iio/light/cm32181.c:405: undefined reference to `i2c_require_smbus_alert'
drivers/iio/light/cm32181.o: In function `cm32181_reg_init':
drivers/iio/light/cm32181.c:95: undefined reference to `i2c_smbus_alert_event'
vim +336 drivers/iio/light/cm32181.c
329
330 static irqreturn_t cm32181_irq(int irq, void *d)
331 {
332 struct cm32181_chip *cm32181 = d;
333
334 if (cm32181->chip_id == CM3218_ID) {
335 /* This is cm3218 chip irq, so handle the smbus alert */
> 336 i2c_smbus_alert_event(cm32181->client);
337 }
338
339 return IRQ_HANDLED;
340 }
341
342 static int cm32181_probe(struct i2c_client *client,
343 const struct i2c_device_id *id)
344 {
345 struct cm32181_chip *cm32181;
346 struct iio_dev *indio_dev;
347 int ret;
348 const struct of_device_id *of_id;
349 const struct acpi_device_id *acpi_id;
350
351 indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
352 if (!indio_dev) {
353 dev_err(&client->dev, "devm_iio_device_alloc failed\n");
354 return -ENOMEM;
355 }
356
357 cm32181 = iio_priv(indio_dev);
358 i2c_set_clientdata(client, indio_dev);
359 cm32181->client = client;
360
361 mutex_init(&cm32181->lock);
362 indio_dev->dev.parent = &client->dev;
363 indio_dev->channels = cm32181_channels;
364 indio_dev->num_channels = ARRAY_SIZE(cm32181_channels);
365 indio_dev->info = &cm32181_info;
366 indio_dev->name = id->name;
367 indio_dev->modes = INDIO_DIRECT_MODE;
368
369 /* Lookup for chip ID from platform, acpi or of device table */
370 if (id) {
371 cm32181->chip_id = id->driver_data;
372 } else if (ACPI_COMPANION(&client->dev)) {
373 acpi_id = acpi_match_device(client->dev.driver->acpi_match_table,
374 &client->dev);
375 if (!acpi_id)
376 return -ENODEV;
377
378 cm32181->chip_id = (kernel_ulong_t)acpi_id->driver_data;
379 } else if (client->dev.of_node) {
380 of_id = of_match_device(client->dev.driver->of_match_table,
381 &client->dev);
382 if (!of_id)
383 return -ENODEV;
384
385 cm32181->chip_id = (kernel_ulong_t)of_id->data;
386 } else {
387 return -ENODEV;
388 }
389
390 if (cm32181->chip_id == CM3218_ID) {
391 if (ACPI_COMPANION(&client->dev) &&
392 client->addr == SMBUS_ARA_ADDR) {
393 /*
394 * In case this device as been enumerated by acpi
395 * with the reserved smbus ARA address (first acpi
396 * connection), request change of address to the second
397 * connection.
398 */
399 ret = i2c_acpi_set_connection(client, 1);
400 if (ret)
401 return ret;
402 }
403
404 /* cm3218 chip require an ara device on his adapter */
> 405 ret = i2c_require_smbus_alert(client);
406 if (ret < 0)
407 return ret;
408
409 /*
410 * If irq is given, request a threaded irq handler to manage
411 * smbus alert.
412 */
413 if (client->irq > 0) {
414 ret = devm_request_threaded_irq(&client->dev,
415 client->irq,
416 NULL, cm32181_irq,
417 IRQF_ONESHOT,
418 "cm32181", cm32181);
419 if (ret)
420 return ret;
421 }
422 }
423
424 ret = cm32181_reg_init(cm32181);
425 if (ret) {
426 dev_err(&client->dev,
427 "%s: register init failed\n",
428 __func__);
429 return ret;
430 }
431
432 ret = devm_iio_device_register(&client->dev, indio_dev);
433 if (ret) {
434 dev_err(&client->dev,
435 "%s: regist device failed\n",
436 __func__);
437 return ret;
438 }
439
440 return 0;
441 }
442
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Monday, December 25, 2017 4:57:22 PM CET Marc CAPDEVILLE wrote: > On asus T100, Capella cm3218 chip is implemented as ambiant light > sensor. This chip expose an smbus ARA protocol device on standard > address 0x0c. The chip is not functional before all alerts are > acknowledged. > On asus T100, this device is enumerated on ACPI bus and the description > give two I2C connection. The first is the connection to the ARA device > and the second gives the real address of the device. > > So, on device probe,If the i2c address is the ARA address and the > device is enumerated via acpi, we change address of the device for > the one in the second acpi serial bus connection. > This free the ara address so we can register with the smbus_alert > driver. > > If an interrupt resource is given, and a smbus_alert device was > found on the adapter, we request a treaded interrupt to > call i2c_smbus_alert_event to handle the alert. > > In somme case, the treaded interrupt is not schedule before the driver > try to initialize chip registers. So if first registers access fail, we > call i2c_smbus_alert_event() to acknowledge initial alert, then retry > register access. > > Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org> > --- > drivers/iio/light/cm32181.c | 120 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 115 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c > index aebf7dd071af..96c08755e6e3 100644 > --- a/drivers/iio/light/cm32181.c > +++ b/drivers/iio/light/cm32181.c > @@ -18,6 +18,9 @@ > #include <linux/iio/sysfs.h> > #include <linux/iio/events.h> > #include <linux/init.h> > +#include <linux/i2c-smbus.h> > +#include <linux/acpi.h> > +#include <linux/of_device.h> > > /* Registers Address */ > #define CM32181_REG_ADDR_CMD 0x00 > @@ -47,6 +50,11 @@ > #define CM32181_CALIBSCALE_RESOLUTION 1000 > #define MLUX_PER_LUX 1000 > > +#define CM32181_ID 0x81 > +#define CM3218_ID 0x18 > + > +#define SMBUS_ARA_ADDR 0x0c > + > static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = { > CM32181_REG_ADDR_CMD, > }; > @@ -57,6 +65,7 @@ static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000, > > struct cm32181_chip { > struct i2c_client *client; > + int chip_id; > struct mutex lock; > u16 conf_regs[CM32181_CONF_REG_NUM]; > int calibscale; > @@ -77,11 +86,22 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181) > s32 ret; > > ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID); > - if (ret < 0) > - return ret; > + if (ret < 0) { > + if (cm32181->chip_id == CM3218_ID) { > + /* > + * On cm3218, smbus alert trigger after probing, > + * so poll for first alert here, then retry. > + */ > + i2c_smbus_alert_event(client); > + ret = i2c_smbus_read_word_data(client, > + CM32181_REG_ADDR_ID); > + } else { > + return ret; > + } > + } > > /* check device ID */ > - if ((ret & 0xFF) != 0x81) > + if ((ret & 0xFF) != cm32181->chip_id) > return -ENODEV; > > /* Default Values */ > @@ -297,12 +317,36 @@ static const struct iio_info cm32181_info = { > .attrs = &cm32181_attribute_group, > }; > > +static void cm3218_alert(struct i2c_client *client, > + enum i2c_alert_protocol type, > + unsigned int data) > +{ > + /* > + * nothing to do for now. > + * This is just here to acknownledge the cm3218 alert. > + */ > +} > + > +static irqreturn_t cm32181_irq(int irq, void *d) > +{ > + struct cm32181_chip *cm32181 = d; > + > + if (cm32181->chip_id == CM3218_ID) { > + /* This is cm3218 chip irq, so handle the smbus alert */ > + i2c_smbus_alert_event(cm32181->client); > + } > + > + return IRQ_HANDLED; > +} > + > static int cm32181_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > struct cm32181_chip *cm32181; > struct iio_dev *indio_dev; > int ret; > + const struct of_device_id *of_id; > + const struct acpi_device_id *acpi_id; > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181)); > if (!indio_dev) { > @@ -322,6 +366,61 @@ static int cm32181_probe(struct i2c_client *client, > indio_dev->name = id->name; > indio_dev->modes = INDIO_DIRECT_MODE; > > + /* Lookup for chip ID from platform, acpi or of device table */ > + if (id) { > + cm32181->chip_id = id->driver_data; > + } else if (ACPI_COMPANION(&client->dev)) { > + acpi_id = acpi_match_device(client->dev.driver->acpi_match_table, > + &client->dev); > + if (!acpi_id) > + return -ENODEV; > + > + cm32181->chip_id = (kernel_ulong_t)acpi_id->driver_data; > + } else if (client->dev.of_node) { > + of_id = of_match_device(client->dev.driver->of_match_table, > + &client->dev); > + if (!of_id) > + return -ENODEV; > + > + cm32181->chip_id = (kernel_ulong_t)of_id->data; > + } else { > + return -ENODEV; > + } > + > + if (cm32181->chip_id == CM3218_ID) { > + if (ACPI_COMPANION(&client->dev) && > + client->addr == SMBUS_ARA_ADDR) { > + /* > + * In case this device as been enumerated by acpi > + * with the reserved smbus ARA address (first acpi > + * connection), request change of address to the second > + * connection. > + */ > + ret = i2c_acpi_set_connection(client, 1); > + if (ret) > + return ret; This looks super-fragile to me. What about making the enumeration aware of the SMBUS_ARA thing and avoid using resources corresponding to that at all? BTW, in comments and similar always spell ACPI in capitals. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 25 Dec 2017 16:57:22 +0100 Marc CAPDEVILLE <m.capdeville@no-log.org> wrote: > On asus T100, Capella cm3218 chip is implemented as ambiant light ambient > sensor. This chip expose an smbus ARA protocol device on standard > address 0x0c. The chip is not functional before all alerts are > acknowledged. > On asus T100, this device is enumerated on ACPI bus and the description > give two I2C connection. The first is the connection to the ARA device > and the second gives the real address of the device. > > So, on device probe,If the i2c address is the ARA address and the if > device is enumerated via acpi, we change address of the device for > the one in the second acpi serial bus connection. > This free the ara address so we can register with the smbus_alert > driver. > > If an interrupt resource is given, and a smbus_alert device was > found on the adapter, we request a treaded interrupt to threaded > call i2c_smbus_alert_event to handle the alert. > > In somme case, the treaded interrupt is not schedule before the driver threaded > try to initialize chip registers. So if first registers access fail, we > call i2c_smbus_alert_event() to acknowledge initial alert, then retry > register access. > > Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org> The main thing that bothers me about this is it is putting a non trivial burden on each individual driver. Perhaps we let it go like this for now, and see how common this is. It would be possible (I think) to do it in a fashion invisible to the client drivers, but I appreciate that would be more complex than this. The multiple devices sharing a bus with individual interrupt lines would need to be handled. Minor comments inline. Jonathan > --- > drivers/iio/light/cm32181.c | 120 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 115 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c > index aebf7dd071af..96c08755e6e3 100644 > --- a/drivers/iio/light/cm32181.c > +++ b/drivers/iio/light/cm32181.c > @@ -18,6 +18,9 @@ > #include <linux/iio/sysfs.h> > #include <linux/iio/events.h> > #include <linux/init.h> > +#include <linux/i2c-smbus.h> > +#include <linux/acpi.h> > +#include <linux/of_device.h> > > /* Registers Address */ > #define CM32181_REG_ADDR_CMD 0x00 > @@ -47,6 +50,11 @@ > #define CM32181_CALIBSCALE_RESOLUTION 1000 > #define MLUX_PER_LUX 1000 > > +#define CM32181_ID 0x81 > +#define CM3218_ID 0x18 > + > +#define SMBUS_ARA_ADDR 0x0c This isn't driver specific - to my mind belongs in the smbus header. > + > static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = { > CM32181_REG_ADDR_CMD, > }; > @@ -57,6 +65,7 @@ static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000, > > struct cm32181_chip { > struct i2c_client *client; > + int chip_id; > struct mutex lock; > u16 conf_regs[CM32181_CONF_REG_NUM]; > int calibscale; > @@ -77,11 +86,22 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181) > s32 ret; > > ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID); > - if (ret < 0) > - return ret; > + if (ret < 0) { > + if (cm32181->chip_id == CM3218_ID) { > + /* > + * On cm3218, smbus alert trigger after probing, > + * so poll for first alert here, then retry. Is it a level interrupt? I'd have thought we could leave this out and let the interrupt handler pick it up if so. The check on the ID is rather paranoid anyway so I wouldn't worry about it failing here. Or are we blocked from finishing configuring the device until this is done? > + */ > + i2c_smbus_alert_event(client); > + ret = i2c_smbus_read_word_data(client, > + CM32181_REG_ADDR_ID); > + } else { > + return ret; > + } > + } > > /* check device ID */ > - if ((ret & 0xFF) != 0x81) > + if ((ret & 0xFF) != cm32181->chip_id) > return -ENODEV; > > /* Default Values */ > @@ -297,12 +317,36 @@ static const struct iio_info cm32181_info = { > .attrs = &cm32181_attribute_group, > }; > > +static void cm3218_alert(struct i2c_client *client, > + enum i2c_alert_protocol type, > + unsigned int data) > +{ > + /* > + * nothing to do for now. > + * This is just here to acknownledge the cm3218 alert. > + */ > +} > + > +static irqreturn_t cm32181_irq(int irq, void *d) > +{ > + struct cm32181_chip *cm32181 = d; > + > + if (cm32181->chip_id == CM3218_ID) { I'd move the check out and only register this handler if the chip supports ARA in the first place. That then makes this handler generic to any ARA device. Not sure there is a generic handler available, but it would make sense to add one if there isn't in the smbus code. > + /* This is cm3218 chip irq, so handle the smbus alert */ > + i2c_smbus_alert_event(cm32181->client); > + } The 32181 also has interrupt support - just looks like it isn't ARA. I guess support for that can be added here at some later date. > + > + return IRQ_HANDLED; > +} > + > static int cm32181_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > struct cm32181_chip *cm32181; > struct iio_dev *indio_dev; > int ret; > + const struct of_device_id *of_id; > + const struct acpi_device_id *acpi_id; > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181)); > if (!indio_dev) { > @@ -322,6 +366,61 @@ static int cm32181_probe(struct i2c_client *client, > indio_dev->name = id->name; > indio_dev->modes = INDIO_DIRECT_MODE; > > + /* Lookup for chip ID from platform, acpi or of device table */ > + if (id) { > + cm32181->chip_id = id->driver_data; > + } else if (ACPI_COMPANION(&client->dev)) { > + acpi_id = acpi_match_device(client->dev.driver->acpi_match_table, > + &client->dev); > + if (!acpi_id) > + return -ENODEV; > + > + cm32181->chip_id = (kernel_ulong_t)acpi_id->driver_data; > + } else if (client->dev.of_node) { > + of_id = of_match_device(client->dev.driver->of_match_table, > + &client->dev); > + if (!of_id) > + return -ENODEV; > + > + cm32181->chip_id = (kernel_ulong_t)of_id->data; of_device_get_match_data perhaps? I thought something similar had been proposed for acpi as well to avoid this boiler plate, but can't find it right now so maybe that never went anywhere. > + } else { > + return -ENODEV; > + } > + > + if (cm32181->chip_id == CM3218_ID) { > + if (ACPI_COMPANION(&client->dev) && > + client->addr == SMBUS_ARA_ADDR) { > + /* > + * In case this device as been enumerated by acpi > + * with the reserved smbus ARA address (first acpi > + * connection), request change of address to the second > + * connection. > + */ > + ret = i2c_acpi_set_connection(client, 1); > + if (ret) > + return ret; > + } > + > + /* cm3218 chip require an ara device on his adapter */ > + ret = i2c_require_smbus_alert(client); > + if (ret < 0) > + return ret; > + > + /* > + * If irq is given, request a threaded irq handler to manage > + * smbus alert. > + */ > + if (client->irq > 0) { If we have a chip that needs ara and no IRQ isn't it a probe failure case? The device won't work as I understand it. > + ret = devm_request_threaded_irq(&client->dev, > + client->irq, > + NULL, cm32181_irq, > + IRQF_ONESHOT, > + "cm32181", cm32181); > + if (ret) > + return ret; > + } > + } > + > ret = cm32181_reg_init(cm32181); > if (ret) { > dev_err(&client->dev, > @@ -342,25 +441,36 @@ static int cm32181_probe(struct i2c_client *client, > } > > static const struct i2c_device_id cm32181_id[] = { > - { "cm32181", 0 }, > + { "cm32181", CM32181_ID }, > + { "cm3218", CM3218_ID }, > { } > }; > > MODULE_DEVICE_TABLE(i2c, cm32181_id); > > static const struct of_device_id cm32181_of_match[] = { > - { .compatible = "capella,cm32181" }, > + { .compatible = "capella,cm32181", (void *)CM32181_ID }, > + { .compatible = "capella,cm3218", (void *)CM3218_ID }, > { } > }; > MODULE_DEVICE_TABLE(of, cm32181_of_match); > > +static const struct acpi_device_id cm32181_acpi_match[] = { > + { "CPLM3218", CM3218_ID }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(acpi, cm32181_acpi_match); > + > static struct i2c_driver cm32181_driver = { > .driver = { > .name = "cm32181", > .of_match_table = of_match_ptr(cm32181_of_match), > + .acpi_match_table = ACPI_PTR(cm32181_acpi_match), > }, > .id_table = cm32181_id, > .probe = cm32181_probe, > + .alert = cm3218_alert, It's a bit ugly - I'm not sure we wouldn't be better registering two separate i2c drivers so as not to specify the alert for devices that don't handle it. > }; > > module_i2c_driver(cm32181_driver); -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 28 Dec 2017 02:19:55 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Monday, December 25, 2017 4:57:22 PM CET Marc CAPDEVILLE wrote: > > On asus T100, Capella cm3218 chip is implemented as ambiant light > > sensor. This chip expose an smbus ARA protocol device on standard > > address 0x0c. The chip is not functional before all alerts are > > acknowledged. > > On asus T100, this device is enumerated on ACPI bus and the description > > give two I2C connection. The first is the connection to the ARA device > > and the second gives the real address of the device. > > > > So, on device probe,If the i2c address is the ARA address and the > > device is enumerated via acpi, we change address of the device for > > the one in the second acpi serial bus connection. > > This free the ara address so we can register with the smbus_alert > > driver. > > > > If an interrupt resource is given, and a smbus_alert device was > > found on the adapter, we request a treaded interrupt to > > call i2c_smbus_alert_event to handle the alert. > > > > In somme case, the treaded interrupt is not schedule before the driver > > try to initialize chip registers. So if first registers access fail, we > > call i2c_smbus_alert_event() to acknowledge initial alert, then retry > > register access. > > > > Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org> > > --- > > drivers/iio/light/cm32181.c | 120 ++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 115 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c > > index aebf7dd071af..96c08755e6e3 100644 > > --- a/drivers/iio/light/cm32181.c > > +++ b/drivers/iio/light/cm32181.c > > @@ -18,6 +18,9 @@ > > #include <linux/iio/sysfs.h> > > #include <linux/iio/events.h> > > #include <linux/init.h> > > +#include <linux/i2c-smbus.h> > > +#include <linux/acpi.h> > > +#include <linux/of_device.h> > > > > /* Registers Address */ > > #define CM32181_REG_ADDR_CMD 0x00 > > @@ -47,6 +50,11 @@ > > #define CM32181_CALIBSCALE_RESOLUTION 1000 > > #define MLUX_PER_LUX 1000 > > > > +#define CM32181_ID 0x81 > > +#define CM3218_ID 0x18 > > + > > +#define SMBUS_ARA_ADDR 0x0c > > + > > static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = { > > CM32181_REG_ADDR_CMD, > > }; > > @@ -57,6 +65,7 @@ static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000, > > > > struct cm32181_chip { > > struct i2c_client *client; > > + int chip_id; > > struct mutex lock; > > u16 conf_regs[CM32181_CONF_REG_NUM]; > > int calibscale; > > @@ -77,11 +86,22 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181) > > s32 ret; > > > > ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID); > > - if (ret < 0) > > - return ret; > > + if (ret < 0) { > > + if (cm32181->chip_id == CM3218_ID) { > > + /* > > + * On cm3218, smbus alert trigger after probing, > > + * so poll for first alert here, then retry. > > + */ > > + i2c_smbus_alert_event(client); > > + ret = i2c_smbus_read_word_data(client, > > + CM32181_REG_ADDR_ID); > > + } else { > > + return ret; > > + } > > + } > > > > /* check device ID */ > > - if ((ret & 0xFF) != 0x81) > > + if ((ret & 0xFF) != cm32181->chip_id) > > return -ENODEV; > > > > /* Default Values */ > > @@ -297,12 +317,36 @@ static const struct iio_info cm32181_info = { > > .attrs = &cm32181_attribute_group, > > }; > > > > +static void cm3218_alert(struct i2c_client *client, > > + enum i2c_alert_protocol type, > > + unsigned int data) > > +{ > > + /* > > + * nothing to do for now. > > + * This is just here to acknownledge the cm3218 alert. > > + */ > > +} > > + > > +static irqreturn_t cm32181_irq(int irq, void *d) > > +{ > > + struct cm32181_chip *cm32181 = d; > > + > > + if (cm32181->chip_id == CM3218_ID) { > > + /* This is cm3218 chip irq, so handle the smbus alert */ > > + i2c_smbus_alert_event(cm32181->client); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > > static int cm32181_probe(struct i2c_client *client, > > const struct i2c_device_id *id) > > { > > struct cm32181_chip *cm32181; > > struct iio_dev *indio_dev; > > int ret; > > + const struct of_device_id *of_id; > > + const struct acpi_device_id *acpi_id; > > > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181)); > > if (!indio_dev) { > > @@ -322,6 +366,61 @@ static int cm32181_probe(struct i2c_client *client, > > indio_dev->name = id->name; > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > + /* Lookup for chip ID from platform, acpi or of device table */ > > + if (id) { > > + cm32181->chip_id = id->driver_data; > > + } else if (ACPI_COMPANION(&client->dev)) { > > + acpi_id = acpi_match_device(client->dev.driver->acpi_match_table, > > + &client->dev); > > + if (!acpi_id) > > + return -ENODEV; > > + > > + cm32181->chip_id = (kernel_ulong_t)acpi_id->driver_data; > > + } else if (client->dev.of_node) { > > + of_id = of_match_device(client->dev.driver->of_match_table, > > + &client->dev); > > + if (!of_id) > > + return -ENODEV; > > + > > + cm32181->chip_id = (kernel_ulong_t)of_id->data; > > + } else { > > + return -ENODEV; > > + } > > + > > + if (cm32181->chip_id == CM3218_ID) { > > + if (ACPI_COMPANION(&client->dev) && > > + client->addr == SMBUS_ARA_ADDR) { > > + /* > > + * In case this device as been enumerated by acpi > > + * with the reserved smbus ARA address (first acpi > > + * connection), request change of address to the second > > + * connection. > > + */ > > + ret = i2c_acpi_set_connection(client, 1); > > + if (ret) > > + return ret; > > This looks super-fragile to me. > > What about making the enumeration aware of the SMBUS_ARA thing and avoid > using resources corresponding to that at all? I agree, if it can be done reasonably cleanly that would be preferable. > > BTW, in comments and similar always spell ACPI in capitals. > > Thanks, > Rafael > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c index aebf7dd071af..96c08755e6e3 100644 --- a/drivers/iio/light/cm32181.c +++ b/drivers/iio/light/cm32181.c @@ -18,6 +18,9 @@ #include <linux/iio/sysfs.h> #include <linux/iio/events.h> #include <linux/init.h> +#include <linux/i2c-smbus.h> +#include <linux/acpi.h> +#include <linux/of_device.h> /* Registers Address */ #define CM32181_REG_ADDR_CMD 0x00 @@ -47,6 +50,11 @@ #define CM32181_CALIBSCALE_RESOLUTION 1000 #define MLUX_PER_LUX 1000 +#define CM32181_ID 0x81 +#define CM3218_ID 0x18 + +#define SMBUS_ARA_ADDR 0x0c + static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = { CM32181_REG_ADDR_CMD, }; @@ -57,6 +65,7 @@ static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000, struct cm32181_chip { struct i2c_client *client; + int chip_id; struct mutex lock; u16 conf_regs[CM32181_CONF_REG_NUM]; int calibscale; @@ -77,11 +86,22 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181) s32 ret; ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID); - if (ret < 0) - return ret; + if (ret < 0) { + if (cm32181->chip_id == CM3218_ID) { + /* + * On cm3218, smbus alert trigger after probing, + * so poll for first alert here, then retry. + */ + i2c_smbus_alert_event(client); + ret = i2c_smbus_read_word_data(client, + CM32181_REG_ADDR_ID); + } else { + return ret; + } + } /* check device ID */ - if ((ret & 0xFF) != 0x81) + if ((ret & 0xFF) != cm32181->chip_id) return -ENODEV; /* Default Values */ @@ -297,12 +317,36 @@ static const struct iio_info cm32181_info = { .attrs = &cm32181_attribute_group, }; +static void cm3218_alert(struct i2c_client *client, + enum i2c_alert_protocol type, + unsigned int data) +{ + /* + * nothing to do for now. + * This is just here to acknownledge the cm3218 alert. + */ +} + +static irqreturn_t cm32181_irq(int irq, void *d) +{ + struct cm32181_chip *cm32181 = d; + + if (cm32181->chip_id == CM3218_ID) { + /* This is cm3218 chip irq, so handle the smbus alert */ + i2c_smbus_alert_event(cm32181->client); + } + + return IRQ_HANDLED; +} + static int cm32181_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct cm32181_chip *cm32181; struct iio_dev *indio_dev; int ret; + const struct of_device_id *of_id; + const struct acpi_device_id *acpi_id; indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181)); if (!indio_dev) { @@ -322,6 +366,61 @@ static int cm32181_probe(struct i2c_client *client, indio_dev->name = id->name; indio_dev->modes = INDIO_DIRECT_MODE; + /* Lookup for chip ID from platform, acpi or of device table */ + if (id) { + cm32181->chip_id = id->driver_data; + } else if (ACPI_COMPANION(&client->dev)) { + acpi_id = acpi_match_device(client->dev.driver->acpi_match_table, + &client->dev); + if (!acpi_id) + return -ENODEV; + + cm32181->chip_id = (kernel_ulong_t)acpi_id->driver_data; + } else if (client->dev.of_node) { + of_id = of_match_device(client->dev.driver->of_match_table, + &client->dev); + if (!of_id) + return -ENODEV; + + cm32181->chip_id = (kernel_ulong_t)of_id->data; + } else { + return -ENODEV; + } + + if (cm32181->chip_id == CM3218_ID) { + if (ACPI_COMPANION(&client->dev) && + client->addr == SMBUS_ARA_ADDR) { + /* + * In case this device as been enumerated by acpi + * with the reserved smbus ARA address (first acpi + * connection), request change of address to the second + * connection. + */ + ret = i2c_acpi_set_connection(client, 1); + if (ret) + return ret; + } + + /* cm3218 chip require an ara device on his adapter */ + ret = i2c_require_smbus_alert(client); + if (ret < 0) + return ret; + + /* + * If irq is given, request a threaded irq handler to manage + * smbus alert. + */ + if (client->irq > 0) { + ret = devm_request_threaded_irq(&client->dev, + client->irq, + NULL, cm32181_irq, + IRQF_ONESHOT, + "cm32181", cm32181); + if (ret) + return ret; + } + } + ret = cm32181_reg_init(cm32181); if (ret) { dev_err(&client->dev, @@ -342,25 +441,36 @@ static int cm32181_probe(struct i2c_client *client, } static const struct i2c_device_id cm32181_id[] = { - { "cm32181", 0 }, + { "cm32181", CM32181_ID }, + { "cm3218", CM3218_ID }, { } }; MODULE_DEVICE_TABLE(i2c, cm32181_id); static const struct of_device_id cm32181_of_match[] = { - { .compatible = "capella,cm32181" }, + { .compatible = "capella,cm32181", (void *)CM32181_ID }, + { .compatible = "capella,cm3218", (void *)CM3218_ID }, { } }; MODULE_DEVICE_TABLE(of, cm32181_of_match); +static const struct acpi_device_id cm32181_acpi_match[] = { + { "CPLM3218", CM3218_ID }, + { } +}; + +MODULE_DEVICE_TABLE(acpi, cm32181_acpi_match); + static struct i2c_driver cm32181_driver = { .driver = { .name = "cm32181", .of_match_table = of_match_ptr(cm32181_of_match), + .acpi_match_table = ACPI_PTR(cm32181_acpi_match), }, .id_table = cm32181_id, .probe = cm32181_probe, + .alert = cm3218_alert, }; module_i2c_driver(cm32181_driver);
On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged. On asus T100, this device is enumerated on ACPI bus and the description give two I2C connection. The first is the connection to the ARA device and the second gives the real address of the device. So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we change address of the device for the one in the second acpi serial bus connection. This free the ara address so we can register with the smbus_alert driver. If an interrupt resource is given, and a smbus_alert device was found on the adapter, we request a treaded interrupt to call i2c_smbus_alert_event to handle the alert. In somme case, the treaded interrupt is not schedule before the driver try to initialize chip registers. So if first registers access fail, we call i2c_smbus_alert_event() to acknowledge initial alert, then retry register access. Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org> --- drivers/iio/light/cm32181.c | 120 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 115 insertions(+), 5 deletions(-)