[v3,1/2] power: supply: Add STC3117 fuel gauge unit driver

Adding minimal support for stc3117 fuel gauge driver
to read battery voltage level

Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
---
Changelogs :

v2 -> v3
- Resolved kernel test robot build warnings
- Aligned included header files in alphabetical order
- Removed unused header files
- Removed unnecessary blank lines
- Aligned all the macros in alphabetical order
- Changed macro LSB_VALUE to VOLTAGE_LSB_VALUE
- Dropped function prototypes and arranged the code accordingly
- Used macros instead of static numbers for array declaration
- Removed redundant code
- Replaced 'power_supply_register' with 'devm_power_supply_register' and 'pr_err' with 'dev_err'
- Removed global variables

v1 -> v2
- No change
---
 drivers/power/supply/Kconfig              |   7 ++
 drivers/power/supply/Makefile             |   1 +
 drivers/power/supply/stc3117_fuel_gauge.c | 107 ++++++++++++++++++++++
 3 files changed, 115 insertions(+)
 create mode 100644 drivers/power/supply/stc3117_fuel_gauge.c

On 05/02/2024 06:13, Bhavin Sharma wrote:
> Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>

You cannot have empty commits.

Subject: It's empty? What happened there?

> ---
> Changelogs :
> 
> v2 -> v3
> - Resolved DTC warnings and errors
> - Formatted the changelogs
> - Added monitored battery properties
> - Replaced 'additionalProperties' with 'unevaluatedProperties'
> - Replaced '&i2c6' with 'i2c'
> 
> v1 -> v2
> - String value is redundantly quoted with any quotes (quoted-strings)
> - Found character '\t' that cannot start any token
> ---

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

>  .../bindings/power/supply/st,stc3117.yaml     | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/st,stc3117.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/st,stc3117.yaml b/Documentation/devicetree/bindings/power/supply/st,stc3117.yaml
> new file mode 100644
> index 000000000000..9ab0b0d6b30e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/st,stc3117.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Dual license, just like checkpatch asks.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/st,stc3117.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STMicroelectronics STC3117 Fuel Gauge Unit Power Supply
> +
> +maintainers:
> +  - Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> +  - Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> +
> +allOf:
> +  - $ref: power-supply.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - st,stc3117
> +
> +  reg:
> +    maxItems: 1

