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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 18-03-17 20:57, Liam Breck wrote:
> On Sat, Mar 18, 2017 at 7:42 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 18-03-17 08:10, Liam Breck wrote:
>>>
>>> Tony, question for you below...
>>>
>>> On Fri, 17 Mar 2017 10:55:22 +0100, Hans de Goede wrote:
>>>
>>>> Add support for monitoring an extcon device with SDP/CDP/DCP and HOST
>>>
>>>
>>> insert: USB X.0
>>>
>>>> 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.
>>>
>>>
>>> This description is rather thin for the extent of the changes. What
>>> specific systems
>>> require this?
>>
>>
>> Systems which use a variant with a PSEL pin where the PSEL pin is hardwired
>> high.
>
> I gathered that :-p  It's illuminating to include detailed background
> in a patch description; please paint us a picture :-)
>
> Also you can set IINLIM via sysfs, so consider doing this from the
> PMIC driver, or pseudo-driver.

sysfs is a userspace API not an in kernel API (there maybe some ways
around that but that is not a good idea in general).

Also if anything is a hack it is the entire BQ24190 sysfs interface
I understand is useful for driver debugging (although not that
useful as we also have i2cdump / i2cset / i2cget in userspace),
but it really belongs behind a DEBUG #ifdef.

> I don't see that BQ24190 generally needs to handle extcon events.

Yet charger drivers handling extcon events to determine how much
current they can draw is a design pattern we already see in use
in the kernel:

[hans@shalem linux]$ grep -l extcon drivers/power/supply/*.c
drivers/power/supply/axp288_charger.c
drivers/power/supply/bq24190_charger.c
drivers/power/supply/charger-manager.c
drivers/power/supply/qcom_smbb.c

Regards,

Hans


>
>>> What drivers generate the event?
>>
>>
>> The driver which registers the extcon with the name "extcon_name"
>> the bq24190_charger driver really does not need to know more.
>> In the use-case we are talking about now the driver is
>> the new extcon-cht-wc.c driver which is part of the patch-set
>> as I initially posted it.
>>
>>> Also provide URLs to docs/datasheets.
>>
>>
>> I've no access to docs / datasheets for the Whiskey Cove PMIC.
>>
>>
>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  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)
>>>> +{
>>>
>>>
>>> Does this need pm_runtime_get_sync(), etc?
>>> (See linux-next patches to driver.)
>>
>>
>> Yes with runtime pm this will need to do a
>> pm_runtime_get_sync(bdi->Dev) before accessing
>> the device.
>>
>>
>>
>>
>>>
>>>> +       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) {
>>>
>>>
>>> Verify that extcon_name has an expected value?
>>
>>
>> The whole idea is that the bq24190 does not need to care about
>> which extcon driver it is told to use, just that it has to
>> use *a* extcon driver.
>>
>>>
>>>> +               bdi->extcon =
>>>> extcon_get_extcon_dev(bdi->pdata->extcon_name);
>>>> +               if (!bdi->extcon)
>>>> +                       return -EPROBE_DEFER;
>>>
>>>
>>> dev_info() a msg about this linkage, including info about the other side.
>>
>>
>> dev_info on success I assume, not on PROBE_DEFER, otherwise we will
>> end up spamming the log quite a lot while modules are loading.
>>
>>
>>
>>
>>>
>>>> +       }
>>>> +
>>>>         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);
>>>>  };
>>>
>>>
>>
>> Regards,
>>
>> Hans