Message-ID: <c2e4540c-a607-d2d7-992f-9ff7d1c2cce7@gmail.com>
Date:   Sun, 19 Mar 2023 14:45:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCHv3 08/14] power: supply: generic-adc-battery: use
 simple-battery API
Content-Language: en-US, en-GB
To:     Sebastian Reichel <sre@kernel.org>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        devicetree@vger.kernel.org
References: <20230317225707.1552512-1-sre@kernel.org>
 <20230317225707.1552512-9-sre@kernel.org>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <20230317225707.1552512-9-sre@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/18/23 00:57, Sebastian Reichel wrote:
> Constant battery data is available through power-supply's simple-battery
> API. This works automatically, so the manual handling can be removed
> without loosing any feature :)
> 
> Note, that the POWER_SUPPLY_STATUS_FULL check for the level variable can
> be dropped, since the variable is never written. It can be re-introduced
> properly once the driver gets functionality to calculate the current
> charge level. Apart from that the check must be done fuzzy anyways,
> since charge estimation usually is not precise enough to always return
> exactly the full charge capacity for a full battery.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

> ---
>   drivers/power/supply/generic-adc-battery.c | 64 ++--------------------
>   include/linux/power/generic-adc-battery.h  | 18 ------
>   2 files changed, 4 insertions(+), 78 deletions(-)
>   delete mode 100644 include/linux/power/generic-adc-battery.h
> 
> diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
> index 771e5cfc49c3..d4f63d945b2c 100644
> --- a/drivers/power/supply/generic-adc-battery.c
> +++ b/drivers/power/supply/generic-adc-battery.c
> @@ -22,7 +22,6 @@
>   #include <linux/slab.h>
>   #include <linux/iio/consumer.h>
>   #include <linux/iio/types.h>
> -#include <linux/power/generic-adc-battery.h>
>   #include <linux/devm-helpers.h>
>   
>   #define JITTER_DEFAULT 10 /* hope 10ms is enough */
> @@ -48,9 +47,7 @@ struct gab {
>   	struct power_supply		*psy;
>   	struct power_supply_desc	psy_desc;
>   	struct iio_channel	*channel[GAB_MAX_CHAN_TYPE];
> -	struct gab_platform_data	*pdata;
>   	struct delayed_work bat_work;
> -	int	level;
>   	int	status;
>   	bool cable_plugged;
>   	struct gpio_desc *charge_finished;
> @@ -70,14 +67,6 @@ static void gab_ext_power_changed(struct power_supply *psy)
>   
>   static const enum power_supply_property gab_props[] = {
>   	POWER_SUPPLY_PROP_STATUS,
> -	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> -	POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN,
> -	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> -	POWER_SUPPLY_PROP_CURRENT_NOW,
> -	POWER_SUPPLY_PROP_TECHNOLOGY,
> -	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
> -	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> -	POWER_SUPPLY_PROP_MODEL_NAME,
>   };
>   
>   /*
> @@ -97,17 +86,6 @@ static bool gab_charge_finished(struct gab *adc_bat)
>   	return gpiod_get_value(adc_bat->charge_finished);
>   }
>   
> -static int gab_get_status(struct gab *adc_bat)
> -{
> -	struct gab_platform_data *pdata = adc_bat->pdata;
> -	struct power_supply_info *bat_info;
> -
> -	bat_info = &pdata->battery_info;
> -	if (adc_bat->level == bat_info->charge_full_design)
> -		return POWER_SUPPLY_STATUS_FULL;
> -	return adc_bat->status;
> -}
> -
>   static enum gab_chan_type gab_prop_to_chan(enum power_supply_property psp)
>   {
>   	switch (psp) {
> @@ -144,27 +122,12 @@ static int read_channel(struct gab *adc_bat, enum power_supply_property psp,
>   static int gab_get_property(struct power_supply *psy,
>   		enum power_supply_property psp, union power_supply_propval *val)
>   {
> -	struct gab *adc_bat;
> -	struct gab_platform_data *pdata;
> -	struct power_supply_info *bat_info;
> -	int result = 0;
> -	int ret = 0;
> -
> -	adc_bat = to_generic_bat(psy);
> -	if (!adc_bat) {
> -		dev_err(&psy->dev, "no battery infos ?!\n");
> -		return -EINVAL;
> -	}
> -	pdata = adc_bat->pdata;
> -	bat_info = &pdata->battery_info;
> +	struct gab *adc_bat = to_generic_bat(psy);
>   
>   	switch (psp) {
>   	case POWER_SUPPLY_PROP_STATUS:
> -		val->intval = gab_get_status(adc_bat);
> -		break;
> -	case POWER_SUPPLY_PROP_CHARGE_EMPTY_DESIGN:
> -		val->intval = 0;
> -		break;
> +		val->intval = adc_bat->status;
> +		return 0;
>   	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>   	case POWER_SUPPLY_PROP_CURRENT_NOW:
>   	case POWER_SUPPLY_PROP_POWER_NOW:
> @@ -173,26 +136,9 @@ static int gab_get_property(struct power_supply *psy,
>   			goto err;
>   		val->intval = result;
>   		break;
> -	case POWER_SUPPLY_PROP_TECHNOLOGY:
> -		val->intval = bat_info->technology;
> -		break;
> -	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> -		val->intval = bat_info->voltage_min_design;
> -		break;
> -	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> -		val->intval = bat_info->voltage_max_design;
> -		break;
> -	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> -		val->intval = bat_info->charge_full_design;
> -		break;
> -	case POWER_SUPPLY_PROP_MODEL_NAME:
> -		val->strval = bat_info->name;
> -		break;
>   	default:
>   		return -EINVAL;
>   	}
> -err:
> -	return ret;
>   }
>   
>   static void gab_work(struct work_struct *work)
> @@ -235,7 +181,6 @@ static int gab_probe(struct platform_device *pdev)
>   	struct gab *adc_bat;
>   	struct power_supply_desc *psy_desc;
>   	struct power_supply_config psy_cfg = {};
> -	struct gab_platform_data *pdata = pdev->dev.platform_data;
>   	enum power_supply_property *properties;
>   	int ret = 0;
>   	int chan;
> @@ -248,7 +193,7 @@ static int gab_probe(struct platform_device *pdev)
>   
>   	psy_cfg.drv_data = adc_bat;
>   	psy_desc = &adc_bat->psy_desc;
> -	psy_desc->name = pdata->battery_info.name;
> +	psy_desc->name = dev_name(&pdev->dev);
>   
>   	/* bootup default values for the battery */
>   	adc_bat->cable_plugged = false;
> @@ -256,7 +201,6 @@ static int gab_probe(struct platform_device *pdev)
>   	psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
>   	psy_desc->get_property = gab_get_property;
>   	psy_desc->external_power_changed = gab_ext_power_changed;
> -	adc_bat->pdata = pdata;
>   
>   	/*
>   	 * copying the static properties and allocating extra memory for holding
> diff --git a/include/linux/power/generic-adc-battery.h b/include/linux/power/generic-adc-battery.h
> deleted file mode 100644
> index 54434e4304d3..000000000000
> --- a/include/linux/power/generic-adc-battery.h
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (C) 2012, Anish Kumar <anish198519851985@gmail.com>
> - */
> -
> -#ifndef GENERIC_ADC_BATTERY_H
> -#define GENERIC_ADC_BATTERY_H
> -
> -/**
> - * struct gab_platform_data - platform_data for generic adc iio battery driver.
> - * @battery_info:         recommended structure to specify static power supply
> - *			   parameters
> - */
> -struct gab_platform_data {
> -	struct power_supply_info battery_info;
> -};
> -
> -#endif /* GENERIC_ADC_BATTERY_H */

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~