Message-ID: <20180120163628.2057e8d9@archlinux>
Date:   Sat, 20 Jan 2018 16:36:28 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Marc CAPDEVILLE" <m.capdeville@...log.org>
Cc:     "Kevin Tsai" <ktsai@...ellamicro.com>,
        "Hartmut Knaack" <knaack.h@....de>,
        "Lars-Peter Clausen" <lars@...afoo.de>,
        "Peter Meerwald-Stadler" <pmeerw@...erw.net>,
        "Mika Westerberg" <mika.westerberg@...ux.intel.com>,
        "Wolfram Sang" <wsa@...-dreams.de>, linux-iio@...r.kernel.org,
        linux-i2c@...r.kernel.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support

On Sun, 14 Jan 2018 15:39:24 +0100
"Marc CAPDEVILLE" <m.capdeville@...log.org> wrote:

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

Powered by blists - more mailing lists