Are you going to answer my questions or just ignore them?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    bat: battery {

Drop node


> +       compatible = "simple-battery";
> +       device-chemistry = "lithium-ion-polymer";
> +       energy-full-design-microwatt-hours = <16800000>;
> +       charge-full-design-microamp-hours = <4000000>;
> +       voltage-min-design-microvolt = <3000000>;
> +    };
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      stc3117@70 {

Still wrong name.

This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.


Best regards,
Krzysztof

Hi,

I have received comments for documentation patch of this driver and I am working on it. 

Any comments or suggestions here?


Regards,
Bhavin Sharma

Hi,

On Mon, Feb 05, 2024 at 10:43:17AM +0530, Bhavin Sharma wrote:
> Adding minimal support for stc3117 fuel gauge driver
> to read battery voltage level

Why only voltage?
It should be easy to support more data exposed by the chip.

> Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> ---
> Changelogs :
> 
> v2 -> v3
> - Resolved kernel test robot build warnings
> - Aligned included header files in alphabetical order
> - Removed unused header files
> - Removed unnecessary blank lines
> - Aligned all the macros in alphabetical order
> - Changed macro LSB_VALUE to VOLTAGE_LSB_VALUE
> - Dropped function prototypes and arranged the code accordingly
> - Used macros instead of static numbers for array declaration
> - Removed redundant code
> - Replaced 'power_supply_register' with 'devm_power_supply_register' and 'pr_err' with 'dev_err'
> - Removed global variables
> 
> v1 -> v2
> - No change
> ---
>  drivers/power/supply/Kconfig              |   7 ++
>  drivers/power/supply/Makefile             |   1 +
>  drivers/power/supply/stc3117_fuel_gauge.c | 107 ++++++++++++++++++++++
>  3 files changed, 115 insertions(+)
>  create mode 100644 drivers/power/supply/stc3117_fuel_gauge.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index f21cb05815ec..e2e3af4bcd5f 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -875,6 +875,13 @@ config FUEL_GAUGE_SC27XX
>  	  Say Y here to enable support for fuel gauge with SC27XX
>  	  PMIC chips.
>  
> +config FUEL_GAUGE_STC3117
> +        tristate "STMicroelectronics STC3117 fuel gauge driver"
> +        depends on I2C
> +        help
> +          Say Y here to enable support for fuel gauge with STC3117
> +          PMIC chips.
> +
>  config CHARGER_UCS1002
>  	tristate "Microchip UCS1002 USB Port Power Controller"
>  	depends on I2C
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 58b567278034..be8961661bd1 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -104,6 +104,7 @@ obj-$(CONFIG_CHARGER_CROS_USBPD)	+= cros_usbpd-charger.o
>  obj-$(CONFIG_CHARGER_CROS_PCHG)	+= cros_peripheral_charger.o
>  obj-$(CONFIG_CHARGER_SC2731)	+= sc2731_charger.o
>  obj-$(CONFIG_FUEL_GAUGE_SC27XX)	+= sc27xx_fuel_gauge.o
> +obj-$(CONFIG_FUEL_GAUGE_STC3117)        += stc3117_fuel_gauge.o
>  obj-$(CONFIG_CHARGER_UCS1002)	+= ucs1002_power.o
>  obj-$(CONFIG_CHARGER_BD99954)	+= bd99954-charger.o
>  obj-$(CONFIG_CHARGER_WILCO)	+= wilco-charger.o
> diff --git a/drivers/power/supply/stc3117_fuel_gauge.c b/drivers/power/supply/stc3117_fuel_gauge.c
> new file mode 100644
> index 000000000000..29eb00b44e21
> --- /dev/null
> +++ b/drivers/power/supply/stc3117_fuel_gauge.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * stc3117_fuel_gauge.c - STMicroelectronics STC3117 Fuel Gauge Driver
> + *
> + * Copyright (c) 2024 Silicon Signals Pvt Ltd.
> + * Author:      Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> + *              Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.com>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/power_supply.h>
> +
> +#define VOLTAGE_DATA_SIZE	2		// in bytes
> +#define VOLTAGE_LSB_VALUE	2200		// in micro-volts
> +#define VOLTAGE_REG_ADDR	0x08
> +#define VOLTAGE_REG_ADDR_SIZE	1		// in bytes
> +
> +static int stc3117_get_batt_volt(const struct i2c_client *stc_client)
> +{
> +	int ret, volt = 0;
> +	char i2c_tx = VOLTAGE_REG_ADDR, i2c_rx[VOLTAGE_DATA_SIZE] = {0};
> +
> +	ret = i2c_master_send(stc_client, &i2c_tx, VOLTAGE_REG_ADDR_SIZE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_master_recv(stc_client, i2c_rx, VOLTAGE_DATA_SIZE);
> +	if (ret < 0)
> +		return ret;
> +
> +	volt = (i2c_rx[1] << 8) + i2c_rx[0];
> +	volt *= VOLTAGE_LSB_VALUE;
> +
> +	return volt;
> +}

Please use regmap.

> +static int stc3117_get_property(struct power_supply *psy,
> +	enum power_supply_property psp, union power_supply_propval *val)
> +{
> +	struct i2c_client *client = to_i2c_client(psy->dev.parent);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		val->intval = stc3117_get_batt_volt(client);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static enum power_supply_property stc3117_battery_props[] = {
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +};
> +
> +static const struct power_supply_desc stc3117_battery_desc = {
> +	.name = "stc3117-battery",
> +	.type = POWER_SUPPLY_TYPE_BATTERY,
> +	.get_property = stc3117_get_property,
> +	.properties = stc3117_battery_props,
> +	.num_properties = ARRAY_SIZE(stc3117_battery_props),
> +};
> +
> +static int stc3117_probe(struct i2c_client *client)
> +{
> +	struct power_supply_config psy_cfg = {};
> +	struct device *dev;
> +	struct power_supply *stc_sply;
> +
> +	dev = &client->dev;

Just initialize this at declaration time.

> +	psy_cfg.of_node = dev->of_node;
> +
> +	stc_sply = devm_power_supply_register(dev, &stc3117_battery_desc, &psy_cfg);
> +	if (IS_ERR(stc_sply))
> +		dev_err(dev, "failed to register battery\n");

return dev_err_probe(dev, PTR_ERR(stc_sply), "failed to register battery\n");

> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id stc3117_id[] = {
> +	{"stc3117", 0},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, stc3117_id);
> +
> +static const struct of_device_id stc3117_of_match[] = {
> +	{ .compatible = "st,stc3117" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, stc3117_of_match);
> +
> +static struct i2c_driver stc3117_i2c_driver = {
> +	.driver = {
> +		.name = "stc3117_i2c_driver",
> +		.of_match_table = stc3117_of_match,
> +	},
> +	.probe = stc3117_probe,
> +	.id_table = stc3117_id,
> +};
> +
> +module_i2c_driver(stc3117_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Bhavin Sharma <bhavin.sharma@siliconsignals.io>");
> +MODULE_AUTHOR("Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>");
> +MODULE_DESCRIPTION("STC3117 Fuel Gauge Driver");

Greetings,

-- Sebastian

Hi Sebastian 

I apologize if I'm mistaken, but I noticed other minimal drivers in the codebase. My understanding is that using a minimal driver shouldn't cause any issues. Additionally, we can easily update this driver at any time, as we're actively updating all other drivers.

Regards,
Bhavin Sharma

Hi,

On Fri, Feb 16, 2024 at 01:34:11PM +0000, Bhavin Sharma wrote:
> I apologize if I'm mistaken, but I noticed other minimal drivers
> in the codebase.

Please point me to a driver, which just exposes the voltage.

> My understanding is that using a minimal driver shouldn't cause
> any issues. Additionally, we can easily update this driver at any
> time, as we're actively updating all other drivers.

So why are you not doing it now? Adding current, state of charge,
temperature and OCV is trivial. I'm not talking about supporting
every feature of the chip, but just the bits that are a simple
mapping between standard power supply properties and a chip
register.

-- Sebastian