[PATCH v3 0/4]: i2c-core improvements for complex ACPI-devices + cht-wc-fuel-gauge driver

[PATCH v3 0/4]: i2c-core improvements for complex ACPI-devices + cht-wc-fuel-gauge driver

[Hans de Goede]

Hi Wolfram, Sebastian, All,

Here is v3 of my i2c-core improvements for complex ACPI-devices and
my Cherry Trail Whiskey Cove PMIC Fuel Gauge driver.

The fuel-gauge driver depends on the i2c-core changes, which is
why I'm posting this as a single series. It might be easiest
to merge the fuel-gauge driver through the i2c tree (once it
passes review). Sebastian is that ok for you ?

See inside the individual patches for a per patch changelog.

Regards,

Hans

[PATCH v3 1/4] i2c: core: Allow getting ACPI info by index

[Hans de Goede]

Modify struct i2c_acpi_lookup and i2c_acpi_fill_info() to allow
using them to get the info from a certain index in the ACPI-resource
list rather then taking the first I2cSerialBus resource.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-No changes
Changes in v2:
-Increment number of found i2c busses for lookup-by-index after checking
 the acpi-resource is an i2c bus rather then before
---
 drivers/i2c/i2c-core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d2402bb..f7faa99 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -112,6 +112,8 @@ struct i2c_acpi_lookup {
 	acpi_handle adapter_handle;
 	acpi_handle device_handle;
 	acpi_handle search_handle;
+	int n;
+	int index;
 	u32 speed;
 	u32 min_speed;
 };
@@ -130,6 +132,9 @@ static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
 	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
 		return 1;
 
+	if (lookup->index != -1 && lookup->n++ != lookup->index)
+		return 1;
+
 	status = acpi_get_handle(lookup->device_handle,
 				 sb->resource_source.string_ptr,
 				 &lookup->adapter_handle);
@@ -182,6 +187,7 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
 
 	memset(&lookup, 0, sizeof(lookup));
 	lookup.info = info;
+	lookup.index = -1;
 
 	ret = i2c_acpi_do_lookup(adev, &lookup);
 	if (ret)
@@ -328,6 +334,7 @@ u32 i2c_acpi_find_bus_speed(struct device *dev)
 	lookup.search_handle = ACPI_HANDLE(dev);
 	lookup.min_speed = UINT_MAX;
 	lookup.info = &dummy;
+	lookup.index = -1;
 
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
 				     I2C_ACPI_MAX_SCAN_DEPTH,
-- 
2.9.3

Re: [PATCH v3 1/4] i2c: core: Allow getting ACPI info by index

[Andy Shevchenko]

On Sat, 2017-03-25 at 14:55 +0100, Hans de Goede wrote:
> Modify struct i2c_acpi_lookup and i2c_acpi_fill_info() to allow
> using them to get the info from a certain index in the ACPI-resource
> list rather then taking the first I2cSerialBus resource.
> 

Fine by me, though same question / note as per patch 3.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -No changes
> Changes in v2:
> -Increment number of found i2c busses for lookup-by-index after
> checking
>  the acpi-resource is an i2c bus rather then before
> ---
>  drivers/i2c/i2c-core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index d2402bb..f7faa99 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -112,6 +112,8 @@ struct i2c_acpi_lookup {
>  	acpi_handle adapter_handle;
>  	acpi_handle device_handle;
>  	acpi_handle search_handle;
> +	int n;
> +	int index;
>  	u32 speed;
>  	u32 min_speed;
>  };
> @@ -130,6 +132,9 @@ static int i2c_acpi_fill_info(struct acpi_resource
> *ares, void *data)
>  	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
>  		return 1;
>  
> +	if (lookup->index != -1 && lookup->n++ != lookup->index)
> +		return 1;
> +
>  	status = acpi_get_handle(lookup->device_handle,
>  				 sb->resource_source.string_ptr,
>  				 &lookup->adapter_handle);
> @@ -182,6 +187,7 @@ static int i2c_acpi_get_info(struct acpi_device
> *adev,
>  
>  	memset(&lookup, 0, sizeof(lookup));
>  	lookup.info = info;
> +	lookup.index = -1;
>  
>  	ret = i2c_acpi_do_lookup(adev, &lookup);
>  	if (ret)
> @@ -328,6 +334,7 @@ u32 i2c_acpi_find_bus_speed(struct device *dev)
>  	lookup.search_handle = ACPI_HANDLE(dev);
>  	lookup.min_speed = UINT_MAX;
>  	lookup.info = &dummy;
> +	lookup.index = -1;
>  
>  	status = acpi_walk_namespace(ACPI_TYPE_DEVICE,
> ACPI_ROOT_OBJECT,
>  				     I2C_ACPI_MAX_SCAN_DEPTH,

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 2/4] i2c: core: Add new i2c_acpi_new_device helper function

[Hans de Goede]

By default the i2c subsys creates an i2c-client for the first I2cSerialBus
resource of an acpi_device, but some acpi_devices have multiple
I2cSerialBus resources and the driver may need access to the others.

This commit adds a new i2c_acpi_new_device function which can be used by
drivers to create an i2c-client for any (other) I2cSerialBus resource of
an acpi_device.

Note that the other resources may even be on a different i2c bus, so just
retrieving the client address is not enough.

Here is an example DSDT excerpt from such a device:

Device (WIDR)
{
    Name (_HID, "INT33FE" /* XPOWER Battery Device */)
    Name (_CID, "INT33FE" /* XPOWER Battery Device */)
    Name (_DDN, "WC PMIC Battery Device")
<snip>
    Name (RBUF, ResourceTemplate ()
    {
        I2cSerialBusV2 (0x005E, ControllerInitiated, 0x000186A0,
            AddressingMode7Bit, "\\_SB.PCI0.I2C7",
            0x00, ResourceConsumer, , Exclusive,
            )
        I2cSerialBusV2 (0x0036, ControllerInitiated, 0x000186A0,
            AddressingMode7Bit, "\\_SB.PCI0.I2C1",
            0x00, ResourceConsumer, , Exclusive,
            )
        I2cSerialBusV2 (0x0022, ControllerInitiated, 0x00061A80,
            AddressingMode7Bit, "\\_SB.PCI0.I2C1",
            0x00, ResourceConsumer, , Exclusive,
            )
        I2cSerialBusV2 (0x0054, ControllerInitiated, 0x00061A80,
            AddressingMode7Bit, "\\_SB.PCI0.I2C1",
            0x00, ResourceConsumer, , Exclusive,
            )
        GpioInt (Level, ActiveLow, Exclusive, PullNone, 0x0000,
            "\\_SB.PCI0.I2C7.PMI5", 0x00, ResourceConsumer, ,
            )
            {   // Pin list
        0x0012
            }
        GpioInt (Edge, ActiveLow, ExclusiveAndWake, PullNone, 0x0000,
            "\\_SB.GPO1", 0x00, ResourceConsumer, ,
            )
            {   // Pin list
        0x0005
            }
        GpioInt (Level, ActiveLow, Exclusive, PullNone, 0x0000,
            "\\_SB.PCI0.I2C7.PMI5", 0x00, ResourceConsumer, ,
            )
            {   // Pin list
        0x0013
            }
    })
    Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
    {
        Return (RBUF) /* \_SB_.PCI0.I2C7.WIDR.RBUF */
    }
<snip>
}

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Changes in v2:
-No changes
Changes in v3:
Added example DSDT snippet to the commit msg
---
 drivers/i2c/i2c-core.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |  5 +++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index f7faa99..062b480 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -421,6 +421,53 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
 static struct notifier_block i2c_acpi_notifier = {
 	.notifier_call = i2c_acpi_notify,
 };
+
+/**
+ * i2c_acpi_new_device - Create i2c client for the Nth I2cSerialBus resource
+ * @dev:     Device owning the acpi resources to get the client from
+ * @index:   Index of acpi resource to get
+ *
+ * By default the i2c subsys creates an i2c-client for the first I2cSerialBus
+ * resource of an acpi_device, but some acpi_devices have multiple
+ * I2cSerialBus resources and the driver may need access to the others.
+ * This function can be used by drivers to create an i2c-client for other
+ * I2cSerialBus resources in the Current Resource Settings table.
+ *
+ * Returns a pointer to the new i2c-client, or NULL if the adapter is not found.
+ */
+struct i2c_client *i2c_acpi_new_device(struct device *dev, int index)
+{
+	struct i2c_acpi_lookup lookup;
+	struct i2c_board_info info;
+	struct i2c_adapter *adapter;
+	struct acpi_device *adev;
+	LIST_HEAD(resource_list);
+	int ret;
+
+	adev = ACPI_COMPANION(dev);
+	if (!adev)
+		return NULL;
+
+	memset(&info, 0, sizeof(info));
+	memset(&lookup, 0, sizeof(lookup));
+	lookup.info = &info;
+	lookup.device_handle = acpi_device_handle(adev);
+	lookup.index = index;
+
+	ret = acpi_dev_get_resources(adev, &resource_list,
+				     i2c_acpi_fill_info, &lookup);
+	acpi_dev_free_resource_list(&resource_list);
+
+	if (ret < 0 || !info.addr)
+		return NULL;
+
+	adapter = i2c_acpi_find_adapter_by_handle(lookup.adapter_handle);
+	if (!adapter)
+		return NULL;
+
+	return i2c_new_device(adapter, &info);
+}
+EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
 #else /* CONFIG_ACPI */
 static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
 extern struct notifier_block i2c_acpi_notifier;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 6b18352..369ebfa 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -824,11 +824,16 @@ static inline const struct of_device_id
 
 #if IS_ENABLED(CONFIG_ACPI)
 u32 i2c_acpi_find_bus_speed(struct device *dev);
+struct i2c_client *i2c_acpi_new_device(struct device *dev, int index);
 #else
 static inline u32 i2c_acpi_find_bus_speed(struct device *dev)
 {
 	return 0;
 }
+static inline struct i2c_client *i2c_acpi_new_device(struct device *d, int i)
+{
+	return NULL;
+}
 #endif /* CONFIG_ACPI */
 
 #endif /* _LINUX_I2C_H */
-- 
2.9.3

[PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Hans de Goede]

Some of or ACPI declared / enumerated devices may have multiple irq
resources declared and the driver may want to use a different irq then
the one with index 0.

This commit adds a new irq_index field to struct i2c_driver and makes
the i2c-core pass this to of_irq_get / acpi_dev_gpio_irq_get.

This is esp. useful for ACPI declared devices where the irq with
index 0 may be entirely useless and cause i2c_device_probe to fail with
-EPROBE_DEFER.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Actually also use the irq_index for of interrupts
Changes in v3:
-Add kernel doc for new i2c_driver irq_index member
-Remove duplicate assignment of driver in i2c_device_probe
---
 drivers/i2c/i2c-core.c | 10 ++++++----
 include/linux/i2c.h    |  4 ++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 062b480..a7dfa6c 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -983,6 +983,8 @@ static int i2c_device_probe(struct device *dev)
 	if (!client)
 		return 0;
 
+	driver = to_i2c_driver(dev->driver);
+
 	if (!client->irq) {
 		int irq = -ENOENT;
 
@@ -992,9 +994,11 @@ static int i2c_device_probe(struct device *dev)
 		} else if (dev->of_node) {
 			irq = of_irq_get_byname(dev->of_node, "irq");
 			if (irq == -EINVAL || irq == -ENODATA)
-				irq = of_irq_get(dev->of_node, 0);
+				irq = of_irq_get(dev->of_node,
+						 driver->irq_index);
 		} else if (ACPI_COMPANION(dev)) {
-			irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
+			irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev),
+						    driver->irq_index);
 		}
 		if (irq == -EPROBE_DEFER)
 			return irq;
