Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1603140 > unrolled thread

[PATCH 00/15] Add Intel Cherry Trail Whiskey Cove PMIC support

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/15] Add Intel Cherry Trail Whiskey Cove PMIC support Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:00 +0100
    [PATCH 02/15] ACPI / PMIC: Add opregion driver for Intel CHT Whiskey Cove PMIC Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:00 +0100
    [PATCH 14/15] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:00 +0100
      Re: [PATCH 14/15] power: supply: Add driver for Cherry Trail  Whiskey Cove PMIC Fuel Gauge Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-17 19:00 +0100
        Re: [PATCH 14/15] power: supply: Add driver for Cherry Trail Whiskey  Cove PMIC Fuel Gauge Hans de Goede <hdegoede@redhat.com> - 2017-03-22 18:10 +0100
      Re: [PATCH 14/15] power: supply: Add driver for Cherry Trail Whiskey  Cove PMIC Fuel Gauge Sebastian Reichel <sre@kernel.org> - 2017-03-20 06:10 +0100
    [PATCH 13/15] i2c: core: Allow drivers to specify index for irq to get from of / ACPI Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:00 +0100
      Re: [PATCH 13/15] i2c: core: Allow drivers to specify index for irq  to get from of / ACPI Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-17 18:50 +0100
      Re: [PATCH 13/15] i2c: core: Allow drivers to specify index for irq  to get from of / ACPI kbuild test robot <lkp@intel.com> - 2017-03-20 10:40 +0100
    [PATCH 01/15] mfd: Add Cherry Trail Whiskey Cove PMIC driver Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:00 +0100
      Re: [PATCH 01/15] mfd: Add Cherry Trail Whiskey Cove PMIC driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-17 18:20 +0100
        Re: [PATCH 01/15] mfd: Add Cherry Trail Whiskey Cove PMIC driver Lee Jones <lee.jones@linaro.org> - 2017-03-20 11:50 +0100
          Re: [PATCH 01/15] mfd: Add Cherry Trail Whiskey Cove PMIC driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-20 14:00 +0100
    [PATCH 15/15] i2c-cht-wc: Add Intel Cherry Trail Whiskey Cove SMBUS controller driver Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:00 +0100
      Re: [PATCH 15/15] i2c-cht-wc: Add Intel Cherry Trail Whiskey Cove  SMBUS controller driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-17 19:30 +0100
        Re: [PATCH 15/15] i2c-cht-wc: Add Intel Cherry Trail Whiskey Cove  SMBUS controller driver Hans de Goede <hdegoede@redhat.com> - 2017-03-23 15:00 +0100
    [PATCH 11/15] i2c: core: Allow getting ACPI info by index Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:00 +0100
      Re: [PATCH 11/15] i2c: core: Allow getting ACPI info by index Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-17 18:40 +0100
    [PATCH 10/15] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:00 +0100
      Re: [PATCH 10/15] power: supply: bq24190_charger: Use extcon to  determine ilimit, 5v boost Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-17 18:40 +0100
        Re: [PATCH 10/15] power: supply: bq24190_charger: Use extcon to  determine ilimit, 5v boost Hans de Goede <hdegoede@redhat.com> - 2017-03-20 23:40 +0100
      Re: [PATCH 10/15] power: supply: bq24190_charger: Use extcon to  determine ilimit, 5v boost Sebastian Reichel <sre@kernel.org> - 2017-03-20 06:00 +0100
    [PATCH 12/15] i2c: core: Add new i2c_acpi_new_device helper function Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:00 +0100
      Re: [PATCH 12/15] i2c: core: Add new i2c_acpi_new_device helper  function Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-17 18:50 +0100
        Re: [PATCH 12/15] i2c: core: Add new i2c_acpi_new_device helper  function Hans de Goede <hdegoede@redhat.com> - 2017-03-22 17:00 +0100
    [PATCH 04/15] power: supply: bq24190_charger: Add no_register_reset pdata flag Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:10 +0100
      Re: [PATCH 04/15] power: supply: bq24190_charger: Add  no_register_reset pdata flag Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-17 18:30 +0100
    [PATCH 07/15] power: supply: bq24190_charger: Add support for bq24192[i] Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:10 +0100
    [PATCH 05/15] power: supply: bq24190_charger: Limit charging voltage to 4.3V Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:10 +0100
    [PATCH 09/15] power: supply: bq24190_charger: Add voltage_max_design prop to battery Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:10 +0100
      Re: [PATCH 09/15] power: supply: bq24190_charger: Add  voltage_max_design prop to battery Sebastian Reichel <sre@kernel.org> - 2017-03-20 06:20 +0100
    [PATCH 06/15] power: supply: bq24190_charger: Use i2c-core irq-mapping code Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:10 +0100
      Re: [PATCH 06/15] power: supply: bq24190_charger: Use i2c-core  irq-mapping code Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-03-17 18:30 +0100
        Re: [PATCH 06/15] power: supply: bq24190_charger: Use i2c-core  irq-mapping code Sebastian Reichel <sre@kernel.org> - 2017-03-20 05:50 +0100
    [PATCH 08/15] power: supply: bq24190_charger: Add support for external fuel gauge Hans de Goede <hdegoede@redhat.com> - 2017-03-17 11:10 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1605181 — Re: [PATCH 10/15] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost

Hi,

On 17-03-17 18:33, Andy Shevchenko wrote:
> On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote:
>> Add support for monitoring an extcon device with SDP/CDP/DCP and HOST
>> cables and adjust ilimit and enable/disable the 5v boost converter
>> accordingly. This is necessary on systems where the PSEL pin is
>> hardwired
>> high and ILIM needs to be set by software based on the detected
>> charger
>> type.
>>
>
>>  config CHARGER_BQ24190
>>  	tristate "TI BQ24190 battery charger driver"
>>  	depends on I2C
>
>> +	depends on EXTCON
>
> I dunno what is preferred here, but if we would like to keep
> compatibility with previous configurations "select" should be used over
> "depends on".

