Date: Wed, 10 Apr 2024 09:56:05 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Mike Looijmans <mike.looijmans@...ic.nl>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] power: supply: ltc3350-charger: Add driver

Hi,

On Tue, Apr 09, 2024 at 03:54:41PM +0200, Mike Looijmans wrote:
> The LTC3350 is a backup power controller that can charge and monitor
> a series stack of one to four supercapacitors.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>
> 
> ---

please share output of
./tools/testing/selftests/power_supply/test_power_supply_properties.sh
below the fold with your next submission. It's useful for verifying,
that you got the unit scaling correct for the standard properties :)

> (no changes since v2)
> 
> Changes in v2:
> Duplicate "vin_ov" and "vin_uv" attributes
> 
>  drivers/power/supply/Kconfig           |  10 +
>  drivers/power/supply/Makefile          |   1 +
>  drivers/power/supply/ltc3350-charger.c | 718 +++++++++++++++++++++++++
>  3 files changed, 729 insertions(+)
>  create mode 100644 drivers/power/supply/ltc3350-charger.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 3e31375491d5..7cb1a66e522d 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -514,6 +514,16 @@ config CHARGER_LT3651
>  	  Say Y to include support for the Analog Devices (Linear Technology)
>  	  LT3651 battery charger which reports its status via GPIO lines.
>  
> +config CHARGER_LTC3350
> +	tristate "LTC3350 Supercapacitor Backup Controller and System Monitor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	select I2C_SMBUS
> +	help
> +	  Say Y to include support for the Analog Devices (Linear Technology)
> +	  LTC3350 Supercapacitor Backup Controller and System Monitor connected
> +	  to I2C.
> +
>  config CHARGER_LTC4162L
>  	tristate "LTC4162-L charger"
>  	depends on I2C
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 58b567278034..a8d618e4ac91 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_CHARGER_LP8788)	+= lp8788-charger.o
>  obj-$(CONFIG_CHARGER_GPIO)	+= gpio-charger.o
>  obj-$(CONFIG_CHARGER_MANAGER)	+= charger-manager.o
>  obj-$(CONFIG_CHARGER_LT3651)	+= lt3651-charger.o
> +obj-$(CONFIG_CHARGER_LTC3350)	+= ltc3350-charger.o
>  obj-$(CONFIG_CHARGER_LTC4162L)	+= ltc4162-l-charger.o
>  obj-$(CONFIG_CHARGER_MAX14577)	+= max14577_charger.o
>  obj-$(CONFIG_CHARGER_DETECTOR_MAX14656)	+= max14656_charger_detector.o
> diff --git a/drivers/power/supply/ltc3350-charger.c b/drivers/power/supply/ltc3350-charger.c
> new file mode 100644
> index 000000000000..84c7a3ca914e
> --- /dev/null
> +++ b/drivers/power/supply/ltc3350-charger.c
> @@ -0,0 +1,718 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for Analog Devices (Linear Technology) LTC3350
> + * High Current Supercapacitor Backup Controller and System Monitor
> + * Copyright (C) 2024, Topic Embedded Products
> + */
> +
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/of_device.h>

replace of_device.h with mod_devicetable.h (which defines of_device_id).

> +#include <linux/pm_runtime.h>
> +#include <linux/power_supply.h>

add

#include <linux/property.h>

since you are using device_property_*

> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +
> +/* Registers (names based on what datasheet uses) */
> +#define LTC3350_REG_CLR_ALARMS		0x00
> +#define LTC3350_REG_MSK_ALARMS		0x01
> +#define LTC3350_REG_MSK_MON_STATUS	0x02
> +#define LTC3350_REG_CAP_ESR_PER		0x04
> +#define LTC3350_REG_VCAPFB_DAC		0x05
> +#define LTC3350_REG_VSHUNT		0x06
> +#define LTC3350_REG_CAP_UV_LVL		0x07
> +#define LTC3350_REG_CAP_OV_LVL		0x08
> +#define LTC3350_REG_GPI_UV_LVL		0x09
> +#define LTC3350_REG_GPI_OV_LVL		0x0A
> +#define LTC3350_REG_VIN_UV_LVL		0x0B
> +#define LTC3350_REG_VIN_OV_LVL		0x0C
> +#define LTC3350_REG_VCAP_UV_LVL		0x0D
> +#define LTC3350_REG_VCAP_OV_LVL		0x0E
> +#define LTC3350_REG_VOUT_UV_LVL		0x0F
> +#define LTC3350_REG_VOUT_OV_LVL		0x10
> +#define LTC3350_REG_IIN_OC_LVL		0x11
> +#define LTC3350_REG_ICHG_UC_LVL		0x12
> +#define LTC3350_REG_DTEMP_COLD_LVL	0x13
> +#define LTC3350_REG_DTEMP_HOT_LVL	0x14
> +#define LTC3350_REG_ESR_HI_LVL		0x15
> +#define LTC3350_REG_CAP_LO_LVL		0x16
> +#define LTC3350_REG_CTL_REG		0x17
> +#define LTC3350_REG_NUM_CAPS		0x1A
> +#define LTC3350_REG_CHRG_STATUS		0x1B
> +#define LTC3350_REG_MON_STATUS		0x1C
> +#define LTC3350_REG_ALARM_REG		0x1D
> +#define LTC3350_REG_MEAS_CAP		0x1E
> +#define LTC3350_REG_MEAS_ESR		0x1F
> +#define LTC3350_REG_MEAS_VCAP1		0x20
> +#define LTC3350_REG_MEAS_VCAP2		0x21
> +#define LTC3350_REG_MEAS_VCAP3		0x22
> +#define LTC3350_REG_MEAS_VCAP4		0x23
> +#define LTC3350_REG_MEAS_GPI		0x24
> +#define LTC3350_REG_MEAS_VIN		0x25
> +#define LTC3350_REG_MEAS_VCAP		0x26
> +#define LTC3350_REG_MEAS_VOUT		0x27
> +#define LTC3350_REG_MEAS_IIN		0x28
> +#define LTC3350_REG_MEAS_ICHG		0x29
> +#define LTC3350_REG_MEAS_DTEMP		0x2A
> +
> +/* LTC3350_REG_CLR_ALARMS, LTC3350_REG_MASK_ALARMS, LTC3350_REG_ALARM_REG */
> +#define LTC3350_MSK_CAP_UV	BIT(0)	/* capacitor undervoltage alarm */
> +#define LTC3350_MSK_CAP_OV	BIT(1)	/* capacitor overvoltage alarm */
> +#define LTC3350_MSK_GPI_UV	BIT(2)	/* GPI undervoltage alarm */
> +#define LTC3350_MSK_GPI_OV	BIT(3)	/* GPI overvoltage alarm */
> +#define LTC3350_MSK_VIN_UV	BIT(4)	/* VIN undervoltage alarm */
> +#define LTC3350_MSK_VIN_OV	BIT(5)	/* VIN overvoltage alarm */
> +#define LTC3350_MSK_VCAP_UV	BIT(6)	/* VCAP undervoltage alarm */
> +#define LTC3350_MSK_VCAP_OV	BIT(7)	/* VCAP overvoltage alarm */
> +#define LTC3350_MSK_VOUT_UV	BIT(8)	/* VOUT undervoltage alarm */
> +#define LTC3350_MSK_VOUT_OV	BIT(9)	/* VOUT overvoltage alarm */
> +#define LTC3350_MSK_IIN_OC	BIT(10)	/* input overcurrent alarm */
> +#define LTC3350_MSK_ICHG_UC	BIT(11)	/* charge undercurrent alarm */
> +#define LTC3350_MSK_DTEMP_COLD	BIT(12)	/* die temperature cold alarm */
> +#define LTC3350_MSK_DTEMP_HOT	BIT(13)	/* die temperature hot alarm */
> +#define LTC3350_MSK_ESR_HI	BIT(14)	/* ESR high alarm */
> +#define LTC3350_MSK_CAP_LO	BIT(15)	/* capacitance low alarm */
> +
> +/* LTC3350_REG_MSK_MON_STATUS masks */
> +#define LTC3350_MSK_MON_CAPESR_ACTIVE		BIT(0)
> +#define LTC3350_MSK_MON_CAPESR_SCHEDULED	BIT(1)
> +#define LTC3350_MSK_MON_CAPESR_PENDING		BIT(2)
> +#define LTC3350_MSK_MON_CAP_DONE		BIT(3)
> +#define LTC3350_MSK_MON_ESR_DONE		BIT(4)
> +#define LTC3350_MSK_MON_CAP_FAILED		BIT(5)
> +#define LTC3350_MSK_MON_ESR_FAILED		BIT(6)
> +#define LTC3350_MSK_MON_POWER_FAILED		BIT(8)
> +#define LTC3350_MSK_MON_POWER_RETURNED		BIT(9)
> +
> +/* LTC3350_REG_CTL_REG */
> +/* Begin a capacitance and ESR measurement when possible */
> +#define LTC3350_CTL_STRT_CAPESR		BIT(0)
> +/* A one in this bit location enables the input buffer on the GPI pin */
> +#define LTC3350_CTL_GPI_BUFFER_EN	BIT(1)
> +/* Stops an active capacitance/ESR measurement */
> +#define LTC3350_CTL_STOP_CAPESR		BIT(2)
> +/* Increases capacitor measurement resolution by 100x for smaller capacitors */
> +#define LTC3350_CTL_CAP_SCALE		BIT(3)
> +
> +/* LTC3350_REG_CHRG_STATUS */
> +#define LTC3350_CHRG_STEPDOWN	BIT(0)	/* Synchronous controller in step-down mode (charging) */
> +#define LTC3350_CHRG_STEPUP	BIT(1)	/* Synchronous controller in step-up mode (backup) */
> +#define LTC3350_CHRG_CV		BIT(2)	/* The charger is in constant voltage mode */
> +#define LTC3350_CHRG_UVLO	BIT(3)	/* The charger is in undervoltage lockout */
> +#define LTC3350_CHRG_INPUT_ILIM	BIT(4)	/* The charger is in input current limit */
> +#define LTC3350_CHRG_CAPPG	BIT(5)	/* The capacitor voltage is above power good threshold */
> +#define LTC3350_CHRG_SHNT	BIT(6)	/* The capacitor manager is shunting */
> +#define LTC3350_CHRG_BAL	BIT(7)	/* The capacitor manager is balancing */
> +#define LTC3350_CHRG_DIS	BIT(8)	/* Charger disabled for capacitance measurement */
> +#define LTC3350_CHRG_CI		BIT(9)	/* The charger is in constant current mode */
> +#define LTC3350_CHRG_PFO	BIT(11)	/* Input voltage is below PFI threshold */
> +
> +/* LTC3350_REG_MON_STATUS */
> +#define LTC3350_MON_CAPESR_ACTIVE	BIT(0)	/* Capacitance/ESR measurement in progress */
> +#define LTC3350_MON_CAPESR_SCHEDULED	BIT(1)	/* Waiting programmed time */
> +#define LTC3350_MON_CAPESR_PENDING	BIT(2)	/* Waiting for satisfactory conditions */
> +#define LTC3350_MON_CAP_DONE		BIT(3)	/* Capacitance measurement has completed */
> +#define LTC3350_MON_ESR_DONE		BIT(4)	/* ESR Measurement has completed */
> +#define LTC3350_MON_CAP_FAILED		BIT(5)	/* Last capacitance measurement failed */
> +#define LTC3350_MON_ESR_FAILED		BIT(6)	/* Last ESR measurement failed */
> +#define LTC3350_MON_POWER_FAILED	BIT(8)	/* Unable to charge */
> +#define LTC3350_MON_POWER_RETURNED	BIT(9)	/* Able to charge */
> +
> +
> +struct ltc3350_info {
> +	struct i2c_client	*client;
> +	struct regmap		*regmap;
> +	struct power_supply	*charger;
> +	u32 rsnsc;	/* Series resistor that sets charge current, microOhm */
> +	u32 rsnsi;	/* Series resistor to measure input current, microOhm */
> +};
> +
> +/*
> + * About LTC3350 "alarm" functions: Setting a bit in the LTC3350_REG_MSK_ALARMS
> + * register enables the alarm. The alarm will trigger an SMBALERT only once.
> + * To reset the alarm, write a "1" bit to LTC3350_REG_CLR_ALARMS. Then the alarm
> + * will trigger another SMBALERT when conditions are met (may be immediately).
> + * After writing to one of the corresponding level registers, enable the alarm,
> + * so that a UEVENT triggers when the alarm goes off.
> + */
> +static void ltc3350_enable_alarm(struct ltc3350_info *info, unsigned int reg)
> +{
> +	unsigned int mask;
> +
> +	/* Register locations correspond to alarm mask bits */
> +	mask = BIT(reg - LTC3350_REG_CAP_UV_LVL);
> +	/* Clear the alarm bit so it may trigger again */
> +	regmap_write(info->regmap, LTC3350_REG_CLR_ALARMS, mask);
> +	/* Enable the alarm */
> +	regmap_update_bits(info->regmap, LTC3350_REG_MSK_ALARMS, mask, mask);
> +}
> +
> +/* Convert enum value to POWER_SUPPLY_STATUS value */
> +static int ltc3350_state_decode(unsigned int value)
> +{
> +	if (value & LTC3350_CHRG_STEPUP)
> +		return POWER_SUPPLY_STATUS_DISCHARGING; /* running on backup */
> +
> +	if (value & LTC3350_CHRG_PFO)
> +		return POWER_SUPPLY_STATUS_NOT_CHARGING;
> +
> +	if (value & LTC3350_CHRG_STEPDOWN) {
> +		/* The chip remains in CV mode indefinitely, hence "full" */
> +		if (value & LTC3350_CHRG_CV)
> +			return POWER_SUPPLY_STATUS_FULL;
> +
> +		return POWER_SUPPLY_STATUS_CHARGING;
> +	}
> +
> +	/* Not in step down? Must be full then (never seen this) */
> +	return POWER_SUPPLY_STATUS_FULL;
> +};
> +
> +static int ltc3350_get_status(struct ltc3350_info *info, union power_supply_propval *val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, LTC3350_REG_CHRG_STATUS, &regval);
> +	if (ret)
> +		return ret;
> +
> +	val->intval = ltc3350_state_decode(regval);
> +
> +	return 0;
> +}
> +
> +static int ltc3350_charge_status_decode(unsigned int value)
> +{
> +	if (!(value & LTC3350_CHRG_STEPDOWN))
> +		return POWER_SUPPLY_CHARGE_TYPE_NONE;
> +
> +	/* constant voltage is "topping off" */
> +	if (value & LTC3350_CHRG_CV)
> +		return POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> +
> +	/* input limiter */
> +	if (value & LTC3350_CHRG_INPUT_ILIM)
> +		return POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE;
> +
> +	/* constant current is "fast" */
> +	if (value & LTC3350_CHRG_CI)
> +		return POWER_SUPPLY_CHARGE_TYPE_FAST;
> +
> +	return POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
> +}
> +
> +static int ltc3350_get_charge_type(struct ltc3350_info *info, union power_supply_propval *val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, LTC3350_REG_CHRG_STATUS, &regval);
> +	if (ret)
> +		return ret;
> +
> +	val->intval = ltc3350_charge_status_decode(regval);
> +
> +	return 0;
> +}
> +
> +static int ltc3350_get_online(struct ltc3350_info *info, union power_supply_propval *val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, LTC3350_REG_MON_STATUS, &regval);
> +	if (ret)
> +		return ret;
> +
> +	/* indicates if input voltage is sufficient to charge */
> +	val->intval = !!(regval & LTC3350_MON_POWER_RETURNED);
> +
> +	return 0;
> +}
> +
> +static int ltc3350_get_input_voltage(struct ltc3350_info *info, union power_supply_propval *val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, LTC3350_REG_MEAS_VIN, &regval);
> +	if (ret)
> +		return ret;
> +
> +	/* 2.21mV/LSB */
> +	val->intval =  regval * 2210;
> +
> +	return 0;
> +}
> +
> +static int ltc3350_get_input_current(struct ltc3350_info *info, union power_supply_propval *val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, LTC3350_REG_MEAS_IIN, &regval);
> +	if (ret)
> +		return ret;
> +
> +	/* 1.983µV/RSNSI amperes per LSB */
> +	ret = regval * 19830;
> +	ret /= info->rsnsi;
> +	ret *= 100;
> +
> +	val->intval = ret;
> +
> +	return 0;
> +}
> +
> +static int ltc3350_get_icharge(struct ltc3350_info *info, union power_supply_propval *val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, LTC3350_REG_MEAS_ICHG, &regval);
> +	if (ret)
> +		return ret;
> +
> +	/* 1.983µV/RSNSC amperes per LSB */
> +	ret = regval * 19830;
> +	ret /= info->rsnsc;
> +	ret *= 100;
> +
> +	val->intval = ret;
> +
> +	return 0;
> +}
> +
> +static int ltc3350_get_icharge_max(struct ltc3350_info *info, union power_supply_propval *val)
> +{
> +	/* I_CHG(MAX) = 32mV / RSNSC (Ampere) */
> +	val->intval = 3200000000U / (info->rsnsc / 10);
> +
> +	return 0;
> +}
> +
> +static int ltc3350_get_iin_max(struct ltc3350_info *info, union power_supply_propval *val)
> +{
> +	/* I_IN(MAX) = 32mV / RSNSI (Ampere) */
> +	val->intval = 3200000000U / (info->rsnsi / 10);
> +
> +	return 0;
> +}
> +
> +
> +static int ltc3350_get_die_temp(struct ltc3350_info *info, unsigned int reg,
> +				union power_supply_propval *val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, reg, &regval);
> +	if (ret)
> +		return ret;
> +
> +	/* 0.028°C per LSB – 251.4°C */
> +	ret = 280 * regval;
> +	ret /= 100; /* Centidegrees scale */
> +	ret -= 25140;
> +	val->intval = ret;
> +
> +	return 0;
> +}
> +
> +static int ltc3350_set_die_temp(struct ltc3350_info *info, unsigned int reg, int val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	/* 0.028°C per LSB – 251.4°C */
> +	regval = val + 25140;
> +	regval *= 100;
> +	regval /= 280;
> +
> +	ret = regmap_write(info->regmap, reg, regval);
> +	if (ret)
> +		return ret;
> +
> +	ltc3350_enable_alarm(info, reg);
> +	return 0;
> +}
> +
> +/* Custom properties */
> +
> +static ssize_t ltc3350_attr_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf,
> +				 unsigned int reg, unsigned int scale)
> +{
> +	struct power_supply *psy = to_power_supply(dev);
> +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, reg, &regval);
> +	if (ret)
> +		return ret;
> +
> +	regval *= scale; /* Scale is in 10 μV units */

