| Message ID | 20230308084419.11934-1-clamor95@gmail.com |
|---|---|
| Headers | show |
| Series | Add optional properties to MAX17040 | expand |
On 08/03/2023 09:44, Svyatoslav Ryhel wrote: > Add simple cell, status, health and temperature properties. > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> > --- > .../bindings/power/supply/maxim,max17040.yaml | 37 +++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml > index 3a529326ecbd..6f1c25b4729f 100644 > --- a/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml > +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17040.yaml > @@ -55,6 +55,20 @@ properties: > interrupts: > maxItems: 1 > > + monitored-battery: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: phandle to the battery node being monitored > + > + power-supplies: true This should be rather specific input name, e.g. vdd-supply. > + > + io-channels: > + items: > + - description: battery temperature max17040 does not have ADC temperature input... so is it system configuration? > + > + io-channel-names: > + items: > + - const: temp Drop the names property, not needed for one item. > + > wakeup-source: > type: boolean > description: | > @@ -95,3 +109,26 @@ examples: > wakeup-source; > }; > }; > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + i2c0 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + fuel-gauge@36 { > + compatible = "maxim,max17043"; > + reg = <0x36>; > + > + interrupt-parent = <&gpio>; > + interrupts = <144 IRQ_TYPE_EDGE_FALLING>; > + > + monitored-battery = <&battery>; > + power-supplies = <&charger>; But here you suggests something else than VDD... The hardware does not take charger as input. It takes power supply - vdd. > + > + io-channels = <&adc 8>; Just add these to existing example. > + io-channel-names = "temp"; > + > + maxim,alert-low-soc-level = <10>; > + wakeup-source; > + }; > + }; Best regards, Krzysztof
On 08/03/2023 09:44, Svyatoslav Ryhel wrote: > Since fuel gauge does not support thermal monitoring, > some vendors may couple this fuel gauge with thermal/adc > sensor to monitor battery cell exact temperature. > > Add this feature by adding optional iio thermal channel. > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> > --- > drivers/power/supply/max17040_battery.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 6dfce7b1309e..8c743c26dc6e 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -18,6 +18,7 @@ > #include <linux/of_device.h> > #include <linux/regmap.h> > #include <linux/slab.h> > +#include <linux/iio/consumer.h> > > #define MAX17040_VCELL 0x02 > #define MAX17040_SOC 0x04 > @@ -143,6 +144,7 @@ struct max17040_chip { > struct power_supply *battery; > struct power_supply_battery_info *batt_info; > struct chip_data data; > + struct iio_channel *channel_temp; > > /* battery capacity */ > int soc; > @@ -416,6 +418,11 @@ static int max17040_get_property(struct power_supply *psy, > case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > val->intval = chip->batt_info->charge_full_design_uah; > break; > + case POWER_SUPPLY_PROP_TEMP: > + iio_read_channel_raw(chip->channel_temp, > + &val->intval); > + val->intval *= 10; I am not convinced this is needed at all. You basically chain two subsystems only to report to user-space via power supply, but it is already reported via IIO. I would understand it if you use the value for something, e.g. control the charger. Here, it's just feeding different user-space interface. Therefore: 1. IO channels are not a hardware property of the fuel gauge, 2. I have doubts this should be even exposed via power supply interface. Best regards, Krzysztof
ср, 8 бер. 2023 р. о 11:08 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> пише: > > On 08/03/2023 09:44, Svyatoslav Ryhel wrote: > > Since fuel gauge does not support thermal monitoring, > > some vendors may couple this fuel gauge with thermal/adc > > sensor to monitor battery cell exact temperature. > > > > Add this feature by adding optional iio thermal channel. > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> > > --- > > drivers/power/supply/max17040_battery.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > > index 6dfce7b1309e..8c743c26dc6e 100644 > > --- a/drivers/power/supply/max17040_battery.c > > +++ b/drivers/power/supply/max17040_battery.c > > @@ -18,6 +18,7 @@ > > #include <linux/of_device.h> > > #include <linux/regmap.h> > > #include <linux/slab.h> > > +#include <linux/iio/consumer.h> > > > > #define MAX17040_VCELL 0x02 > > #define MAX17040_SOC 0x04 > > @@ -143,6 +144,7 @@ struct max17040_chip { > > struct power_supply *battery; > > struct power_supply_battery_info *batt_info; > > struct chip_data data; > > + struct iio_channel *channel_temp; > > > > /* battery capacity */ > > int soc; > > @@ -416,6 +418,11 @@ static int max17040_get_property(struct power_supply *psy, > > case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > > val->intval = chip->batt_info->charge_full_design_uah; > > break; > > + case POWER_SUPPLY_PROP_TEMP: > > + iio_read_channel_raw(chip->channel_temp, > > + &val->intval); > > + val->intval *= 10; > > I am not convinced this is needed at all. You basically chain two > subsystems only to report to user-space via power supply, but it is > already reported via IIO. I would understand it if you use the value for > something, e.g. control the charger. Here, it's just feeding different > user-space interface. Therefore: > 1. IO channels are not a hardware property of the fuel gauge, > 2. I have doubts this should be even exposed via power supply interface. > I can assure you that this is only the beginning of weird vendor solutions I have discovered. Nonetheless, max17040 has no battery temp property, this means in case I have a critical battery overheating, userspace will tell me nothing since instead of having direct battery temp property under power supply I have separate iio sensor, which may not even be monitored. It is always nice to have battery explosions. > > Best regards, > Krzysztof >
On 08/03/2023 10:15, Svyatoslav Ryhel wrote: >> max17040 does not have ADC temperature input... so is it system >> configuration? >> > > yes, I own a device (LG Optimus Vu P895) which uses max17043 > coupled with ADC thermal sensor > >>> + >>> + io-channel-names: >>> + items: >>> + - const: temp >> >> Drop the names property, not needed for one item. >> > > Alright, but driver patch expects temp name. I will look if this > is adjustable. I think I saw cases without names. > >>> + >>> wakeup-source: >>> type: boolean >>> description: | >>> @@ -95,3 +109,26 @@ examples: >>> wakeup-source; >>> }; >>> }; >>> + - | >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + i2c0 { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + fuel-gauge@36 { >>> + compatible = "maxim,max17043"; >>> + reg = <0x36>; >>> + >>> + interrupt-parent = <&gpio>; >>> + interrupts = <144 IRQ_TYPE_EDGE_FALLING>; >>> + >>> + monitored-battery = <&battery>; >>> + power-supplies = <&charger>; >> >> But here you suggests something else than VDD... The hardware does not >> take charger as input. It takes power supply - vdd. >> > > Power system allows passing properties from other power devices. > In this case battery health and status are passed from charger. So this is not an input to device? Then it does not really look like property of this hardware. Fuel gauge does not control the charger, also from system configuration point of view. Best regards, Krzysztof
ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> пише: > > On 08/03/2023 10:15, Svyatoslav Ryhel wrote: > > >> max17040 does not have ADC temperature input... so is it system > >> configuration? > >> > > > > yes, I own a device (LG Optimus Vu P895) which uses max17043 > > coupled with ADC thermal sensor > > > >>> + > >>> + io-channel-names: > >>> + items: > >>> + - const: temp > >> > >> Drop the names property, not needed for one item. > >> > > > > Alright, but driver patch expects temp name. I will look if this > > is adjustable. > > I think I saw cases without names. > There is no io-channel without a name. And io-channels are mostly used by power supply devices. > > > >>> + > >>> wakeup-source: > >>> type: boolean > >>> description: | > >>> @@ -95,3 +109,26 @@ examples: > >>> wakeup-source; > >>> }; > >>> }; > >>> + - | > >>> + #include <dt-bindings/interrupt-controller/irq.h> > >>> + i2c0 { > >>> + #address-cells = <1>; > >>> + #size-cells = <0>; > >>> + > >>> + fuel-gauge@36 { > >>> + compatible = "maxim,max17043"; > >>> + reg = <0x36>; > >>> + > >>> + interrupt-parent = <&gpio>; > >>> + interrupts = <144 IRQ_TYPE_EDGE_FALLING>; > >>> + > >>> + monitored-battery = <&battery>; > >>> + power-supplies = <&charger>; > >> > >> But here you suggests something else than VDD... The hardware does not > >> take charger as input. It takes power supply - vdd. > >> > > > > Power system allows passing properties from other power devices. > > In this case battery health and status are passed from charger. > > So this is not an input to device? Then it does not really look like > property of this hardware. Fuel gauge does not control the charger, also > from system configuration point of view. > It is not controlling charger, the charger provides the status and health of the battery to the fuel gauge. This option is also used in other fuel gauges. > Best regards, > Krzysztof >
ср, 8 бер. 2023 р. о 12:53 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> пише: > > On 08/03/2023 11:51, Svyatoslav Ryhel wrote: > > ср, 8 бер. 2023 р. о 12:44 Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> пише: > >> > >> On 08/03/2023 10:15, Svyatoslav Ryhel wrote: > >> > >>>> max17040 does not have ADC temperature input... so is it system > >>>> configuration? > >>>> > >>> > >>> yes, I own a device (LG Optimus Vu P895) which uses max17043 > >>> coupled with ADC thermal sensor > >>> > >>>>> + > >>>>> + io-channel-names: > >>>>> + items: > >>>>> + - const: temp > >>>> > >>>> Drop the names property, not needed for one item. > >>>> > >>> > >>> Alright, but driver patch expects temp name. I will look if this > >>> is adjustable. > >> > >> I think I saw cases without names. > >> > > > > There is no io-channel without a name. And io-channels are mostly used > > by power supply devices. > > > >>> > >>>>> + > >>>>> wakeup-source: > >>>>> type: boolean > >>>>> description: | > >>>>> @@ -95,3 +109,26 @@ examples: > >>>>> wakeup-source; > >>>>> }; > >>>>> }; > >>>>> + - | > >>>>> + #include <dt-bindings/interrupt-controller/irq.h> > >>>>> + i2c0 { > >>>>> + #address-cells = <1>; > >>>>> + #size-cells = <0>; > >>>>> + > >>>>> + fuel-gauge@36 { > >>>>> + compatible = "maxim,max17043"; > >>>>> + reg = <0x36>; > >>>>> + > >>>>> + interrupt-parent = <&gpio>; > >>>>> + interrupts = <144 IRQ_TYPE_EDGE_FALLING>; > >>>>> + > >>>>> + monitored-battery = <&battery>; > >>>>> + power-supplies = <&charger>; > >>>> > >>>> But here you suggests something else than VDD... The hardware does not > >>>> take charger as input. It takes power supply - vdd. > >>>> > >>> > >>> Power system allows passing properties from other power devices. > >>> In this case battery health and status are passed from charger. > >> > >> So this is not an input to device? Then it does not really look like > >> property of this hardware. Fuel gauge does not control the charger, also > >> from system configuration point of view. > >> > > > > It is not controlling charger, the charger provides the status and > > health of the battery to the fuel gauge. This option is also used in > > other fuel gauges. > > How regulator provides health and status of the battery? I don't understand. > It is not a regulator, it is a charger! Dedicated chip responsible for controlling charging. And its configuration allows it to get battery health and status, because this fuel gauge does not have this function. > Best regards, > Krzysztof >