| Message ID | 20221009191839.102686-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [1/2] power: supply: bq25890: Add CC voltage to ADC properties | expand |
On Sun, Oct 09, 2022 at 09:18:38PM +0200, Marek Vasut wrote: > The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE , representing register > REG0E field BATV is an ADC conversion of Battery Voltage (VBAT). Mark > the property as ADC one. In this case I believe the property is representing wrong value: it should be REG06 - the programmed voltage limit, and _MAX should reflect maximum setting possible. Though I think there is no proper property for the VSYS value that is currently occupying VOLTAGE_NOW - this might be better modelled as a separate regulator maybe? But, for the time being this look ok. > Fixes: 21d90eda433f ("power: bq25890: fix ADC mode configuration") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Cc: Sebastian Reichel <sebastian.reichel@collabora.com> > To: linux-pm@vger.kernel.org > --- > drivers/power/supply/bq25890_charger.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c > index 6020b58c641d2..34dbd498f0f51 100644 > --- a/drivers/power/supply/bq25890_charger.c > +++ b/drivers/power/supply/bq25890_charger.c > @@ -432,6 +432,7 @@ static bool bq25890_is_adc_property(enum power_supply_property psp) > { > switch (psp) { > case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: > case POWER_SUPPLY_PROP_CURRENT_NOW: > case POWER_SUPPLY_PROP_TEMP: > return true; > -- > 2.35.1 >
Hi, On 10/10/22 01:08, Michał Mirosław wrote: > On Sun, Oct 09, 2022 at 09:18:38PM +0200, Marek Vasut wrote: >> The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE , representing register >> REG0E field BATV is an ADC conversion of Battery Voltage (VBAT). Mark >> the property as ADC one. > > In this case I believe the property is representing wrong value: it > should be REG06 - the programmed voltage limit, and _MAX should reflect > maximum setting possible. I believe this is correct, POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE should contain the voltage up to which the charger will charge the connected battery. The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE and POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT basically reflect 2 configuration settings / values for the 2 phases most charge-ICs use: Phase 1, when the voltage of the battery is below the constant-charge-voltage the charger operates in constant-current mode, operating as a current source delivering constant-charge-current Ampere into the battery. Phase 2, when the open-clamp voltage has reached the constant-charge-voltage value, then the charger IC will switch to constant-voltage mode, operating as a voltage source, feeding a smaller current into the battery for as long as there is a voltage differential between the open-clamp voltage of the battery and the voltage source. Once the 2 reach equilibrium the current becomes 0 Amps and charging is done. The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE and POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT are supposed to reflect the current resp. voltage settings for these 2 phases and are not supposed to be ADC readings (an ADC reading is not constant). As for VOLTAGE_NOW, that is supposed to be an ADC reading, but as I mentioned about current_now in my earlier reply to patch 2/2 in this series, VOLTAGE_NOW normally is only used on battery-type power_supply class devices (so with fuel-gauge ICs). The problem with using VOLTAGE_NOW with a charger IC is that (like currents) a charger IC basically has 3 voltages: 1. The input-voltage from an external powersupply (if present) 2. The output-voltage going to the rest of the system 3. The voltage of the connected battery With 3. being complicated by there being both an actually measured voltage, which is the result of the internal battery voltage + an offset caused by the (dis)charge current. as well as a so called open-clamp voltage which is purely the internal battery voltage (which gets measured or guessed through magic) For battery-type power-supply class devices VOLTAGE_NOW is used to reflect the actual measured voltage of the battery connections. So using it for Vsys here, which I assume is 2. from my list of 3 voltages above pretty much feels wrong. As I mentioned in my reply to 2/2 I believe that for CURRENT_NOW and likewise VOLTAGE_NOW we should document that when these are used with charger type power-supply class devices these 2 should reflect the current/voltage of the battery connection of the charger, so that they match with their use in battery-type power-supply devices (where they are found much more often, many charger-ICs don't have an ADC for this at all. We don't really have any existing properties to convey the Vsys voltage (nor current) atm. So if we want to export this then IMHO we should add new: POWER_SUPPLY_PROP_SYSTEM_VOLTAGE, POWER_SUPPLY_PROP_SYSTEM_CURRENT, properties for these. > Though I think there is no proper property > for the VSYS value that is currently occupying VOLTAGE_NOW - this > might be better modelled as a separate regulator maybe? Ack, see above. > > But, for the time being this look ok. I disagree the current abuse of POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE should be fixed, "REG0E field BATV is an ADC conversion of Battery Voltage (VBAT)" should be the value used for VOLTAGE_NOW and the current Vsys reading exported in VOLTAGE_NOW should instead be reported in a new property. And then POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE should be used to actually report the 4.2 or 4.35 volt limit (guessing here) to which the charger will actually charge the battery. As with my other reply I hope this helps to clarify how these properties should be used. Or at least how I strongly believe that they should be used... Regards, Hans > >> Fixes: 21d90eda433f ("power: bq25890: fix ADC mode configuration") >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Cc: Hans de Goede <hdegoede@redhat.com> >> Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl> >> Cc: Sebastian Reichel <sebastian.reichel@collabora.com> >> To: linux-pm@vger.kernel.org >> --- >> drivers/power/supply/bq25890_charger.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c >> index 6020b58c641d2..34dbd498f0f51 100644 >> --- a/drivers/power/supply/bq25890_charger.c >> +++ b/drivers/power/supply/bq25890_charger.c >> @@ -432,6 +432,7 @@ static bool bq25890_is_adc_property(enum power_supply_property psp) >> { >> switch (psp) { >> case POWER_SUPPLY_PROP_VOLTAGE_NOW: >> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: >> case POWER_SUPPLY_PROP_CURRENT_NOW: >> case POWER_SUPPLY_PROP_TEMP: >> return true; >> -- >> 2.35.1 >> >
On 10/10/22 15:50, Hans de Goede wrote: > Hi, Hi, [...] >> Though I think there is no proper property >> for the VSYS value that is currently occupying VOLTAGE_NOW - this >> might be better modelled as a separate regulator maybe? > > Ack, see above. We already do have a regulator in the bq25890 driver. The regulator is used as a switch to toggle OTG boost mode (supply from battery to VBUS), but I don't see any users of this functionality, and I cannot imagine how this would be modeled in DT. (Hans, can you clarify?) There is the usb_work (usb_register_notifier()) which triggers workqueue which does the same, toggles OTG boost mode, but this is only used in case a valid USB PHY is found. I didn't find any users of this either. Anyway, maybe we can extend the regulator to report VBus and register another one to report VSys, where the VSys one can be plugged e.g. as supply for PMIC in DT ? [...]
Hi, On 10/10/22 21:22, Marek Vasut wrote: > On 10/10/22 15:50, Hans de Goede wrote: >> Hi, > > Hi, > > [...] > >>> Though I think there is no proper property >>> for the VSYS value that is currently occupying VOLTAGE_NOW - this >>> might be better modelled as a separate regulator maybe? >> >> Ack, see above. > > We already do have a regulator in the bq25890 driver. The regulator is used as a switch to toggle OTG boost mode (supply from battery to VBUS), but I don't see any users of this functionality, and I cannot imagine how this would be modeled in DT. (Hans, can you clarify?) Ah, with the Ack I meant ack for the "I think there is no proper property for the VSYS value" I did not meant to ack the regulator bit, I don't directly see how having registering a regulator device for Vsys would be useful, sorry. As mentioned in my original email I believe that just adding a new property for Vsys makes the most sense. Note the OTG regulator is useful to enable/disable 5V boost output when used with e.g. a micro-usb connector and that micro-usb connector is used with an micro-USB OTG host-mode cable / dongle. > There is the usb_work (usb_register_notifier()) which triggers workqueue which does the same, toggles OTG boost mode, but this is only used in case a valid USB PHY is found. I didn't find any users of this either. This is used, but the use is hidden away pretty well I admit. Some X86 tablets from the Cherry Trail era have ACPI tables where the charging / fuel-gauge ICs are not handled in ACPI (as they typically are on x86) so we need to do it ourselves. Specifically the charger IC is connected to a special I2c bus of the PMIC (which itself is an i2c-device so we have an i2c-attached i2c-controller, crazy ...) the driver for this special PMIC embedded i2c-controller is also responsible for instantiating the charger chip i2c-client and this setup uses the OTG regulator device, see: drivers/i2c/busses/i2c-cht-wc.c specifically these bits: /********** Xiaomi Mi Pad 2 charger IC settings **********/ static struct regulator_consumer_supply bq2589x_vbus_consumer = { .supply = "vbus", .dev_name = "cht_wcove_pwrsrc", }; static const struct regulator_init_data bq2589x_vbus_init_data = { .constraints = { .valid_ops_mask = REGULATOR_CHANGE_STATUS, }, .consumer_supplies = &bq2589x_vbus_consumer, .num_consumer_supplies = 1, }; static struct bq25890_platform_data bq2589x_pdata = { .regulator_init_data = &bq2589x_vbus_init_data, }; static const struct property_entry xiaomi_mipad2_props[] = { PROPERTY_ENTRY_BOOL("linux,skip-reset"), PROPERTY_ENTRY_BOOL("linux,read-back-settings"), { } }; static const struct software_node xiaomi_mipad2_node = { .properties = xiaomi_mipad2_props, }; static struct i2c_board_info xiaomi_mipad2_board_info = { .type = "bq25890", .addr = 0x6a, .dev_name = "bq25890", .swnode = &xiaomi_mipad2_node, .platform_data = &bq2589x_pdata, }; And then the drivers/extcon/extcon-intel-cht-wc.c uses the vbus regulator to control the 5V out on the micro-USB. (also note these devices do not instantiate a usb_phy device anywhere, which is why the V5 boost is handled through the regulator framework) TL;DR: the regulator device for the V5 boost output is used, please don't remove it :) > Anyway, maybe we can extend the regulator to report VBus and register another one to report VSys, where the VSys one can be plugged e.g. as supply for PMIC in DT ? I'm not sure if a regulator device for Vsys is really useful, AFAIK Vsys can not be turned off, nor can the voltage level be controlled... Regards, Hans
On 10/11/22 09:38, Hans de Goede wrote: > Hi, Hi, >>>> Though I think there is no proper property >>>> for the VSYS value that is currently occupying VOLTAGE_NOW - this >>>> might be better modelled as a separate regulator maybe? >>> >>> Ack, see above. >> >> We already do have a regulator in the bq25890 driver. The regulator is used as a switch to toggle OTG boost mode (supply from battery to VBUS), but I don't see any users of this functionality, and I cannot imagine how this would be modeled in DT. (Hans, can you clarify?) > > Ah, with the Ack I meant ack for the "I think there is no proper property for the VSYS value" I did not meant to ack the regulator bit, I don't directly see how having registering a regulator device for Vsys would be useful, sorry. As mentioned in my original email I believe that just adding a new property for Vsys makes the most sense. I disagree here, have a look at the new series I posted that adds the Vsys regulator, esp. patch 7/7 . I think the Vsys regulator does make sense, since it can be the supply for PMIC, which would let us model that hardware connection between the charger and PMIC properly, and even in DT. > Note the OTG regulator is useful to enable/disable 5V boost output when used with e.g. a micro-usb connector and that micro-usb connector is used with an micro-USB OTG host-mode cable / dongle. > >> There is the usb_work (usb_register_notifier()) which triggers workqueue which does the same, toggles OTG boost mode, but this is only used in case a valid USB PHY is found. I didn't find any users of this either. > > This is used, but the use is hidden away pretty well I admit. Some X86 tablets from the Cherry Trail era have ACPI tables where the charging / fuel-gauge ICs are not handled in ACPI (as they typically are on x86) so we need to do it ourselves. > > Specifically the charger IC is connected to a special I2c bus of the PMIC (which itself is an i2c-device so we have an i2c-attached i2c-controller, crazy ...) the driver for this special PMIC embedded i2c-controller is also responsible for instantiating the charger chip i2c-client and this setup uses the OTG regulator device, see: drivers/i2c/busses/i2c-cht-wc.c specifically these bits: [...] > And then the drivers/extcon/extcon-intel-cht-wc.c uses > the vbus regulator to control the 5V out on the micro-USB. Uh, now I understand, thanks for clarification. > (also note these devices do not instantiate a usb_phy device anywhere, which is why the V5 boost is handled through the regulator framework) > > TL;DR: the regulator device for the V5 boost output is used, please don't remove it :) I won't, I only extended the current regulator registration in the end. >> Anyway, maybe we can extend the regulator to report VBus and register another one to report VSys, where the VSys one can be plugged e.g. as supply for PMIC in DT ? > > I'm not sure if a regulator device for Vsys is really useful, AFAIK Vsys can not be turned off, nor can the voltage level be controlled... It can be used as a supply for system PMIC, so I think it is actually useful. Sure, you cannot turn the Vsys on/off, but that's not the sole purpose of the regulator. The PMIC can get its input voltage and configure itself accordingly, which without the Vsys regulator providing its current voltage is not possible.
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index 6020b58c641d2..34dbd498f0f51 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -432,6 +432,7 @@ static bool bq25890_is_adc_property(enum power_supply_property psp) { switch (psp) { case POWER_SUPPLY_PROP_VOLTAGE_NOW: + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: case POWER_SUPPLY_PROP_CURRENT_NOW: case POWER_SUPPLY_PROP_TEMP: return true;
The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE , representing register REG0E field BATV is an ADC conversion of Battery Voltage (VBAT). Mark the property as ADC one. Fixes: 21d90eda433f ("power: bq25890: fix ADC mode configuration") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl> Cc: Sebastian Reichel <sebastian.reichel@collabora.com> To: linux-pm@vger.kernel.org --- drivers/power/supply/bq25890_charger.c | 1 + 1 file changed, 1 insertion(+)