@@ -1005,8 +1009,6 @@ static int i2c_device_probe(struct device *dev)
 		client->irq = irq;
 	}
 
-	driver = to_i2c_driver(dev->driver);
-
 	/*
 	 * An I2C ID table is not mandatory, if and only if, a suitable Device
 	 * Tree match table entry is supplied for the probing device.
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 369ebfa..79de1d1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -149,6 +149,7 @@ enum i2c_alert_protocol {
  * @detect: Callback for device detection
  * @address_list: The I2C addresses to probe (for detect)
  * @clients: List of detected clients we created (for i2c-core use only)
+ * @irq_index: IRQ index for retreiving irq from OF/ACPI
  *
  * The driver.owner field should be set to the module owner of this driver.
  * The driver.name field should be set to the name of this driver.
@@ -212,6 +213,9 @@ struct i2c_driver {
 	int (*detect)(struct i2c_client *, struct i2c_board_info *);
 	const unsigned short *address_list;
 	struct list_head clients;
+
+	/* IRQ index for retreiving irq from OF/ACPI */
+	int irq_index;
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
 
-- 
2.9.3

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Andy Shevchenko]

On Sat, 2017-03-25 at 14:55 +0100, Hans de Goede wrote:
> Some of or ACPI declared / enumerated devices may have multiple irq
> resources declared and the driver may want to use a different irq then
> the one with index 0.
> 
> This commit adds a new irq_index field to struct i2c_driver and makes
> the i2c-core pass this to of_irq_get / acpi_dev_gpio_irq_get.
> 
> This is esp. useful for ACPI declared devices where the irq with
> index 0 may be entirely useless and cause i2c_device_probe to fail
> with
> -EPROBE_DEFER.

Just a side note / question: are we assuming that the index of
I2cSerialBus() resource and index of GpioInt() resource should be 1:1
mapped?

One nit below, and

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Actually also use the irq_index for of interrupts
> Changes in v3:
> -Add kernel doc for new i2c_driver irq_index member
> -Remove duplicate assignment of driver in i2c_device_probe
> ---
>  drivers/i2c/i2c-core.c | 10 ++++++----
>  include/linux/i2c.h    |  4 ++++
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 062b480..a7dfa6c 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -983,6 +983,8 @@ static int i2c_device_probe(struct device *dev)
>  	if (!client)
>  		return 0;
>  
> +	driver = to_i2c_driver(dev->driver);
> +
>  	if (!client->irq) {
>  		int irq = -ENOENT;
>  
> @@ -992,9 +994,11 @@ static int i2c_device_probe(struct device *dev)
>  		} else if (dev->of_node) {
>  			irq = of_irq_get_byname(dev->of_node, "irq");
>  			if (irq == -EINVAL || irq == -ENODATA)
> -				irq = of_irq_get(dev->of_node, 0);
> +				irq = of_irq_get(dev->of_node,
> +						 driver->irq_index);
>  		} else if (ACPI_COMPANION(dev)) {
> -			irq =
> acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
> +			irq =
> acpi_dev_gpio_irq_get(ACPI_COMPANION(dev),
> +						    driver-
> >irq_index);
>  		}
>  		if (irq == -EPROBE_DEFER)
>  			return irq;
> @@ -1005,8 +1009,6 @@ static int i2c_device_probe(struct device *dev)
>  		client->irq = irq;
>  	}
>  
> -	driver = to_i2c_driver(dev->driver);
> -
>  	/*
>  	 * An I2C ID table is not mandatory, if and only if, a
> suitable Device
>  	 * Tree match table entry is supplied for the probing device.
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 369ebfa..79de1d1 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -149,6 +149,7 @@ enum i2c_alert_protocol {
>   * @detect: Callback for device detection
>   * @address_list: The I2C addresses to probe (for detect)
>   * @clients: List of detected clients we created (for i2c-core use
> only)
> + * @irq_index: IRQ index for retreiving irq from OF/ACPI
>   *
>   * The driver.owner field should be set to the module owner of this
> driver.
>   * The driver.name field should be set to the name of this driver.
> @@ -212,6 +213,9 @@ struct i2c_driver {
>  	int (*detect)(struct i2c_client *, struct i2c_board_info *);
>  	const unsigned short *address_list;
>  	struct list_head clients;

> +
> +	/* IRQ index for retreiving irq from OF/ACPI */

Since kernel doc in place the above is redundant.

> +	int irq_index;
>  };
>  #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Hans de Goede]

Hi,

On 26-03-17 14:15, Andy Shevchenko wrote:
> On Sat, 2017-03-25 at 14:55 +0100, Hans de Goede wrote:
>> Some of or ACPI declared / enumerated devices may have multiple irq
>> resources declared and the driver may want to use a different irq then
>> the one with index 0.
>>
>> This commit adds a new irq_index field to struct i2c_driver and makes
>> the i2c-core pass this to of_irq_get / acpi_dev_gpio_irq_get.
>>
>> This is esp. useful for ACPI declared devices where the irq with
>> index 0 may be entirely useless and cause i2c_device_probe to fail
>> with
>> -EPROBE_DEFER.
>
> Just a side note / question: are we assuming that the index of
> I2cSerialBus() resource and index of GpioInt() resource should be 1:1
> mapped?

TL;DR: No

Drivers can use irq_index and/or i2c_acpi_new_device
independently of each other. If they want both the irq at index
0 and others, they should not use irq_index, so they get the
irq at index 0 as usual and they can call acpi_dev_gpio_irq_get
themselves to get other irqs, like how they can call the new
i2c_acpi_new_device function to get i2c-clients for I2cSerialBus
with another index then 0.

The purpose of irq_index in the i2c_driver struct is for drivers
which don't care about the irq at index 0 and want another one
instead, this could even be a driver for an acpi device with
only a single I2cSerialBus resource and multiple irq resources.

Specifically this is useful for driver where the irq-resource
at index 0 points to a irq-host which does not have a driver
in Linux, as otherwise i2c_device_probe will always fail with
-EPROBE_DEFER waiting for the non-existent irq-host/irqchip
driver to load. Alternatively we could add a no_irq boolean
flag to struct i2c_driver, that would work fine for my purposes
too and might be more generic, as it fixes the no irqchip
driver, but we don't care anyways, case for all possible
scenarios. Where as the irq_index fix relies on their being
another irq-resource which does have a matching irqchip
driver.

Drivers which want another irq would then need to combine
the no_irq flag with calling acpi_dev_gpio_irq_get with
a non 0 index to get the irq which they do actually want
(just like drivers which want to listen to multiple irqs
would have to do).

> One nit below, and
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
>
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Actually also use the irq_index for of interrupts
>> Changes in v3:
>> -Add kernel doc for new i2c_driver irq_index member
>> -Remove duplicate assignment of driver in i2c_device_probe
>> ---
>>  drivers/i2c/i2c-core.c | 10 ++++++----
>>  include/linux/i2c.h    |  4 ++++
>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 062b480..a7dfa6c 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -983,6 +983,8 @@ static int i2c_device_probe(struct device *dev)
>>  	if (!client)
>>  		return 0;
>>
>> +	driver = to_i2c_driver(dev->driver);
>> +
>>  	if (!client->irq) {
>>  		int irq = -ENOENT;
>>
>> @@ -992,9 +994,11 @@ static int i2c_device_probe(struct device *dev)
>>  		} else if (dev->of_node) {
>>  			irq = of_irq_get_byname(dev->of_node, "irq");
>>  			if (irq == -EINVAL || irq == -ENODATA)
>> -				irq = of_irq_get(dev->of_node, 0);
>> +				irq = of_irq_get(dev->of_node,
>> +						 driver->irq_index);
>>  		} else if (ACPI_COMPANION(dev)) {
>> -			irq =
>> acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
>> +			irq =
>> acpi_dev_gpio_irq_get(ACPI_COMPANION(dev),
>> +						    driver-
>>> irq_index);
>>  		}
>>  		if (irq == -EPROBE_DEFER)
>>  			return irq;
>> @@ -1005,8 +1009,6 @@ static int i2c_device_probe(struct device *dev)
>>  		client->irq = irq;
>>  	}
>>
>> -	driver = to_i2c_driver(dev->driver);
>> -
>>  	/*
>>  	 * An I2C ID table is not mandatory, if and only if, a
>> suitable Device
>>  	 * Tree match table entry is supplied for the probing device.
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 369ebfa..79de1d1 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -149,6 +149,7 @@ enum i2c_alert_protocol {
>>   * @detect: Callback for device detection
>>   * @address_list: The I2C addresses to probe (for detect)
>>   * @clients: List of detected clients we created (for i2c-core use
>> only)
>> + * @irq_index: IRQ index for retreiving irq from OF/ACPI
>>   *
>>   * The driver.owner field should be set to the module owner of this
>> driver.
>>   * The driver.name field should be set to the name of this driver.
>> @@ -212,6 +213,9 @@ struct i2c_driver {
>>  	int (*detect)(struct i2c_client *, struct i2c_board_info *);
>>  	const unsigned short *address_list;
>>  	struct list_head clients;
>
>> +
>> +	/* IRQ index for retreiving irq from OF/ACPI */
>
> Since kernel doc in place the above is redundant.

