* [PATCH v7 2/4] i2c-acpi : exclude ARA address for smbus device
2018-01-13 13:37 [PATCH v7 1/4] i2c-smbus : Add client discovered ARA support Marc CAPDEVILLE
@ 2018-01-13 13:37 ` Marc CAPDEVILLE
[not found] ` <20180113133705.25044-2-m.capdeville-n+LsquliYkMdnm+yROfE0A@public.gmane.org>
2018-01-13 13:37 ` [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support Marc CAPDEVILLE
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Marc CAPDEVILLE @ 2018-01-13 13:37 UTC (permalink / raw)
To: Kevin Tsai
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mika Westerberg, Wolfram Sang, linux-iio,
linux-i2c, linux-acpi, linux-kernel, Marc CAPDEVILLE
Somme ACPI enumerated devices are known to support smbus alert protocol.
Theses devices may be miss-enumerated with the reserved smbus ARA address.
This is the case on Asus T100 tablet where cm3218 ambiant light sensor
expose two i2c serial bus connections, with the first one being the alert
response address.
This patch make a match on known ACPI ids for which devices are smbus ARA
capable, then skip the connection if it has the reserved 0x0c address and
mark it with I2C_CLIENT_ALERT flag. So device is enumerated with the
correct address.
Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
---
drivers/i2c/i2c-core-acpi.c | 23 +++++++++++++++++++++--
include/linux/i2c.h | 1 +
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index a9126b3cda61..5a8886f14329 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -59,8 +59,14 @@ static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
return 1;
- if (lookup->index != -1 && lookup->n++ != lookup->index)
- return 1;
+ if (lookup->index != -1) {
+ if (lookup->n++ != lookup->index)
+ return 1;
+ } else {
+ if (lookup->info->flags & I2C_CLIENT_ALERT &&
+ sb->slave_address == 0x0c)
+ return 1;
+ }
status = acpi_get_handle(lookup->device_handle,
sb->resource_source.string_ptr,
@@ -85,6 +91,15 @@ static const struct acpi_device_id i2c_acpi_ignored_device_ids[] = {
{}
};
+static const struct acpi_device_id i2c_acpi_alert_device_ids[] = {
+ /*
+ * Smbus alert capable device which may have the reserved ARA address
+ * in their serial bus resources list.
+ */
+ { "CPLM3218", 0 },
+ {}
+};
+
static int i2c_acpi_do_lookup(struct acpi_device *adev,
struct i2c_acpi_lookup *lookup)
{
@@ -100,6 +115,10 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
return -ENODEV;
memset(info, 0, sizeof(*info));
+
+ if (acpi_match_device_ids(adev, i2c_acpi_alert_device_ids) == 0)
+ info->flags |= I2C_CLIENT_ALERT;
+
lookup->device_handle = acpi_device_handle(adev);
/* Look up for I2cSerialBus resource */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 7592dce12923..b0d6f1333442 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -743,6 +743,7 @@ i2c_unlock_adapter(struct i2c_adapter *adapter)
#define I2C_CLIENT_SLAVE 0x20 /* we are the slave */
#define I2C_CLIENT_HOST_NOTIFY 0x40 /* We want to use I2C host notify */
#define I2C_CLIENT_WAKE 0x80 /* for board_info; true iff can wake */
+#define I2C_CLIENT_ALERT 0x100 /* Client use SMBUS alert protocol */
#define I2C_CLIENT_SCCB 0x9000 /* Use Omnivision SCCB protocol */
/* Must match I2C_M_STOP|IGNORE_NAK */
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support
2018-01-13 13:37 [PATCH v7 1/4] i2c-smbus : Add client discovered ARA support Marc CAPDEVILLE
2018-01-13 13:37 ` [PATCH v7 2/4] i2c-acpi : exclude ARA address for smbus device Marc CAPDEVILLE
@ 2018-01-13 13:37 ` Marc CAPDEVILLE
2018-01-14 11:45 ` Jonathan Cameron
2018-01-13 13:37 ` [PATCH v7 4/4] iio : cm32181 : cosmetic cleanup Marc CAPDEVILLE
2018-01-14 11:45 ` [PATCH v7 1/4] i2c-smbus : Add client discovered ARA support Jonathan Cameron
3 siblings, 1 reply; 13+ messages in thread
From: Marc CAPDEVILLE @ 2018-01-13 13:37 UTC (permalink / raw)
To: Kevin Tsai
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mika Westerberg, Wolfram Sang, linux-iio,
linux-i2c, linux-acpi, linux-kernel, Marc CAPDEVILLE
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.
The driver call i2c_require_smbus_alert() to set the smbus alert
protocol driver on its adapter. If an interrupt resource is given,
we register this irq with the smbus alert driver.
If no irq is given, the driver call i2c_smbus_alert_event() to trigger
the alert process to clear the initial alert event before initializing
cm3218 registers.
Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
---
drivers/iio/light/cm32181.c | 94 +++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 91 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index aebf7dd071af..eae5b7cc6878 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,9 @@
#define CM32181_CALIBSCALE_RESOLUTION 1000
#define MLUX_PER_LUX 1000
+#define CM32181_ID 0x81
+#define CM3218_ID 0x18
+
static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
CM32181_REG_ADDR_CMD,
};
@@ -57,6 +63,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;
@@ -81,7 +88,7 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
return ret;
/* check device ID */
- if ((ret & 0xFF) != 0x81)
+ if ((ret & 0xFF) != cm32181->chip_id)
return -ENODEV;
/* Default Values */
@@ -297,12 +304,23 @@ 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 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 acpi_device_id *a_id;
indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
if (!indio_dev) {
@@ -322,11 +340,55 @@ 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)) {
+ a_id = acpi_match_device(client->dev.driver->acpi_match_table,
+ &client->dev);
+ if (!a_id)
+ return -ENODEV;
+
+ cm32181->chip_id = (int)a_id->driver_data;
+ } else if (client->dev.of_node) {
+ cm32181->chip_id = (int)of_device_get_match_data(&client->dev);
+ if (!cm32181->chip_id)
+ return -ENODEV;
+ } else {
+ return -ENODEV;
+ }
+
+ if (cm32181->chip_id == CM3218_ID) {
+ /* cm3218 chip require an ARA device on his adapter */
+ ret = i2c_require_smbus_alert(client);
+ if (ret < 0)
+ return ret;
+
+ /* If irq is given, register it with the smbus alert driver */
+ if (client->irq > 0) {
+ ret = i2c_smbus_alert_add_irq(client, client->irq);
+ if (ret < 0)
+ return ret;
+ } else {
+ /*
+ * if no irq is given, acknownledge initial ARA
+ * event so cm32181_reg_init() will not fail.
+ */
+ ret = i2c_smbus_alert_event(client);
+ if (ret)
+ return ret;
+ }
+ }
+
ret = cm32181_reg_init(cm32181);
if (ret) {
dev_err(&client->dev,
"%s: register init failed\n",
__func__);
+
+ if (cm32181->chip_id == CM3218_ID && client->irq)
+ i2c_smbus_alert_free_irq(client, client->irq);
+
return ret;
}
@@ -335,32 +397,58 @@ static int cm32181_probe(struct i2c_client *client,
dev_err(&client->dev,
"%s: regist device failed\n",
__func__);
+
+ if (cm32181->chip_id == CM3218_ID && client->irq)
+ i2c_smbus_alert_free_irq(client, client->irq);
+
return ret;
}
return 0;
}
+static int cm32181_remove(struct i2c_client *client)
+{
+ struct cm32181_chip *cm32181 = i2c_get_clientdata(client);
+
+ if (cm32181->chip_id == CM3218_ID && client->irq)
+ i2c_smbus_alert_free_irq(client, client->irq);
+
+ return 0;
+}
+
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,
+ .remove = cm32181_remove,
+ .alert = cm3218_alert,
};
module_i2c_driver(cm32181_driver);
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support
2018-01-13 13:37 ` [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support Marc CAPDEVILLE
@ 2018-01-14 11:45 ` Jonathan Cameron
2018-01-14 14:39 ` Marc CAPDEVILLE
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2018-01-14 11:45 UTC (permalink / raw)
To: Marc CAPDEVILLE
Cc: Kevin Tsai, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mika Westerberg, Wolfram Sang, linux-iio,
linux-i2c, linux-acpi, linux-kernel
On Sat, 13 Jan 2018 14:37:04 +0100
Marc CAPDEVILLE <m.capdeville@no-log.org> 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.
>
> The driver call i2c_require_smbus_alert() to set the smbus alert
> protocol driver on its adapter. If an interrupt resource is given,
> we register this irq with the smbus alert driver.
> If no irq is given, the driver call i2c_smbus_alert_event() to trigger
> the alert process to clear the initial alert event before initializing
> cm3218 registers.
>
> Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
Ultimately I think we can move most of the logic here into the i2c core,
but that can be a job for another day.
I'm not sure there isn't a nasty mess with this device if we have
a bus master created ara. I raised it below. Not sure there is
much we can do about it though.
Jonathan
> ---
> drivers/iio/light/cm32181.c | 94 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index aebf7dd071af..eae5b7cc6878 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,9 @@
> #define CM32181_CALIBSCALE_RESOLUTION 1000
> #define MLUX_PER_LUX 1000
>
> +#define CM32181_ID 0x81
> +#define CM3218_ID 0x18
> +
> static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
> CM32181_REG_ADDR_CMD,
> };
> @@ -57,6 +63,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;
> @@ -81,7 +88,7 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
> return ret;
>
> /* check device ID */
> - if ((ret & 0xFF) != 0x81)
> + if ((ret & 0xFF) != cm32181->chip_id)
> return -ENODEV;
>
> /* Default Values */
> @@ -297,12 +304,23 @@ 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 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 acpi_device_id *a_id;
>
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
> if (!indio_dev) {
> @@ -322,11 +340,55 @@ 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)) {
> + a_id = acpi_match_device(client->dev.driver->acpi_match_table,
> + &client->dev);
> + if (!a_id)
> + return -ENODEV;
> +
> + cm32181->chip_id = (int)a_id->driver_data;
> + } else if (client->dev.of_node) {
> + cm32181->chip_id = (int)of_device_get_match_data(&client->dev);
> + if (!cm32181->chip_id)
> + return -ENODEV;
> + } else {
> + return -ENODEV;
> + }
> +
> + if (cm32181->chip_id == CM3218_ID) {
> + /* cm3218 chip require an ARA device on his adapter */
> + ret = i2c_require_smbus_alert(client);
> + if (ret < 0)
> + return ret;
This should be apparent to the core as we have an alert callback set.
I suppose there may be nasty corner cases where the alert can be cleared
without acknowledging explicitly (I think some of the Maxim parts do
this).
> +
> + /* If irq is given, register it with the smbus alert driver */
> + if (client->irq > 0) {
> + ret = i2c_smbus_alert_add_irq(client, client->irq);
> + if (ret < 0)
> + return ret;
> + } else {
> + /*
> + * if no irq is given, acknownledge initial ARA
> + * event so cm32181_reg_init() will not fail.
Logic here has me a bit confused. If we don't have an irq then it
could be the i2c master already registered the relevant support.
Now smbalert is IIRC a level interrupt (stays high until all sources
have been dealt with) so it should have fired when enabled and be
continuously firing until someone deals with it (which is decidedly nasty)
Anyhow to my mind that core irq should be masked until someone says
they are ready to handle it.
Upshot I think is that any device which can send ARA before driver
is loaded is inherently horribly broken if that alert line is shared
with other devices as then even enabling only on first
device saying it handles an ARA doesn't work. Oh goody.
Anyhow, is this path tested? I.e. did you make your master
driver register the ara support?
> + */
> + ret = i2c_smbus_alert_event(client);
> + if (ret)
> + return ret;
> + }
> + }
> +
> ret = cm32181_reg_init(cm32181);
> if (ret) {
> dev_err(&client->dev,
> "%s: register init failed\n",
> __func__);
> +
I would use a goto and unify the unwind of this in an erro rblock at
the end fo the driver.
> + if (cm32181->chip_id == CM3218_ID && client->irq)
> + i2c_smbus_alert_free_irq(client, client->irq);
> +
> return ret;
> }
>
> @@ -335,32 +397,58 @@ static int cm32181_probe(struct i2c_client *client,
> dev_err(&client->dev,
> "%s: regist device failed\n",
> __func__);
> +
> + if (cm32181->chip_id == CM3218_ID && client->irq)
> + i2c_smbus_alert_free_irq(client, client->irq);
> +
> return ret;
> }
>
> return 0;
> }
>
> +static int cm32181_remove(struct i2c_client *client)
> +{
> + struct cm32181_chip *cm32181 = i2c_get_clientdata(client);
> +
> + if (cm32181->chip_id == CM3218_ID && client->irq)
> + i2c_smbus_alert_free_irq(client, client->irq);
> +
> + return 0;
> +}
> +
> 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,
> + .remove = cm32181_remove,
> + .alert = cm3218_alert,
> };
>
> module_i2c_driver(cm32181_driver);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support
2018-01-14 11:45 ` Jonathan Cameron
@ 2018-01-14 14:39 ` Marc CAPDEVILLE
2018-01-20 16:36 ` Jonathan Cameron
0 siblings, 1 reply; 13+ messages in thread
From: Marc CAPDEVILLE @ 2018-01-14 14:39 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Marc CAPDEVILLE, Kevin Tsai, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mika Westerberg, Wolfram Sang, linux-iio,
linux-i2c, linux-acpi, linux-kernel
> On Sat, 13 Jan 2018 14:37:04 +0100
> Marc CAPDEVILLE <m.capdeville@no-log.org> 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.
>>
>> The driver call i2c_require_smbus_alert() to set the smbus alert
>> protocol driver on its adapter. If an interrupt resource is given,
>> we register this irq with the smbus alert driver.
>> If no irq is given, the driver call i2c_smbus_alert_event() to trigger
>> the alert process to clear the initial alert event before initializing
>> cm3218 registers.
>>
>> Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
>
> Ultimately I think we can move most of the logic here into the i2c core,
> but that can be a job for another day.
This can be done in the core in i2c_device_probe(), but can we suppose
that client irq is the smbus alert line when the device is flagged with
I2C_CLIENT_ALERT and/or the driver define an alert callback ?
>
> I'm not sure there isn't a nasty mess with this device if we have
> a bus master created ara. I raised it below. Not sure there is
> much we can do about it though.
If the adapter has already created the smbus alert device, the
i2c_require_smbus_alert() do nothing. We just have to register an
additional handler for the irq line if it differ from the adapter one.
This is handle by i2c_smbus_alert_add_irq().
>
> Jonathan
>
>> ---
>> drivers/iio/light/cm32181.c | 94
>> +++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 91 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
>> index aebf7dd071af..eae5b7cc6878 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,9 @@
>> #define CM32181_CALIBSCALE_RESOLUTION 1000
>> #define MLUX_PER_LUX 1000
>>
>> +#define CM32181_ID 0x81
>> +#define CM3218_ID 0x18
>> +
>> static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
>> CM32181_REG_ADDR_CMD,
>> };
>> @@ -57,6 +63,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;
>> @@ -81,7 +88,7 @@ static int cm32181_reg_init(struct cm32181_chip
>> *cm32181)
>> return ret;
>>
>> /* check device ID */
>> - if ((ret & 0xFF) != 0x81)
>> + if ((ret & 0xFF) != cm32181->chip_id)
>> return -ENODEV;
>>
>> /* Default Values */
>> @@ -297,12 +304,23 @@ 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 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 acpi_device_id *a_id;
>>
>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
>> if (!indio_dev) {
>> @@ -322,11 +340,55 @@ 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)) {
>> + a_id = acpi_match_device(client->dev.driver->acpi_match_table,
>> + &client->dev);
>> + if (!a_id)
>> + return -ENODEV;
>> +
>> + cm32181->chip_id = (int)a_id->driver_data;
>> + } else if (client->dev.of_node) {
>> + cm32181->chip_id = (int)of_device_get_match_data(&client->dev);
>> + if (!cm32181->chip_id)
>> + return -ENODEV;
>> + } else {
>> + return -ENODEV;
>> + }
>> +
>> + if (cm32181->chip_id == CM3218_ID) {
>> + /* cm3218 chip require an ARA device on his adapter */
>> + ret = i2c_require_smbus_alert(client);
>> + if (ret < 0)
>> + return ret;
>
> This should be apparent to the core as we have an alert callback set.
Yes, I think that i2c_require_smbus_alert() may be handle by the core if
the driver have an alert callback defined and/or the client is flagged
with I2C_CLIENT_ALERT. I think this can be done in i2c_device_probe().
> I suppose there may be nasty corner cases where the alert can be cleared
> without acknowledging explicitly (I think some of the Maxim parts do
> this).
>
>> +
>> + /* If irq is given, register it with the smbus alert driver */
>> + if (client->irq > 0) {
>> + ret = i2c_smbus_alert_add_irq(client, client->irq);
>> + if (ret < 0)
>> + return ret;
>> + } else {
>> + /*
>> + * if no irq is given, acknownledge initial ARA
>> + * event so cm32181_reg_init() will not fail.
>
> Logic here has me a bit confused. If we don't have an irq then it
> could be the i2c master already registered the relevant support.
Yes this code can be removed for now, as all board I have found using this
chip enumerated by ACPI define an interrupt. (Asus T100, Chuwi Hi8, Lenovo
Carbon X1, Acer Aspire Switch 10).
> Now smbalert is IIRC a level interrupt (stays high until all sources
> have been dealt with) so it should have fired when enabled and be
> continuously firing until someone deals with it (which is decidedly nasty)
The smbus_alert driver will always deal with each alert on the bus,
acknowledging each device until interrupt line is cleared. It don't care
if there are drivers handling the alert or not.
>
> Anyhow to my mind that core irq should be masked until someone says
> they are ready to handle it.
>
> Upshot I think is that any device which can send ARA before driver
> is loaded is inherently horribly broken if that alert line is shared
> with other devices as then even enabling only on first
> device saying it handles an ARA doesn't work. Oh goody.
This is the case of cm3218. Irq line is stuck low at power-on until the
first read of alert address on that chip. The driver just can't access
register until the alert is cleared. So It need the smbus alert process to
be fired before initializing the device.
There is another problem, the treaded irq hander sometime is not run
before cm32181_reg_init() is called. So in that case , I think we need to
defer the probe and retry later.
>
> Anyhow, is this path tested? I.e. did you make your master
> driver register the ara support?
No, So I will remove that in next version.
>
>> + */
>> + ret = i2c_smbus_alert_event(client);
>> + if (ret)
>> + return ret;
>> + }
>> + }
>> +
>> ret = cm32181_reg_init(cm32181);
>> if (ret) {
>> dev_err(&client->dev,
>> "%s: register init failed\n",
>> __func__);
>> +
> I would use a goto and unify the unwind of this in an erro rblock at
> the end fo the driver.
>> + if (cm32181->chip_id == CM3218_ID && client->irq)
>> + i2c_smbus_alert_free_irq(client, client->irq);
>> +
>> return ret;
>> }
>>
>> @@ -335,32 +397,58 @@ static int cm32181_probe(struct i2c_client
>> *client,
>> dev_err(&client->dev,
>> "%s: regist device failed\n",
>> __func__);
>> +
>> + if (cm32181->chip_id == CM3218_ID && client->irq)
>> + i2c_smbus_alert_free_irq(client, client->irq);
>> +
>> return ret;
>> }
>>
>> return 0;
>> }
>>
>> +static int cm32181_remove(struct i2c_client *client)
>> +{
>> + struct cm32181_chip *cm32181 = i2c_get_clientdata(client);
>> +
>> + if (cm32181->chip_id == CM3218_ID && client->irq)
>> + i2c_smbus_alert_free_irq(client, client->irq);
>> +
>> + return 0;
>> +}
>> +
>> 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,
>> + .remove = cm32181_remove,
>> + .alert = cm3218_alert,
>> };
>>
>> module_i2c_driver(cm32181_driver);
>
>
--
Marc CAPDEVILLE
<m.capdeville@no-log.org>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support
2018-01-14 14:39 ` Marc CAPDEVILLE
@ 2018-01-20 16:36 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2018-01-20 16:36 UTC (permalink / raw)
To: Marc CAPDEVILLE
Cc: Kevin Tsai, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mika Westerberg, Wolfram Sang, linux-iio,
linux-i2c, linux-acpi, linux-kernel
On Sun, 14 Jan 2018 15:39:24 +0100
"Marc CAPDEVILLE" <m.capdeville@no-log.org> wrote:
>
> > On Sat, 13 Jan 2018 14:37:04 +0100
> > Marc CAPDEVILLE <m.capdeville@no-log.org> 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.
> >>
> >> The driver call i2c_require_smbus_alert() to set the smbus alert
> >> protocol driver on its adapter. If an interrupt resource is given,
> >> we register this irq with the smbus alert driver.
> >> If no irq is given, the driver call i2c_smbus_alert_event() to trigger
> >> the alert process to clear the initial alert event before initializing
> >> cm3218 registers.
> >>
> >> Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
> >
> > Ultimately I think we can move most of the logic here into the i2c core,
> > but that can be a job for another day.
>
> This can be done in the core in i2c_device_probe(), but can we suppose
> that client irq is the smbus alert line when the device is flagged with
> I2C_CLIENT_ALERT and/or the driver define an alert callback ?
We'd have to audit the drivers and check this was the case. Unless
we have have devices with ARA and another interrupt it seems likely
this would be fine.
> >
> > I'm not sure there isn't a nasty mess with this device if we have
> > a bus master created ara. I raised it below. Not sure there is
> > much we can do about it though.
>
> If the adapter has already created the smbus alert device, the
> i2c_require_smbus_alert() do nothing. We just have to register an
> additional handler for the irq line if it differ from the adapter one.
> This is handle by i2c_smbus_alert_add_irq().
> >
> > Jonathan
> >
> >> ---
> >> drivers/iio/light/cm32181.c | 94
> >> +++++++++++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 91 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> >> index aebf7dd071af..eae5b7cc6878 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,9 @@
> >> #define CM32181_CALIBSCALE_RESOLUTION 1000
> >> #define MLUX_PER_LUX 1000
> >>
> >> +#define CM32181_ID 0x81
> >> +#define CM3218_ID 0x18
> >> +
> >> static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
> >> CM32181_REG_ADDR_CMD,
> >> };
> >> @@ -57,6 +63,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;
> >> @@ -81,7 +88,7 @@ static int cm32181_reg_init(struct cm32181_chip
> >> *cm32181)
> >> return ret;
> >>
> >> /* check device ID */
> >> - if ((ret & 0xFF) != 0x81)
> >> + if ((ret & 0xFF) != cm32181->chip_id)
> >> return -ENODEV;
> >>
> >> /* Default Values */
> >> @@ -297,12 +304,23 @@ 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 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 acpi_device_id *a_id;
> >>
> >> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
> >> if (!indio_dev) {
> >> @@ -322,11 +340,55 @@ 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)) {
> >> + a_id = acpi_match_device(client->dev.driver->acpi_match_table,
> >> + &client->dev);
> >> + if (!a_id)
> >> + return -ENODEV;
> >> +
> >> + cm32181->chip_id = (int)a_id->driver_data;
> >> + } else if (client->dev.of_node) {
> >> + cm32181->chip_id = (int)of_device_get_match_data(&client->dev);
> >> + if (!cm32181->chip_id)
> >> + return -ENODEV;
> >> + } else {
> >> + return -ENODEV;
> >> + }
> >> +
> >> + if (cm32181->chip_id == CM3218_ID) {
> >> + /* cm3218 chip require an ARA device on his adapter */
> >> + ret = i2c_require_smbus_alert(client);
> >> + if (ret < 0)
> >> + return ret;
> >
> > This should be apparent to the core as we have an alert callback set.
>
> Yes, I think that i2c_require_smbus_alert() may be handle by the core if
> the driver have an alert callback defined and/or the client is flagged
> with I2C_CLIENT_ALERT. I think this can be done in i2c_device_probe().
>
> > I suppose there may be nasty corner cases where the alert can be cleared
> > without acknowledging explicitly (I think some of the Maxim parts do
> > this).
> >
> >> +
> >> + /* If irq is given, register it with the smbus alert driver */
> >> + if (client->irq > 0) {
> >> + ret = i2c_smbus_alert_add_irq(client, client->irq);
> >> + if (ret < 0)
> >> + return ret;
> >> + } else {
> >> + /*
> >> + * if no irq is given, acknownledge initial ARA
> >> + * event so cm32181_reg_init() will not fail.
> >
> > Logic here has me a bit confused. If we don't have an irq then it
> > could be the i2c master already registered the relevant support.
>
> Yes this code can be removed for now, as all board I have found using this
> chip enumerated by ACPI define an interrupt. (Asus T100, Chuwi Hi8, Lenovo
> Carbon X1, Acer Aspire Switch 10).
>
> > Now smbalert is IIRC a level interrupt (stays high until all sources
> > have been dealt with) so it should have fired when enabled and be
> > continuously firing until someone deals with it (which is decidedly nasty)
>
> The smbus_alert driver will always deal with each alert on the bus,
> acknowledging each device until interrupt line is cleared. It don't care
> if there are drivers handling the alert or not.
> >
> > Anyhow to my mind that core irq should be masked until someone says
> > they are ready to handle it.
> >
> > Upshot I think is that any device which can send ARA before driver
> > is loaded is inherently horribly broken if that alert line is shared
> > with other devices as then even enabling only on first
> > device saying it handles an ARA doesn't work. Oh goody.
>
> This is the case of cm3218. Irq line is stuck low at power-on until the
> first read of alert address on that chip. The driver just can't access
> register until the alert is cleared. So It need the smbus alert process to
> be fired before initializing the device.
If another driver probes first and hence the interrupt is enabled
we are in an interrupt storm territory. Oh goody.
>
> There is another problem, the treaded irq hander sometime is not run
> before cm32181_reg_init() is called. So in that case , I think we need to
> defer the probe and retry later.
Be more lazy and sleep a bit?
> >
> > Anyhow, is this path tested? I.e. did you make your master
> > driver register the ara support?
>
> No, So I will remove that in next version.
>
> >
> >> + */
> >> + ret = i2c_smbus_alert_event(client);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> + }
> >> +
> >> ret = cm32181_reg_init(cm32181);
> >> if (ret) {
> >> dev_err(&client->dev,
> >> "%s: register init failed\n",
> >> __func__);
> >> +
> > I would use a goto and unify the unwind of this in an erro rblock at
> > the end fo the driver.
> >> + if (cm32181->chip_id == CM3218_ID && client->irq)
> >> + i2c_smbus_alert_free_irq(client, client->irq);
> >> +
> >> return ret;
> >> }
> >>
> >> @@ -335,32 +397,58 @@ static int cm32181_probe(struct i2c_client
> >> *client,
> >> dev_err(&client->dev,
> >> "%s: regist device failed\n",
> >> __func__);
> >> +
> >> + if (cm32181->chip_id == CM3218_ID && client->irq)
> >> + i2c_smbus_alert_free_irq(client, client->irq);
> >> +
> >> return ret;
> >> }
> >>
> >> return 0;
> >> }
> >>
> >> +static int cm32181_remove(struct i2c_client *client)
> >> +{
> >> + struct cm32181_chip *cm32181 = i2c_get_clientdata(client);
> >> +
> >> + if (cm32181->chip_id == CM3218_ID && client->irq)
> >> + i2c_smbus_alert_free_irq(client, client->irq);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> 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,
> >> + .remove = cm32181_remove,
> >> + .alert = cm3218_alert,
> >> };
> >>
> >> module_i2c_driver(cm32181_driver);
> >
> >
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v7 4/4] iio : cm32181 : cosmetic cleanup
2018-01-13 13:37 [PATCH v7 1/4] i2c-smbus : Add client discovered ARA support Marc CAPDEVILLE
2018-01-13 13:37 ` [PATCH v7 2/4] i2c-acpi : exclude ARA address for smbus device Marc CAPDEVILLE
2018-01-13 13:37 ` [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support Marc CAPDEVILLE
@ 2018-01-13 13:37 ` Marc CAPDEVILLE
2018-01-14 11:45 ` [PATCH v7 1/4] i2c-smbus : Add client discovered ARA support Jonathan Cameron
3 siblings, 0 replies; 13+ messages in thread
From: Marc CAPDEVILLE @ 2018-01-13 13:37 UTC (permalink / raw)
To: Kevin Tsai
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mika Westerberg, Wolfram Sang, linux-iio,
linux-i2c, linux-acpi, linux-kernel, Marc CAPDEVILLE
Somme cosmetic cleanup suggested by Peter Meerwald-Stadler.
Macro name :
MLUX_PER_LUX => CM32181_MLUX_PER_LUX
Constante name :
als_it_bits => cm32181_als_it_bits
als_it_value => cm32181_als_it_value
Comment :
Registers Address => Register Addresses
Suggested-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
drivers/iio/light/cm32181.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index eae5b7cc6878..8fc01c8d4522 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -22,7 +22,7 @@
#include <linux/acpi.h>
#include <linux/of_device.h>
-/* Registers Address */
+/* Register Addresses */
#define CM32181_REG_ADDR_CMD 0x00
#define CM32181_REG_ADDR_ALS 0x04
#define CM32181_REG_ADDR_STATUS 0x06
@@ -48,7 +48,7 @@
#define CM32181_MLUX_PER_BIT_BASE_IT 800000 /* Based on IT=800ms */
#define CM32181_CALIBSCALE_DEFAULT 1000
#define CM32181_CALIBSCALE_RESOLUTION 1000
-#define MLUX_PER_LUX 1000
+#define CM32181_MLUX_PER_LUX 1000
#define CM32181_ID 0x81
#define CM3218_ID 0x18
@@ -57,8 +57,8 @@ static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
CM32181_REG_ADDR_CMD,
};
-static const int als_it_bits[] = {12, 8, 0, 1, 2, 3};
-static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000,
+static const int cm32181_als_it_bits[] = {12, 8, 0, 1, 2, 3};
+static const int cm32181_als_it_value[] = {25000, 50000, 100000, 200000, 400000,
800000};
struct cm32181_chip {
@@ -124,9 +124,9 @@ static int cm32181_read_als_it(struct cm32181_chip *cm32181, int *val2)
als_it = cm32181->conf_regs[CM32181_REG_ADDR_CMD];
als_it &= CM32181_CMD_ALS_IT_MASK;
als_it >>= CM32181_CMD_ALS_IT_SHIFT;
- for (i = 0; i < ARRAY_SIZE(als_it_bits); i++) {
- if (als_it == als_it_bits[i]) {
- *val2 = als_it_value[i];
+ for (i = 0; i < ARRAY_SIZE(cm32181_als_it_bits); i++) {
+ if (als_it == cm32181_als_it_bits[i]) {
+ *val2 = cm32181_als_it_value[i];
return IIO_VAL_INT_PLUS_MICRO;
}
}
@@ -149,14 +149,14 @@ static int cm32181_write_als_it(struct cm32181_chip *cm32181, int val)
u16 als_it;
int ret, i, n;
- n = ARRAY_SIZE(als_it_value);
+ n = ARRAY_SIZE(cm32181_als_it_value);
for (i = 0; i < n; i++)
- if (val <= als_it_value[i])
+ if (val <= cm32181_als_it_value[i])
break;
if (i >= n)
i = n - 1;
- als_it = als_it_bits[i];
+ als_it = cm32181_als_it_bits[i];
als_it <<= CM32181_CMD_ALS_IT_SHIFT;
mutex_lock(&cm32181->lock);
@@ -202,7 +202,7 @@ static int cm32181_get_lux(struct cm32181_chip *cm32181)
lux *= ret;
lux *= cm32181->calibscale;
lux /= CM32181_CALIBSCALE_RESOLUTION;
- lux /= MLUX_PER_LUX;
+ lux /= CM32181_MLUX_PER_LUX;
if (lux > 0xFFFF)
lux = 0xFFFF;
@@ -270,9 +270,9 @@ static ssize_t cm32181_get_it_available(struct device *dev,
{
int i, n, len;
- n = ARRAY_SIZE(als_it_value);
+ n = ARRAY_SIZE(cm32181_als_it_value);
for (i = 0, len = 0; i < n; i++)
- len += sprintf(buf + len, "0.%06u ", als_it_value[i]);
+ len += sprintf(buf + len, "0.%06u ", cm32181_als_it_value[i]);
return len + sprintf(buf + len, "\n");
}
--
2.11.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v7 1/4] i2c-smbus : Add client discovered ARA support
2018-01-13 13:37 [PATCH v7 1/4] i2c-smbus : Add client discovered ARA support Marc CAPDEVILLE
` (2 preceding siblings ...)
2018-01-13 13:37 ` [PATCH v7 4/4] iio : cm32181 : cosmetic cleanup Marc CAPDEVILLE
@ 2018-01-14 11:45 ` Jonathan Cameron
3 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2018-01-14 11:45 UTC (permalink / raw)
To: Marc CAPDEVILLE
Cc: Kevin Tsai, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Mika Westerberg, Wolfram Sang, linux-iio,
linux-i2c, linux-acpi, linux-kernel
On Sat, 13 Jan 2018 14:37:02 +0100
Marc CAPDEVILLE <m.capdeville@no-log.org> wrote:
> This is from rfc by Alan Cox : https://patchwork.ozlabs.org/patch/381773
>
> The idea is as follows (extract from above rfc) :
> - If an adapter knows about its ARA and smbus alerts then the adapter
> creates its own interrupt handler as before
>
> - If a client knows it needs smbus alerts it calls
> i2c_require_smbus_alert(). This ensures that there is an ARA handler
> registered and tells the client whether the adapter is handling it
> anyway or not.
>
> - When the client learns that an ARA event has occurred it calls
> i2c_smbus_alert_event() which uses the existing ARA mechanism to kick
> things off.
>
> In addition, if a client have an irq line to trigger smbus alert, client
> driver register its irq with i2c_smbus_alert_add_irq() to the smbus alert
> driver to use the generic alert interrupt handler. On detach, the client
> must unregister its irq from the smbus alert driver with
> i2c_smbus_alert_free_irq().
A trivial comment inline...
Also, I think this whole thing could do with some documentation as it is
a little 'unusual' :)
Perhaps put a cover letter explaining how whole series has changed as
well.
Jonathan
>
> Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
> ---
> drivers/i2c/i2c-core-smbus.c | 42 +++++++++++
> drivers/i2c/i2c-smbus.c | 171 +++++++++++++++++++++++++++++++++++++++++--
> include/linux/i2c-smbus.h | 22 ++++++
> include/linux/i2c.h | 2 +
> 4 files changed, 232 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 1238c94fe5a1..0d84ac9669e9 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -659,6 +659,48 @@ struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
> }
> EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert);
>
> +/**
> + * i2c_require_smbus_alert - Client discovered SMBus alert
> + * @client: client requiring ARA support
> + *
> + * When a client needs an ARA it calls this method. If the bus adapter
> + * supports ARA and already knows how to do so then it will already have
> + * configured for ARA and this is a no-op. If not then we set up an ARA
> + * on the adapter.
> + *
> + * Return:
> + * 0 if ARA support already registered
> + * 1 if call register a new smbus_alert device
> + * <0 on error
> + */
> +int i2c_require_smbus_alert(struct i2c_client *client)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + struct i2c_smbus_alert_setup setup;
> + struct i2c_client *ara;
> + int ret;
> +
> + if (adapter->smbus_ara) {
> + /*
> + * ARA is already known and handled by the adapter (ideal case)
> + * or another client has specified ARA is needed
> + */
> + ret = 0;
> + } else {
> + /* First client requesting alert and no adapter has
> + * has setup one
> + */
> + setup.irq = 0;
> + ara = i2c_setup_smbus_alert(adapter, &setup);
> + if (!ara)
> + return -ENODEV;
> + ret = 1;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i2c_require_smbus_alert);
> +
> #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
> int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter)
> {
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 5a1dd7f13bac..1699333ac950 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -25,9 +25,16 @@
> #include <linux/slab.h>
> #include <linux/workqueue.h>
>
> +struct i2c_smbus_irq_usage {
> + struct list_head list;
> + int irq;
> + int count;
> +};
> +
> struct i2c_smbus_alert {
> struct work_struct alert;
> struct i2c_client *ara; /* Alert response address */
> + struct list_head irq_usage; /* irq usage list */
> };
>
> struct alert_data {
> @@ -126,6 +133,124 @@ static void smbalert_work(struct work_struct *work)
>
> }
>
> +/**
> + * i2c_smbus_alert_add_irq - Add a new irq handler to ARA client
> + * @client: The client which want to add an smbus alert irq handler
> + * @irq: The irq number to be added to the smbus alert device
> + * return: 0 if irq handler already exist, 1 if a new handler has been
> + * registered, <0 on error
> + *
> + * This is used by the smbalert_probe and by smbus client to check if an
> + * irq handler already exist for that irq and if not register a new one
> + * Clients must free their irq with i2c_smbus_alert_free_irq() on driver
> + * detach.
> + */
> +int i2c_smbus_alert_add_irq(struct i2c_client *client, int irq)
> +{
> + int res;
> + struct i2c_smbus_irq_usage *irq_usage;
> + struct i2c_client *ara;
> + struct i2c_smbus_alert *alert;
> +
> + ara = client->adapter->smbus_ara;
> + if (!ara)
> + return -EINVAL;
> +
> + alert = i2c_get_clientdata(client->adapter->smbus_ara);
> + if (!alert)
> + return -EINVAL;
> +
> + if (!irq)
> + return 0;
> +
> + /* Check if handler exist for that irq */
> + list_for_each_entry(irq_usage, &alert->irq_usage, list)
> + if (irq_usage->irq == irq)
> + break;
> +
> + if (irq_usage->irq == irq) {
> + irq_usage->count++;
> + } else {
> + /* setup a new handler for that irq */
> + res = devm_request_threaded_irq(&ara->dev, irq,
> + NULL, smbus_alert,
> + IRQF_SHARED | IRQF_ONESHOT,
> + "smbus_alert", alert);
> + if (res)
> + return res;
> +
> + /* Add adapter irq number to used irq list with a count of 1 */
> + irq_usage = devm_kmalloc(&ara->dev,
> + sizeof(struct i2c_smbus_irq_usage),
> + GFP_KERNEL);
> + INIT_LIST_HEAD(&irq_usage->list);
> + irq_usage->irq = irq;
> + irq_usage->count = 1;
> + list_add(&irq_usage->list, &alert->irq_usage);
> +
> + return 0;
> + }
> +
> + return 1;
> +}
> +EXPORT_SYMBOL_GPL(i2c_smbus_alert_add_irq);
> +
> +/**
> + * i2c_smbus_alert_free_irq - free irq added with i2c_smbus_alert_add_irq()
> + * @client: The client which want to free its smbus alert irq
> + * @irq: The irq number to be freed
> + * return: 0 if irq handler still exist for other client,
> + * 1 if client is the last one using this handler and handler have been
> + * removed,
> + * <0 on error.
> + *
> + * This is used by smbus clients to free their irq usage from smbus alert
> + * device.
> + */
> +int i2c_smbus_alert_free_irq(struct i2c_client *client, int irq)
> +{
> + struct i2c_smbus_irq_usage *irq_usage;
> + struct i2c_client *ara;
> + struct i2c_smbus_alert *alert;
> +
> + ara = client->adapter->smbus_ara;
> + if (!ara)
> + return -EINVAL;
> +
> + alert = i2c_get_clientdata(client->adapter->smbus_ara);
> + if (!alert)
> + return -EINVAL;
> +
> + if (!irq)
> + return 0;
> +
> + /* Check if handler exist for that irq */
> + list_for_each_entry(irq_usage, &alert->irq_usage, list)
> + if (irq_usage->irq == irq)
> + break;
> +
> + if (irq_usage->irq == irq) {
> + irq_usage->count--;
> + if (!irq_usage->count) {
/*
* usage
> + /* usage count goes to 0
> + * so remove irq_usage from list
> + */
> + list_del(&irq_usage->list);
> + devm_kfree(&ara->dev, irq_usage);
> +
> + /* remove irq handler */
> + devm_free_irq(&ara->dev, irq, alert);
> +
> + return 1;
> + }
> +
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(i2c_smbus_alert_free_irq);
> +
> /* Setup SMBALERT# infrastructure */
> static int smbalert_probe(struct i2c_client *ara,
> const struct i2c_device_id *id)
> @@ -151,16 +276,18 @@ static int smbalert_probe(struct i2c_client *ara,
> INIT_WORK(&alert->alert, smbalert_work);
> alert->ara = ara;
>
> + INIT_LIST_HEAD(&alert->irq_usage);
> +
> + i2c_set_clientdata(ara, alert);
> +
> + ara->adapter->smbus_ara = ara;
> +
> if (irq > 0) {
> - res = devm_request_threaded_irq(&ara->dev, irq,
> - NULL, smbus_alert,
> - IRQF_SHARED | IRQF_ONESHOT,
> - "smbus_alert", alert);
> + res = i2c_smbus_alert_add_irq(ara, irq);
> if (res)
> return res;
> }
>
> - i2c_set_clientdata(ara, alert);
> dev_info(&adapter->dev, "supports SMBALERT#\n");
>
> return 0;
> @@ -172,6 +299,9 @@ static int smbalert_remove(struct i2c_client *ara)
> struct i2c_smbus_alert *alert = i2c_get_clientdata(ara);
>
> cancel_work_sync(&alert->alert);
> +
> + ara->adapter->smbus_ara = NULL;
> +
> return 0;
> }
>
> @@ -210,6 +340,37 @@ int i2c_handle_smbus_alert(struct i2c_client *ara)
> }
> EXPORT_SYMBOL_GPL(i2c_handle_smbus_alert);
>
> +/**
> + * i2c_smbus_alert_event
> + * @client: the client who known of a probable ARA event
> + * Context: can't sleep
> + *
> + * Helper function to be called from an I2C device driver's interrupt
> + * handler. It will schedule the alert work, in turn calling the
> + * corresponding I2C device driver's alert function.
> + *
> + * It is assumed that client is an i2c client who previously call
> + * i2c_require_smbus_alert().
> + *
> + * return: <0 on error
> + */
> +int i2c_smbus_alert_event(struct i2c_client *client)
> +{
> + struct i2c_client *ara;
> + struct i2c_smbus_alert *alert;
> +
> + ara = client->adapter->smbus_ara;
> + if (!ara)
> + return -EINVAL;
> +
> + alert = i2c_get_clientdata(ara);
> + if (!alert)
> + return -EINVAL;
> +
> + return schedule_work(&alert->alert);
> +}
> +EXPORT_SYMBOL_GPL(i2c_smbus_alert_event);
> +
> module_i2c_driver(smbalert_driver);
>
> MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index fb0e040b1abb..cf777f29cc79 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -47,6 +47,7 @@ struct i2c_smbus_alert_setup {
>
> struct i2c_client *i2c_setup_smbus_alert(struct i2c_adapter *adapter,
> struct i2c_smbus_alert_setup *setup);
> +int i2c_require_smbus_alert(struct i2c_client *client);
> int i2c_handle_smbus_alert(struct i2c_client *ara);
>
> #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF)
> @@ -58,4 +59,25 @@ static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
> }
> #endif
>
> +#if IS_ENABLED(CONFIG_I2C_SMBUS)
> +int i2c_smbus_alert_add_irq(struct i2c_client *client, int irq);
> +int i2c_smbus_alert_free_irq(struct i2c_client *client, int irq);
> +int i2c_smbus_alert_event(struct i2c_client *client);
> +#else
> +static inline int i2c_smbus_alert_add_irq(struct i2c_client *client, int irq)
> +{
> + return -EINVAL;
> +}
> +
> +static inline int i2c_smbus_alert_free_irq(struct i2c_client *client, int irq)
> +{
> + return -EINVAL;
> +}
> +
> +static inline int i2c_smbus_alert_event(struct i2c_client *client)
> +{
> + return -EINVAL;
> +}
> +#endif
> +
> #endif /* _LINUX_I2C_SMBUS_H */
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 5d7f3c1853ae..7592dce12923 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -652,6 +652,8 @@ struct i2c_adapter {
> const struct i2c_adapter_quirks *quirks;
>
> struct irq_domain *host_notify_domain;
> +
> + struct i2c_client *smbus_ara; /* ARA client address if any or NULL */
> };
> #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>
^ permalink raw reply [flat|nested] 13+ messages in thread