Re: [RFC 3/5] power: supply: bq257xx: Add support for BQ257XX charger manager

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Thu, Aug 29, 2024 at 04:31:00PM GMT, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> 
> Add support for the charger function of the BQ257XX. The device is
> capable of charging batteries with a layout of 1 to 4 cells in
> series.
> 
> Signed-off-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
> ---

I skipped reviewing the parts related to the requested binding changes.

>  drivers/power/supply/Kconfig           |   7 +
>  drivers/power/supply/Makefile          |   1 +
>  drivers/power/supply/bq257xx_charger.c | 811 +++++++++++++++++++++++++
>  3 files changed, 819 insertions(+)
>  create mode 100644 drivers/power/supply/bq257xx_charger.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index bcfa63fb9f1e..9c5327245264 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -720,6 +720,13 @@ config CHARGER_BQ2515X
>  	  rail, ADC for battery and system monitoring, and push-button
>  	  controller.
>  
> +config CHARGER_BQ257XX
> +	tristate "TI BQ257XX battery charger family"
> +	depends on MFD_BQ257XX
> +	help
> +	  Say Y to enable support for the TI BQ257XX family of battery
> +	  charging integrated circuits.
> +
>  config CHARGER_BQ25890
>  	tristate "TI BQ25890 battery charger driver"
>  	depends on I2C
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 8dcb41545317..7652e9575f75 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -93,6 +93,7 @@ obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
>  obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
>  obj-$(CONFIG_CHARGER_BQ24735)	+= bq24735-charger.o
>  obj-$(CONFIG_CHARGER_BQ2515X)	+= bq2515x_charger.o
> +obj-$(CONFIG_CHARGER_BQ257XX)	+= bq257xx_charger.o
>  obj-$(CONFIG_CHARGER_BQ25890)	+= bq25890_charger.o
>  obj-$(CONFIG_CHARGER_BQ25980)	+= bq25980_charger.o
>  obj-$(CONFIG_CHARGER_BQ256XX)	+= bq256xx_charger.o
> diff --git a/drivers/power/supply/bq257xx_charger.c b/drivers/power/supply/bq257xx_charger.c
> new file mode 100644
> index 000000000000..58dcb8583897
> --- /dev/null
> +++ b/drivers/power/supply/bq257xx_charger.c
> @@ -0,0 +1,811 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* BQ257XX Battery Charger Driver
> + * Copyright (C) 2024 Chris Morgan <macromorgan@xxxxxxxxxxx>
> + * Based off of BQ256XX Battery Charger Driver
> + * Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>

is the i2c header actually used?

> +#include <linux/mfd/bq257xx.h>
> +#include <linux/moduleparam.h>

development leftover?

> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +
> +/* Forward declaration of driver data. */
> +struct bq257xx_chg;
> +
> +/**
> + * struct bq257xx_chip_info - chip specific routines
> + * @bq257xx_hw_init: init function for hw
> + * @bq257xx_hw_shutdown: shutdown function for hw
> + * @bq257xx_get_state: get and update state of hardware
> + * @bq257xx_set_ichg: set maximum charge current (in uA)
> + * @bq257xx_set_vbatreg: set maximum charge voltage (in uV)
> + * @bq257xx_set_iindpm: set maximum input current (in uA)
> + *
> + * Functions for variants of the BQ257XX IC revisions that are made
> + * available generic portions of the driver code (such as the main
> + * probe function).
> + */
> +struct bq257xx_chip_info {
> +	int (*bq257xx_hw_init)(struct bq257xx_chg *pdata);
> +	void (*bq257xx_hw_shutdown)(struct bq257xx_chg *pdata);
> +	int (*bq257xx_get_state)(struct bq257xx_chg *pdata);
> +	int (*bq257xx_set_ichg)(struct bq257xx_chg *pdata, int ichg);
> +	int (*bq257xx_set_vbatreg)(struct bq257xx_chg *pdata, int vbatreg);
> +	int (*bq257xx_set_iindpm)(struct bq257xx_chg *pdata, int iindpm);
> +};
> +
> +/**
> + * struct bq257xx_chg - driver data for charger
> + * @bq257xx_chip_info: hw specific functions
> + * @bq257xx_device: parent MFD device
> + * @charger: power supply device
> + * @online: charger input is present
> + * @fast_charge: charger is in fast charge mode
> + * @pre_charge: charger is in pre-charge mode
> + * @ov_fault: charger reports over voltage fault
> + * @batoc_fault: charger reports battery over current fault
> + * @oc_fault: charger reports over current fault
> + * @usb_type: USB type reported from parent power supply
> + * @supplied: Status of parent power supply
> + * @iindpm_max: maximum input current limit (uA)
> + * @vbat_max: maximum charge voltage (uV)
> + * @ichg_max: maximum charge current (uA)
> + * @vsys_min: minimum system voltage (uV)
> + */
> +struct bq257xx_chg {
> +	const struct bq257xx_chip_info *chip;
> +	struct bq257xx_device *bq;
> +	struct power_supply *charger;
> +	bool online;
> +	bool fast_charge;
> +	bool pre_charge;
> +	bool ov_fault;
> +	bool batoc_fault;
> +	bool oc_fault;
> +	int usb_type;
> +	int supplied;
> +	u32 iindpm_max;
> +	u32 vbat_max;
> +	u32 ichg_max;
> +	u32 vsys_min;
> +};
> +
> +/**
> + * bq25703_get_state() - Get the current state of the device
> + * @pdata: driver platform data
> + *
> + * Get the current state of the charger. Check if the charger is
> + * powered, what kind of charge state (if any) the device is in,
> + * and if there are any active faults.
> + *
> + * Return: Returns 0 on success, or error on failure to read device.
> + */
> +static int bq25703_get_state(struct bq257xx_chg *pdata)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(pdata->bq->regmap, BQ25703_CHARGER_STATUS, &reg);
> +	if (ret)
> +		return ret;
> +
> +	pdata->online = reg & BQ25703_STS_AC_STAT;
> +	pdata->fast_charge = reg & BQ25703_STS_IN_FCHRG;
> +	pdata->pre_charge = reg & BQ25703_STS_IN_PCHRG;
> +	pdata->ov_fault = reg & BQ25703_STS_FAULT_ACOV;
> +	pdata->batoc_fault = reg & BQ25703_STS_FAULT_BATOC;
> +	pdata->oc_fault = reg & BQ25703_STS_FAULT_ACOC;
> +
> +	return 0;
> +}
> +
> +/**
> + * bq25703_get_min_vsys() - Get the minimum system voltage
> + * @pdata: driver platform data
> + *
> + * Return: Returns minimum system voltage in uV on success, or error on
> + *         failure to read device.
> + */
> +static int bq25703_get_min_vsys(struct bq257xx_chg *pdata)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(pdata->bq->regmap, BQ25703_MIN_VSYS,
> +			  &reg);
> +	if (ret)
> +		return ret;
> +
> +	reg = FIELD_GET(BQ25703_MINVSYS_MASK, reg);
> +	return (reg * BQ25703_MINVSYS_STEP_UV) + BQ25703_MINVSYS_MIN_UV;
> +}
> +
> +/**
> + * bq25703_set_min_vsys() - Set the minimum system voltage
> + * @pdata: driver platform data
> + * @ichg: voltage value to set in uV.
> + *
> + * This function takes a requested minimum system voltage value, clamps
> + * it between the minimum supported value by the charger and a user
> + * defined minimum system value, and then writes the value to the
> + * appropriate register.
> + *
> + * Return: Returns 0 on success or error if an error occurs.
> + */
> +static int bq25703_set_min_vsys(struct bq257xx_chg *pdata, int vsys)
> +{
> +	unsigned int reg;
> +	int vsys_min = pdata->vsys_min;
> +
> +	vsys = clamp(vsys, BQ25703_MINVSYS_MIN_UV, vsys_min);
> +	reg = ((vsys - BQ25703_MINVSYS_MIN_UV) / BQ25703_MINVSYS_STEP_UV);
> +	reg = FIELD_PREP(BQ25703_MINVSYS_MASK, reg);
> +
> +	return regmap_write(pdata->bq->regmap, BQ25703_MIN_VSYS,
> +			    reg);
> +}
> +
> +/**
> + * bq25703_get_cur() - Get the reported current from the battery
> + * @pdata: driver platform data
> + *
> + * Return: Returns reported current from battery in uA as an absolute
> + *         value (charge and discharge are both positive integers).
> + *         Returns error if unable to read register value.
> + */
> +static int bq25703_get_cur(struct bq257xx_chg *pdata)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(pdata->bq->regmap, BQ25703_ADCIBAT_CHG, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (pdata->online)
> +		return FIELD_GET(BQ25703_ADCIBAT_CHG_MASK, reg) *
> +			BQ25703_ADCIBAT_CHG_STEP_UA;
> +	else
> +		return FIELD_GET(BQ25703_ADCIBAT_DISCHG_MASK, reg) *
> +			BQ25703_ADCIBAT_DIS_STEP_UA;
> +}
> +
> +/**
> + * bq25703_get_ichg_cur() - Get the maximum reported charge current
> + * @pdata: driver platform data
> + *
> + * Return: Returns maximum set charge current from charger in uA.
> + *         Returns error if unable to read register value.
> + */
> +static int bq25703_get_ichg_cur(struct bq257xx_chg *pdata)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(pdata->bq->regmap, BQ25703_CHARGE_CURRENT, &reg);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(BQ25703_ICHG_MASK, reg) * BQ25703_ICHG_STEP_UA;
> +}
> +
> +/**
> + * bq25703_set_ichg_cur() - Set the maximum charge current
> + * @pdata: driver platform data
> + * @ichg: current value to set in uA.
> + *
> + * This function takes a requested maximum charge current value, clamps
> + * it between the minimum supported value by the charger and a user
> + * defined maximum charging value, and then writes the value to the
> + * appropriate register.
> + *
> + * Return: Returns 0 on success or error if an error occurs.
> + */
> +static int bq25703_set_ichg_cur(struct bq257xx_chg *pdata, int ichg)
> +{
> +	unsigned int reg;
> +	int ichg_max = pdata->ichg_max;
> +
> +	ichg = clamp(ichg, BQ25703_ICHG_MIN_UA, ichg_max);
> +	reg = FIELD_PREP(BQ25703_ICHG_MASK, (ichg / BQ25703_ICHG_STEP_UA));
> +
> +	return regmap_write(pdata->bq->regmap, BQ25703_CHARGE_CURRENT,
> +			    reg);
> +}
> +
> +/**
> + * bq25703_get_chrg_volt() - Get the maximum set charge voltage
> + * @pdata: driver platform data
> + *
> + * Return: Returns maximum set charge voltage from charger in uV.
> + *         Returns error if unable to read register value.
> + */
> +static int bq25703_get_chrg_volt(struct bq257xx_chg *pdata)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(pdata->bq->regmap, BQ25703_MAX_CHARGE_VOLT,
> +			  &reg);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(BQ25703_MAX_CHARGE_VOLT_MASK, reg) *
> +		BQ25703_VBATREG_STEP_UV;
> +}
> +
> +/**
> + * bq25703_set_chrg_volt() - Set the maximum charge voltage
> + * @pdata: driver platform data
> + * @vbat: voltage value to set in uV.
> + *
> + * This function takes a requested maximum charge voltage value, clamps
> + * it between the minimum supported value by the charger and a user
> + * defined maximum charging value, and then writes the value to the
> + * appropriate register.
> + *
> + * Return: Returns 0 on success or error if an error occurs.
> + */
> +static int bq25703_set_chrg_volt(struct bq257xx_chg *pdata, int vbat)
> +{
> +	unsigned int reg;
> +	int vbat_max = pdata->vbat_max;
> +
> +	vbat = clamp(vbat, BQ25703_VBATREG_MIN_UV, vbat_max);
> +
> +	reg = FIELD_PREP(BQ25703_MAX_CHARGE_VOLT_MASK,
> +			 (vbat / BQ25703_VBATREG_STEP_UV));
> +
> +	return regmap_write(pdata->bq->regmap, BQ25703_MAX_CHARGE_VOLT,
> +			    reg);
> +}
> +
> +/**
> + * bq25703_get_iindpm() - Get the maximum set input current
> + * @pdata: driver platform data
> + *
> + * Read the actual input current limit from the device. This can differ
> + * from the value programmed due to some autonomous functions that may
> + * be enabled (but are not currently). This is why there is a different
> + * register used.
> + *
> + * Return: Returns maximum set input current from charger in uA.
> + *         Returns error if unable to read register value.
> + */
> +static int bq25703_get_iindpm(struct bq257xx_chg *pdata)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(pdata->bq->regmap, BQ25703_IIN_DPM, &reg);
> +	if (ret)
> +		return ret;
> +
> +	reg = FIELD_GET(BQ25703_IINDPM_MASK, reg);
> +	return (reg * BQ25703_IINDPM_STEP_UA) + BQ25703_IINDPM_OFFSET_UA;
> +}
> +
> +/**
> + * bq25703_set_iindpm() - Set the maximum input current
> + * @pdata: driver platform data
> + * @iindpm: current value in uA.
> + *
> + * This function takes a requested maximum input current value, clamps
> + * it between the minimum supported value by the charger and a user
> + * defined maximum input value, and then writes the value to the
> + * appropriate register.
> + *
> + * Return: Returns 0 on success or error if an error occurs.
> + */
> +static int bq25703_set_iindpm(struct bq257xx_chg *pdata, int iindpm)
> +{
> +	unsigned int reg;
> +	int iindpm_max = pdata->iindpm_max;
> +
> +	iindpm = clamp(iindpm, BQ25703_IINDPM_MIN_UA, iindpm_max);
> +
> +	reg = ((iindpm - BQ25703_IINDPM_OFFSET_UA) / BQ25703_IINDPM_STEP_UA);
> +
> +	return regmap_write(pdata->bq->regmap, BQ25703_IIN_HOST,
> +			    FIELD_PREP(BQ25703_IINDPM_MASK, reg));
> +}
> +
> +/**
> + * bq25703_get_vbat() - Get the reported voltage from the battery
> + * @pdata: driver platform data
> + *
> + * Return: Returns reported voltage from battery in uV.
> + *         Returns error if unable to read register value.
> + */
> +static int bq25703_get_vbat(struct bq257xx_chg *pdata)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(pdata->bq->regmap, BQ25703_ADCVSYSVBAT, &reg);
> +	if (ret)
> +		return ret;
> +
> +	reg = FIELD_GET(BQ25703_ADCVBAT_MASK, reg);
> +
> +	return (reg * BQ25703_ADCVSYSVBAT_STEP) + BQ25703_ADCVSYSVBAT_OFFSET_UV;
> +
> +}
> +
> +/**
> + * bq25703_hw_init() - Set all the required registers to init the charger
> + * @pdata: driver platform data
> + *
> + * Initialize the BQ25703 by first disabling the watchdog timer (which
> + * shuts off the charger in the absence of periodic writes). Then, set
> + * the charge current, charge voltage, minimum system voltage, and
> + * input current limit. Disable low power mode to allow ADCs and
> + * interrupts. Enable the ADC, start the ADC, set the ADC scale to
> + * full, and enable each individual ADC channel.
> + *
> + * Return: Returns 0 on success or error code on error.
> + */
> +static int bq25703_hw_init(struct bq257xx_chg *pdata)
> +{
> +	int ret = 0;

You need the regmap a bunch of times, so create a local variable for
it :)