True, but most other fields (which also have kerneldoc) have a comment
explaining them inside the struct declaration too, so I followed that
example.

Regards,

Hans

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Hans de Goede]

Hi,

Sorry for doing this in the middle of the thread, but this is the only
mail I could find in my archives (I guess I delete mail to aggressively).

Anyways self-nack for this series, there are multiple issues with it:

1) In hindsight the irq_index is a bad idea and instead we need a noirq
flag in i2c_driver

2) The i2c_acpi_new_device() function needs to have a board_info
argument

3) The cht_wc_fuel_gauge driver actually is a driver for the
Maxim MAX17047 Fuel Gauge. After much poking and reading of
the dsdt I've finally found out what the mysterious INT33FE
ACPI device in the dsdt represents, here is a comment from
a new platform/x86 driver I'm writing for it:

/*
  * Intel Cherry Trail ACPI INT33FE pseudo device driver
  * Some Intel Cherry Trail based device which ship with Windows 10, have
  * this weird INT33FE ACPI device with a CRS table with 4 I2cSerialBusV2
  * resources, for 4 different chips attached to various i2c busses:
  * 1. The Whiskey Cove pmic, which is also described by the INT34D3 ACPI device
  * 2. Maxim MAX17047 Fuel Gauge Controller
  * 3. FUSB300C USB Type-C Controller
  * 4. PI3USB30532 USB switch
  *
  * So this driver is a stub / pseudo driver whose only purpose is to
  * instantiate i2c-clients for chips 2 - 4, so that standard i2c drivers
  * for these chips can bind to the them.
  */

So the plan is to have this stub driver instantiate an i2c client
with an id of max17047 and then have a proper max17047 battery
power-supply driver which will get bound to that i2c-client.

Regards,

Hans

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

Hi Hans,

> Sorry for doing this in the middle of the thread, but this is the only
> mail I could find in my archives (I guess I delete mail to aggressively).

I use patchwork to track patches (which is mentioned in MAINTAINERS).
And patchwork has the nice feature to download patches or series of
patches as mbox. And from there you can reply directly, or bounce to
yourself first, etc. That's what I do when I lost a mail.

> Anyways self-nack for this series, there are multiple issues with it:
> 
> 1) In hindsight the irq_index is a bad idea and instead we need a noirq
> flag in i2c_driver

What do you mean by that? No IRQ assigned? In Linux, 0 is no irq. I
guess you mean something else...

Thanks for the heads up,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Hans de Goede]

Hi,

On 30-03-17 22:39, Wolfram Sang wrote:
> Hi Hans,
>
>> Sorry for doing this in the middle of the thread, but this is the only
>> mail I could find in my archives (I guess I delete mail to aggressively).
>
> I use patchwork to track patches (which is mentioned in MAINTAINERS).
> And patchwork has the nice feature to download patches or series of
> patches as mbox. And from there you can reply directly, or bounce to
> yourself first, etc. That's what I do when I lost a mail.

Ok, I will do that the next time.

>> Anyways self-nack for this series, there are multiple issues with it:
>>
>> 1) In hindsight the irq_index is a bad idea and instead we need a noirq
>> flag in i2c_driver
>
> What do you mean by that? No IRQ assigned? In Linux, 0 is no irq. I
> guess you mean something else...

Note I said "flag in i2c_driver" the idea is that the driver can tell
the i2c_core that it is not going to use i2c_client->irq by
setting i2c_driver->no_irq and that the i2c_core then will not bother
with trying to get an irq in i2c_device_probe(), this is esp. useful
for ACPI i2c instantiated devices where we otherwise might end up
returning -EPROBE_DEFER (waiting for an irq to show up) without
needing the irq, which is esp. troublesome when there is no driver
for the irqchip the ACPI irq resource points to as then we end up
waiting indefinitely.

I hope this makes things more clear, if you've another suggestion
for handling this I'm all ears.

Regards,

Hans

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]


> Note I said "flag in i2c_driver" the idea is that the driver can tell
> the i2c_core that it is not going to use i2c_client->irq by
> setting i2c_driver->no_irq and that the i2c_core then will not bother
> with trying to get an irq in i2c_device_probe(), this is esp. useful
> for ACPI i2c instantiated devices where we otherwise might end up
> returning -EPROBE_DEFER (waiting for an irq to show up) without
> needing the irq, which is esp. troublesome when there is no driver
> for the irqchip the ACPI irq resource points to as then we end up
> waiting indefinitely.

Okay, thanks. I understand the big picture. But does it really need to
be fixed in I2C core? Independent of I2C: if an irq is described in ACPI
and the driver for the needed irq controller is not available, that can
lead to various problems everywhere.

Or maybe you simply want to be early and don't want to get deferred? Are
we talking then about boot optimizations or necessities?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Hans de Goede]

Hi,

On 31-03-17 18:23, Wolfram Sang wrote:
>
>> Note I said "flag in i2c_driver" the idea is that the driver can tell
>> the i2c_core that it is not going to use i2c_client->irq by
>> setting i2c_driver->no_irq and that the i2c_core then will not bother
>> with trying to get an irq in i2c_device_probe(), this is esp. useful
>> for ACPI i2c instantiated devices where we otherwise might end up
>> returning -EPROBE_DEFER (waiting for an irq to show up) without
>> needing the irq, which is esp. troublesome when there is no driver
>> for the irqchip the ACPI irq resource points to as then we end up
>> waiting indefinitely.
>
> Okay, thanks. I understand the big picture. But does it really need to
> be fixed in I2C core? Independent of I2C: if an irq is described in ACPI
> and the driver for the needed irq controller is not available, that can
> lead to various problems everywhere.

Normally drivers call acpi_dev_gpio_irq_get themselves in their probe
method and the -EPROBE_DEFER handling is done in the drivers probe
itself, giving drivers various options to deal with irqs.

The problem here is that the i2c system is somewhat special in that
it does irq mapping on behalf of the driver and does not even bother
to call the driver's probe() if the acpi_dev_gpio_irq_get() call
returns -EPROBE_DEFER.

> Or maybe you simply want to be early and don't want to get deferred? Are
> we talking then about boot optimizations or necessities?

The problem which I'm trying to fix is not having to write a (complex)
gpio driver for an undocumented PMIC which I (and AFAICT no-one) needs (*)
just because the i2c-core needs to be "special" and do the acpi_dev_gpio_irq_get
for me even though in this specific driver I don't need the irq at index
0 at all. IOW the problem which I'm trying to fix is to get i2c_device_probe
to not out-smart me and never call my driver's probe method for
invalid reasons.

My previous patch for this added an irq_index member to i2c_driver instead,
because my use-case does actually need an irq, but the one with
resource-index 1, not the useless one at index 0. Which is yet another
indication that the naive irq-handling in the i2c-core sometimes get
somewhat in the way. In my experience many more complex i2c devices have
more then 1 irq line.

That previous patch works for me too (and even simplifies my driver somewhat),
if you like it better I can go back to that. But thinking more about this
I decided that having a "dear i2c-core please don't try to out-smart the
driver, leave irq handling to me" flag would be better / more generally
useful.

Regards,

Hans


*) And because there is no use-case for it other then satisfying the
i2c-core's desire to be able to do acpi_dev_gpio_irq_get(0) also no
way to test!

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2437 bytes --]

Hi Hans,

> The problem here is that the i2c system is somewhat special in that
> it does irq mapping on behalf of the driver and does not even bother
> to call the driver's probe() if the acpi_dev_gpio_irq_get() call
> returns -EPROBE_DEFER.

Yes, the client->irq handling is cruft, I am fully aware of that.

> >Or maybe you simply want to be early and don't want to get deferred? Are
> >we talking then about boot optimizations or necessities?
> 
> The problem which I'm trying to fix is not having to write a (complex)
> gpio driver for an undocumented PMIC which I (and AFAICT no-one) needs (*)
> just because the i2c-core needs to be "special" and do the acpi_dev_gpio_irq_get
> for me even though in this specific driver I don't need the irq at index
> 0 at all. IOW the problem which I'm trying to fix is to get i2c_device_probe
> to not out-smart me and never call my driver's probe method for
> invalid reasons.

So, it is a necessity because you have an ACPI entry to a PMIC which is
totally undocumented. Point taken.

> 
> My previous patch for this added an irq_index member to i2c_driver instead,
> because my use-case does actually need an irq, but the one with
> resource-index 1, not the useless one at index 0. Which is yet another
> indication that the naive irq-handling in the i2c-core sometimes get
> somewhat in the way. In my experience many more complex i2c devices have
> more then 1 irq line.

I am aware of this problem as well.

> That previous patch works for me too (and even simplifies my driver somewhat),
> if you like it better I can go back to that. But thinking more about this
> I decided that having a "dear i2c-core please don't try to out-smart the
> driver, leave irq handling to me" flag would be better / more generally
> useful.

I agree. The last sentence made me understand that you want to flag
"this driver wants custom irq handling" more than "this driver does not
use irq" what I misinterpreted before. This is a much broader use case
and we can probably help more people with that. I like it. Maybe we
should name the flag something like "custom_irq_handling" to prevent
similar misunderstandings? Just an idea.

Sorry if you got annoyed by my questions, yet with the prospect of
not only adding code to the core but also adding something to
i2c_driver, I really want to make sure it is worth it.

Have a nice weekend,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Andy Shevchenko]

On Fri, 2017-03-31 at 21:54 +0200, Wolfram Sang wrote:

> > That previous patch works for me too (and even simplifies my driver
> > somewhat),
> > if you like it better I can go back to that. But thinking more about
> > this
> > I decided that having a "dear i2c-core please don't try to out-smart 
> > the
> > driver, leave irq handling to me" flag would be better / more
> > generally
> > useful.
> 
> I agree. The last sentence made me understand that you want to flag
> "this driver wants custom irq handling" more than "this driver does
> not
> use irq" what I misinterpreted before.