please keep custom uAPI consistent with the general uAPI and use µV.

> +	regval /= 10;
> +
> +	return sprintf(buf, "%u\n", regval);
> +}
> +
> +static ssize_t ltc3350_attr_store(struct device *dev, struct device_attribute *attr,
> +				  const char *buf, size_t count,
> +				  unsigned int reg, unsigned int scale)
> +{
> +	struct power_supply *psy = to_power_supply(dev);
> +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	val *= 10;
> +	val = DIV_ROUND_CLOSEST(val, scale); /* Scale is in 10 μV units */

obviously also applied to writing.

> +
> +	ret = regmap_write(info->regmap, reg, val);
> +	if (ret)
> +		return ret;
> +
> +	/* When writing to one of the LVL registers, update the alarm mask */
> +	if (reg >= LTC3350_REG_CAP_UV_LVL && reg <= LTC3350_REG_CAP_LO_LVL)
> +		ltc3350_enable_alarm(info, reg);
> +
> +	return count;
> +}
> +
> +#define LTC3350_DEVICE_ATTR_RO(_name, _reg, _scale)				\
> +static ssize_t _name##_show(struct device *dev, struct device_attribute *attr,	\
> +			    char *buf)						\
> +{										\
> +	return ltc3350_attr_show(dev, attr, buf, _reg, _scale);			\
> +}										\
> +static DEVICE_ATTR_RO(_name)
> +
> +#define LTC3350_DEVICE_ATTR_RW(_name, _reg, _scale)				\
> +static ssize_t _name##_show(struct device *dev, struct device_attribute *attr,	\
> +			    char *buf)						\
> +{										\
> +	return ltc3350_attr_show(dev, attr, buf, _reg, _scale);			\
> +}										\
> +static ssize_t _name##_store(struct device *dev, struct device_attribute *attr, \
> +			     const char *buf, size_t count)			\
> +{										\
> +	return ltc3350_attr_store(dev, attr, buf, count, _reg, _scale);		\
> +}										\
> +static DEVICE_ATTR_RW(_name)
> +
> +/* Shunt voltage, 183.5μV per LSB */
> +LTC3350_DEVICE_ATTR_RW(vshunt, LTC3350_REG_VSHUNT, 1835);
> +
> +/* Single capacitor voltages, 183.5μV per LSB */
> +LTC3350_DEVICE_ATTR_RO(vcap1, LTC3350_REG_MEAS_VCAP1, 1835);
> +LTC3350_DEVICE_ATTR_RO(vcap2, LTC3350_REG_MEAS_VCAP2, 1835);
> +LTC3350_DEVICE_ATTR_RO(vcap3, LTC3350_REG_MEAS_VCAP3, 1835);
> +LTC3350_DEVICE_ATTR_RO(vcap4, LTC3350_REG_MEAS_VCAP4, 1835);
> +LTC3350_DEVICE_ATTR_RW(cap_uv, LTC3350_REG_CAP_UV_LVL, 1835);
> +LTC3350_DEVICE_ATTR_RW(cap_ov, LTC3350_REG_CAP_OV_LVL, 1835);
> +
> +/* General purpose input, 183.5μV per LSB */
> +LTC3350_DEVICE_ATTR_RO(gpi, LTC3350_REG_MEAS_GPI, 1835);
> +LTC3350_DEVICE_ATTR_RW(gpi_uv, LTC3350_REG_GPI_UV_LVL, 1835);
> +LTC3350_DEVICE_ATTR_RW(gpi_ov, LTC3350_REG_GPI_OV_LVL, 1835);
> +
> +/* Input voltage, 2.21mV per LSB */
> +LTC3350_DEVICE_ATTR_RO(vin, LTC3350_REG_MEAS_VIN, 22100);
> +LTC3350_DEVICE_ATTR_RW(vin_uv, LTC3350_REG_VIN_UV_LVL, 22100);
> +LTC3350_DEVICE_ATTR_RW(vin_ov, LTC3350_REG_VIN_OV_LVL, 22100);
> +
> +/* Capacitor stack voltage, 1.476 mV per LSB */
> +LTC3350_DEVICE_ATTR_RO(vcap, LTC3350_REG_MEAS_VCAP, 14760);
> +LTC3350_DEVICE_ATTR_RW(vcap_uv, LTC3350_REG_VCAP_UV_LVL, 14760);
> +LTC3350_DEVICE_ATTR_RW(vcap_ov, LTC3350_REG_VCAP_OV_LVL, 14760);

I suppose it would be sensible to expose this as a second
power_supply device of type battery with ltc3350_config.supplied_to
set to this second power_supply device.

Then you can map

LTC3350_REG_MEAS_VCAP -> VOLTAGE_NOW
LTC3350_REG_VCAP_UV_LVL -> VOLTAGE_MIN
LTC3350_REG_VCAP_OV_LVL -> VOLTAGE_MAX
LTC3350_REG_VSHUNT -> CURRENT_NOW
TECHNOLOGY = POWER_SUPPLY_TECHNOLOGY_CAPACITOR (new)

Also the single capacitor voltages are similar to per-cell voltage
information, so I think we should create generic properties for
that:

LTC3350_REG_NUM_CAPS   -> NUMBER_OF_CELLS (new)
LTC3350_REG_MEAS_VCAP1 -> CELL1_VOLTAGE_NOW (new)
LTC3350_REG_MEAS_VCAP2 -> CELL2_VOLTAGE_NOW (new)
LTC3350_REG_MEAS_VCAP3 -> CELL3_VOLTAGE_NOW (new)
LTC3350_REG_MEAS_VCAP4 -> CELL4_VOLTAGE_NOW (new)
LTC3350_REG_CAP_UV_LVL -> CELL_VOLTAGE_MIN (new)
LTC3350_REG_CAP_OV_LVL -> CELL_VOLTAGE_MAX (new)

