Message-ID: <775fab33-0964-67f0-837a-b5fdd7ae7a2b@gmail.com>
Date:   Thu, 16 Mar 2023 09:38:21 +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: [PATCHv2 08/12] 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: <20230314225535.1321736-1-sre@kernel.org>
 <20230314225535.1321736-9-sre@kernel.org>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
In-Reply-To: <20230314225535.1321736-9-sre@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/15/23 00:55, Sebastian Reichel wrote:
> Use standard simple-battery API for constant battery
> information like min and max voltage. This simplifies
> the driver a lot and brings automatic support for DT.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>   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;

Not sure if this is intentional but I don't see the 
POWER_SUPPLY_STATUS_FULL being reported after applying your series. If 
this is intended, maybe it could be mentioned in commit log?

Other than that - this really cleans up the driver in a nice way!

Yours,
	-- Matti

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

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