Re: [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support

[Marc CAPDEVILLE]

> On Sat, 13 Jan 2018 14:37:04 +0100
> Marc CAPDEVILLE <m.capdeville@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>