> +/* Output, 2.21mV per LSB */
> +LTC3350_DEVICE_ATTR_RO(vout, LTC3350_REG_MEAS_VOUT, 22100);
> +LTC3350_DEVICE_ATTR_RW(vout_uv, LTC3350_REG_VOUT_UV_LVL, 22100);
> +LTC3350_DEVICE_ATTR_RW(vout_ov, LTC3350_REG_VOUT_OV_LVL, 22100);
> +
> +static ssize_t num_caps_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct power_supply *psy = to_power_supply(dev);
> +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, LTC3350_REG_NUM_CAPS, &regval);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "%u\n", regval + 1);
> +}
> +static DEVICE_ATTR_RO(num_caps);
> +
> +static ssize_t charge_status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct power_supply *psy = to_power_supply(dev);
> +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, LTC3350_REG_CHRG_STATUS, &regval);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "0x%x\n", regval);
> +}
> +static DEVICE_ATTR_RO(charge_status);
> +
> +static struct attribute *ltc3350_sysfs_entries[] = {
> +	&dev_attr_vshunt.attr,
> +	&dev_attr_vcap1.attr,
> +	&dev_attr_vcap2.attr,
> +	&dev_attr_vcap3.attr,
> +	&dev_attr_vcap4.attr,
> +	&dev_attr_cap_uv.attr,
> +	&dev_attr_cap_ov.attr,
> +	&dev_attr_gpi.attr,
> +	&dev_attr_gpi_uv.attr,
> +	&dev_attr_gpi_ov.attr,
> +	&dev_attr_vin.attr,
> +	&dev_attr_vin_uv.attr,
> +	&dev_attr_vin_ov.attr,
> +	&dev_attr_vcap.attr,
> +	&dev_attr_vcap_uv.attr,
> +	&dev_attr_vcap_ov.attr,
> +	&dev_attr_vout.attr,
> +	&dev_attr_vout_uv.attr,
> +	&dev_attr_vout_ov.attr,
> +	&dev_attr_num_caps.attr,
> +	&dev_attr_charge_status.attr,
> +	NULL,
> +};

Exposing these to sysfs makes them ABI and ABI needs to be
documented in Documentation/ABI, see for example

Documentation/ABI/testing/sysfs-class-power-rt9467

> +static const struct attribute_group ltc3350_attr_group = {
> +	.name	= NULL,	/* put in device directory */
> +	.attrs	= ltc3350_sysfs_entries,
> +};
> +
> +static const struct attribute_group *ltc3350_attr_groups[] = {
> +	&ltc3350_attr_group,
> +	NULL,
> +};
> +
> +static int ltc3350_get_property(struct power_supply *psy, enum power_supply_property psp,
> +				union power_supply_propval *val)
> +{
> +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		return ltc3350_get_status(info, val);
> +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> +		return ltc3350_get_charge_type(info, val);
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		return ltc3350_get_online(info, val);
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		return ltc3350_get_input_voltage(info, val);
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		return ltc3350_get_input_current(info, val);
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> +		return ltc3350_get_icharge(info, val);
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		return ltc3350_get_icharge_max(info, val);
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		return ltc3350_get_iin_max(info, val);
> +	case POWER_SUPPLY_PROP_TEMP:
> +		return ltc3350_get_die_temp(info, LTC3350_REG_MEAS_DTEMP, val);
> +	case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> +		return ltc3350_get_die_temp(info, LTC3350_REG_DTEMP_COLD_LVL, val);
> +	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> +		return ltc3350_get_die_temp(info, LTC3350_REG_DTEMP_HOT_LVL, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ltc3350_set_property(struct power_supply *psy, enum power_supply_property psp,
> +				const union power_supply_propval *val)
> +{
> +	struct ltc3350_info *info = power_supply_get_drvdata(psy);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> +		return ltc3350_set_die_temp(info, LTC3350_REG_DTEMP_COLD_LVL, val->intval);
> +	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> +		return ltc3350_set_die_temp(info, LTC3350_REG_DTEMP_HOT_LVL, val->intval);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ltc3350_property_is_writeable(struct power_supply *psy, enum power_supply_property psp)
> +{
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> +	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +/* Charger power supply property routines */
> +static enum power_supply_property ltc3350_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CHARGE_TYPE,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_TEMP,
> +	POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
> +	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
> +};
> +
> +static const struct power_supply_desc ltc3350_desc = {
> +	.name		= "ltc3350",
> +	.type		= POWER_SUPPLY_TYPE_MAINS,
> +	.properties	= ltc3350_properties,
> +	.num_properties	= ARRAY_SIZE(ltc3350_properties),
> +	.get_property	= ltc3350_get_property,
> +	.set_property	= ltc3350_set_property,
> +	.property_is_writeable = ltc3350_property_is_writeable,
> +};
> +
> +static bool ltc3350_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	/* all registers up to this one are writeable */
> +	return reg < LTC3350_REG_NUM_CAPS;
> +}
> +
> +static bool ltc3350_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	/* read-only status registers and self-clearing register */
> +	return reg > LTC3350_REG_NUM_CAPS || reg == LTC3350_REG_CLR_ALARMS;
> +}
> +
> +static const struct regmap_config ltc3350_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 16,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.writeable_reg	= ltc3350_is_writeable_reg,
> +	.volatile_reg	= ltc3350_is_volatile_reg,
> +	.max_register	= LTC3350_REG_MEAS_DTEMP,
> +	.cache_type	= REGCACHE_MAPLE,
> +};
> +
> +static int ltc3350_probe(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	struct device *dev = &client->dev;
> +	struct ltc3350_info *info;
> +	struct power_supply_config ltc3350_config = {};
> +	int ret;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> +		dev_err(dev, "No support for SMBUS_WORD_DATA\n");
> +		return -ENODEV;
> +	}

