| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20171229125448.339e7778@archlinux>
Date: Fri, 29 Dec 2017 12:54:48 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: Marc CAPDEVILLE <m.capdeville@...log.org>,
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 v6 3/4] iio : Add cm3218 smbus ara and acpi support
On Thu, 28 Dec 2017 02:19:55 +0100
"Rafael J. Wysocki" <rjw@...ysocki.net> wrote:
> On Monday, December 25, 2017 4:57:22 PM CET 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.
> > On asus T100, this device is enumerated on ACPI bus and the description
> > give two I2C connection. The first is the connection to the ARA device
> > and the second gives the real address of the device.
> >
> > So, on device probe,If the i2c address is the ARA address and the
> > device is enumerated via acpi, we change address of the device for
> > the one in the second acpi serial bus connection.
> > This free the ara address so we can register with the smbus_alert
> > driver.
> >
> > If an interrupt resource is given, and a smbus_alert device was
> > found on the adapter, we request a treaded interrupt to
> > call i2c_smbus_alert_event to handle the alert.
> >
> > In somme case, the treaded interrupt is not schedule before the driver
> > try to initialize chip registers. So if first registers access fail, we
> > call i2c_smbus_alert_event() to acknowledge initial alert, then retry
> > register access.
> >
> > Signed-off-by: Marc CAPDEVILLE <m.capdeville@...log.org>
> > ---
> > drivers/iio/light/cm32181.c | 120 ++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 115 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> > index aebf7dd071af..96c08755e6e3 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,11 @@
> > #define CM32181_CALIBSCALE_RESOLUTION 1000
> > #define MLUX_PER_LUX 1000
> >
> > +#define CM32181_ID 0x81
> > +#define CM3218_ID 0x18
> > +
> > +#define SMBUS_ARA_ADDR 0x0c
> > +
> > static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
> > CM32181_REG_ADDR_CMD,
> > };
> > @@ -57,6 +65,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;
> > @@ -77,11 +86,22 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
> > s32 ret;
> >
> > ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID);
> > - if (ret < 0)
> > - return ret;
> > + if (ret < 0) {
> > + if (cm32181->chip_id == CM3218_ID) {
> > + /*
> > + * On cm3218, smbus alert trigger after probing,
> > + * so poll for first alert here, then retry.
> > + */
> > + i2c_smbus_alert_event(client);
> > + ret = i2c_smbus_read_word_data(client,
> > + CM32181_REG_ADDR_ID);
> > + } else {
> > + return ret;
> > + }
> > + }
> >
> > /* check device ID */
> > - if ((ret & 0xFF) != 0x81)
> > + if ((ret & 0xFF) != cm32181->chip_id)
> > return -ENODEV;
> >
> > /* Default Values */
> > @@ -297,12 +317,36 @@ 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 irqreturn_t cm32181_irq(int irq, void *d)
> > +{
> > + struct cm32181_chip *cm32181 = d;
> > +
> > + if (cm32181->chip_id == CM3218_ID) {
> > + /* This is cm3218 chip irq, so handle the smbus alert */
> > + i2c_smbus_alert_event(cm32181->client);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > 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 of_device_id *of_id;
> > + const struct acpi_device_id *acpi_id;
> >
> > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
> > if (!indio_dev) {
> > @@ -322,6 +366,61 @@ 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)) {
> > + acpi_id = acpi_match_device(client->dev.driver->acpi_match_table,
> > + &client->dev);
> > + if (!acpi_id)
> > + return -ENODEV;
> > +
> > + cm32181->chip_id = (kernel_ulong_t)acpi_id->driver_data;
> > + } else if (client->dev.of_node) {
> > + of_id = of_match_device(client->dev.driver->of_match_table,
> > + &client->dev);
> > + if (!of_id)
> > + return -ENODEV;
> > +
> > + cm32181->chip_id = (kernel_ulong_t)of_id->data;
> > + } else {
> > + return -ENODEV;
> > + }
> > +
> > + if (cm32181->chip_id == CM3218_ID) {
> > + if (ACPI_COMPANION(&client->dev) &&
> > + client->addr == SMBUS_ARA_ADDR) {
> > + /*
> > + * In case this device as been enumerated by acpi
> > + * with the reserved smbus ARA address (first acpi
> > + * connection), request change of address to the second
> > + * connection.
> > + */
> > + ret = i2c_acpi_set_connection(client, 1);
> > + if (ret)
> > + return ret;
>
> This looks super-fragile to me.
>
> What about making the enumeration aware of the SMBUS_ARA thing and avoid
> using resources corresponding to that at all?
I agree, if it can be done reasonably cleanly that would be preferable.
>
> BTW, in comments and similar always spell ACPI in capitals.
>
> Thanks,
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists