From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756466AbeATQgq (ORCPT ); Sat, 20 Jan 2018 11:36:46 -0500 Received: from mail.kernel.org ([198.145.29.99]:43998 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756071AbeATQge (ORCPT ); Sat, 20 Jan 2018 11:36:34 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 98B5D2175D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=jic23@kernel.org Date: Sat, 20 Jan 2018 16:36:28 +0000 From: Jonathan Cameron To: "Marc CAPDEVILLE" Cc: "Kevin Tsai" , "Hartmut Knaack" , "Lars-Peter Clausen" , "Peter Meerwald-Stadler" , "Mika Westerberg" , "Wolfram Sang" , linux-iio@vger.kernel.org, linux-i2c@vger.kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 3/4] iio : Add cm3218 smbus ARA and ACPI support Message-ID: <20180120163628.2057e8d9@archlinux> In-Reply-To: <093635ddcdb9788f80620442ec81eadf.squirrel@webmail.no-log.org> References: <20180113133705.25044-1-m.capdeville@no-log.org> <20180113133705.25044-3-m.capdeville@no-log.org> <20180114114510.1207b191@archlinux> <093635ddcdb9788f80620442ec81eadf.squirrel@webmail.no-log.org> X-Mailer: Claws Mail 3.16.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 14 Jan 2018 15:39:24 +0100 "Marc CAPDEVILLE" wrote: > > > On Sat, 13 Jan 2018 14:37:04 +0100 > > 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. > >> > >> 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 > > > > 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 > >> #include > >> #include > >> +#include > >> +#include > >> +#include > >> > >> /* 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); > > > > > >