| Message ID | 20170323083235.15072-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, On Thu, Mar 23, 2017 at 09:32:35AM +0100, Hans de Goede wrote: > Add support for monitoring an extcon device with USB 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, as well as on systems where the 5V boost converter is used, as > that always needs to be enabled from software. > > Cc: Liam Breck <kernel@networkimprov.net> > Cc: Tony Lindgren <tony@atomide.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Acked-by: Sebastian Reichel <sre@kernel.org> > --- > Changes in v2: > -Use a device-property for extcon-name instead of platform_data > -Delay 300ms before applying the extcon detected charger-type to iinlim > (see the added comment for why this is done) > Changes in v3: > -Print a msg with dev_info when an extcon is used do determine iinlim > --- Thanks, queued. -- Sebastian
On Thu, Mar 23, 2017 at 1:32 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Add support for monitoring an extcon device with USB 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, as well as on systems where the 5V boost converter is used, as > that always needs to be enabled from software. > > Cc: Liam Breck <kernel@networkimprov.net> > Cc: Tony Lindgren <tony@atomide.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Acked-by: Sebastian Reichel <sre@kernel.org> I don't have a way to test this, so I won't ack it, but I'm not opposed since it's hidden behind a property. > --- > Changes in v2: > -Use a device-property for extcon-name instead of platform_data > -Delay 300ms before applying the extcon detected charger-type to iinlim > (see the added comment for why this is done) > Changes in v3: > -Print a msg with dev_info when an extcon is used do determine iinlim > --- > drivers/power/supply/Kconfig | 1 + > drivers/power/supply/bq24190_charger.c | 109 +++++++++++++++++++++++++++++++++ > 2 files changed, 110 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 d74c8cc..2c404a5 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> > > @@ -36,6 +38,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) > @@ -148,6 +152,9 @@ struct bq24190_dev_info { > struct device *dev; > struct power_supply *charger; > struct power_supply *battery; > + struct extcon_dev *extcon; > + struct notifier_block extcon_nb; > + struct delayed_work extcon_work; > char model_name[I2C_NAME_SIZE]; > bool initialized; > bool irq_event; > @@ -164,6 +171,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, > @@ -1277,6 +1289,78 @@ 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.work); > + int ret, iinlim = 0; > + > + ret = pm_runtime_get_sync(bdi->dev); > + if (ret < 0) { > + dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret); > + return; > + } > + > + 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); > + > + pm_runtime_mark_last_busy(bdi->dev); > + pm_runtime_put_autosuspend(bdi->dev); > +} > + > +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); > + > + /* > + * The Power-Good detection may take up to 220ms, sometimes > + * the external charger detection is quicker, and the bq24190 will > + * reset to iinlim based on its own charger detection (which is not > + * hooked up when using external charger detection) resulting in > + * a too low default 500mA iinlim. Delay applying the extcon value > + * for 300ms to avoid this. > + */ > + queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300)); > + > + return NOTIFY_OK; > +} > + > static int bq24190_hw_init(struct bq24190_dev_info *bdi) > { > u8 v; > @@ -1314,6 +1398,7 @@ static int bq24190_probe(struct i2c_client *client, > struct device *dev = &client->dev; > struct power_supply_config charger_cfg = {}, battery_cfg = {}; > struct bq24190_dev_info *bdi; > + const char *name; > int ret; > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { > @@ -1341,6 +1426,18 @@ static int bq24190_probe(struct i2c_client *client, > return -EINVAL; > } > > + /* > + * The extcon-name property is purely an in kernel interface for > + * x86/ACPI use, DT platforms should get extcon via phandle. > + */ > + if (device_property_read_string(dev, "extcon-name", &name) == 0) { > + bdi->extcon = extcon_get_extcon_dev(name); > + if (!bdi->extcon) > + return -EPROBE_DEFER; > + > + dev_info(bdi->dev, "using extcon device %s\n", name); > + } > + > pm_runtime_enable(dev); > pm_runtime_use_autosuspend(dev); > pm_runtime_set_autosuspend_delay(dev, 600); > @@ -1391,6 +1488,18 @@ static int bq24190_probe(struct i2c_client *client, > goto out5; > } > > + if (bdi->extcon) { > + INIT_DELAYED_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 out5; > + > + /* Sync initial cable state */ > + queue_delayed_work(system_wq, &bdi->extcon_work, 0); > + } > + > enable_irq_wake(client->irq); > > pm_runtime_mark_last_busy(dev); > -- > 2.9.3 >
Sebastian, this patch needs some work, could you drop it from -next and look for v4 from Hans? Hans, I regret I couldn't study this sooner, but 5-day turnaround isn't too bad :-) I will send a v3 of "Uniform pm_runtime_get ..." not rebased to this. On Thu, Mar 23, 2017 at 1:32 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Add support for monitoring an extcon device with USB 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, as well as on systems where the 5V boost converter is used, as > that always needs to be enabled from software. > > Cc: Liam Breck <kernel@networkimprov.net> > Cc: Tony Lindgren <tony@atomide.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Acked-by: Sebastian Reichel <sre@kernel.org> > --- > Changes in v2: > -Use a device-property for extcon-name instead of platform_data > -Delay 300ms before applying the extcon detected charger-type to iinlim > (see the added comment for why this is done) > Changes in v3: > -Print a msg with dev_info when an extcon is used do determine iinlim > --- > drivers/power/supply/Kconfig | 1 + > drivers/power/supply/bq24190_charger.c | 109 +++++++++++++++++++++++++++++++++ > 2 files changed, 110 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 d74c8cc..2c404a5 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> > > @@ -36,6 +38,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 Place these at the end of the reg_poc section, so the mask & shift names are together. > #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) > @@ -148,6 +152,9 @@ struct bq24190_dev_info { > struct device *dev; > struct power_supply *charger; > struct power_supply *battery; > + struct extcon_dev *extcon; > + struct notifier_block extcon_nb; > + struct delayed_work extcon_work; > char model_name[I2C_NAME_SIZE]; > bool initialized; > bool irq_event; > @@ -164,6 +171,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 }; Correct name is bq24190_isc_iinlim_values, following convention of others. > /* REG02[7:2] (ICHG) in uAh */ > static const int bq24190_ccc_ichg_values[] = { > 512000, 576000, 640000, 704000, 768000, 832000, 896000, 960000, > @@ -1277,6 +1289,78 @@ 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.work); > + int ret, iinlim = 0; > + > + ret = pm_runtime_get_sync(bdi->dev); > + if (ret < 0) { > + dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret); Needs pm_runtime_put_noidle() Use same error msg as other pm_runtime_get_sync failures. > + return; > + } > + > + 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) ret < 0 > + 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. > + */ u8 val = BQ24190_REG_POC_CHG_CONFIG_CHARGE; > + if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1) { if (above) val = BQ24190_REG_POC_CHG_CONFIG_OTG; Then call write_mask once, with val. > + 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) ret < 0 > + dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret); > + > + pm_runtime_mark_last_busy(bdi->dev); > + pm_runtime_put_autosuspend(bdi->dev); > +} > + > +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); > + > + /* > + * The Power-Good detection may take up to 220ms, sometimes > + * the external charger detection is quicker, and the bq24190 will > + * reset to iinlim based on its own charger detection (which is not > + * hooked up when using external charger detection) resulting in > + * a too low default 500mA iinlim. Delay applying the extcon value > + * for 300ms to avoid this. > + */ > + queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300)); > + > + return NOTIFY_OK; > +} > + > static int bq24190_hw_init(struct bq24190_dev_info *bdi) > { > u8 v; > @@ -1314,6 +1398,7 @@ static int bq24190_probe(struct i2c_client *client, > struct device *dev = &client->dev; > struct power_supply_config charger_cfg = {}, battery_cfg = {}; > struct bq24190_dev_info *bdi; > + const char *name; > int ret; > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { > @@ -1341,6 +1426,18 @@ static int bq24190_probe(struct i2c_client *client, > return -EINVAL; > } > > + /* > + * The extcon-name property is purely an in kernel interface for > + * x86/ACPI use, DT platforms should get extcon via phandle. > + */ > + if (device_property_read_string(dev, "extcon-name", &name) == 0) { Is the "extcon-name" in-kernel interface documented, or a convention, or custom to your gauge/setup? > + bdi->extcon = extcon_get_extcon_dev(name); > + if (!bdi->extcon) > + return -EPROBE_DEFER; > + > + dev_info(bdi->dev, "using extcon device %s\n", name); > + } > + > pm_runtime_enable(dev); > pm_runtime_use_autosuspend(dev); > pm_runtime_set_autosuspend_delay(dev, 600); > @@ -1391,6 +1488,18 @@ static int bq24190_probe(struct i2c_client *client, > goto out5; > } > > + if (bdi->extcon) { > + INIT_DELAYED_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 out5; Needs a dev_err msg. Also out5 changes to out_sysfs in my recent patch. > + /* Sync initial cable state */ > + queue_delayed_work(system_wq, &bdi->extcon_work, 0); > + } > + > enable_irq_wake(client->irq); > > pm_runtime_mark_last_busy(dev); > -- > 2.9.3 >
Hi, On 29-03-17 03:12, Liam Breck wrote: > Sebastian, this patch needs some work, could you drop it from -next > and look for v4 from Hans? Nack. Liam this whole dropping patches from -next business is not how things are normally done in kernel-land, please stop expecting it. The whole dropping patches all the time thing means people don't have a stable base to base future patches on which is unworkable. > Hans, I regret I couldn't study this sooner, but 5-day turnaround > isn't too bad :-) You actually already reviewed v2 which is why v3 has: >> Changes in v3: >> -Print a msg with dev_info when an extcon is used do determine iinlim As you requested, so it is a bit weird to now ask for a version with the changes you requested to be dropped because you decided you want more changes now... > I will send a v3 of "Uniform pm_runtime_get ..." not rebased to this. Please base it on top instead and also fix the runtime_pm_get_sync() error-handling in the newly added function, that was copy pasted from what is in hindsight a bad example, so fixing it in the same patch makes sense. > On Thu, Mar 23, 2017 at 1:32 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Add support for monitoring an extcon device with USB 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, as well as on systems where the 5V boost converter is used, as >> that always needs to be enabled from software. >> >> Cc: Liam Breck <kernel@networkimprov.net> >> Cc: Tony Lindgren <tony@atomide.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> Acked-by: Sebastian Reichel <sre@kernel.org> >> --- >> Changes in v2: >> -Use a device-property for extcon-name instead of platform_data >> -Delay 300ms before applying the extcon detected charger-type to iinlim >> (see the added comment for why this is done) >> Changes in v3: >> -Print a msg with dev_info when an extcon is used do determine iinlim >> --- >> drivers/power/supply/Kconfig | 1 + >> drivers/power/supply/bq24190_charger.c | 109 +++++++++++++++++++++++++++++++++ >> 2 files changed, 110 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 d74c8cc..2c404a5 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> >> >> @@ -36,6 +38,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 > > Place these at the end of the reg_poc section, so the mask & shift > names are together. > >> #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) >> @@ -148,6 +152,9 @@ struct bq24190_dev_info { >> struct device *dev; >> struct power_supply *charger; >> struct power_supply *battery; >> + struct extcon_dev *extcon; >> + struct notifier_block extcon_nb; >> + struct delayed_work extcon_work; >> char model_name[I2C_NAME_SIZE]; >> bool initialized; >> bool irq_event; >> @@ -164,6 +171,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 }; > > Correct name is bq24190_isc_iinlim_values, following convention of others. > >> /* REG02[7:2] (ICHG) in uAh */ >> static const int bq24190_ccc_ichg_values[] = { >> 512000, 576000, 640000, 704000, 768000, 832000, 896000, 960000, >> @@ -1277,6 +1289,78 @@ 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.work); >> + int ret, iinlim = 0; >> + >> + ret = pm_runtime_get_sync(bdi->dev); >> + if (ret < 0) { >> + dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret); > > Needs pm_runtime_put_noidle() > Use same error msg as other pm_runtime_get_sync failures. This should be fixed in a v3 of your: "power: bq24190_charger: Uniform pm_runtime_get() failure handling" patch. > >> + return; >> + } >> + >> + 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) > > ret < 0 > >> + 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. >> + */ > > u8 val = BQ24190_REG_POC_CHG_CONFIG_CHARGE; > >> + if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1) { > > if (above) val = BQ24190_REG_POC_CHG_CONFIG_OTG; > > Then call write_mask once, with val. Good idea, but actually I found out by booting my board with a host-cable (USB-C equivalent of id-pin connected to ground) plugged in that there is a second 5v boost converter on the board and that that-one gets used by the firmware, so I will submit a patch soon-ish dropping the EXTCON_USB_HOST handling all together. As Sebastian already said (I believe) it would be best to export the 5V boost converter as a regulator, but that can wait until we actually need it somewhere. > >> + 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) > > ret < 0 > >> + dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret); >> + >> + pm_runtime_mark_last_busy(bdi->dev); >> + pm_runtime_put_autosuspend(bdi->dev); >> +} >> + >> +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); >> + >> + /* >> + * The Power-Good detection may take up to 220ms, sometimes >> + * the external charger detection is quicker, and the bq24190 will >> + * reset to iinlim based on its own charger detection (which is not >> + * hooked up when using external charger detection) resulting in >> + * a too low default 500mA iinlim. Delay applying the extcon value >> + * for 300ms to avoid this. >> + */ >> + queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300)); >> + >> + return NOTIFY_OK; >> +} >> + >> static int bq24190_hw_init(struct bq24190_dev_info *bdi) >> { >> u8 v; >> @@ -1314,6 +1398,7 @@ static int bq24190_probe(struct i2c_client *client, >> struct device *dev = &client->dev; >> struct power_supply_config charger_cfg = {}, battery_cfg = {}; >> struct bq24190_dev_info *bdi; >> + const char *name; >> int ret; >> >> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { >> @@ -1341,6 +1426,18 @@ static int bq24190_probe(struct i2c_client *client, >> return -EINVAL; >> } >> >> + /* >> + * The extcon-name property is purely an in kernel interface for >> + * x86/ACPI use, DT platforms should get extcon via phandle. >> + */ >> + if (device_property_read_string(dev, "extcon-name", &name) == 0) { > > Is the "extcon-name" in-kernel interface documented, or a convention, > or custom to your gauge/setup? It is documented right above the use of it :) Since this is the first case of passing an extcon-name through a device property AFAIK we are starting the convention of doing this this way I guess. >> + bdi->extcon = extcon_get_extcon_dev(name); >> + if (!bdi->extcon) >> + return -EPROBE_DEFER; >> + >> + dev_info(bdi->dev, "using extcon device %s\n", name); >> + } >> + >> pm_runtime_enable(dev); >> pm_runtime_use_autosuspend(dev); >> pm_runtime_set_autosuspend_delay(dev, 600); >> @@ -1391,6 +1488,18 @@ static int bq24190_probe(struct i2c_client *client, >> goto out5; >> } >> >> + if (bdi->extcon) { >> + INIT_DELAYED_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 out5; > > Needs a dev_err msg. Also out5 changes to out_sysfs in my recent patch. Again you should rebase your patch on top of next, not expect me to do a new version of my already merged patch, and then fix this up in your new version. Regards, Hans > >> + /* Sync initial cable state */ >> + queue_delayed_work(system_wq, &bdi->extcon_work, 0); >> + } >> + >> enable_irq_wake(client->irq); >> >> pm_runtime_mark_last_busy(dev); >> -- >> 2.9.3 >>
On Wed, Mar 29, 2017 at 12:34 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 29-03-17 03:12, Liam Breck wrote: >> >> Sebastian, this patch needs some work, could you drop it from -next >> and look for v4 from Hans? > > > Nack. > > Liam this whole dropping patches from -next business is not how > things are normally done in kernel-land, please stop expecting it. > > The whole dropping patches all the time thing means people don't have > a stable base to base future patches on which is unworkable. This was merged 3h after you posted it without acks from Tony or me. So maybe asking for a do-over in this case isn't unreasonable :-) Another of your patches has a replacement pending already. We'd both benefit from a clean history wherever possible. And new-feature patches should really simmer on the list for a little while. >> Hans, I regret I couldn't study this sooner, but 5-day turnaround >> isn't too bad :-) > > > You actually already reviewed v2 which is why v3 has: > >>> Changes in v3: >>> -Print a msg with dev_info when an extcon is used do determine iinlim > > As you requested, so it is a bit weird to now ask for a version with > the changes you requested to be dropped because you decided you want > more changes now... I took a quick look at v2. I didn't have a chance to respond to v3 before it landed in -next. >> I will send a v3 of "Uniform pm_runtime_get ..." not rebased to this. > > > Please base it on top instead and also fix the runtime_pm_get_sync() > error-handling in the newly added function, that was copy pasted from > what is in hindsight a bad example, so fixing it in the same patch > makes sense. The v2 of "Uniform..." I just posted is actually based on -next, but doesn't touch your code, so has to be redone itself. >> On Thu, Mar 23, 2017 at 1:32 AM, Hans de Goede <hdegoede@redhat.com> >> wrote: >>> >>> Add support for monitoring an extcon device with USB 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, as well as on systems where the 5V boost converter is used, as >>> that always needs to be enabled from software. >>> >>> Cc: Liam Breck <kernel@networkimprov.net> >>> Cc: Tony Lindgren <tony@atomide.com> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> Acked-by: Sebastian Reichel <sre@kernel.org> >>> --- >>> Changes in v2: >>> -Use a device-property for extcon-name instead of platform_data >>> -Delay 300ms before applying the extcon detected charger-type to iinlim >>> (see the added comment for why this is done) >>> Changes in v3: >>> -Print a msg with dev_info when an extcon is used do determine iinlim >>> --- >>> drivers/power/supply/Kconfig | 1 + >>> drivers/power/supply/bq24190_charger.c | 109 >>> +++++++++++++++++++++++++++++++++ >>> 2 files changed, 110 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 d74c8cc..2c404a5 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> >>> >>> @@ -36,6 +38,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 >> >> >> Place these at the end of the reg_poc section, so the mask & shift >> names are together. >> >>> #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) >>> @@ -148,6 +152,9 @@ struct bq24190_dev_info { >>> struct device *dev; >>> struct power_supply *charger; >>> struct power_supply *battery; >>> + struct extcon_dev *extcon; >>> + struct notifier_block extcon_nb; >>> + struct delayed_work extcon_work; >>> char model_name[I2C_NAME_SIZE]; >>> bool initialized; >>> bool irq_event; >>> @@ -164,6 +171,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 }; >> >> >> Correct name is bq24190_isc_iinlim_values, following convention of others. >> >>> /* REG02[7:2] (ICHG) in uAh */ >>> static const int bq24190_ccc_ichg_values[] = { >>> 512000, 576000, 640000, 704000, 768000, 832000, 896000, >>> 960000, >>> @@ -1277,6 +1289,78 @@ 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.work); >>> + int ret, iinlim = 0; >>> + >>> + ret = pm_runtime_get_sync(bdi->dev); >>> + if (ret < 0) { >>> + dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", >>> ret); >> >> >> Needs pm_runtime_put_noidle() >> Use same error msg as other pm_runtime_get_sync failures. > > > This should be fixed in a v3 of your: > "power: bq24190_charger: Uniform pm_runtime_get() failure handling" > patch. > > > >> >>> + return; >>> + } >>> + >>> + 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) >> >> >> ret < 0 >> >>> + 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. >>> + */ >> >> >> u8 val = BQ24190_REG_POC_CHG_CONFIG_CHARGE; >> >>> + if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == >>> 1) { >> >> >> if (above) val = BQ24190_REG_POC_CHG_CONFIG_OTG; >> >> Then call write_mask once, with val. > > > Good idea, but actually I found out by booting my board with a host-cable > (USB-C equivalent of id-pin connected to ground) plugged in that there is > a second 5v boost converter on the board and that that-one gets used by > the firmware, so I will submit a patch soon-ish dropping the EXTCON_USB_HOST > handling all together. Since you're planning this fix anyway, perhaps re-do this patch with that change and my feedback? > As Sebastian already said (I believe) it would be best to export the 5V > boost > converter as a regulator, but that can wait until we actually need it > somewhere. > > >> >>> + 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) >> >> >> ret < 0 >> >>> + dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret); >>> + >>> + pm_runtime_mark_last_busy(bdi->dev); >>> + pm_runtime_put_autosuspend(bdi->dev); >>> +} >>> + >>> +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); >>> + >>> + /* >>> + * The Power-Good detection may take up to 220ms, sometimes >>> + * the external charger detection is quicker, and the bq24190 >>> will >>> + * reset to iinlim based on its own charger detection (which is >>> not >>> + * hooked up when using external charger detection) resulting in >>> + * a too low default 500mA iinlim. Delay applying the extcon >>> value >>> + * for 300ms to avoid this. >>> + */ >>> + queue_delayed_work(system_wq, &bdi->extcon_work, >>> msecs_to_jiffies(300)); >>> + >>> + return NOTIFY_OK; >>> +} >>> + >>> static int bq24190_hw_init(struct bq24190_dev_info *bdi) >>> { >>> u8 v; >>> @@ -1314,6 +1398,7 @@ static int bq24190_probe(struct i2c_client *client, >>> struct device *dev = &client->dev; >>> struct power_supply_config charger_cfg = {}, battery_cfg = {}; >>> struct bq24190_dev_info *bdi; >>> + const char *name; >>> int ret; >>> >>> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) >>> { >>> @@ -1341,6 +1426,18 @@ static int bq24190_probe(struct i2c_client >>> *client, >>> return -EINVAL; >>> } >>> >>> + /* >>> + * The extcon-name property is purely an in kernel interface for >>> + * x86/ACPI use, DT platforms should get extcon via phandle. >>> + */ >>> + if (device_property_read_string(dev, "extcon-name", &name) == 0) >>> { >> >> >> Is the "extcon-name" in-kernel interface documented, or a convention, >> or custom to your gauge/setup? > > > It is documented right above the use of it :) Since this is the first > case of passing an extcon-name through a device property AFAIK we > are starting the convention of doing this this way I guess. Is there somewhere we could document this convention where others might find it? :-) >>> + bdi->extcon = extcon_get_extcon_dev(name); >>> + if (!bdi->extcon) >>> + return -EPROBE_DEFER; >>> + >>> + dev_info(bdi->dev, "using extcon device %s\n", name); >>> + } >>> + >>> pm_runtime_enable(dev); >>> pm_runtime_use_autosuspend(dev); >>> pm_runtime_set_autosuspend_delay(dev, 600); >>> @@ -1391,6 +1488,18 @@ static int bq24190_probe(struct i2c_client >>> *client, >>> goto out5; >>> } >>> >>> + if (bdi->extcon) { >>> + INIT_DELAYED_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 out5; >> >> >> Needs a dev_err msg. Also out5 changes to out_sysfs in my recent patch. > > > Again you should rebase your patch on top of next, not expect me to > do a new version of my already merged patch, and then fix this up > in your new version. > > Regards, > > Hans > > > >> >>> + /* Sync initial cable state */ >>> + queue_delayed_work(system_wq, &bdi->extcon_work, 0); >>> + } >>> + >>> enable_irq_wake(client->irq); >>> >>> pm_runtime_mark_last_busy(dev); >>> -- >>> 2.9.3 >>> >
Hi, On 29-03-17 10:21, Liam Breck wrote: > On Wed, Mar 29, 2017 at 12:34 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 29-03-17 03:12, Liam Breck wrote: >>> >>> Sebastian, this patch needs some work, could you drop it from -next >>> and look for v4 from Hans? >> >> >> Nack. >> >> Liam this whole dropping patches from -next business is not how >> things are normally done in kernel-land, please stop expecting it. >> >> The whole dropping patches all the time thing means people don't have >> a stable base to base future patches on which is unworkable. > > This was merged 3h after you posted it without acks from Tony or me. > So maybe asking for a do-over in this case isn't unreasonable :-) > Another of your patches has a replacement pending already. We'd both > benefit from a clean history wherever possible. And new-feature > patches should really simmer on the list for a little while. When I posted my initial series you asked me to rebase it onto -next, which I agreed to as that is the normal way to do things. All I'm asking of you is to do the same you've asked of me and base your patches on top of -next too. Dropping patches out of -next is a rare thing and nothing something which we should do every other patch. This can all easily be fixed up with a follow-up patch without breaking bisectability or some-such. Actually having a follow-up patch to remove the 5v boost support is useful from a history pov as it documents why there is no 5v boost support in the extcon handling. >>> Hans, I regret I couldn't study this sooner, but 5-day turnaround >>> isn't too bad :-) >> >> >> You actually already reviewed v2 which is why v3 has: >> >>>> Changes in v3: >>>> -Print a msg with dev_info when an extcon is used do determine iinlim >> >> As you requested, so it is a bit weird to now ask for a version with >> the changes you requested to be dropped because you decided you want >> more changes now... > > I took a quick look at v2. I didn't have a chance to respond to v3 > before it landed in -next. > >>> I will send a v3 of "Uniform pm_runtime_get ..." not rebased to this. >> >> >> Please base it on top instead and also fix the runtime_pm_get_sync() >> error-handling in the newly added function, that was copy pasted from >> what is in hindsight a bad example, so fixing it in the same patch >> makes sense. > > The v2 of "Uniform..." I just posted is actually based on -next, but > doesn't touch your code, so has to be redone itself. Which is why I asked for a v3, you're asking me to do a new version of a patch to fix a bug which you are already fixing in other places in a *later* patch so why not fix the copy/paste of the bug my patch introduced in your *later* patch. That is the normal way of doing things everywhere in the kernel. >>> On Thu, Mar 23, 2017 at 1:32 AM, Hans de Goede <hdegoede@redhat.com> >>> wrote: >>>> >>>> Add support for monitoring an extcon device with USB 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, as well as on systems where the 5V boost converter is used, as >>>> that always needs to be enabled from software. >>>> >>>> Cc: Liam Breck <kernel@networkimprov.net> >>>> Cc: Tony Lindgren <tony@atomide.com> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> Acked-by: Sebastian Reichel <sre@kernel.org> >>>> --- >>>> Changes in v2: >>>> -Use a device-property for extcon-name instead of platform_data >>>> -Delay 300ms before applying the extcon detected charger-type to iinlim >>>> (see the added comment for why this is done) >>>> Changes in v3: >>>> -Print a msg with dev_info when an extcon is used do determine iinlim >>>> --- >>>> drivers/power/supply/Kconfig | 1 + >>>> drivers/power/supply/bq24190_charger.c | 109 >>>> +++++++++++++++++++++++++++++++++ >>>> 2 files changed, 110 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 d74c8cc..2c404a5 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> >>>> >>>> @@ -36,6 +38,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 >>> >>> >>> Place these at the end of the reg_poc section, so the mask & shift >>> names are together. >>> >>>> #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) >>>> @@ -148,6 +152,9 @@ struct bq24190_dev_info { >>>> struct device *dev; >>>> struct power_supply *charger; >>>> struct power_supply *battery; >>>> + struct extcon_dev *extcon; >>>> + struct notifier_block extcon_nb; >>>> + struct delayed_work extcon_work; >>>> char model_name[I2C_NAME_SIZE]; >>>> bool initialized; >>>> bool irq_event; >>>> @@ -164,6 +171,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 }; >>> >>> >>> Correct name is bq24190_isc_iinlim_values, following convention of others. >>> >>>> /* REG02[7:2] (ICHG) in uAh */ >>>> static const int bq24190_ccc_ichg_values[] = { >>>> 512000, 576000, 640000, 704000, 768000, 832000, 896000, >>>> 960000, >>>> @@ -1277,6 +1289,78 @@ 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.work); >>>> + int ret, iinlim = 0; >>>> + >>>> + ret = pm_runtime_get_sync(bdi->dev); >>>> + if (ret < 0) { >>>> + dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", >>>> ret); >>> >>> >>> Needs pm_runtime_put_noidle() >>> Use same error msg as other pm_runtime_get_sync failures. >> >> >> This should be fixed in a v3 of your: >> "power: bq24190_charger: Uniform pm_runtime_get() failure handling" >> patch. >> >> >> >>> >>>> + return; >>>> + } >>>> + >>>> + 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) >>> >>> >>> ret < 0 >>> >>>> + 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. >>>> + */ >>> >>> >>> u8 val = BQ24190_REG_POC_CHG_CONFIG_CHARGE; >>> >>>> + if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == >>>> 1) { >>> >>> >>> if (above) val = BQ24190_REG_POC_CHG_CONFIG_OTG; >>> >>> Then call write_mask once, with val. >> >> >> Good idea, but actually I found out by booting my board with a host-cable >> (USB-C equivalent of id-pin connected to ground) plugged in that there is >> a second 5v boost converter on the board and that that-one gets used by >> the firmware, so I will submit a patch soon-ish dropping the EXTCON_USB_HOST >> handling all together. > > Since you're planning this fix anyway, perhaps re-do this patch with > that change and my feedback? That is not how the kernel-development process works and I really don't see anything special about the bq24190_charger, or the requested changes to derive from the process here. > >> As Sebastian already said (I believe) it would be best to export the 5V >> boost >> converter as a regulator, but that can wait until we actually need it >> somewhere. >> >> >>> >>>> + 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) >>> >>> >>> ret < 0 >>> >>>> + dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret); >>>> + >>>> + pm_runtime_mark_last_busy(bdi->dev); >>>> + pm_runtime_put_autosuspend(bdi->dev); >>>> +} >>>> + >>>> +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); >>>> + >>>> + /* >>>> + * The Power-Good detection may take up to 220ms, sometimes >>>> + * the external charger detection is quicker, and the bq24190 >>>> will >>>> + * reset to iinlim based on its own charger detection (which is >>>> not >>>> + * hooked up when using external charger detection) resulting in >>>> + * a too low default 500mA iinlim. Delay applying the extcon >>>> value >>>> + * for 300ms to avoid this. >>>> + */ >>>> + queue_delayed_work(system_wq, &bdi->extcon_work, >>>> msecs_to_jiffies(300)); >>>> + >>>> + return NOTIFY_OK; >>>> +} >>>> + >>>> static int bq24190_hw_init(struct bq24190_dev_info *bdi) >>>> { >>>> u8 v; >>>> @@ -1314,6 +1398,7 @@ static int bq24190_probe(struct i2c_client *client, >>>> struct device *dev = &client->dev; >>>> struct power_supply_config charger_cfg = {}, battery_cfg = {}; >>>> struct bq24190_dev_info *bdi; >>>> + const char *name; >>>> int ret; >>>> >>>> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) >>>> { >>>> @@ -1341,6 +1426,18 @@ static int bq24190_probe(struct i2c_client >>>> *client, >>>> return -EINVAL; >>>> } >>>> >>>> + /* >>>> + * The extcon-name property is purely an in kernel interface for >>>> + * x86/ACPI use, DT platforms should get extcon via phandle. >>>> + */ >>>> + if (device_property_read_string(dev, "extcon-name", &name) == 0) >>>> { >>> >>> >>> Is the "extcon-name" in-kernel interface documented, or a convention, >>> or custom to your gauge/setup? >> >> >> It is documented right above the use of it :) Since this is the first >> case of passing an extcon-name through a device property AFAIK we >> are starting the convention of doing this this way I guess. > > Is there somewhere we could document this convention where others > might find it? :-) Yes near the code where it is used, because that is where people will look. > >>>> + bdi->extcon = extcon_get_extcon_dev(name); >>>> + if (!bdi->extcon) >>>> + return -EPROBE_DEFER; >>>> + >>>> + dev_info(bdi->dev, "using extcon device %s\n", name); >>>> + } >>>> + >>>> pm_runtime_enable(dev); >>>> pm_runtime_use_autosuspend(dev); >>>> pm_runtime_set_autosuspend_delay(dev, 600); >>>> @@ -1391,6 +1488,18 @@ static int bq24190_probe(struct i2c_client >>>> *client, >>>> goto out5; >>>> } >>>> >>>> + if (bdi->extcon) { >>>> + INIT_DELAYED_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 out5; >>> >>> >>> Needs a dev_err msg. Also out5 changes to out_sysfs in my recent patch. >> >> >> Again you should rebase your patch on top of next, not expect me to >> do a new version of my already merged patch, and then fix this up >> in your new version. And this still stands. Regards, Hans
* Hans de Goede <hdegoede@redhat.com> [170329 02:36]: > Hi, > > On 29-03-17 10:21, Liam Breck wrote: > > On Wed, Mar 29, 2017 at 12:34 AM, Hans de Goede <hdegoede@redhat.com> wrote: > > > Hi, > > > > > > On 29-03-17 03:12, Liam Breck wrote: > > > > > > > > Sebastian, this patch needs some work, could you drop it from -next > > > > and look for v4 from Hans? > > > > > > > > > Nack. > > > > > > Liam this whole dropping patches from -next business is not how > > > things are normally done in kernel-land, please stop expecting it. > > > > > > The whole dropping patches all the time thing means people don't have > > > a stable base to base future patches on which is unworkable. > > > > This was merged 3h after you posted it without acks from Tony or me. > > So maybe asking for a do-over in this case isn't unreasonable :-) > > Another of your patches has a replacement pending already. We'd both > > benefit from a clean history wherever possible. And new-feature > > patches should really simmer on the list for a little while. > > When I posted my initial series you asked me to rebase it onto > -next, which I agreed to as that is the normal way to do things. > > All I'm asking of you is to do the same you've asked of me and > base your patches on top of -next too. > > Dropping patches out of -next is a rare thing and nothing something > which we should do every other patch. This can all easily be > fixed up with a follow-up patch without breaking bisectability > or some-such. Yeah agreed. Let's not start redoing branches if possible and instead do incremental changes on top. Mistakes can happen, but with multiple people patching the same code it's important to have a branch we can all pull from. > Actually having a follow-up patch to remove the 5v boost support > is useful from a history pov as it documents why there is no 5v > boost support in the extcon handling. Yeah works for me. Regards, Tony
Hi, On 29-03-17 09:34, Hans de Goede wrote: <snip> >>> @@ -36,6 +38,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 >> >> Place these at the end of the reg_poc section, so the mask & shift >> names are together. Small note while I'm sending an email anyways: the BQ24190_REG_VPRS defines do things the same way and I believe that keeping the masks and the values for the bits together is better. <snip> >>> + /* >>> + * If no charger has been detected and host mode is requested, activate >>> + * the 5V boost converter, otherwise deactivate it. >>> + */ >> >> u8 val = BQ24190_REG_POC_CHG_CONFIG_CHARGE; >> >>> + if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1) { >> >> if (above) val = BQ24190_REG_POC_CHG_CONFIG_OTG; >> >> Then call write_mask once, with val. > > Good idea, Scrap that I thought you were trying to fold the ilimit and boost-converter writes into 1 write which would be good I now see that those are 2 different regs and what you're suggesting is a coding style change only. I don't like your proposed change, this is a clear example where using an if-else construct is much more readable then using the: val=condition-false-value; if (condition) val=condition-true-value construct you suggest. > but actually I found out by booting my board with a host-cable > (USB-C equivalent of id-pin connected to ground) plugged in that there is > a second 5v boost converter on the board and that that-one gets used by > the firmware, so I will submit a patch soon-ish dropping the EXTCON_USB_HOST > handling all together. Ok scrap that, yes my board has 2 5v boost converters, yes the firmware uses the one which is not part of the bq24190, but that is a BAD idea, because even if we stop the bq24190 from trying to charge the battery from the 5v boost converter, by driving not-CE high, it still tries to supply Vsys from the 5v boost converter causing an extra 300mA of battery drain as long as the other 5v boost converter is active. So what the firmware is doing is stupid and I'm not going to send a patch to remove the 5v boost converter enabling from the bq24190 extcon handling as that is actually what we want. Instead I'll do a patch for the pmic-extcon driver to turn off the other 5v boost converter at boot, as it should never be used on machines with a bq24190. Regards, Hans
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 d74c8cc..2c404a5 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> @@ -36,6 +38,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) @@ -148,6 +152,9 @@ struct bq24190_dev_info { struct device *dev; struct power_supply *charger; struct power_supply *battery; + struct extcon_dev *extcon; + struct notifier_block extcon_nb; + struct delayed_work extcon_work; char model_name[I2C_NAME_SIZE]; bool initialized; bool irq_event; @@ -164,6 +171,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, @@ -1277,6 +1289,78 @@ 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.work); + int ret, iinlim = 0; + + ret = pm_runtime_get_sync(bdi->dev); + if (ret < 0) { + dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret); + return; + } + + 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); + + pm_runtime_mark_last_busy(bdi->dev); + pm_runtime_put_autosuspend(bdi->dev); +} + +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); + + /* + * The Power-Good detection may take up to 220ms, sometimes + * the external charger detection is quicker, and the bq24190 will + * reset to iinlim based on its own charger detection (which is not + * hooked up when using external charger detection) resulting in + * a too low default 500mA iinlim. Delay applying the extcon value + * for 300ms to avoid this. + */ + queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300)); + + return NOTIFY_OK; +} + static int bq24190_hw_init(struct bq24190_dev_info *bdi) { u8 v; @@ -1314,6 +1398,7 @@ static int bq24190_probe(struct i2c_client *client, struct device *dev = &client->dev; struct power_supply_config charger_cfg = {}, battery_cfg = {}; struct bq24190_dev_info *bdi; + const char *name; int ret; if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { @@ -1341,6 +1426,18 @@ static int bq24190_probe(struct i2c_client *client, return -EINVAL; } + /* + * The extcon-name property is purely an in kernel interface for + * x86/ACPI use, DT platforms should get extcon via phandle. + */ + if (device_property_read_string(dev, "extcon-name", &name) == 0) { + bdi->extcon = extcon_get_extcon_dev(name); + if (!bdi->extcon) + return -EPROBE_DEFER; + + dev_info(bdi->dev, "using extcon device %s\n", name); + } + pm_runtime_enable(dev); pm_runtime_use_autosuspend(dev); pm_runtime_set_autosuspend_delay(dev, 600); @@ -1391,6 +1488,18 @@ static int bq24190_probe(struct i2c_client *client, goto out5; } + if (bdi->extcon) { + INIT_DELAYED_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 out5; + + /* Sync initial cable state */ + queue_delayed_work(system_wq, &bdi->extcon_work, 0); + } + enable_irq_wake(client->irq); pm_runtime_mark_last_busy(dev);