Latest Hans' explanation gets it clear to me either.

>  This is a much broader use case
> and we can probably help more people with that. I like it. Maybe we
> should name the flag something like "custom_irq_handling" to prevent
> similar misunderstandings? Just an idea.

I would go with custom_irq_resource (or custom_irq_mapping).
IRQ handling sounds a bit confusing (do the core handles interrupts on
behalf of my device?).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Hans de Goede]

Hi,

On 31-03-17 21:54, Wolfram Sang wrote:
> Hi Hans,
>
>> The problem here is that the i2c system is somewhat special in that
>> it does irq mapping on behalf of the driver and does not even bother
>> to call the driver's probe() if the acpi_dev_gpio_irq_get() call
>> returns -EPROBE_DEFER.
>
> Yes, the client->irq handling is cruft, I am fully aware of that.
>
>>> Or maybe you simply want to be early and don't want to get deferred? Are
>>> we talking then about boot optimizations or necessities?
>>
>> The problem which I'm trying to fix is not having to write a (complex)
>> gpio driver for an undocumented PMIC which I (and AFAICT no-one) needs (*)
>> just because the i2c-core needs to be "special" and do the acpi_dev_gpio_irq_get
>> for me even though in this specific driver I don't need the irq at index
>> 0 at all. IOW the problem which I'm trying to fix is to get i2c_device_probe
>> to not out-smart me and never call my driver's probe method for
>> invalid reasons.
>
> So, it is a necessity because you have an ACPI entry to a PMIC which is
> totally undocumented. Point taken.
>
>>
>> My previous patch for this added an irq_index member to i2c_driver instead,
>> because my use-case does actually need an irq, but the one with
>> resource-index 1, not the useless one at index 0. Which is yet another
>> indication that the naive irq-handling in the i2c-core sometimes get
>> somewhat in the way. In my experience many more complex i2c devices have
>> more then 1 irq line.
>
> I am aware of this problem as well.
>
>> That previous patch works for me too (and even simplifies my driver somewhat),
>> if you like it better I can go back to that. But thinking more about this
>> I decided that having a "dear i2c-core please don't try to out-smart the
>> driver, leave irq handling to me" flag would be better / more generally
>> useful.
>
> I agree. The last sentence made me understand that you want to flag
> "this driver wants custom irq handling" more than "this driver does not
> use irq" what I misinterpreted before. This is a much broader use case
> and we can probably help more people with that. I like it.

Good, I'm glad to hear that :)

 > Maybe we
> should name the flag something like "custom_irq_handling" to prevent
> similar misunderstandings? Just an idea.

Ok, so bringing in Andy's suggestions here as well (thank you Andy),
proposed names for the flag are:

"no_irq" - my initial attempt at a name, not really descriptive)
"custom_irq_handling" - I think Andy's right that the handling in the name may throw people of
"custom_irq_resource" - Does not feel entirely right either
"custom_irq_mapping"  - Idem

So I'm going to throw in a fifth option for a name for the flag:

"disable_i2c_core_irq_mapping"

It is a bit long, but IMHO best describes what it does, "custom"
is a bit vague and IMHO makes the flag sound more special then it is.

Shorter alternatives would be: "disable_irq_mapping".

Wolfram, if you can let me know which name you prefer then I
will update my local patches accordingly and post a new version
of my patch-set.

> Sorry if you got annoyed by my questions, yet with the prospect of
> not only adding code to the core but also adding something to
> i2c_driver, I really want to make sure it is worth it.

Understood, thank you for understanding my use-case / problem and
sorry if I sounded a bit annoyed.

I wish you both a nice weekend too,

Hans

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 239 bytes --]

Hans,

> So I'm going to throw in a fifth option for a name for the flag:
> 
> "disable_i2c_core_irq_mapping"

I am fine with that. It is only used once per driver, so "clarity over
brevity" from my POV.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Dmitry Torokhov]

On Fri, Mar 31, 2017 at 08:22:55PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 31-03-17 18:23, Wolfram Sang wrote:
> >
> >>Note I said "flag in i2c_driver" the idea is that the driver can tell
> >>the i2c_core that it is not going to use i2c_client->irq by
> >>setting i2c_driver->no_irq and that the i2c_core then will not bother
> >>with trying to get an irq in i2c_device_probe(), this is esp. useful
> >>for ACPI i2c instantiated devices where we otherwise might end up
> >>returning -EPROBE_DEFER (waiting for an irq to show up) without
> >>needing the irq, which is esp. troublesome when there is no driver
> >>for the irqchip the ACPI irq resource points to as then we end up
> >>waiting indefinitely.
> >
> >Okay, thanks. I understand the big picture. But does it really need to
> >be fixed in I2C core? Independent of I2C: if an irq is described in ACPI
> >and the driver for the needed irq controller is not available, that can
> >lead to various problems everywhere.
> 
> Normally drivers call acpi_dev_gpio_irq_get themselves in their probe
> method and the -EPROBE_DEFER handling is done in the drivers probe
> itself, giving drivers various options to deal with irqs.
> 
> The problem here is that the i2c system is somewhat special in that
> it does irq mapping on behalf of the driver and does not even bother
> to call the driver's probe() if the acpi_dev_gpio_irq_get() call
> returns -EPROBE_DEFER.
> 
> >Or maybe you simply want to be early and don't want to get deferred? Are
> >we talking then about boot optimizations or necessities?
> 
> The problem which I'm trying to fix is not having to write a (complex)
> gpio driver for an undocumented PMIC which I (and AFAICT no-one) needs (*)
> just because the i2c-core needs to be "special" and do the acpi_dev_gpio_irq_get
> for me even though in this specific driver I don't need the irq at index
> 0 at all. IOW the problem which I'm trying to fix is to get i2c_device_probe
> to not out-smart me and never call my driver's probe method for
> invalid reasons.

I wonder if, instead of adding flags to I2C core (which I think behaves
quite sanely), we could not have a "simple" GPIO stub driver for that
PMIC that just registers gpiochip and does nothing (until somebody finds
a use case and actually adds meat to it). This will stop
acpi_dev_gpio_irq_get() returning -EPROBE_DEFER and all will be well in
the kingdom.

Thanks.

-- 
Dmitry

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Hans de Goede]

Hi,

On 01-04-17 18:33, Dmitry Torokhov wrote:
> On Fri, Mar 31, 2017 at 08:22:55PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 31-03-17 18:23, Wolfram Sang wrote:
>>>
>>>> Note I said "flag in i2c_driver" the idea is that the driver can tell
>>>> the i2c_core that it is not going to use i2c_client->irq by
>>>> setting i2c_driver->no_irq and that the i2c_core then will not bother
>>>> with trying to get an irq in i2c_device_probe(), this is esp. useful
>>>> for ACPI i2c instantiated devices where we otherwise might end up
>>>> returning -EPROBE_DEFER (waiting for an irq to show up) without
>>>> needing the irq, which is esp. troublesome when there is no driver
>>>> for the irqchip the ACPI irq resource points to as then we end up
>>>> waiting indefinitely.
>>>
>>> Okay, thanks. I understand the big picture. But does it really need to
>>> be fixed in I2C core? Independent of I2C: if an irq is described in ACPI
>>> and the driver for the needed irq controller is not available, that can
>>> lead to various problems everywhere.
>>
>> Normally drivers call acpi_dev_gpio_irq_get themselves in their probe
>> method and the -EPROBE_DEFER handling is done in the drivers probe
>> itself, giving drivers various options to deal with irqs.
>>
>> The problem here is that the i2c system is somewhat special in that
>> it does irq mapping on behalf of the driver and does not even bother
>> to call the driver's probe() if the acpi_dev_gpio_irq_get() call
>> returns -EPROBE_DEFER.
>>
>>> Or maybe you simply want to be early and don't want to get deferred? Are
>>> we talking then about boot optimizations or necessities?
>>
>> The problem which I'm trying to fix is not having to write a (complex)
>> gpio driver for an undocumented PMIC which I (and AFAICT no-one) needs (*)
>> just because the i2c-core needs to be "special" and do the acpi_dev_gpio_irq_get
>> for me even though in this specific driver I don't need the irq at index
>> 0 at all. IOW the problem which I'm trying to fix is to get i2c_device_probe
>> to not out-smart me and never call my driver's probe method for
>> invalid reasons.
>
> I wonder if, instead of adding flags to I2C core (which I think behaves
> quite sanely), we could not have a "simple" GPIO stub driver for that
> PMIC that just registers gpiochip and does nothing (until somebody finds
> a use case and actually adds meat to it). This will stop
> acpi_dev_gpio_irq_get() returning -EPROBE_DEFER and all will be well in
> the kingdom.

I'm afraid that that may cause problems when an i2c device shows up
with a driver which actually does need a working irq, faking to have
a working irq then will lead to lots of head scratching why the irq
never triggers then.

And this is also useful in other scenarios, I agree that in many
cases the i2c-core irq handling is useful, but in more complex cases
not so much.

Regards,

Hans

Re: [PATCH v3 3/4] i2c: core: Allow drivers to specify index for irq to get from of / ACPI

[Dmitry Torokhov]

On Sun, Apr 02, 2017 at 02:17:59PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 01-04-17 18:33, Dmitry Torokhov wrote:
> >On Fri, Mar 31, 2017 at 08:22:55PM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 31-03-17 18:23, Wolfram Sang wrote:
> >>>
> >>>>Note I said "flag in i2c_driver" the idea is that the driver can tell
> >>>>the i2c_core that it is not going to use i2c_client->irq by
> >>>>setting i2c_driver->no_irq and that the i2c_core then will not bother
> >>>>with trying to get an irq in i2c_device_probe(), this is esp. useful
> >>>>for ACPI i2c instantiated devices where we otherwise might end up
> >>>>returning -EPROBE_DEFER (waiting for an irq to show up) without
> >>>>needing the irq, which is esp. troublesome when there is no driver
> >>>>for the irqchip the ACPI irq resource points to as then we end up
> >>>>waiting indefinitely.
> >>>
> >>>Okay, thanks. I understand the big picture. But does it really need to
> >>>be fixed in I2C core? Independent of I2C: if an irq is described in ACPI
> >>>and the driver for the needed irq controller is not available, that can
> >>>lead to various problems everywhere.
> >>
> >>Normally drivers call acpi_dev_gpio_irq_get themselves in their probe
> >>method and the -EPROBE_DEFER handling is done in the drivers probe
> >>itself, giving drivers various options to deal with irqs.
> >>
> >>The problem here is that the i2c system is somewhat special in that
> >>it does irq mapping on behalf of the driver and does not even bother
> >>to call the driver's probe() if the acpi_dev_gpio_irq_get() call
> >>returns -EPROBE_DEFER.
> >>
> >>>Or maybe you simply want to be early and don't want to get deferred? Are
> >>>we talking then about boot optimizations or necessities?
> >>
> >>The problem which I'm trying to fix is not having to write a (complex)
> >>gpio driver for an undocumented PMIC which I (and AFAICT no-one) needs (*)
> >>just because the i2c-core needs to be "special" and do the acpi_dev_gpio_irq_get
> >>for me even though in this specific driver I don't need the irq at index
> >>0 at all. IOW the problem which I'm trying to fix is to get i2c_device_probe
> >>to not out-smart me and never call my driver's probe method for
> >>invalid reasons.
> >
> >I wonder if, instead of adding flags to I2C core (which I think behaves
> >quite sanely), we could not have a "simple" GPIO stub driver for that
> >PMIC that just registers gpiochip and does nothing (until somebody finds
> >a use case and actually adds meat to it). This will stop
> >acpi_dev_gpio_irq_get() returning -EPROBE_DEFER and all will be well in
> >the kingdom.
> 
> I'm afraid that that may cause problems when an i2c device shows up
> with a driver which actually does need a working irq, faking to have
> a working irq then will lead to lots of head scratching why the irq
> never triggers then.

I was talking about this particular PMIC that you do not want to write
full driver for, not doing this at i2c core level. If somebody does need
interrupt form that device, hey will have to write driver.

> 
> And this is also useful in other scenarios, I agree that in many
> cases the i2c-core irq handling is useful, but in more complex cases
> not so much.

-PPROBE_DEFER is the only one case where driver does not get to run its
probe() method. In all other scenarios driver is free to ignore
client->irq and implement its own interrupt parsing and mapping.

Thanks.

-- 
Dmitry

[PATCH v3 4/4] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge

[Hans de Goede]

Add a driver for the Cherry Trail Whiskey Cove PMIC Fuel Gauge.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-There was no v2, it is skipped to the version in sync with the rest of
 the patch-set
Changes in v3:
-Change into a stand-alone power_supply battery driver instead of being a
 provider of extra properties for another battery driver
-Rename Kconfig symbol from CHT_WC_FUEL_GAUGE to BATTERY_INTEL_CHT_WC
---
 drivers/power/supply/Kconfig             |   9 +
 drivers/power/supply/Makefile            |   1 +
 drivers/power/supply/cht_wc_fuel_gauge.c | 319 +++++++++++++++++++++++++++++++
 3 files changed, 329 insertions(+)
 create mode 100644 drivers/power/supply/cht_wc_fuel_gauge.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index fd93110..350d86a 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -538,4 +538,13 @@ config AXP20X_POWER
 	  This driver provides support for the power supply features of
 	  AXP20x PMIC.
 
+config BATTERY_INTEL_CHT_WC
+	tristate "Intel Cherry Trail Whiskey Cove PMIC Fuel Gauge"
+	depends on INTEL_SOC_PMIC_CHTWC
+	help
+	  This adds support for the battery fuel gauge found in the Intel
+	  Cherry Trail Whiskey Cove PMIC. This driver allows monitoring
+	  of the charge level of the battery on Intel Cherry Trail systems
+	  with a Whiskey Cove PMIC.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 3789a2c..46dca5c 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
 obj-$(CONFIG_CHARGER_TPS65217)	+= tps65217_charger.o
 obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
 obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
+obj-$(CONFIG_BATTERY_INTEL_CHT_WC) += cht_wc_fuel_gauge.o
diff --git a/drivers/power/supply/cht_wc_fuel_gauge.c b/drivers/power/supply/cht_wc_fuel_gauge.c
new file mode 100644
index 0000000..0db83ad
--- /dev/null
+++ b/drivers/power/supply/cht_wc_fuel_gauge.c
@@ -0,0 +1,319 @@
+/*
+ * Intel Cherry Trail Whiskey Cove Fuel Gauge driver
+ * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/extcon.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/intel_soc_pmic.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+
+#define FG_CHARGE_NOW			0x05
+#define FG_VOLTAGE_NOW			0x09
+#define FG_CURRENT_NOW			0x0a
+#define FG_CURRENT_AVG			0x0b
+#define FG_CHARGE_FULL			0x10
+#define FG_CHARGE_DESIGN		0x18
+#define FG_VOLTAGE_AVG			0x19
+#define FG_VOLTAGE_OCV			0x1b /* Only updated during charging */
+
+#define PMIC_CHGRSTATUS			0x1a
+#define PMIC_CHGRSTATUS_NOT_CHARGING	BIT(0)
+
+#define CHT_WC_FG_PTYPE		4
+
+static const unsigned int charger_cable_ids[] = {
+	EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP,
+	EXTCON_CHG_USB_DCP, EXTCON_CHG_USB_ACA
+};
+
+struct cht_wc_fg_data {
+	struct device *dev;
+	/*
+	 * The ACPI _CRS table contains info for 4 clients, 1 for the charger-
+	 * manager part of the pmic and 3 for the actual fuel-gauge (which has
+	 * 3 i2c addresses) note we use only 1 fg address/client.
+	 */
+	struct i2c_client *pmic_client;
+	struct i2c_client *fg_client;
+	struct extcon_dev *extcon;
+	struct power_supply *battery;
+	struct work_struct changed_work;
+	struct notifier_block extcon_nb;
+};
+
+static int cht_wc_fg_read(struct cht_wc_fg_data *fg, u8 reg,
+			  union power_supply_propval *val, int scale,
+			  int sign_extend)
+{
+	int ret;
+
+	ret = i2c_smbus_read_word_data(fg->fg_client, reg);
+	if (ret < 0)
+		return ret;
+
+	if (sign_extend)
+		ret = sign_extend32(ret, 15);
+
+	val->intval = ret * scale;
+
+	return 0;
+}
+
+static int cht_wc_fg_get_status(struct cht_wc_fg_data *fg,
+				union power_supply_propval *val)
+{
+	int i, ret;
+	bool vbus_present = false;
+
+	for (i = 0; i < ARRAY_SIZE(charger_cable_ids); i++) {
+		if (extcon_get_state(fg->extcon, charger_cable_ids[i]) > 0) {
+			vbus_present = true;
+			break;
+		}
+	}
+
+	if (!vbus_present) {
+		val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		return 0;
+	}
+
+	ret = i2c_smbus_read_byte_data(fg->pmic_client, PMIC_CHGRSTATUS);
+	if (ret < 0)
+		return ret;
+
+	/* Not charging while we have Vbus means the battery is full */
+	if (ret & PMIC_CHGRSTATUS_NOT_CHARGING)
+		val->intval = POWER_SUPPLY_STATUS_FULL;
+	else
+		val->intval = POWER_SUPPLY_STATUS_CHARGING;
+
+	return 0;
+}
+
+static int cht_wc_fg_get_property(struct power_supply *psy,
+	enum power_supply_property prop, union power_supply_propval *val)
+{
+	struct cht_wc_fg_data *fg = power_supply_get_drvdata(psy);
+	int ret = 0;
+
+	switch (prop) {
+	case POWER_SUPPLY_PROP_STATUS:
+		ret = cht_wc_fg_get_status(fg, val);
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		break;
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		ret = cht_wc_fg_read(fg, FG_VOLTAGE_NOW, val, 75, 0);
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_AVG:
+		ret = cht_wc_fg_read(fg, FG_VOLTAGE_AVG, val, 75, 0);
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
+		ret = cht_wc_fg_read(fg, FG_VOLTAGE_OCV, val, 75, 0);
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		ret = cht_wc_fg_read(fg, FG_CURRENT_NOW, val, 150, 1);
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_AVG:
+		ret = cht_wc_fg_read(fg, FG_CURRENT_AVG, val, 150, 1);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
+		ret = cht_wc_fg_read(fg, FG_CHARGE_DESIGN, val, 500, 0);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_FULL:
+		ret = cht_wc_fg_read(fg, FG_CHARGE_FULL, val, 500, 0);
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_NOW:
+		ret = cht_wc_fg_read(fg, FG_CHARGE_NOW, val, 500, 0);
+		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
+	default:
+		ret = -ENODATA;
+	}
+
+	return ret;
+}
+
+static void cht_wc_fg_external_power_changed(struct power_supply *psy)
+{
+	struct cht_wc_fg_data *fg = power_supply_get_drvdata(psy);
+
+	schedule_work(&fg->changed_work);
+}
+
+static enum power_supply_property cht_wc_fg_properties[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_AVG,
+	POWER_SUPPLY_PROP_VOLTAGE_OCV,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CURRENT_AVG,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_FULL,
+	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_SCOPE,
+};
+
+static const struct power_supply_desc bat_desc = {
+	/* Matches charger.supplied_to for external_power_changed callback */
+	.name			= "main-battery",
+	.type			= POWER_SUPPLY_TYPE_BATTERY,
+	.properties		= cht_wc_fg_properties,
+	.num_properties		= ARRAY_SIZE(cht_wc_fg_properties),
+	.get_property		= cht_wc_fg_get_property,
+	.external_power_changed = cht_wc_fg_external_power_changed,
+};
+
+static void cht_wc_fg_changed_work(struct work_struct *work)
+{
+	struct cht_wc_fg_data *fg =
+		container_of(work, struct cht_wc_fg_data, changed_work);
+
+	/* Wait a bit to allow the fuel-gauge to also detect the new status */
+	msleep(200);
+
+	power_supply_changed(fg->battery);
+}
+
+static int cht_wc_fg_extcon_event(struct notifier_block *nb,
+				  unsigned long event, void *param)
+{
+	struct cht_wc_fg_data *fg =
+		container_of(nb, struct cht_wc_fg_data, extcon_nb);
+
+	schedule_work(&fg->changed_work);
+
+	return NOTIFY_OK;
+}
+
+static int cht_wc_fg_probe(struct i2c_client *client,
+			const struct i2c_device_id *i2c_id)
+{
+	struct device *dev = &client->dev;
+	struct power_supply_config bat_cfg = {};
+	struct cht_wc_fg_data *fg;
+	acpi_status status;
+	unsigned long long ptyp;
+	int ret;
+
+	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp);
+	if (ACPI_FAILURE(status)) {
+		dev_err(dev, "Failed to get PTYPE\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * The same ACPI HID is used with different PMICs check PTYP to
+	 * ensure that we are dealing with a Whiskey Cove PMIC.
+	 */
+	if (ptyp != CHT_WC_FG_PTYPE)
+		return -ENODEV;
+
+	fg = devm_kzalloc(dev, sizeof(*fg), GFP_KERNEL);
+	if (!fg)
+		return -ENOMEM;
+
+	fg->dev = dev;
+	fg->pmic_client = client;
+	INIT_WORK(&fg->changed_work, cht_wc_fg_changed_work);
+
+	/*
+	 * We use the Whiskey Cove PMIC's pwrsrc detection block to see
+	 * if we are charging or not. We could access the pwrsrc regs
+	 * ourselves, but that requires re-implementing the extcon code,
+	 * so we just use the extcon interface.
+	 */
+	fg->extcon = extcon_get_extcon_dev("cht_wcove_pwrsrc");
+	if (!fg->extcon)
+		return -EPROBE_DEFER;
+
+	/*
+	 * The current resource settings table for the fuel gauge contains
+	 * multiple i2c devices on 2 different i2c-busses.
+	 */
+	fg->fg_client = i2c_acpi_new_device(dev, 1);
+	if (!fg->fg_client)
+		return -EPROBE_DEFER;
+
+	bat_cfg.drv_data = fg;
+	fg->battery = devm_power_supply_register(dev, &bat_desc, &bat_cfg);
+	if (IS_ERR(fg->battery)) {
+		i2c_unregister_device(fg->fg_client);
+		return PTR_ERR(fg->battery);
+	}
+
+	fg->extcon_nb.notifier_call = cht_wc_fg_extcon_event;
+	ret = devm_extcon_register_notifier(dev, fg->extcon, -1,
+					    &fg->extcon_nb);
+	if (ret) {
+		i2c_unregister_device(fg->fg_client);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, fg);
+
+	return 0;
+}
+
+static int cht_wc_fg_remove(struct i2c_client *i2c)
+{
+	struct cht_wc_fg_data *fg = i2c_get_clientdata(i2c);
+
+	i2c_unregister_device(fg->fg_client);
+
+	return 0;
+}
+
+static const struct i2c_device_id cht_wc_fg_i2c_id[] = {
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, cht_wc_fg_i2c_id);
+
+static const struct acpi_device_id cht_wc_fg_acpi_ids[] = {
+	{ "INT33FE", },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, cht_wc_fg_acpi_ids);
+
+static struct i2c_driver cht_wc_fg_driver = {
+	.driver	= {
+		.name	= "CHT Whiskey Cove PMIC Fuel Gauge",
+		.acpi_match_table = ACPI_PTR(cht_wc_fg_acpi_ids),
+	},
+	.probe = cht_wc_fg_probe,
+	.remove = cht_wc_fg_remove,
+	.id_table = cht_wc_fg_i2c_id,
+	.irq_index = 1,
+};
+
+module_i2c_driver(cht_wc_fg_driver);
+
+MODULE_DESCRIPTION("Intel CHT Whiskey Cove PMIC Fuel Gauge driver");
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_LICENSE("GPL");
-- 
2.9.3

Re: [PATCH v3 4/4] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 12559 bytes --]

