by Sebastian Reichel

[permalink] [raw]

Subject: Re: [PATCH v4 7/8] power: supply: rt5033_battery: Adopt status property from charger

Hi,

On Sat, May 06, 2023 at 05:54:34PM +0200, Jakob Hauser wrote:
> The rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger
> can, let's get this value.
>
> Tested-by: Raymond Hackley <[email protected]>
> Signed-off-by: Jakob Hauser <[email protected]>
> ---
> drivers/power/supply/rt5033_battery.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/power/supply/rt5033_battery.c b/drivers/power/supply/rt5033_battery.c
> index 5c04cf305219..a6520716d813 100644
> --- a/drivers/power/supply/rt5033_battery.c
> +++ b/drivers/power/supply/rt5033_battery.c
> @@ -12,6 +12,26 @@
> #include <linux/mfd/rt5033-private.h>
> #include <linux/mfd/rt5033.h>
>
> +static int rt5033_battery_get_status(struct i2c_client *client)
> +{
> + struct power_supply *charger;
> + union power_supply_propval val;
> + int ret;
> +
> + charger = power_supply_get_by_name("rt5033-charger");
> + if (!charger)
> + return POWER_SUPPLY_STATUS_UNKNOWN;
> +
> + ret = power_supply_get_property(charger, POWER_SUPPLY_PROP_STATUS, &val);
> + if (ret) {
> + power_supply_put(charger);
> + return POWER_SUPPLY_STATUS_UNKNOWN;
> + }

struct rt5033_battery *battery = i2c_get_clientdata(client);
ret = power_supply_get_property_from_supplier(battery->psy, POWER_SUPPLY_PROP_STATUS, &val);
if (ret)
val.intval = POWER_SUPPLY_STATUS_UNKNOWN;

> +
> + power_supply_put(charger);
> + return val.intval;
> +}
> +
> static int rt5033_battery_get_capacity(struct i2c_client *client)
> {
> struct rt5033_battery *battery = i2c_get_clientdata(client);
> @@ -84,6 +104,9 @@ static int rt5033_battery_get_property(struct power_supply *psy,
> case POWER_SUPPLY_PROP_CAPACITY:
> val->intval = rt5033_battery_get_capacity(battery->client);
> break;
> + case POWER_SUPPLY_PROP_STATUS:
> + val->intval = rt5033_battery_get_status(battery->client);
> + break;
> default:
> return -EINVAL;
> }
> @@ -96,6 +119,7 @@ static enum power_supply_property rt5033_battery_props[] = {
> POWER_SUPPLY_PROP_VOLTAGE_OCV,
> POWER_SUPPLY_PROP_PRESENT,
> POWER_SUPPLY_PROP_CAPACITY,
> + POWER_SUPPLY_PROP_STATUS,
> };
>
> static const struct regmap_config rt5033_battery_regmap_config = {
> --
> 2.39.2
>

Otherwise LGTM.

-- Sebastian

Attachments:

(No filename) (2.31 kB)
signature.asc (849.00 B)
Download all attachments

2023-05-08 16:49:17

by Sebastian Reichel

[permalink] [raw]

Subject: Re: [PATCH v4 6/8] power: supply: rt5033_charger: Add cable detection and USB OTG supply

Hi,

On Sat, May 06, 2023 at 05:54:33PM +0200, Jakob Hauser wrote:
> Implement cable detection by extcon and handle the driver according to the
> connector type.
>
> There are basically three types of action: "set_charging", "set_otg" and
> "set_disconnect".
>
> A forth helper function to "unset_otg" was added because this is used in both
> "set_charging" and "set_disconnect". In the first case it covers the rather
> rare event that someone changes from OTG to charging without disconnect. In
> the second case, when disconnecting, the values are set back to the ones from
> initialization to return into a defined state.
>
> Additionally, there is "set_mivr". When connecting to e.g. a laptop/PC, the
> minimum input voltage regulation (MIVR) shall prevent a voltage drop if the
> cable or the supply is weak. The MIVR value is set to 4600MV, same as in the
> Android driver [1]. When disconnecting, MIVR is set back to DISABLED.
>
> In the function rt5033_get_charger_state(): When in OTG mode, the chip
> reports status "charging". Change this to "discharging" because there is
> no charging going on in OTG mode [2].
>
> [1] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L499
> [2] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/battery/rt5033_charger.c#L686-L687
>
> Tested-by: Raymond Hackley <[email protected]>
> Signed-off-by: Jakob Hauser <[email protected]>
> ---
> [...]
> diff --git a/include/linux/mfd/rt5033.h b/include/linux/mfd/rt5033.h
> index e99e2ab0c1c1..d2c613764756 100644
> --- a/include/linux/mfd/rt5033.h
> +++ b/include/linux/mfd/rt5033.h
> @@ -53,6 +53,14 @@ struct rt5033_charger {
> struct rt5033_dev *rt5033;
> struct power_supply *psy;
> struct rt5033_charger_data *chg;
> + struct extcon_dev *edev;
> + struct notifier_block extcon_nb;
> + struct work_struct extcon_work;
> + struct mutex lock;
> + bool online;
> + bool otg;
> + bool mivr_enabled;
> + u8 cv_regval;
> };

Please move 'struct rt5033_charger' and 'struct rt5033_charger_data'
to drivers/power/supply/rt5033_charger.c; they are not supposed to
be used directly by others. Making it private helps to avoid
cross-subsystem series for future maintanence patches.

Otherwise LGTM.

-- Sebastian

Attachments:

(No filename) (2.31 kB)
signature.asc (849.00 B)
Download all attachments

2023-05-08 17:06:20

Hi,

On Mon, May 08, 2023 at 11:18:28PM +0200, Jakob Hauser wrote:
> On 08.05.23 13:35, Sebastian Reichel wrote:
> > On Sat, May 06, 2023 at 05:54:34PM +0200, Jakob Hauser wrote:
> > > The rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger
> > > can, let's get this value.
> > >
> > > Tested-by: Raymond Hackley <[email protected]>
> > > Signed-off-by: Jakob Hauser <[email protected]>
> > > ---
> > > drivers/power/supply/rt5033_battery.c | 24 ++++++++++++++++++++++++
> > > 1 file changed, 24 insertions(+)
> > >
> > > diff --git a/drivers/power/supply/rt5033_battery.c b/drivers/power/supply/rt5033_battery.c
> > > index 5c04cf305219..a6520716d813 100644
> > > --- a/drivers/power/supply/rt5033_battery.c
> > > +++ b/drivers/power/supply/rt5033_battery.c
> > > @@ -12,6 +12,26 @@
> > > #include <linux/mfd/rt5033-private.h>
> > > #include <linux/mfd/rt5033.h>
> > > +static int rt5033_battery_get_status(struct i2c_client *client)
> > > +{
> > > + struct power_supply *charger;
> > > + union power_supply_propval val;
> > > + int ret;
> > > +
> > > + charger = power_supply_get_by_name("rt5033-charger");
> > > + if (!charger)
> > > + return POWER_SUPPLY_STATUS_UNKNOWN;
> > > +
> > > + ret = power_supply_get_property(charger, POWER_SUPPLY_PROP_STATUS, &val);
> > > + if (ret) {
> > > + power_supply_put(charger);
> > > + return POWER_SUPPLY_STATUS_UNKNOWN;
> > > + }
> >
> > struct rt5033_battery *battery = i2c_get_clientdata(client);
> > ret = power_supply_get_property_from_supplier(battery->psy, POWER_SUPPLY_PROP_STATUS, &val);
> > if (ret)
> > val.intval = POWER_SUPPLY_STATUS_UNKNOWN;
>
> I don't think this works. There is no direct relationship between
> rt5033-charger and rt5033-battery. They operate independently from each
> other.

That should be fine as long as the supply dependency is properly declared.

> I had a short try and the status property of rt5033-battery was "unknown".
>
> Just for the record, the full function I tried was:
>
> static int rt5033_battery_get_status(struct i2c_client *client)
> {
> struct rt5033_battery *battery = i2c_get_clientdata(client);
> union power_supply_propval val;
> int ret;
>
> ret = power_supply_get_property_from_supplier(battery->psy,
> POWER_SUPPLY_PROP_STATUS,
> &val);
> if (ret)
> val.intval = POWER_SUPPLY_STATUS_UNKNOWN;
>
> return val.intval;
> }
>
> Later on I added a read-out of the "ret" value. It is "-19". I guess that's
> the "return -ENODEV;" from function
> power_supply_get_property_from_supplier(). [2]
>
> [2] https://github.com/torvalds/linux/blob/v6.4-rc1/drivers/power/supply/power_supply_core.c#L397-L421

I suppose your DT is missing the connection between the charger and
the battery:

rt5033_charger: charger {
compatible = "rt5033-charger";
...
}

fuel-gauge {
compatible = "rt5033-battery";
...
power-supplies = <&rt5033_charger>; // you are probably missing this
};

See also Documentation/devicetree/bindings/power/supply/power-supply.yaml

-- Sebastian

>
> > > +
> > > + power_supply_put(charger);
> > > + return val.intval;
> > > +}
> > > +
> > > static int rt5033_battery_get_capacity(struct i2c_client *client)
> > > {
> > > struct rt5033_battery *battery = i2c_get_clientdata(client);
> > > @@ -84,6 +104,9 @@ static int rt5033_battery_get_property(struct power_supply *psy,
> > > case POWER_SUPPLY_PROP_CAPACITY:
> > > val->intval = rt5033_battery_get_capacity(battery->client);
> > > break;
> > > + case POWER_SUPPLY_PROP_STATUS:
> > > + val->intval = rt5033_battery_get_status(battery->client);
> > > + break;
> > > default:
> > > return -EINVAL;
> > > }
> > > @@ -96,6 +119,7 @@ static enum power_supply_property rt5033_battery_props[] = {
> > > POWER_SUPPLY_PROP_VOLTAGE_OCV,
> > > POWER_SUPPLY_PROP_PRESENT,
> > > POWER_SUPPLY_PROP_CAPACITY,
> > > + POWER_SUPPLY_PROP_STATUS,
> > > };
> > > static const struct regmap_config rt5033_battery_regmap_config = {
> > > --
> > > 2.39.2
> > >
> >
> > Otherwise LGTM.
> >
> > -- Sebastian
>
> Kind regards,
> Jakob

Attachments:

(No filename) (4.28 kB)
signature.asc (849.00 B)
Download all attachments

2023-05-09 01:11:33

by Jakob Hauser

[permalink] [raw]

Subject: Re: [PATCH v4 7/8] power: supply: rt5033_battery: Adopt status property from charger

Hi Sebastian,

On 09.05.23 00:06, Sebastian Reichel wrote:
> Hi,
>
> On Mon, May 08, 2023 at 11:18:28PM +0200, Jakob Hauser wrote:
>> On 08.05.23 13:35, Sebastian Reichel wrote:
>>> On Sat, May 06, 2023 at 05:54:34PM +0200, Jakob Hauser wrote:
>>>> The rt5033-battery fuelgauge can't get a status by itself. The rt5033-charger
>>>> can, let's get this value.
>>>>
>>>> Tested-by: Raymond Hackley <[email protected]>
>>>> Signed-off-by: Jakob Hauser <[email protected]>
>>>> ---
>>>> drivers/power/supply/rt5033_battery.c | 24 ++++++++++++++++++++++++
>>>> 1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/drivers/power/supply/rt5033_battery.c b/drivers/power/supply/rt5033_battery.c
>>>> index 5c04cf305219..a6520716d813 100644
>>>> --- a/drivers/power/supply/rt5033_battery.c
>>>> +++ b/drivers/power/supply/rt5033_battery.c
>>>> @@ -12,6 +12,26 @@
>>>> #include <linux/mfd/rt5033-private.h>
>>>> #include <linux/mfd/rt5033.h>
>>>> +static int rt5033_battery_get_status(struct i2c_client *client)
>>>> +{
>>>> + struct power_supply *charger;
>>>> + union power_supply_propval val;
>>>> + int ret;
>>>> +
>>>> + charger = power_supply_get_by_name("rt5033-charger");
>>>> + if (!charger)
>>>> + return POWER_SUPPLY_STATUS_UNKNOWN;
>>>> +
>>>> + ret = power_supply_get_property(charger, POWER_SUPPLY_PROP_STATUS, &val);
>>>> + if (ret) {
>>>> + power_supply_put(charger);
>>>> + return POWER_SUPPLY_STATUS_UNKNOWN;
>>>> + }
>>>
>>> struct rt5033_battery *battery = i2c_get_clientdata(client);
>>> ret = power_supply_get_property_from_supplier(battery->psy, POWER_SUPPLY_PROP_STATUS, &val);
>>> if (ret)
>>> val.intval = POWER_SUPPLY_STATUS_UNKNOWN;
>>
>> I don't think this works. There is no direct relationship between
>> rt5033-charger and rt5033-battery. They operate independently from each
>> other.
>
> That should be fine as long as the supply dependency is properly declared.
>
>> I had a short try and the status property of rt5033-battery was "unknown".
>>
>> Just for the record, the full function I tried was:
>>
>> static int rt5033_battery_get_status(struct i2c_client *client)
>> {
>> struct rt5033_battery *battery = i2c_get_clientdata(client);
>> union power_supply_propval val;
>> int ret;
>>
>> ret = power_supply_get_property_from_supplier(battery->psy,
>> POWER_SUPPLY_PROP_STATUS,
>> &val);
>> if (ret)
>> val.intval = POWER_SUPPLY_STATUS_UNKNOWN;
>>
>> return val.intval;
>> }
>>
>> Later on I added a read-out of the "ret" value. It is "-19". I guess that's
>> the "return -ENODEV;" from function
>> power_supply_get_property_from_supplier(). [2]
>>
>> [2] https://github.com/torvalds/linux/blob/v6.4-rc1/drivers/power/supply/power_supply_core.c#L397-L421
>
> I suppose your DT is missing the connection between the charger and
> the battery:
>
> rt5033_charger: charger {
> compatible = "rt5033-charger";
> ...
> }
>
> fuel-gauge {
> compatible = "rt5033-battery";
> ...
> power-supplies = <&rt5033_charger>; // you are probably missing this
> };
>
> See also Documentation/devicetree/bindings/power/supply/power-supply.yaml

...

Thanks for the hints.

This leads to updating the dt-bindings because adding the
"power-supplies" property is important to be aware of.

Btw. first it didn't work. It took me quite some time to debug. I needed
to add "psy_cfg.of_node = client->dev.of_node;" to the rt5033-battery
probe function.

Now it works. However, there is a new problem. The battery driver gets
probed first. The charger driver a bit later. In the meantime the
battery driver spams dmesg with several "Failed to register power
supply" because the charger driver isn't available yet. Once the charger
driver is there, it works fine and dmesg becomes silent.

With the current state of the patchset:
dmesg | grep rt5033
[ 13.628030] rt5033 6-0034: Device found (rev. 6)
[ 13.633552] rt5033-led: Failed to locate of_node [id: -1]
[ 13.790478] rt5033-charger rt5033-charger: DMA mask not set

With the changes discussed here:
dmesg | grep rt5033
[ 15.741915] rt5033-battery 4-0035: Failed to register power supply
[ 15.752894] rt5033-battery 4-0035: Failed to register power supply
[ 15.795458] rt5033-battery 4-0035: Failed to register power supply
[ 15.910760] rt5033-battery 4-0035: Failed to register power supply
[ 15.913187] rt5033 6-0034: Device found (rev. 6)
[ 15.914341] rt5033-led: Failed to locate of_node [id: -1]
[ 15.920052] rt5033-battery 4-0035: Failed to register power supply
[ 15.927262] rt5033-battery 4-0035: Failed to register power supply
[ 16.017131] rt5033-battery 4-0035: Failed to register power supply
[ 16.017401] rt5033-charger rt5033-charger: DMA mask not set

The message is comming from the rt5033-battery probe function, it's the
power_supply_register() that fails.

Any ideas what could be done about this?

Kind regards,
Jakob

2023-05-09 07:46:52

Hi Krzysztof,

On 09.05.23 11:18, Krzysztof Kozlowski wrote:
> On 06/05/2023 17:54, Jakob Hauser wrote:

...

>> diff --git a/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> new file mode 100644
>> index 000000000000..b8607cc6ec63
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
>> @@ -0,0 +1,64 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/supply/richtek,rt5033-charger.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Richtek RT5033 PIMC Battery Charger
>
> PMIC

Good catch.

>> +
>> +maintainers:
>> + - Jakob Hauser <[email protected]>
>> +
>> +description:
>> + The battery charger of the multifunction device RT5033 has to be instantiated
>> + under sub-node named "charger" using the following format.
>> +
>> +properties:
>> + compatible:
>> + const: richtek,rt5033-charger
>> +
>> + monitored-battery:
>> + description: |
>> + Phandle to the monitored battery according to battery.yaml. The battery
>> + node needs to contain five parameters.
>> +
>> + precharge-current-microamp:
>> + Current of pre-charge mode. The pre-charge current levels are 350 mA
>> + to 650 mA programmed by I2C per 100 mA.
>> +
>> + constant-charge-current-max-microamp:
>> + Current of fast-charge mode. The fast-charge current levels are 700 mA
>> + to 2000 mA programmed by I2C per 100 mA.
>> +
>> + charge-term-current-microamp:
>> + This property is end of charge current. Its level ranges from 150 mA
>> + to 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and
>> + 600 mA in 100 mA steps.
>> +
>> + precharge-upper-limit-microvolt:
>> + Voltage of pre-charge mode. If the battery voltage is below the pre-charge
>> + threshold voltage, the charger is in pre-charge mode with pre-charge
>> + current. Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
>> +
>> + constant-charge-voltage-max-microvolt:
>> + Battery regulation voltage of constant voltage mode. This voltage levels
>> + from 3.65 V to 4.4 V by I2C per 0.025 V.
>> +
>> + connector:
>> + description:
>> + Phandle to a USB connector according to usb-connector.yaml. The connector
>> + should be a child of the extcon device.
>> + maxItems: 1
>
> Missing type/ref... but then you will notice you have conflicting ref
> with existing connector. connector is usb-connector.yaml, not phandle. I
> am not sure if we need generic property, so let's go with device
> specific - richtek,usb-connector

Actually, as we already switched to the connector thing, I would have
liked to do it properly, if at all. Again looking for examples how it
should be done, the usb-connector.yaml schema gets mostly used in the
USB and PHY domain. Those examples don't fit the specific case well. I
didn't find examples in the power supply domain.

Therefore, giving in, I'll chage it to richtek,usb-connector.

It's still hard for me to understand why phandle excton shouldn't fit
the specific simple case. Now we have neither a proper "connector" thing
nor a simple extcon phandle.

...

Kind regards,
Jakob