return dev_err_probe(dev, -ENODEV, "No support for SMBUS_WORD_DATA\n");

But I think this can just be dropped. devm_regmap_init_i2c() should
generate an error, if the i2c adapter requirements are not met.

> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->client = client;
> +	i2c_set_clientdata(client, info);
> +
> +	info->regmap = devm_regmap_init_i2c(client, &ltc3350_regmap_config);
> +	if (IS_ERR(info->regmap)) {
> +		dev_err(dev, "Failed to initialize register map\n");
> +		return PTR_ERR(info->regmap);
> +	}

dev_err_probe()

> +
> +	ret = device_property_read_u32(dev, "lltc,rsnsc-micro-ohms",
> +				       &info->rsnsc);
> +	if (ret) {
> +		dev_err(dev, "Missing lltc,rsnsc-micro-ohms property\n");
> +		return ret;
> +	}

dev_err_probe()

> +	if (!info->rsnsc)
> +		return -EINVAL;
> +
> +	ret = device_property_read_u32(dev, "lltc,rsnsi-micro-ohms",
> +				       &info->rsnsi);
> +	if (ret) {
> +		dev_err(dev, "Missing lltc,rsnsi-micro-ohms property\n");
> +		return ret;
> +	}

dev_err_probe()

> +	if (!info->rsnsi)
> +		return -EINVAL;
> +
> +	/* Clear and disable all interrupt sources*/
> +	ret = regmap_write(info->regmap, LTC3350_REG_CLR_ALARMS, 0xFFFF);
> +	if (ret) {
> +		dev_err(dev, "Failed to write configuration\n");
> +		return ret;
> +	}