select really should only be used for hidden options,
using select in other scenarios leads to all sort of
problems (hard to debug Kconfig dependency loops).

>
>> +static void bq24190_extcon_work(struct work_struct *work)
>> +{
>> +	struct bq24190_dev_info *bdi =
>> +		container_of(work, struct bq24190_dev_info,
>> extcon_work);
>> +	int ret, iinlim = 0;
>> +
>> +	if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
>> +		iinlim = 500000;
>> +	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) ==
>> 1 ||
>> +		 extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) ==
>> 1)
>> +		iinlim = 1500000;
>> +	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) ==
>> 1)
>> +		iinlim = 2000000;
>> +
>
>> +	if (iinlim) {
>
> Could be possible to call below unconditionally here (use 0)?

If there is no Vbus setting iinlim is not useful, the charger
will reset it to a default as soon as Vbus comes up and i2c
transactions are not free.

>
>> +		ret = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
>> +				BQ24190_REG_ISC_IINLIM_MASK,
>> +				BQ24190_REG_ISC_IINLIM_SHIFT,
>> +				bq24190_iinlim_values,
>> +				ARRAY_SIZE(bq24190_iinlim_values),
>> +				iinlim);
>> +		if (ret)
>> +			dev_err(bdi->dev, "Can't set IINLIM: %d\n",
>> ret);
>> +	}
>
> Perhaps make above as a helper?
>
> In that case no need for "if (iinlim)" and perhaps switch-case might be
> used instead of if-else-if (latter is up to you).

I prefer to keep this as is.

Thanks & Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1604172 — Re: [PATCH 10/15] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Mar 17, 2017 at 10:55:22AM +0100, Hans de Goede wrote:
> Add support for monitoring an extcon device with SDP/CDP/DCP and HOST
> cables and adjust ilimit and enable/disable the 5v boost converter
> accordingly. This is necessary on systems where the PSEL pin is hardwired
> high and ILIM needs to be set by software based on the detected charger
> type.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

> ---
>  drivers/power/supply/Kconfig           |  1 +
>  drivers/power/supply/bq24190_charger.c | 85 ++++++++++++++++++++++++++++++++++
>  include/linux/power/bq24190_charger.h  |  1 +
>  3 files changed, 87 insertions(+)
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index f8b6e64..fd93110 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -442,6 +442,7 @@ config CHARGER_BQ2415X
>  config CHARGER_BQ24190
>  	tristate "TI BQ24190 battery charger driver"
>  	depends on I2C
> +	depends on EXTCON
>  	depends on GPIOLIB || COMPILE_TEST
>  	help
>  	  Say Y to enable support for the TI BQ24190 battery charger.
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 82cb33d..03990e2 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -11,10 +11,12 @@
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> +#include <linux/extcon.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/power_supply.h>
> +#include <linux/workqueue.h>
>  #include <linux/gpio.h>
>  #include <linux/i2c.h>
>  
> @@ -39,6 +41,8 @@
>  #define BQ24190_REG_POC_WDT_RESET_SHIFT		6
>  #define BQ24190_REG_POC_CHG_CONFIG_MASK		(BIT(5) | BIT(4))
>  #define BQ24190_REG_POC_CHG_CONFIG_SHIFT	4
> +#define BQ24190_REG_POC_CHG_CONFIG_CHARGE	1
> +#define BQ24190_REG_POC_CHG_CONFIG_OTG		2
>  #define BQ24190_REG_POC_SYS_MIN_MASK		(BIT(3) | BIT(2) | BIT(1))
>  #define BQ24190_REG_POC_SYS_MIN_SHIFT		1
>  #define BQ24190_REG_POC_BOOST_LIM_MASK		BIT(0)
> @@ -152,6 +156,9 @@ struct bq24190_dev_info {
>  	struct power_supply		*charger;
>  	struct power_supply		*battery;
>  	struct bq24190_platform_data	*pdata;
> +	struct extcon_dev		*extcon;
> +	struct notifier_block		extcon_nb;
> +	struct work_struct		extcon_work;
>  	char				model_name[I2C_NAME_SIZE];
>  	kernel_ulong_t			model;
>  	struct mutex			f_reg_lock;
> @@ -167,6 +174,11 @@ struct bq24190_dev_info {
>   * number at that index in the array is the real-world value that it
>   * represents.
>   */
> +
> +/* REG00[2:0] (IINLIM) in uAh */
> +static const int bq24190_iinlim_values[] = {
> +	100000, 150000, 500000, 900000, 1200000, 1500000, 2000000, 3000000 };
> +
>  /* REG02[7:2] (ICHG) in uAh */
>  static const int bq24190_ccc_ichg_values[] = {
>  	 512000,  576000,  640000,  704000,  768000,  832000,  896000,  960000,
> @@ -1290,6 +1302,61 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static void bq24190_extcon_work(struct work_struct *work)
> +{
> +	struct bq24190_dev_info *bdi =
> +		container_of(work, struct bq24190_dev_info, extcon_work);
> +	int ret, iinlim = 0;
> +
> +	if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
> +		iinlim = 500000;
> +	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
> +		 extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
> +		iinlim = 1500000;
> +	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
> +		iinlim = 2000000;
> +
> +	if (iinlim) {
> +		ret = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
> +				BQ24190_REG_ISC_IINLIM_MASK,
> +				BQ24190_REG_ISC_IINLIM_SHIFT,
> +				bq24190_iinlim_values,
> +				ARRAY_SIZE(bq24190_iinlim_values),
> +				iinlim);
> +		if (ret)
> +			dev_err(bdi->dev, "Can't set IINLIM: %d\n", ret);
> +	}
> +
> +	/*
> +	 * If no charger has been detected and host mode is requested, activate
> +	 * the 5V boost converter, otherwise deactivate it.
> +	 */
> +	if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1) {
> +		ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
> +					 BQ24190_REG_POC_CHG_CONFIG_MASK,
> +					 BQ24190_REG_POC_CHG_CONFIG_SHIFT,
> +					 BQ24190_REG_POC_CHG_CONFIG_OTG);
> +	} else {
> +		ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
> +					 BQ24190_REG_POC_CHG_CONFIG_MASK,
> +					 BQ24190_REG_POC_CHG_CONFIG_SHIFT,
> +					 BQ24190_REG_POC_CHG_CONFIG_CHARGE);
> +	}
> +	if (ret)
> +		dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
> +}
> +
> +static int bq24190_extcon_event(struct notifier_block *nb, unsigned long event,
> +				void *param)
> +{
> +	struct bq24190_dev_info *bdi =
> +		container_of(nb, struct bq24190_dev_info, extcon_nb);
> +
> +	schedule_work(&bdi->extcon_work);
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>  {
>  	u8 v;
> @@ -1375,6 +1442,12 @@ static int bq24190_probe(struct i2c_client *client,
>  			return -EPROBE_DEFER;
>  	}
>  
> +	if (bdi->pdata && bdi->pdata->extcon_name) {
> +		bdi->extcon = extcon_get_extcon_dev(bdi->pdata->extcon_name);
> +		if (!bdi->extcon)
> +			return -EPROBE_DEFER;
> +	}
> +
>  	pm_runtime_enable(dev);
>  	pm_runtime_resume(dev);
>  
> @@ -1423,6 +1496,18 @@ static int bq24190_probe(struct i2c_client *client,
>  		goto out4;
>  	}
>  
> +	if (bdi->extcon) {
> +		INIT_WORK(&bdi->extcon_work, bq24190_extcon_work);
> +		bdi->extcon_nb.notifier_call = bq24190_extcon_event;
> +		ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
> +						    &bdi->extcon_nb);
> +		if (ret)
> +			goto out4;
> +
> +		/* Sync initial cable state */
> +		schedule_work(&bdi->extcon_work);
> +	}
> +
>  	return 0;
>  
>  out4:
> diff --git a/include/linux/power/bq24190_charger.h b/include/linux/power/bq24190_charger.h
> index 02d248b..909c5b9 100644
> --- a/include/linux/power/bq24190_charger.h
> +++ b/include/linux/power/bq24190_charger.h
> @@ -13,6 +13,7 @@
>  
>  struct bq24190_platform_data {
>  	bool no_register_reset;
> +	const char *extcon_name;
>  	int (*get_ext_bat_property)(enum power_supply_property prop,
>  				    union power_supply_propval *val);
>  };
> -- 
> 2.9.3
> 

[toc] | [prev] | [next] | [standalone]

#1603151 — [PATCH 12/15] i2c: core: Add new i2c_acpi_new_device helper function

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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |  5 +++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 32b58fb..fd45207 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -421,6 +421,54 @@ 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 acpi resource of dev
+ * @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 any
+ * resource of an acpi_device.
+ *
+ * Returns a pointer to the new i2c-client, or NULL if the resource or
+ * adapter were 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

[toc] | [prev] | [next] | [standalone]

#1603508 — Re: [PATCH 12/15] i2c: core: Add new i2c_acpi_new_device helper function

On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote:
> 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.
> 

Looks sane to me:

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

Nevertheless, one nit, can you update commit message with real excerpt
of DSDT?

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/i2c/i2c-core.c | 48
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h    |  5 +++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 32b58fb..fd45207 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -421,6 +421,54 @@ 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 acpi resource
> of dev
> + * @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
> any
> + * resource of an acpi_device.
> + *
> + * Returns a pointer to the new i2c-client, or NULL if the resource
> or
> + * adapter were 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 */

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

[toc] | [prev] | [next] | [standalone]

#1606675 — Re: [PATCH 12/15] i2c: core: Add new i2c_acpi_new_device helper function

Hi,

On 17-03-17 18:37, Andy Shevchenko wrote:
> On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote:
>> 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.
>>
>
> Looks sane to me:
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Nevertheless, one nit, can you update commit message with real excerpt
> of DSDT?

Sure, will do for v2.

Regards,

Hans


>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/i2c/i2c-core.c | 48
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/i2c.h    |  5 +++++
>>  2 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 32b58fb..fd45207 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -421,6 +421,54 @@ 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 acpi resource
>> of dev
>> + * @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
>> any
>> + * resource of an acpi_device.
>> + *
>> + * Returns a pointer to the new i2c-client, or NULL if the resource
>> or
>> + * adapter were 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 */
>

[toc] | [prev] | [next] | [standalone]

#1603154 — [PATCH 04/15] power: supply: bq24190_charger: Add no_register_reset pdata flag

On some platforms the register have been setup with platform specific
values by the firmware and should not be reset.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 5 +++++
 include/linux/power/bq24190_charger.h  | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index a4f0849..c92a40e4 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -151,6 +151,7 @@ struct bq24190_dev_info {
 	struct device			*dev;
 	struct power_supply		*charger;
 	struct power_supply		*battery;
+	struct bq24190_platform_data	*pdata;
 	char				model_name[I2C_NAME_SIZE];
 	kernel_ulong_t			model;
 	unsigned int			gpio_int;
@@ -506,6 +507,9 @@ static int bq24190_register_reset(struct bq24190_dev_info *bdi)
 	int ret, limit = 100;
 	u8 v;
 
+	if (bdi->pdata && bdi->pdata->no_register_reset)
+		return 0;
+
 	/* Reset the registers */
 	ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
 			BQ24190_REG_POC_RESET_MASK,
@@ -1339,6 +1343,7 @@ static int bq24190_probe(struct i2c_client *client,
 	bdi->client = client;
 	bdi->dev = dev;
 	bdi->model = id->driver_data;
+	bdi->pdata = client->dev.platform_data;
 	strncpy(bdi->model_name, id->name, I2C_NAME_SIZE);
 	mutex_init(&bdi->f_reg_lock);
 	bdi->f_reg = 0;
diff --git a/include/linux/power/bq24190_charger.h b/include/linux/power/bq24190_charger.h
index 9f02837..cb49717 100644
--- a/include/linux/power/bq24190_charger.h
+++ b/include/linux/power/bq24190_charger.h
@@ -11,6 +11,7 @@
 
 struct bq24190_platform_data {
 	unsigned int	gpio_int;	/* GPIO pin that's connected to INT# */
+	bool no_register_reset;
 };
 
 #endif
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1603477 — Re: [PATCH 04/15] power: supply: bq24190_charger: Add no_register_reset pdata flag

On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote:
> On some platforms the register have been setup with platform specific
> values by the firmware and should not be reset.
> 

I would not extend platform data at all.
For GPIO we may use GPIO lookup tables, for the rest -- unified (built-
in) device properties API.

Consider to get rid of 
 include/linux/power/bq24190_charger.h
completely.

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

[toc] | [prev] | [next] | [standalone]

#1603157 — [PATCH 07/15] power: supply: bq24190_charger: Add support for bq24192[i]

The bq24192 and bq24192i are mostly identical to the bq24190, TI even
published a single datasheet for all 3 of them. The difference
between the bq24190 and bq24192[i] is the way charger-type detection
is done, the bq24190 is to be directly connected to the USB a/b lines,
where as the the bq24192[i] has a gpio which should be driven high/low
externally depending on the type of charger connected, from a register
level access pov there is no difference.

The differences between the bq24192 and bq24192i are:
1) Lower default charge rate on the bq24192i
2) Pre-charge-current can be max 640 mA on the bq24192i

Since we do not provide an API for setting the pre-charge-current,
these differences can be ignored and we can simply use the existing
code as-is with the bq24192 and bq24192i.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 9c4b171..9014dee 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1275,7 +1275,14 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	if (ret < 0)
 		goto out;
 
-	if (v != bdi->model) {
+	switch (v) {
+	case BQ24190_REG_VPRS_PN_24190:
+	case BQ24190_REG_VPRS_PN_24192:
+	case BQ24190_REG_VPRS_PN_24192I:
+		bdi->model = v;
+		break;
+	default:
+		dev_err(bdi->dev, "Error unknown model: 0x%02x\n", v);
 		ret = -ENODEV;
 		goto out;
 	}
@@ -1316,7 +1323,6 @@ static int bq24190_probe(struct i2c_client *client,
 
 	bdi->client = client;
 	bdi->dev = dev;
-	bdi->model = id->driver_data;
 	bdi->pdata = client->dev.platform_data;
 	strncpy(bdi->model_name, id->name, I2C_NAME_SIZE);
 	mutex_init(&bdi->f_reg_lock);
@@ -1450,6 +1456,8 @@ static SIMPLE_DEV_PM_OPS(bq24190_pm_ops, bq24190_pm_suspend, bq24190_pm_resume);
  */
 static const struct i2c_device_id bq24190_i2c_ids[] = {
 	{ "bq24190", BQ24190_REG_VPRS_PN_24190 },
+	{ "bq24192", BQ24190_REG_VPRS_PN_24192 },
+	{ "bq24192i", BQ24190_REG_VPRS_PN_24192I },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1603158 — [PATCH 05/15] power: supply: bq24190_charger: Limit charging voltage to 4.3V

When the platform data asks us to not reset the charger to its default
values and instead trust the firmware set values check the charging
voltage and clamp it to 4.304V.

Some firmwares set really too high voltages, e.g. the GPD-win I've been
working on uses 4.384V. New LiHV (High Voltage) batteries may be charged
upto 4.35V but that significantly impacts their lifetime, limit charging
to 4.304V for safety and lifetime reasons.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index c92a40e4..7bca8d0 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -504,11 +504,32 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
 
 static int bq24190_register_reset(struct bq24190_dev_info *bdi)
 {
-	int ret, limit = 100;
+	int ret, voltage, limit = 100;
 	u8 v;
 
-	if (bdi->pdata && bdi->pdata->no_register_reset)
-		return 0;
+	if (bdi->pdata && bdi->pdata->no_register_reset) {
+		/*
+		 * We've been asked to keep the firmware settings as is, but
+		 * some firmwares set really too high voltages (e.g. 4.384V).
+		 * New LiHV (High Voltage) batteries may be charged upto 4.35V
+		 * but that significantly impacts their lifetime, limit
+		 * charging to 4.304V for safety and lifetime reasons.
+		 */
+		ret = bq24190_get_field_val(bdi, BQ24190_REG_CVC,
+			BQ24190_REG_CVC_VREG_MASK, BQ24190_REG_CVC_VREG_SHIFT,
+			bq24190_cvc_vreg_values,
+			ARRAY_SIZE(bq24190_cvc_vreg_values), &voltage);
+		if (ret < 0)
+			return ret;
+
+		if (voltage <= 4304000)
+			return 0;
+
+		return bq24190_set_field_val(bdi, BQ24190_REG_CVC,
+			BQ24190_REG_CVC_VREG_MASK, BQ24190_REG_CVC_VREG_SHIFT,
+			bq24190_cvc_vreg_values,
+			ARRAY_SIZE(bq24190_cvc_vreg_values), 4304000);
+	}
 
 	/* Reset the registers */
 	ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1603161 — [PATCH 09/15] power: supply: bq24190_charger: Add voltage_max_design prop to battery

When combined with an external fuel-gauge, upower needs voltage_max_design
as it internally does all its calculations in Watts and converts the
charge_foo properties from A to Watts by using voltage_max_design.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 9fe69a5..82cb33d 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1103,6 +1103,13 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
 		ret = 0;
 		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+		/*
+		 * Report charger configured voltage as max design voltage,
+		 * not entirely correct, but userspace needs something here.
+		 */
+		ret = bq24190_charger_get_voltage(bdi, val);
+		break;
 	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
 		ret = bq24190_battery_get_temp_alert_max(bdi, val);
 		break;
@@ -1169,6 +1176,7 @@ static enum power_supply_property bq24190_battery_properties[] = {
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
 	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
 	POWER_SUPPLY_PROP_SCOPE,
 	/* Begin of extended battery properties */
@@ -1186,7 +1194,7 @@ static const struct power_supply_desc bq24190_battery_desc = {
 	.name			= "bq24190-battery",
 	.type			= POWER_SUPPLY_TYPE_BATTERY,
 	.properties		= bq24190_battery_properties,
-	.num_properties		= 6,
+	.num_properties		= 7,
 	.get_property		= bq24190_battery_get_property,
 	.set_property		= bq24190_battery_set_property,
 	.property_is_writeable	= bq24190_battery_property_is_writeable,
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1604179 — Re: [PATCH 09/15] power: supply: bq24190_charger: Add voltage_max_design prop to battery

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Mar 17, 2017 at 10:55:21AM +0100, Hans de Goede wrote:
> When combined with an external fuel-gauge, upower needs voltage_max_design
> as it internally does all its calculations in Watts and converts the
> charge_foo properties from A to Watts by using voltage_max_design.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Assuming, that my comment on patch 14 works POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN
is required in the fuel-gauge driver instead. As far as I understand
it the charger voltage was too high for your device anyways, so that
you capped it at 4.3V. So I assume, that you can just provide it via
device properties.

-- Sebastian

>  drivers/power/supply/bq24190_charger.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 9fe69a5..82cb33d 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1103,6 +1103,13 @@ static int bq24190_battery_get_property(struct power_supply *psy,
>  		val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
>  		ret = 0;
>  		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> +		/*
> +		 * Report charger configured voltage as max design voltage,
> +		 * not entirely correct, but userspace needs something here.
> +		 */
> +		ret = bq24190_charger_get_voltage(bdi, val);
> +		break;
>  	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>  		ret = bq24190_battery_get_temp_alert_max(bdi, val);
>  		break;
> @@ -1169,6 +1176,7 @@ static enum power_supply_property bq24190_battery_properties[] = {
>  	POWER_SUPPLY_PROP_HEALTH,
>  	POWER_SUPPLY_PROP_ONLINE,
>  	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
>  	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
>  	POWER_SUPPLY_PROP_SCOPE,
>  	/* Begin of extended battery properties */
> @@ -1186,7 +1194,7 @@ static const struct power_supply_desc bq24190_battery_desc = {
>  	.name			= "bq24190-battery",
>  	.type			= POWER_SUPPLY_TYPE_BATTERY,
>  	.properties		= bq24190_battery_properties,
> -	.num_properties		= 6,
> +	.num_properties		= 7,
>  	.get_property		= bq24190_battery_get_property,
>  	.set_property		= bq24190_battery_set_property,
>  	.property_is_writeable	= bq24190_battery_property_is_writeable,
> -- 
> 2.9.3
> 

[toc] | [prev] | [next] | [standalone]

#1603168 — [PATCH 06/15] power: supply: bq24190_charger: Use i2c-core irq-mapping code

The i2c-core already maps of irqs before calling the driver's probe
function and there are no in tree users of
bq24190_platform_data->gpio_int.

Remove the redundant custom irq-mapping code and just use client->irq.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 61 ++--------------------------------
 include/linux/power/bq24190_charger.h  |  1 -
 2 files changed, 2 insertions(+), 60 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 7bca8d0..9c4b171 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -154,8 +154,6 @@ struct bq24190_dev_info {
 	struct bq24190_platform_data	*pdata;
 	char				model_name[I2C_NAME_SIZE];
 	kernel_ulong_t			model;
-	unsigned int			gpio_int;
-	unsigned int			irq;
 	struct mutex			f_reg_lock;
 	u8				f_reg;
 	u8				ss_reg;
@@ -1296,56 +1294,11 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	return ret;
 }
 
-#ifdef CONFIG_OF
-static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
-{
-	bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0);
-	if (bdi->irq <= 0)
-		return -1;
-
-	return 0;
-}
-#else
-static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
-{
-	return -1;
-}
-#endif
-
-static int bq24190_setup_pdata(struct bq24190_dev_info *bdi,
-		struct bq24190_platform_data *pdata)
-{
-	int ret;
-
-	if (!gpio_is_valid(pdata->gpio_int))
-		return -1;
-
-	ret = gpio_request(pdata->gpio_int, dev_name(bdi->dev));
-	if (ret < 0)
-		return -1;
-
-	ret = gpio_direction_input(pdata->gpio_int);
-	if (ret < 0)
-		goto out;
-
-	bdi->irq = gpio_to_irq(pdata->gpio_int);
-	if (!bdi->irq)
-		goto out;
-
-	bdi->gpio_int = pdata->gpio_int;
-	return 0;
-
-out:
-	gpio_free(pdata->gpio_int);
-	return -1;
-}
-
 static int bq24190_probe(struct i2c_client *client,
 		const struct i2c_device_id *id)
 {
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct device *dev = &client->dev;
-	struct bq24190_platform_data *pdata = client->dev.platform_data;
 	struct power_supply_config charger_cfg = {}, battery_cfg = {};
 	struct bq24190_dev_info *bdi;
 	int ret;
@@ -1372,12 +1325,7 @@ static int bq24190_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, bdi);
 
-	if (dev->of_node)
-		ret = bq24190_setup_dt(bdi);
-	else
-		ret = bq24190_setup_pdata(bdi, pdata);
-
-	if (ret) {
+	if (!client->irq) {
 		dev_err(dev, "Can't get irq info\n");
 		return -EINVAL;
 	}
@@ -1417,7 +1365,7 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out3;
 	}
 
-	ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
+	ret = devm_request_threaded_irq(dev, client->irq, NULL,
 			bq24190_irq_handler_thread,
 			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 			"bq24190-charger", bdi);
@@ -1439,8 +1387,6 @@ static int bq24190_probe(struct i2c_client *client,
 
 out1:
 	pm_runtime_disable(dev);
-	if (bdi->gpio_int)
-		gpio_free(bdi->gpio_int);
 	return ret;
 }
 
@@ -1457,9 +1403,6 @@ static int bq24190_remove(struct i2c_client *client)
 	power_supply_unregister(bdi->charger);
 	pm_runtime_disable(bdi->dev);
 
-	if (bdi->gpio_int)
-		gpio_free(bdi->gpio_int);
-
 	return 0;
 }
 
diff --git a/include/linux/power/bq24190_charger.h b/include/linux/power/bq24190_charger.h
index cb49717..8d918cb 100644
--- a/include/linux/power/bq24190_charger.h
+++ b/include/linux/power/bq24190_charger.h
@@ -10,7 +10,6 @@
 #define _BQ24190_CHARGER_H_
 
 struct bq24190_platform_data {
-	unsigned int	gpio_int;	/* GPIO pin that's connected to INT# */
 	bool no_register_reset;
 };
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1603473 — Re: [PATCH 06/15] power: supply: bq24190_charger: Use i2c-core irq-mapping code

On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote:
> The i2c-core already maps of irqs before calling the driver's probe
> function and there are no in tree users of
> bq24190_platform_data->gpio_int.
> 
> Remove the redundant custom irq-mapping code and just use client->irq.
> 

Nice! I did similar clean up to some other driver, so,

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

P.S. This should be first in the series against bq24190.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 61 ++-----------------------
> ---------
>  include/linux/power/bq24190_charger.h  |  1 -
>  2 files changed, 2 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c
> b/drivers/power/supply/bq24190_charger.c
> index 7bca8d0..9c4b171 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -154,8 +154,6 @@ struct bq24190_dev_info {
>  	struct bq24190_platform_data	*pdata;
>  	char				model_name[I2C_NAME_SIZE]
> ;
>  	kernel_ulong_t			model;
> -	unsigned int			gpio_int;
> -	unsigned int			irq;
>  	struct mutex			f_reg_lock;
>  	u8				f_reg;
>  	u8				ss_reg;
> @@ -1296,56 +1294,11 @@ static int bq24190_hw_init(struct
> bq24190_dev_info *bdi)
>  	return ret;
>  }
>  
> -#ifdef CONFIG_OF
> -static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
> -{
> -	bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0);
> -	if (bdi->irq <= 0)
> -		return -1;
> -
> -	return 0;
> -}
> -#else
> -static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
> -{
> -	return -1;
> -}
> -#endif
> -
> -static int bq24190_setup_pdata(struct bq24190_dev_info *bdi,
> -		struct bq24190_platform_data *pdata)
> -{
> -	int ret;
> -
> -	if (!gpio_is_valid(pdata->gpio_int))
> -		return -1;
> -
> -	ret = gpio_request(pdata->gpio_int, dev_name(bdi->dev));
> -	if (ret < 0)
> -		return -1;
> -
> -	ret = gpio_direction_input(pdata->gpio_int);
> -	if (ret < 0)
> -		goto out;
> -
> -	bdi->irq = gpio_to_irq(pdata->gpio_int);
> -	if (!bdi->irq)
> -		goto out;
> -
> -	bdi->gpio_int = pdata->gpio_int;
> -	return 0;
> -
> -out:
> -	gpio_free(pdata->gpio_int);
> -	return -1;
> -}
> -
>  static int bq24190_probe(struct i2c_client *client,
>  		const struct i2c_device_id *id)
>  {
>  	struct i2c_adapter *adapter = to_i2c_adapter(client-
> >dev.parent);
>  	struct device *dev = &client->dev;
> -	struct bq24190_platform_data *pdata = client-
> >dev.platform_data;
>  	struct power_supply_config charger_cfg = {}, battery_cfg =
> {};
>  	struct bq24190_dev_info *bdi;
>  	int ret;
> @@ -1372,12 +1325,7 @@ static int bq24190_probe(struct i2c_client
> *client,
>  
>  	i2c_set_clientdata(client, bdi);
>  
> -	if (dev->of_node)
> -		ret = bq24190_setup_dt(bdi);
> -	else
> -		ret = bq24190_setup_pdata(bdi, pdata);
> -
> -	if (ret) {
> +	if (!client->irq) {
>  		dev_err(dev, "Can't get irq info\n");
>  		return -EINVAL;
>  	}
> @@ -1417,7 +1365,7 @@ static int bq24190_probe(struct i2c_client
> *client,
>  		goto out3;
>  	}
>  
> -	ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
>  			bq24190_irq_handler_thread,
>  			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  			"bq24190-charger", bdi);
> @@ -1439,8 +1387,6 @@ static int bq24190_probe(struct i2c_client
> *client,
>  
>  out1:
>  	pm_runtime_disable(dev);
> -	if (bdi->gpio_int)
> -		gpio_free(bdi->gpio_int);
>  	return ret;
>  }
>  
> @@ -1457,9 +1403,6 @@ static int bq24190_remove(struct i2c_client
> *client)
>  	power_supply_unregister(bdi->charger);
>  	pm_runtime_disable(bdi->dev);
>  
> -	if (bdi->gpio_int)
> -		gpio_free(bdi->gpio_int);
> -
>  	return 0;
>  }
>  
> diff --git a/include/linux/power/bq24190_charger.h
> b/include/linux/power/bq24190_charger.h
> index cb49717..8d918cb 100644
> --- a/include/linux/power/bq24190_charger.h
> +++ b/include/linux/power/bq24190_charger.h
> @@ -10,7 +10,6 @@
>  #define _BQ24190_CHARGER_H_
>  
>  struct bq24190_platform_data {
> -	unsigned int	gpio_int;	/* GPIO pin that's
> connected to INT# */
>  	bool no_register_reset;
>  };
>  

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

[toc] | [prev] | [next] | [standalone]

#1604170 — Re: [PATCH 06/15] power: supply: bq24190_charger: Use i2c-core irq-mapping code

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Mar 17, 2017 at 07:24:14PM +0200, Andy Shevchenko wrote:
> On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote:
> > The i2c-core already maps of irqs before calling the driver's probe
> > function and there are no in tree users of
> > bq24190_platform_data->gpio_int.
> > 
> > Remove the redundant custom irq-mapping code and just use client->irq.
> > 
> 
> Nice! I did similar clean up to some other driver, so,
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> P.S. This should be first in the series against bq24190.

Right.

Acked-by: Sebastian Reichel <sre@kernel.org>

-- Sebastian

> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/power/supply/bq24190_charger.c | 61 ++-----------------------
> > ---------
> >  include/linux/power/bq24190_charger.h  |  1 -
> >  2 files changed, 2 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/power/supply/bq24190_charger.c
> > b/drivers/power/supply/bq24190_charger.c
> > index 7bca8d0..9c4b171 100644
> > --- a/drivers/power/supply/bq24190_charger.c
> > +++ b/drivers/power/supply/bq24190_charger.c
> > @@ -154,8 +154,6 @@ struct bq24190_dev_info {
> >  	struct bq24190_platform_data	*pdata;
> >  	char				model_name[I2C_NAME_SIZE]
> > ;
> >  	kernel_ulong_t			model;
> > -	unsigned int			gpio_int;
> > -	unsigned int			irq;
> >  	struct mutex			f_reg_lock;
> >  	u8				f_reg;
> >  	u8				ss_reg;
> > @@ -1296,56 +1294,11 @@ static int bq24190_hw_init(struct
> > bq24190_dev_info *bdi)
> >  	return ret;
> >  }
> >  
> > -#ifdef CONFIG_OF
> > -static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
> > -{
> > -	bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0);
> > -	if (bdi->irq <= 0)
> > -		return -1;
> > -
> > -	return 0;
> > -}
> > -#else
> > -static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
> > -{
> > -	return -1;
> > -}
> > -#endif
> > -
> > -static int bq24190_setup_pdata(struct bq24190_dev_info *bdi,
> > -		struct bq24190_platform_data *pdata)
> > -{
> > -	int ret;
> > -
> > -	if (!gpio_is_valid(pdata->gpio_int))
> > -		return -1;
> > -
> > -	ret = gpio_request(pdata->gpio_int, dev_name(bdi->dev));
> > -	if (ret < 0)
> > -		return -1;
> > -
> > -	ret = gpio_direction_input(pdata->gpio_int);
> > -	if (ret < 0)
> > -		goto out;
> > -
> > -	bdi->irq = gpio_to_irq(pdata->gpio_int);
> > -	if (!bdi->irq)
> > -		goto out;
> > -
> > -	bdi->gpio_int = pdata->gpio_int;
> > -	return 0;
> > -
> > -out:
> > -	gpio_free(pdata->gpio_int);
> > -	return -1;
> > -}
> > -
> >  static int bq24190_probe(struct i2c_client *client,
> >  		const struct i2c_device_id *id)
> >  {
> >  	struct i2c_adapter *adapter = to_i2c_adapter(client-
> > >dev.parent);
> >  	struct device *dev = &client->dev;
> > -	struct bq24190_platform_data *pdata = client-
> > >dev.platform_data;
> >  	struct power_supply_config charger_cfg = {}, battery_cfg =
> > {};
> >  	struct bq24190_dev_info *bdi;
> >  	int ret;
> > @@ -1372,12 +1325,7 @@ static int bq24190_probe(struct i2c_client
> > *client,
> >  
> >  	i2c_set_clientdata(client, bdi);
> >  
> > -	if (dev->of_node)
> > -		ret = bq24190_setup_dt(bdi);
> > -	else
> > -		ret = bq24190_setup_pdata(bdi, pdata);
> > -
> > -	if (ret) {
> > +	if (!client->irq) {
> >  		dev_err(dev, "Can't get irq info\n");
> >  		return -EINVAL;
> >  	}
> > @@ -1417,7 +1365,7 @@ static int bq24190_probe(struct i2c_client
> > *client,
> >  		goto out3;
> >  	}
> >  
> > -	ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
> > +	ret = devm_request_threaded_irq(dev, client->irq, NULL,
> >  			bq24190_irq_handler_thread,
> >  			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> >  			"bq24190-charger", bdi);
> > @@ -1439,8 +1387,6 @@ static int bq24190_probe(struct i2c_client
> > *client,
> >  
> >  out1:
> >  	pm_runtime_disable(dev);
> > -	if (bdi->gpio_int)
> > -		gpio_free(bdi->gpio_int);
> >  	return ret;
> >  }
> >  
> > @@ -1457,9 +1403,6 @@ static int bq24190_remove(struct i2c_client
> > *client)
> >  	power_supply_unregister(bdi->charger);
> >  	pm_runtime_disable(bdi->dev);
> >  
> > -	if (bdi->gpio_int)
> > -		gpio_free(bdi->gpio_int);
> > -
> >  	return 0;
> >  }
> >  
> > diff --git a/include/linux/power/bq24190_charger.h
> > b/include/linux/power/bq24190_charger.h
> > index cb49717..8d918cb 100644
> > --- a/include/linux/power/bq24190_charger.h
> > +++ b/include/linux/power/bq24190_charger.h
> > @@ -10,7 +10,6 @@
> >  #define _BQ24190_CHARGER_H_
> >  
> >  struct bq24190_platform_data {
> > -	unsigned int	gpio_int;	/* GPIO pin that's
> > connected to INT# */
> >  	bool no_register_reset;
> >  };
> >  
> 
> -- 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1603169 — [PATCH 08/15] power: supply: bq24190_charger: Add support for external fuel gauge

Some platforms with a bq24190_charger have an external fuel gauge which
makes it possible to reliably report battery (dis)charge state, at
support for this by adding an optional get_ext_bat_property callback
to the platform_data and using this if the platform provides it.

By convention the callback will return -ENXIO when it is not ready yet,
or the driver providing it has been unbound from its device. Since it
returns the same error when unbound it cannot return -EPROBE_DEFER
as that is not a valid errno.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 41 +++++++++++++++++++++++++++++++---
 include/linux/power/bq24190_charger.h  |  4 ++++
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 9014dee..9fe69a5 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1111,7 +1111,10 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = 0;
 		break;
 	default:
-		ret = -ENODATA;
+		if (bdi->pdata && bdi->pdata->get_ext_bat_property)
+			ret = bdi->pdata->get_ext_bat_property(psp, val);
+		else
+			ret = -ENODATA;
 	}
 
 	pm_runtime_put_sync(bdi->dev);
@@ -1168,12 +1171,31 @@ static enum power_supply_property bq24190_battery_properties[] = {
 	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
 	POWER_SUPPLY_PROP_SCOPE,
+	/* Begin of extended battery properties */
+	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,
 };
 
 static const struct power_supply_desc bq24190_battery_desc = {
 	.name			= "bq24190-battery",
 	.type			= POWER_SUPPLY_TYPE_BATTERY,
 	.properties		= bq24190_battery_properties,
+	.num_properties		= 6,
+	.get_property		= bq24190_battery_get_property,
+	.set_property		= bq24190_battery_set_property,
+	.property_is_writeable	= bq24190_battery_property_is_writeable,
+};
+
+static const struct power_supply_desc bq24190_ext_battery_desc = {
+	.name			= "bq24190-battery",
+	.type			= POWER_SUPPLY_TYPE_BATTERY,
+	.properties		= bq24190_battery_properties,
 	.num_properties		= ARRAY_SIZE(bq24190_battery_properties),
 	.get_property		= bq24190_battery_get_property,
 	.set_property		= bq24190_battery_set_property,
@@ -1336,6 +1358,15 @@ static int bq24190_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
+	if (bdi->pdata && bdi->pdata->get_ext_bat_property) {
+		union power_supply_propval val;
+
+		/* Check external fuel gauge is ready */
+		ret = bdi->pdata->get_ext_bat_property(0, &val);
+		if (ret == -ENXIO)
+			return -EPROBE_DEFER;
+	}
+
 	pm_runtime_enable(dev);
 	pm_runtime_resume(dev);
 
@@ -1357,8 +1388,12 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	battery_cfg.drv_data = bdi;
-	bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
-						&battery_cfg);
+	if (bdi->pdata && bdi->pdata->get_ext_bat_property)
+		bdi->battery = power_supply_register(dev,
+				    &bq24190_ext_battery_desc, &battery_cfg);
+	else
+		bdi->battery = power_supply_register(dev,
+				    &bq24190_battery_desc, &battery_cfg);
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
diff --git a/include/linux/power/bq24190_charger.h b/include/linux/power/bq24190_charger.h
index 8d918cb..02d248b 100644
--- a/include/linux/power/bq24190_charger.h
+++ b/include/linux/power/bq24190_charger.h
@@ -9,8 +9,12 @@
 #ifndef _BQ24190_CHARGER_H_
 #define _BQ24190_CHARGER_H_
 
+#include <linux/power_supply.h>
+
 struct bq24190_platform_data {
 	bool no_register_reset;
+	int (*get_ext_bat_property)(enum power_supply_property prop,
+				    union power_supply_propval *val);
 };
 
 #endif
-- 
2.9.3

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web