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

[Hans de Goede]

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 = 50;
+   else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) ==
1 ||
+extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) ==
1)
+   iinlim = 150;
+   else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) ==
1)
+   iinlim = 200;
+



+   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

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

[Hans de Goede]

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 = 50;
+   else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) ==
1 ||
+extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) ==
1)
+   iinlim = 150;
+   else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) ==
1)
+   iinlim = 200;
+



+   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

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

[Sebastian Reichel]

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 

Acked-By: Sebastian Reichel 

-- 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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -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_CHARGE1
> +#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_SHIFT1
>  #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;
>   charmodel_name[I2C_NAME_SIZE];
>   kernel_ulong_t  model;
>   struct mutexf_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[] = {
> + 10, 15, 50, 90, 120, 150, 200, 300 };
> +
>  /* REG02[7:2] (ICHG) in uAh */
>  static const int bq24190_ccc_ichg_values[] = {
>512000,  576000,  64,  704000,  768000,  832000,  896000,  96,
> @@ -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 = 50;
> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
> +  extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
> + iinlim = 150;
> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
> + iinlim = 200;
> +
> + 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,
> +  

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

[Sebastian Reichel]

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 

Acked-By: Sebastian Reichel 

-- 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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -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_CHARGE1
> +#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_SHIFT1
>  #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;
>   charmodel_name[I2C_NAME_SIZE];
>   kernel_ulong_t  model;
>   struct mutexf_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[] = {
> + 10, 15, 50, 90, 120, 150, 200, 300 };
> +
>  /* REG02[7:2] (ICHG) in uAh */
>  static const int bq24190_ccc_ichg_values[] = {
>512000,  576000,  64,  704000,  768000,  832000,  896000,  96,
> @@ -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 = 50;
> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
> +  extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
> + iinlim = 150;
> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
> + iinlim = 200;
> +
> + 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,
> +  

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

[Andy Shevchenko]

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".

> +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 = 50;
> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) ==
> 1 ||
> +  extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) ==
> 1)
> + iinlim = 150;
> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) ==
> 1)
> + iinlim = 200;
> +

> + if (iinlim) {

Could be possible to call below unconditionally here (use 0)?

> + 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).

-- 
Andy Shevchenko 
Intel Finland Oy

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

[Andy Shevchenko]

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".

> +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 = 50;
> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) ==
> 1 ||
> +  extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) ==
> 1)
> + iinlim = 150;
> + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) ==
> 1)
> + iinlim = 200;
> +

> + if (iinlim) {

Could be possible to call below unconditionally here (use 0)?

> + 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).

-- 
Andy Shevchenko 
Intel Finland Oy