Hi Hans,

On Sat, Mar 25, 2017 at 02:55:50PM +0100, Hans de Goede wrote:
> Add a driver for the Cherry Trail Whiskey Cove PMIC Fuel Gauge.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

The driver looks fine to me. I think it would be nice to use
regmap for accessing the device, though. Its capability to
dump registers in debugfs is often useful, especially for
reverse engineered drivers.

Also I assume, that you made sure the reported values are correctly
scaled to uV/uA and not mV/mA? Just asking, since people regularly
get this wrong and your scaling looks suspiciously small.

-- Sebastian

> ---
> Changes in v2:
> -There was no v2, it is skipped to the version in sync with the rest of
>  the patch-set
> Changes in v3:
> -Change into a stand-alone power_supply battery driver instead of being a
>  provider of extra properties for another battery driver
> -Rename Kconfig symbol from CHT_WC_FUEL_GAUGE to BATTERY_INTEL_CHT_WC
> ---
>  drivers/power/supply/Kconfig             |   9 +
>  drivers/power/supply/Makefile            |   1 +
>  drivers/power/supply/cht_wc_fuel_gauge.c | 319 +++++++++++++++++++++++++++++++
>  3 files changed, 329 insertions(+)
>  create mode 100644 drivers/power/supply/cht_wc_fuel_gauge.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index fd93110..350d86a 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -538,4 +538,13 @@ config AXP20X_POWER
>  	  This driver provides support for the power supply features of
>  	  AXP20x PMIC.
>  
> +config BATTERY_INTEL_CHT_WC
> +	tristate "Intel Cherry Trail Whiskey Cove PMIC Fuel Gauge"
> +	depends on INTEL_SOC_PMIC_CHTWC
> +	help
> +	  This adds support for the battery fuel gauge found in the Intel
> +	  Cherry Trail Whiskey Cove PMIC. This driver allows monitoring
> +	  of the charge level of the battery on Intel Cherry Trail systems
> +	  with a Whiskey Cove PMIC.
> +
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 3789a2c..46dca5c 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
>  obj-$(CONFIG_CHARGER_TPS65217)	+= tps65217_charger.o
>  obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>  obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
> +obj-$(CONFIG_BATTERY_INTEL_CHT_WC) += cht_wc_fuel_gauge.o
> diff --git a/drivers/power/supply/cht_wc_fuel_gauge.c b/drivers/power/supply/cht_wc_fuel_gauge.c
> new file mode 100644
> index 0000000..0db83ad
> --- /dev/null
> +++ b/drivers/power/supply/cht_wc_fuel_gauge.c
> @@ -0,0 +1,319 @@
> +/*
> + * Intel Cherry Trail Whiskey Cove Fuel Gauge driver
> + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/extcon.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/intel_soc_pmic.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +
> +#define FG_CHARGE_NOW			0x05
> +#define FG_VOLTAGE_NOW			0x09
> +#define FG_CURRENT_NOW			0x0a
> +#define FG_CURRENT_AVG			0x0b
> +#define FG_CHARGE_FULL			0x10
> +#define FG_CHARGE_DESIGN		0x18
> +#define FG_VOLTAGE_AVG			0x19
> +#define FG_VOLTAGE_OCV			0x1b /* Only updated during charging */
> +
> +#define PMIC_CHGRSTATUS			0x1a
> +#define PMIC_CHGRSTATUS_NOT_CHARGING	BIT(0)
> +
> +#define CHT_WC_FG_PTYPE		4
> +
> +static const unsigned int charger_cable_ids[] = {
> +	EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP,
> +	EXTCON_CHG_USB_DCP, EXTCON_CHG_USB_ACA
> +};
> +
> +struct cht_wc_fg_data {
> +	struct device *dev;
> +	/*
> +	 * The ACPI _CRS table contains info for 4 clients, 1 for the charger-
> +	 * manager part of the pmic and 3 for the actual fuel-gauge (which has
> +	 * 3 i2c addresses) note we use only 1 fg address/client.
> +	 */
> +	struct i2c_client *pmic_client;
> +	struct i2c_client *fg_client;
> +	struct extcon_dev *extcon;
> +	struct power_supply *battery;
> +	struct work_struct changed_work;
> +	struct notifier_block extcon_nb;
> +};
> +
> +static int cht_wc_fg_read(struct cht_wc_fg_data *fg, u8 reg,
> +			  union power_supply_propval *val, int scale,
> +			  int sign_extend)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_word_data(fg->fg_client, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (sign_extend)
> +		ret = sign_extend32(ret, 15);
> +
> +	val->intval = ret * scale;
> +
> +	return 0;
> +}
> +
> +static int cht_wc_fg_get_status(struct cht_wc_fg_data *fg,
> +				union power_supply_propval *val)
> +{
> +	int i, ret;
> +	bool vbus_present = false;
> +
> +	for (i = 0; i < ARRAY_SIZE(charger_cable_ids); i++) {
> +		if (extcon_get_state(fg->extcon, charger_cable_ids[i]) > 0) {
> +			vbus_present = true;
> +			break;
> +		}
> +	}
> +
> +	if (!vbus_present) {
> +		val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> +		return 0;
> +	}
> +
> +	ret = i2c_smbus_read_byte_data(fg->pmic_client, PMIC_CHGRSTATUS);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Not charging while we have Vbus means the battery is full */
> +	if (ret & PMIC_CHGRSTATUS_NOT_CHARGING)
> +		val->intval = POWER_SUPPLY_STATUS_FULL;
> +	else
> +		val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +
> +	return 0;
> +}
> +
> +static int cht_wc_fg_get_property(struct power_supply *psy,
> +	enum power_supply_property prop, union power_supply_propval *val)
> +{
> +	struct cht_wc_fg_data *fg = power_supply_get_drvdata(psy);
> +	int ret = 0;
> +
> +	switch (prop) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		ret = cht_wc_fg_get_status(fg, val);
> +		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = 1;
> +		break;
> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		ret = cht_wc_fg_read(fg, FG_VOLTAGE_NOW, val, 75, 0);
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_AVG:
> +		ret = cht_wc_fg_read(fg, FG_VOLTAGE_AVG, val, 75, 0);
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
> +		ret = cht_wc_fg_read(fg, FG_VOLTAGE_OCV, val, 75, 0);
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		ret = cht_wc_fg_read(fg, FG_CURRENT_NOW, val, 150, 1);
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_AVG:
> +		ret = cht_wc_fg_read(fg, FG_CURRENT_AVG, val, 150, 1);
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> +		ret = cht_wc_fg_read(fg, FG_CHARGE_DESIGN, val, 500, 0);
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_FULL:
> +		ret = cht_wc_fg_read(fg, FG_CHARGE_FULL, val, 500, 0);
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +		ret = cht_wc_fg_read(fg, FG_CHARGE_NOW, val, 500, 0);
> +		break;
> +	case POWER_SUPPLY_PROP_SCOPE:
> +		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
> +		break;
> +	default:
> +		ret = -ENODATA;
> +	}
> +
> +	return ret;
> +}
> +
> +static void cht_wc_fg_external_power_changed(struct power_supply *psy)
> +{
> +	struct cht_wc_fg_data *fg = power_supply_get_drvdata(psy);
> +
> +	schedule_work(&fg->changed_work);
> +}
> +
> +static enum power_supply_property cht_wc_fg_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_AVG,
> +	POWER_SUPPLY_PROP_VOLTAGE_OCV,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_AVG,
> +	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> +	POWER_SUPPLY_PROP_CHARGE_FULL,
> +	POWER_SUPPLY_PROP_CHARGE_NOW,
> +	POWER_SUPPLY_PROP_SCOPE,
> +};
> +
> +static const struct power_supply_desc bat_desc = {
> +	/* Matches charger.supplied_to for external_power_changed callback */
> +	.name			= "main-battery",
> +	.type			= POWER_SUPPLY_TYPE_BATTERY,
> +	.properties		= cht_wc_fg_properties,
> +	.num_properties		= ARRAY_SIZE(cht_wc_fg_properties),
> +	.get_property		= cht_wc_fg_get_property,
> +	.external_power_changed = cht_wc_fg_external_power_changed,
> +};
> +
> +static void cht_wc_fg_changed_work(struct work_struct *work)
> +{
> +	struct cht_wc_fg_data *fg =
> +		container_of(work, struct cht_wc_fg_data, changed_work);
> +
> +	/* Wait a bit to allow the fuel-gauge to also detect the new status */
> +	msleep(200);
> +
> +	power_supply_changed(fg->battery);
> +}
> +
> +static int cht_wc_fg_extcon_event(struct notifier_block *nb,
> +				  unsigned long event, void *param)
> +{
> +	struct cht_wc_fg_data *fg =
> +		container_of(nb, struct cht_wc_fg_data, extcon_nb);
> +
> +	schedule_work(&fg->changed_work);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int cht_wc_fg_probe(struct i2c_client *client,
> +			const struct i2c_device_id *i2c_id)
> +{
> +	struct device *dev = &client->dev;
> +	struct power_supply_config bat_cfg = {};
> +	struct cht_wc_fg_data *fg;
> +	acpi_status status;
> +	unsigned long long ptyp;
> +	int ret;
> +
> +	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(dev, "Failed to get PTYPE\n");
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * The same ACPI HID is used with different PMICs check PTYP to
> +	 * ensure that we are dealing with a Whiskey Cove PMIC.
> +	 */
> +	if (ptyp != CHT_WC_FG_PTYPE)
> +		return -ENODEV;
> +
> +	fg = devm_kzalloc(dev, sizeof(*fg), GFP_KERNEL);
> +	if (!fg)
> +		return -ENOMEM;
> +
> +	fg->dev = dev;
> +	fg->pmic_client = client;
> +	INIT_WORK(&fg->changed_work, cht_wc_fg_changed_work);
> +
> +	/*
> +	 * We use the Whiskey Cove PMIC's pwrsrc detection block to see
> +	 * if we are charging or not. We could access the pwrsrc regs
> +	 * ourselves, but that requires re-implementing the extcon code,
> +	 * so we just use the extcon interface.
> +	 */
> +	fg->extcon = extcon_get_extcon_dev("cht_wcove_pwrsrc");
> +	if (!fg->extcon)
> +		return -EPROBE_DEFER;
> +
> +	/*
> +	 * The current resource settings table for the fuel gauge contains
> +	 * multiple i2c devices on 2 different i2c-busses.
> +	 */
> +	fg->fg_client = i2c_acpi_new_device(dev, 1);
> +	if (!fg->fg_client)
> +		return -EPROBE_DEFER;
> +
> +	bat_cfg.drv_data = fg;
> +	fg->battery = devm_power_supply_register(dev, &bat_desc, &bat_cfg);
> +	if (IS_ERR(fg->battery)) {
> +		i2c_unregister_device(fg->fg_client);
> +		return PTR_ERR(fg->battery);
> +	}
> +
> +	fg->extcon_nb.notifier_call = cht_wc_fg_extcon_event;
> +	ret = devm_extcon_register_notifier(dev, fg->extcon, -1,
> +					    &fg->extcon_nb);
> +	if (ret) {
> +		i2c_unregister_device(fg->fg_client);
> +		return ret;
> +	}
> +
> +	i2c_set_clientdata(client, fg);
> +
> +	return 0;
> +}
> +
> +static int cht_wc_fg_remove(struct i2c_client *i2c)
> +{
> +	struct cht_wc_fg_data *fg = i2c_get_clientdata(i2c);
> +
> +	i2c_unregister_device(fg->fg_client);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id cht_wc_fg_i2c_id[] = {
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, cht_wc_fg_i2c_id);
> +
> +static const struct acpi_device_id cht_wc_fg_acpi_ids[] = {
> +	{ "INT33FE", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, cht_wc_fg_acpi_ids);
> +
> +static struct i2c_driver cht_wc_fg_driver = {
> +	.driver	= {
> +		.name	= "CHT Whiskey Cove PMIC Fuel Gauge",
> +		.acpi_match_table = ACPI_PTR(cht_wc_fg_acpi_ids),
> +	},
> +	.probe = cht_wc_fg_probe,
> +	.remove = cht_wc_fg_remove,
> +	.id_table = cht_wc_fg_i2c_id,
> +	.irq_index = 1,
> +};
> +
> +module_i2c_driver(cht_wc_fg_driver);
> +
> +MODULE_DESCRIPTION("Intel CHT Whiskey Cove PMIC Fuel Gauge driver");
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.9.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 4/4] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge

[Hans de Goede]

Hi,

On 25-03-17 19:42, Sebastian Reichel wrote:
> Hi Hans,
>
> On Sat, Mar 25, 2017 at 02:55:50PM +0100, Hans de Goede wrote:
>> Add a driver for the Cherry Trail Whiskey Cove PMIC Fuel Gauge.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> The driver looks fine to me. I think it would be nice to use
> regmap for accessing the device, though. Its capability to
> dump registers in debugfs is often useful, especially for
> reverse engineered drivers.

I just use i2cdump for that ..., TBH I don't see much added
value in using regmap here, but if you insist I can do a
v3 using regmap. Note this will require creating 2 regmaps,
one for each i2c client used.

> Also I assume, that you made sure the reported values are correctly
> scaled to uV/uA and not mV/mA?

Yes my values are in uV and uA the fuelgauge has quite
a good resolution for current and voltage measuring and
the mAh measures are in 0.5 mAh units.

> Just asking, since people regularly
> get this wrong and your scaling looks suspiciously small.

Understood.

Regards,

Hans


>
> -- Sebastian
>
>> ---
>> Changes in v2:
>> -There was no v2, it is skipped to the version in sync with the rest of
>>  the patch-set
>> Changes in v3:
>> -Change into a stand-alone power_supply battery driver instead of being a
>>  provider of extra properties for another battery driver
>> -Rename Kconfig symbol from CHT_WC_FUEL_GAUGE to BATTERY_INTEL_CHT_WC
>> ---
>>  drivers/power/supply/Kconfig             |   9 +
>>  drivers/power/supply/Makefile            |   1 +
>>  drivers/power/supply/cht_wc_fuel_gauge.c | 319 +++++++++++++++++++++++++++++++
>>  3 files changed, 329 insertions(+)
>>  create mode 100644 drivers/power/supply/cht_wc_fuel_gauge.c
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index fd93110..350d86a 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -538,4 +538,13 @@ config AXP20X_POWER
>>  	  This driver provides support for the power supply features of
>>  	  AXP20x PMIC.
>>
>> +config BATTERY_INTEL_CHT_WC
>> +	tristate "Intel Cherry Trail Whiskey Cove PMIC Fuel Gauge"
>> +	depends on INTEL_SOC_PMIC_CHTWC
>> +	help
>> +	  This adds support for the battery fuel gauge found in the Intel
>> +	  Cherry Trail Whiskey Cove PMIC. This driver allows monitoring
>> +	  of the charge level of the battery on Intel Cherry Trail systems
>> +	  with a Whiskey Cove PMIC.
>> +
>>  endif # POWER_SUPPLY
>> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
>> index 3789a2c..46dca5c 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
>>  obj-$(CONFIG_CHARGER_TPS65217)	+= tps65217_charger.o
>>  obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>>  obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
>> +obj-$(CONFIG_BATTERY_INTEL_CHT_WC) += cht_wc_fuel_gauge.o
>> diff --git a/drivers/power/supply/cht_wc_fuel_gauge.c b/drivers/power/supply/cht_wc_fuel_gauge.c
>> new file mode 100644
>> index 0000000..0db83ad
>> --- /dev/null
>> +++ b/drivers/power/supply/cht_wc_fuel_gauge.c
>> @@ -0,0 +1,319 @@
>> +/*
>> + * Intel Cherry Trail Whiskey Cove Fuel Gauge driver
>> + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/delay.h>
>> +#include <linux/extcon.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/intel_soc_pmic.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/slab.h>
>> +#include <linux/workqueue.h>
>> +
>> +#define FG_CHARGE_NOW			0x05
>> +#define FG_VOLTAGE_NOW			0x09
>> +#define FG_CURRENT_NOW			0x0a
>> +#define FG_CURRENT_AVG			0x0b
>> +#define FG_CHARGE_FULL			0x10
>> +#define FG_CHARGE_DESIGN		0x18
>> +#define FG_VOLTAGE_AVG			0x19
>> +#define FG_VOLTAGE_OCV			0x1b /* Only updated during charging */
>> +
>> +#define PMIC_CHGRSTATUS			0x1a
>> +#define PMIC_CHGRSTATUS_NOT_CHARGING	BIT(0)
>> +
>> +#define CHT_WC_FG_PTYPE		4
>> +
>> +static const unsigned int charger_cable_ids[] = {
>> +	EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP,
>> +	EXTCON_CHG_USB_DCP, EXTCON_CHG_USB_ACA
>> +};
>> +
>> +struct cht_wc_fg_data {
>> +	struct device *dev;
>> +	/*
>> +	 * The ACPI _CRS table contains info for 4 clients, 1 for the charger-
>> +	 * manager part of the pmic and 3 for the actual fuel-gauge (which has
>> +	 * 3 i2c addresses) note we use only 1 fg address/client.
>> +	 */
>> +	struct i2c_client *pmic_client;
>> +	struct i2c_client *fg_client;
>> +	struct extcon_dev *extcon;
>> +	struct power_supply *battery;
>> +	struct work_struct changed_work;
>> +	struct notifier_block extcon_nb;
>> +};
>> +
>> +static int cht_wc_fg_read(struct cht_wc_fg_data *fg, u8 reg,
>> +			  union power_supply_propval *val, int scale,
>> +			  int sign_extend)
>> +{
>> +	int ret;
>> +
>> +	ret = i2c_smbus_read_word_data(fg->fg_client, reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (sign_extend)
>> +		ret = sign_extend32(ret, 15);
>> +
>> +	val->intval = ret * scale;
>> +
>> +	return 0;
>> +}
>> +
>> +static int cht_wc_fg_get_status(struct cht_wc_fg_data *fg,
>> +				union power_supply_propval *val)
>> +{
>> +	int i, ret;
>> +	bool vbus_present = false;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(charger_cable_ids); i++) {
>> +		if (extcon_get_state(fg->extcon, charger_cable_ids[i]) > 0) {
>> +			vbus_present = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!vbus_present) {
>> +		val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
>> +		return 0;
>> +	}
>> +
>> +	ret = i2c_smbus_read_byte_data(fg->pmic_client, PMIC_CHGRSTATUS);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Not charging while we have Vbus means the battery is full */
>> +	if (ret & PMIC_CHGRSTATUS_NOT_CHARGING)
>> +		val->intval = POWER_SUPPLY_STATUS_FULL;
>> +	else
>> +		val->intval = POWER_SUPPLY_STATUS_CHARGING;
>> +
>> +	return 0;
>> +}
>> +
>> +static int cht_wc_fg_get_property(struct power_supply *psy,
>> +	enum power_supply_property prop, union power_supply_propval *val)
>> +{
>> +	struct cht_wc_fg_data *fg = power_supply_get_drvdata(psy);
>> +	int ret = 0;
>> +
>> +	switch (prop) {
>> +	case POWER_SUPPLY_PROP_STATUS:
>> +		ret = cht_wc_fg_get_status(fg, val);
>> +		break;
>> +	case POWER_SUPPLY_PROP_PRESENT:
>> +		val->intval = 1;
>> +		break;
>> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
>> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
>> +		break;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +		ret = cht_wc_fg_read(fg, FG_VOLTAGE_NOW, val, 75, 0);
>> +		break;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_AVG:
>> +		ret = cht_wc_fg_read(fg, FG_VOLTAGE_AVG, val, 75, 0);
>> +		break;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
>> +		ret = cht_wc_fg_read(fg, FG_VOLTAGE_OCV, val, 75, 0);
>> +		break;
>> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
>> +		ret = cht_wc_fg_read(fg, FG_CURRENT_NOW, val, 150, 1);
>> +		break;
>> +	case POWER_SUPPLY_PROP_CURRENT_AVG:
>> +		ret = cht_wc_fg_read(fg, FG_CURRENT_AVG, val, 150, 1);
>> +		break;
>> +	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>> +		ret = cht_wc_fg_read(fg, FG_CHARGE_DESIGN, val, 500, 0);
>> +		break;
>> +	case POWER_SUPPLY_PROP_CHARGE_FULL:
>> +		ret = cht_wc_fg_read(fg, FG_CHARGE_FULL, val, 500, 0);
>> +		break;
>> +	case POWER_SUPPLY_PROP_CHARGE_NOW:
>> +		ret = cht_wc_fg_read(fg, FG_CHARGE_NOW, val, 500, 0);
>> +		break;
>> +	case POWER_SUPPLY_PROP_SCOPE:
>> +		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
>> +		break;
>> +	default:
>> +		ret = -ENODATA;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void cht_wc_fg_external_power_changed(struct power_supply *psy)
>> +{
>> +	struct cht_wc_fg_data *fg = power_supply_get_drvdata(psy);
>> +
>> +	schedule_work(&fg->changed_work);
>> +}
>> +
>> +static enum power_supply_property cht_wc_fg_properties[] = {
>> +	POWER_SUPPLY_PROP_STATUS,
>> +	POWER_SUPPLY_PROP_PRESENT,
>> +	POWER_SUPPLY_PROP_TECHNOLOGY,
>> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>> +	POWER_SUPPLY_PROP_VOLTAGE_AVG,
>> +	POWER_SUPPLY_PROP_VOLTAGE_OCV,
>> +	POWER_SUPPLY_PROP_CURRENT_NOW,
>> +	POWER_SUPPLY_PROP_CURRENT_AVG,
>> +	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>> +	POWER_SUPPLY_PROP_CHARGE_FULL,
>> +	POWER_SUPPLY_PROP_CHARGE_NOW,
>> +	POWER_SUPPLY_PROP_SCOPE,
>> +};
>> +
>> +static const struct power_supply_desc bat_desc = {
>> +	/* Matches charger.supplied_to for external_power_changed callback */
>> +	.name			= "main-battery",
>> +	.type			= POWER_SUPPLY_TYPE_BATTERY,
>> +	.properties		= cht_wc_fg_properties,
>> +	.num_properties		= ARRAY_SIZE(cht_wc_fg_properties),
>> +	.get_property		= cht_wc_fg_get_property,
>> +	.external_power_changed = cht_wc_fg_external_power_changed,
>> +};
>> +
>> +static void cht_wc_fg_changed_work(struct work_struct *work)
>> +{
>> +	struct cht_wc_fg_data *fg =
>> +		container_of(work, struct cht_wc_fg_data, changed_work);
>> +
>> +	/* Wait a bit to allow the fuel-gauge to also detect the new status */
>> +	msleep(200);
>> +
>> +	power_supply_changed(fg->battery);
>> +}
>> +
>> +static int cht_wc_fg_extcon_event(struct notifier_block *nb,
>> +				  unsigned long event, void *param)
>> +{
>> +	struct cht_wc_fg_data *fg =
>> +		container_of(nb, struct cht_wc_fg_data, extcon_nb);
>> +
>> +	schedule_work(&fg->changed_work);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static int cht_wc_fg_probe(struct i2c_client *client,
>> +			const struct i2c_device_id *i2c_id)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct power_supply_config bat_cfg = {};
>> +	struct cht_wc_fg_data *fg;
>> +	acpi_status status;
>> +	unsigned long long ptyp;
>> +	int ret;
>> +
>> +	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp);
>> +	if (ACPI_FAILURE(status)) {
>> +		dev_err(dev, "Failed to get PTYPE\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	/*
>> +	 * The same ACPI HID is used with different PMICs check PTYP to
>> +	 * ensure that we are dealing with a Whiskey Cove PMIC.
>> +	 */
>> +	if (ptyp != CHT_WC_FG_PTYPE)
>> +		return -ENODEV;
>> +
>> +	fg = devm_kzalloc(dev, sizeof(*fg), GFP_KERNEL);
>> +	if (!fg)
>> +		return -ENOMEM;
>> +
>> +	fg->dev = dev;
>> +	fg->pmic_client = client;
>> +	INIT_WORK(&fg->changed_work, cht_wc_fg_changed_work);
>> +
>> +	/*
>> +	 * We use the Whiskey Cove PMIC's pwrsrc detection block to see
>> +	 * if we are charging or not. We could access the pwrsrc regs
>> +	 * ourselves, but that requires re-implementing the extcon code,
>> +	 * so we just use the extcon interface.
>> +	 */
>> +	fg->extcon = extcon_get_extcon_dev("cht_wcove_pwrsrc");
>> +	if (!fg->extcon)
>> +		return -EPROBE_DEFER;
>> +
>> +	/*
>> +	 * The current resource settings table for the fuel gauge contains
>> +	 * multiple i2c devices on 2 different i2c-busses.
>> +	 */
>> +	fg->fg_client = i2c_acpi_new_device(dev, 1);
>> +	if (!fg->fg_client)
>> +		return -EPROBE_DEFER;
>> +
>> +	bat_cfg.drv_data = fg;
>> +	fg->battery = devm_power_supply_register(dev, &bat_desc, &bat_cfg);
>> +	if (IS_ERR(fg->battery)) {
>> +		i2c_unregister_device(fg->fg_client);
>> +		return PTR_ERR(fg->battery);
>> +	}
>> +
>> +	fg->extcon_nb.notifier_call = cht_wc_fg_extcon_event;
>> +	ret = devm_extcon_register_notifier(dev, fg->extcon, -1,
>> +					    &fg->extcon_nb);
>> +	if (ret) {
>> +		i2c_unregister_device(fg->fg_client);
>> +		return ret;
>> +	}
>> +
>> +	i2c_set_clientdata(client, fg);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cht_wc_fg_remove(struct i2c_client *i2c)
>> +{
>> +	struct cht_wc_fg_data *fg = i2c_get_clientdata(i2c);
>> +
>> +	i2c_unregister_device(fg->fg_client);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id cht_wc_fg_i2c_id[] = {
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, cht_wc_fg_i2c_id);
>> +
>> +static const struct acpi_device_id cht_wc_fg_acpi_ids[] = {
>> +	{ "INT33FE", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, cht_wc_fg_acpi_ids);
>> +
>> +static struct i2c_driver cht_wc_fg_driver = {
>> +	.driver	= {
>> +		.name	= "CHT Whiskey Cove PMIC Fuel Gauge",
>> +		.acpi_match_table = ACPI_PTR(cht_wc_fg_acpi_ids),
>> +	},
>> +	.probe = cht_wc_fg_probe,
>> +	.remove = cht_wc_fg_remove,
>> +	.id_table = cht_wc_fg_i2c_id,
>> +	.irq_index = 1,
>> +};
>> +
>> +module_i2c_driver(cht_wc_fg_driver);
>> +
>> +MODULE_DESCRIPTION("Intel CHT Whiskey Cove PMIC Fuel Gauge driver");
>> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.9.3
>>