> +
> +	regmap_update_bits(pdata->bq->regmap, BQ25703_CHARGE_OPTION_0,
> +			   BQ25703_WDTMR_ADJ_MASK,
> +			   FIELD_PREP(BQ25703_WDTMR_ADJ_MASK,
> +				      BQ25703_WDTMR_DISABLE));
> +
> +	ret = pdata->chip->bq257xx_set_ichg(pdata, pdata->ichg_max);
> +	if (ret)
> +		return ret;
> +
> +	ret = pdata->chip->bq257xx_set_vbatreg(pdata, pdata->vbat_max);
> +	if (ret)
> +		return ret;
> +
> +	ret = bq25703_set_min_vsys(pdata, pdata->vsys_min);
> +	if (ret)
> +		return ret;
> +
> +	ret = pdata->chip->bq257xx_set_iindpm(pdata, pdata->iindpm_max);
> +	if (ret)
> +		return ret;
> +
> +	regmap_update_bits(pdata->bq->regmap, BQ25703_CHARGE_OPTION_0,
> +			   BQ25703_EN_LWPWR, !BQ25703_EN_LWPWR);
> +
> +	regmap_update_bits(pdata->bq->regmap, BQ25703_ADC_OPTION,
> +			   BQ25703_ADC_CONV_EN, BQ25703_ADC_CONV_EN);
> +
> +	regmap_update_bits(pdata->bq->regmap, BQ25703_ADC_OPTION,
> +			   BQ25703_ADC_START, BQ25703_ADC_START);
> +
> +	regmap_update_bits(pdata->bq->regmap, BQ25703_ADC_OPTION,
> +			   BQ25703_ADC_FULL_SCALE, BQ25703_ADC_FULL_SCALE);
> +
> +	regmap_update_bits(pdata->bq->regmap, BQ25703_ADC_OPTION,
> +			   BQ25703_ADC_CH_MASK,
> +			   (BQ25703_ADC_CMPIN_EN | BQ25703_ADC_VBUS_EN |
> +			   BQ25703_ADC_PSYS_EN | BQ25703_ADC_IIN_EN |
> +			   BQ25703_ADC_IDCHG_EN | BQ25703_ADC_ICHG_EN |
> +			   BQ25703_ADC_VSYS_EN | BQ25703_ADC_VBAT_EN));
> +
> +	return ret;
> +}
> +
> +/**
> + * bq25703_hw_shutdown() - Set registers for shutdown
> + * @pdata: driver platform data
> + *
> + * Enable low power mode for the device while in shutdown.
> + */
> +static void bq25703_hw_shutdown(struct bq257xx_chg *pdata)
> +{
> +	regmap_update_bits(pdata->bq->regmap, BQ25703_CHARGE_OPTION_0,
> +			   BQ25703_EN_LWPWR, BQ25703_EN_LWPWR);
> +}
> +
> +static int bq257xx_set_charger_property(struct power_supply *psy,
> +		enum power_supply_property prop,
> +		const union power_supply_propval *val)
> +{
> +	struct bq257xx_chg *pdata = power_supply_get_drvdata(psy);
> +	int ret = -EINVAL;
> +
> +	switch (prop) {
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		ret = pdata->chip->bq257xx_set_iindpm(pdata, val->intval);
> +		if (ret)
> +			return ret;
> +		break;

return pdata->chip->bq257xx_set_iindpm(...);

> +
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> +		ret = pdata->chip->bq257xx_set_vbatreg(pdata, val->intval);
> +		if (ret)
> +			return ret;
> +		break;

same.

> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		ret = pdata->chip->bq257xx_set_ichg(pdata, val->intval);
> +		if (ret)
> +			return ret;
> +		break;

same.

> +	default:
> +		break;

return -EINVAL

> +	}
> +
> +	return ret;

get rid of this and ret.

> +}
> +
> +static int bq257xx_get_charger_property(struct power_supply *psy,
> +				enum power_supply_property psp,
> +				union power_supply_propval *val)
> +{
> +	struct bq257xx_chg *pdata = power_supply_get_drvdata(psy);
> +	int ret = 0;
> +
> +	ret = pdata->chip->bq257xx_get_state(pdata);
> +	if (ret)
> +		return ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		if (!pdata->online)
> +			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> +		else if (!bq25703_get_cur(pdata))
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		else if (pdata->fast_charge || pdata->pre_charge)
> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +		else
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		if (pdata->ov_fault || pdata->batoc_fault)
> +			val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +		else if (pdata->oc_fault)
> +			val->intval = POWER_SUPPLY_HEALTH_OVERCURRENT;
> +		else
> +			val->intval = POWER_SUPPLY_HEALTH_GOOD;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = BQ257XX_MANUFACTURER;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = pdata->online;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		ret = bq25703_get_iindpm(pdata);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;

Maybe feed val as second argument to bq25703_get_iindpm(), so
that you can just do

return bq25703_get_iindpm(pdata, val);

> +		break;
> +
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> +		ret = bq25703_get_chrg_volt(pdata);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
> +		break;

same.

> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		ret = bq25703_get_cur(pdata);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (pdata->online)
> +			val->intval = ret;
> +		else
> +			val->intval = -ret;
> +		break;

same, if you move the online check into bq25703_get_cur.

> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		ret = bq25703_get_vbat(pdata);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
> +		break;

same.

> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +		ret = bq25703_get_ichg_cur(pdata);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
> +		break;

same.

> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> +		ret = bq25703_get_min_vsys(pdata);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
> +		break;

same.

> +	case POWER_SUPPLY_PROP_USB_TYPE:
> +		val->intval = pdata->usb_type;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static enum power_supply_property bq257xx_power_supply_props[] = {
> +	POWER_SUPPLY_PROP_MANUFACTURER,
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> +	POWER_SUPPLY_PROP_USB_TYPE,
> +};
> +
> +static int bq257xx_property_is_writeable(struct power_supply *psy,
> +					 enum power_supply_property prop)
> +{
> +	switch (prop) {
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static enum power_supply_usb_type bq25703_usb_types[] = {
> +	POWER_SUPPLY_USB_TYPE_C,
> +	POWER_SUPPLY_USB_TYPE_PD,
> +	POWER_SUPPLY_USB_TYPE_PD_DRP,
> +	POWER_SUPPLY_USB_TYPE_PD_PPS,
> +	POWER_SUPPLY_USB_TYPE_UNKNOWN,
> +};
> +
> +/**
> + * bq257xx_external_power_changed() - Handler for external power change
> + * @psy: Power supply data
> + *
> + * When the external power into the charger is changed, check the USB
> + * type so that it can be reported. Additionally, update the max input
> + * current and max charging current to the value reported if it is a
> + * USB PD charger, otherwise use the default value. Note that each time
> + * a charger is removed the max charge current register is erased, so
> + * it must be set again each time the input changes or the device will
> + * not charge.
> + */
> +static void bq257xx_external_power_changed(struct power_supply *psy)
> +{
> +	struct bq257xx_chg *pdata = power_supply_get_drvdata(psy);
> +	union power_supply_propval val;
> +	int ret;
> +	int imax = pdata->iindpm_max;
> +
> +	pdata->chip->bq257xx_get_state(pdata);
> +
> +	pdata->supplied = power_supply_am_i_supplied(pdata->charger);
> +	if (pdata->supplied < 0)
> +		return;
> +
> +	if (pdata->supplied == 0)
> +		goto out;
> +
> +	ret = power_supply_get_property_from_supplier(psy,
> +						      POWER_SUPPLY_PROP_USB_TYPE,
> +						      &val);
> +	if (ret)
> +		return;
> +
> +	pdata->usb_type = val.intval;
> +
> +	if ((pdata->usb_type == POWER_SUPPLY_USB_TYPE_PD) ||
> +	    (pdata->usb_type == POWER_SUPPLY_USB_TYPE_PD_DRP) ||
> +	    (pdata->usb_type == POWER_SUPPLY_USB_TYPE_PD)) {
> +		ret = power_supply_get_property_from_supplier(psy,
> +							      POWER_SUPPLY_PROP_CURRENT_MAX,
> +							      &val);
> +		if (ret)
> +			return;
> +
> +		if (val.intval)
> +			imax = val.intval;
> +	}
> +
> +	if (pdata->supplied) {
> +		pdata->chip->bq257xx_set_ichg(pdata, imax);
> +		pdata->chip->bq257xx_set_iindpm(pdata, imax);
> +		pdata->chip->bq257xx_set_vbatreg(pdata, pdata->vbat_max);
> +	}
> +
> +out:
> +	power_supply_changed(psy);
> +}
> +
> +static irqreturn_t bq257xx_irq_handler_thread(int irq, void *private)
> +{
> +	struct bq257xx_chg *pdata = private;
> +
> +	bq257xx_external_power_changed(pdata->charger);
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct power_supply_desc bq257xx_power_supply_desc = {
> +	.name = "bq257xx-charger",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.usb_types = bq25703_usb_types,
> +	.num_usb_types = ARRAY_SIZE(bq25703_usb_types),

This recently changed into a bitmask.

> +	.properties = bq257xx_power_supply_props,
> +	.num_properties = ARRAY_SIZE(bq257xx_power_supply_props),
> +	.get_property = bq257xx_get_charger_property,
> +	.set_property = bq257xx_set_charger_property,
> +	.property_is_writeable = bq257xx_property_is_writeable,
> +	.external_power_changed = bq257xx_external_power_changed,
> +};
> +
> +static const struct bq257xx_chip_info bq25703_chip_info = {
> +		.bq257xx_hw_init = &bq25703_hw_init,
> +		.bq257xx_hw_shutdown = &bq25703_hw_shutdown,
> +		.bq257xx_get_state = &bq25703_get_state,
> +		.bq257xx_set_ichg = &bq25703_set_ichg_cur,
> +		.bq257xx_set_vbatreg = &bq25703_set_chrg_volt,
> +		.bq257xx_set_iindpm = &bq25703_set_iindpm,
> +};
> +
> +static int bq257xx_power_supply_init(struct bq257xx_chg *pdata,
> +		struct power_supply_config *psy_cfg, struct device *dev)
> +{
> +	pdata->charger = devm_power_supply_register(dev,
> +						    &bq257xx_power_supply_desc,
> +						    psy_cfg);
> +	if (IS_ERR(pdata->charger)) {
> +		dev_err_probe(dev, PTR_ERR(pdata->charger),
> +			      "power supply register charger failed: %ld\n",
> +			      PTR_ERR(pdata->charger));
> +		return PTR_ERR(pdata->charger);

dev_err_probe() returns the error code, so you can just do
return dev_err_probe(...). Also it prints the error code,
so no need for the ": %ld" part either.

After that simplification I don't see the point in having
an extra function for this.

> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * bq257xx_parse_dt() - Parse the device tree for required properties
> + * @pdata: driver platform data
> + * @psy_cfg: power supply config data
> + * @dev: device struct
> + *
> + * Read the device tree to identify the minimum system voltage, the
> + * maximum charge current, the maximum charge voltage, and the maximum
> + * input current.
> + *
> + * Return: Returns 0 on success or error code on error.
> + */
> +static int bq257xx_parse_dt(struct bq257xx_chg *pdata,
> +		struct power_supply_config *psy_cfg, struct device *dev)
> +{
> +	int ret = 0;
> +
> +	psy_cfg->drv_data = pdata;
> +	psy_cfg->of_node = dev->of_node;

replace the last one with

psy_cfg.fwnode = dev_fwnode(dev);

> +	ret = device_property_read_u32(dev,
> +				       "ti,minimum-sys-voltage",
> +				       &pdata->vsys_min);
> +	if (ret) {
> +		dev_err(dev, "Cannot read ti,minimum-sys-voltage from dt\n");
> +		return ret;

return dev_err_probe(dev, ret, ...);

> +	}
> +
> +	ret = device_property_read_u32(dev,
> +				       "ti,charge-current",
> +				       &pdata->ichg_max);
> +	if (ret) {
> +		dev_err(dev, "Cannot read ti,charge-current from dt\n");
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev,
> +				       "ti,max-charge-voltage",
> +				       &pdata->vbat_max);
> +	if (ret) {
> +		dev_err(dev, "Cannot read ti,max-charge-voltage from dt\n");
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev,
> +				       "ti,current-limit",
> +				       &pdata->iindpm_max);
> +	if (ret)
> +		pdata->iindpm_max = BQ25703_IINDPM_DEFAULT_UA;

same for the above, but will change anyways because of binding
change.

> +
> +	return 0;
> +}
> +
> +static int bq257xx_charger_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct bq257xx_device *bq = dev_get_drvdata(pdev->dev.parent);
> +	struct bq257xx_chg *pdata;
> +	struct power_supply_config psy_cfg = { };
> +	int ret;
> +
> +	pdev->dev.of_node = pdev->dev.parent->of_node;
> +	pdev->dev.of_node_reused = true;

replace above two lines with

device_set_of_node_from_dev(dev, pdev->dev.parent);

> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(struct bq257xx_chg), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;

use sizeof(*pdata)

> +	pdata->bq = bq;
> +
> +	switch (pdata->bq->variant) {
> +	case BQ25703A:
> +		pdata->chip = &bq25703_chip_info;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = bq257xx_parse_dt(pdata, &psy_cfg, dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to read device tree properties%d\n", ret);

no need for this error message, since bq257xx_parse_dt already prints a
message for all error cases.

> +		return ret;
> +	}
> +
> +	ret = pdata->chip->bq257xx_hw_init(pdata);
> +	if (ret) {
> +		dev_err(dev, "Cannot initialize the chip.\n");
> +		return ret;
> +	}
> +
> +	ret = bq257xx_power_supply_init(pdata, &psy_cfg, dev);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, pdata);
> +
> +	if (bq->client->irq) {
> +		ret = devm_request_threaded_irq(dev, bq->client->irq, NULL,
> +						bq257xx_irq_handler_thread,
> +						IRQF_TRIGGER_RISING |
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						dev_name(&bq->client->dev), pdata);
> +		if (ret < 0) {
> +			dev_err(dev, "get irq fail: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void bq257xx_charger_shutdown(struct platform_device *pdev)
> +{
> +	struct bq257xx_chg *pdata = platform_get_drvdata(pdev);
> +
> +	pdata->chip->bq257xx_hw_shutdown(pdata);
> +}
> +
> +static struct platform_driver bq257xx_chg_driver = {
> +	.driver = {
> +		.name = "bq257xx-charger",
> +	},
> +	.probe = bq257xx_charger_probe,
> +	.shutdown = bq257xx_charger_shutdown,
> +};
> +module_platform_driver(bq257xx_chg_driver);
> +
> +MODULE_DESCRIPTION("bq257xx charger driver");
> +MODULE_AUTHOR("Chris Morgan <macromorgan@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");

Greetings,

-- Sebastian

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux