On Sun, 14 Jan 2018 15:39:24 +0100
"Marc CAPDEVILLE" <m.capdeville@xxxxxxxxxx> wrote:
>
> > 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 ?
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);
> >
> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