dev_err_probe()

> +	regmap_write(info->regmap, LTC3350_REG_MSK_ALARMS, 0);
> +	regmap_write(info->regmap, LTC3350_REG_MSK_MON_STATUS, 0);
> +
> +	ltc3350_config.of_node = dev->of_node;

replace with

ltc3350_config.fwnode = dev_fwnode(dev);

> +	ltc3350_config.drv_data = info;
> +	ltc3350_config.attr_grp = ltc3350_attr_groups;
> +
> +	info->charger = devm_power_supply_register(dev, &ltc3350_desc,
> +						   &ltc3350_config);
> +	if (IS_ERR(info->charger)) {
> +		dev_err(dev, "Failed to register charger\n");
> +		return PTR_ERR(info->charger);
> +	}

dev_err_probe()

> +
> +	/* Enable interrupts on status changes */
> +	regmap_write(info->regmap, LTC3350_REG_MSK_MON_STATUS,
> +		     LTC3350_MON_POWER_FAILED | LTC3350_MON_POWER_RETURNED);
> +
> +	return 0;
> +}
> +
> +static void ltc3350_alert(struct i2c_client *client, enum i2c_alert_protocol type,
> +			  unsigned int flag)
> +{
> +	struct ltc3350_info *info = i2c_get_clientdata(client);
> +
> +	if (type != I2C_PROTOCOL_SMBUS_ALERT)
> +		return;
> +
> +	power_supply_changed(info->charger);
> +}
> +
> +static const struct i2c_device_id ltc3350_i2c_id_table[] = {
> +	{ "ltc3350", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc3350_i2c_id_table);
> +
> +static const struct of_device_id ltc3350_of_match[] = {
> +	{ .compatible = "lltc,ltc3350", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ltc3350_of_match);
> +
> +static struct i2c_driver ltc3350_driver = {
> +	.probe		= ltc3350_probe,
> +	.alert		= ltc3350_alert,
> +	.id_table	= ltc3350_i2c_id_table,
> +	.driver = {
> +		.name		= "ltc3350-charger",
> +		.of_match_table	= of_match_ptr(ltc3350_of_match),

Please check what of_match_ptr() does and then drop it :)

> +	},
> +};
> +module_i2c_driver(ltc3350_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Mike Looijmans <mike.looijmans@...ic.nl>");
> +MODULE_DESCRIPTION("LTC3350 charger driver");

Thanks for your patch,

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists