linux-kernel - Re: [PATCH v2 2/2] power: supply: bq24190

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20231116191815.jrjwcw5nywtdpplk@mercury.elektranox.org>
Date:   Thu, 16 Nov 2023 20:18:15 +0100
From:   Sebastian Reichel <sebastian.reichel@...labora.com>
To:     Hermes Zhang <Hermes.Zhang@...s.com>
Cc:     krzysztof.kozlowski+dt@...aro.org, robh+dt@...nel.org,
        linux-kernel@...r.kernel.org, kernel@...s.com,
        Hermes Zhang <chenhuiz@...s.com>, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 2/2] power: supply: bq24190_charger: Add support for
 BQ24296

Hi,

On Mon, Oct 30, 2023 at 04:43:02PM +0800, Hermes Zhang wrote:
> The BQ24296 is most similar to the BQ24196, but the:
> 1. OTG config is split from CHG config (REG01)
> 2. ICHG (Fast Charge Current limit) range is smaller (<=3008mA)
> 3. NTC fault is simplified to 2 bits
> 
> Signed-off-by: Hermes Zhang <chenhuiz@...s.com>
> ---
> 
> Notes:
>     v2: restructured the code to support bq24296 and add Acked-by tag
>         from Conor
>  drivers/power/supply/bq24190_charger.c | 437 +++++++++++++++++++------
>  1 file changed, 338 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 3f99cb9590ba..5b60d924cf9f 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c

[...]

> +static const struct bq24190_chip_info bq24190_chip_info_tbl[] = {
> +	[BQ24190] = {
> +		.ichg_array_size = ARRAY_SIZE(bq24190_ccc_ichg_values),
> +		.vbus_desc = bq24190_vbus_desc,
> +		.check_chip = bq24190_check_chip,
> +		.set_chg_config = bq24190_battery_set_chg_config,
> +		.ntc_fault_mask = BQ24190_REG_F_NTC_FAULT_MASK,
> +		.get_ntc_status = bq24190_charger_get_ntc_status,
> +		.set_otg_vbus = bq24190_set_otg_vbus,
> +	},
> +	[BQ24192] = {
> +		.ichg_array_size = ARRAY_SIZE(bq24190_ccc_ichg_values),
> +		.vbus_desc = bq24190_vbus_desc,
> +		.check_chip = bq24190_check_chip,
> +		.set_chg_config = bq24190_battery_set_chg_config,
> +		.ntc_fault_mask = BQ24190_REG_F_NTC_FAULT_MASK,
> +		.get_ntc_status = bq24190_charger_get_ntc_status,
> +		.set_otg_vbus = bq24190_set_otg_vbus,
> +	},
> +	[BQ24192i] = {
> +		.ichg_array_size = ARRAY_SIZE(bq24190_ccc_ichg_values),
> +		.vbus_desc = bq24190_vbus_desc,
> +		.check_chip = bq24190_check_chip,
> +		.set_chg_config = bq24190_battery_set_chg_config,
> +		.ntc_fault_mask = BQ24190_REG_F_NTC_FAULT_MASK,
> +		.get_ntc_status = bq24190_charger_get_ntc_status,
> +		.set_otg_vbus = bq24190_set_otg_vbus,
> +	},
> +	[BQ24196] = {
> +		.ichg_array_size = ARRAY_SIZE(bq24190_ccc_ichg_values),
> +		.vbus_desc = bq24190_vbus_desc,
> +		.check_chip = bq24190_check_chip,
> +		.set_chg_config = bq24190_battery_set_chg_config,
> +		.ntc_fault_mask = BQ24190_REG_F_NTC_FAULT_MASK,
> +		.get_ntc_status = bq24190_charger_get_ntc_status,
> +		.set_otg_vbus = bq24190_set_otg_vbus,
> +	},
> +	[BQ24296] = {
> +		.ichg_array_size = BQ24296_CCC_ICHG_VALUES_LEN,
> +		.vbus_desc = bq24296_vbus_desc,
> +		.check_chip = bq24296_check_chip,
> +		.set_chg_config = bq24296_battery_set_chg_config,
> +		.ntc_fault_mask = BQ24296_REG_F_NTC_FAULT_MASK,
> +		.get_ntc_status = bq24296_charger_get_ntc_status,
> +		.set_otg_vbus = bq24296_set_otg_vbus,
> +	},
> +};
> +
>  static int bq24190_probe(struct i2c_client *client)
>  {
>  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> @@ -1804,6 +2040,7 @@ static int bq24190_probe(struct i2c_client *client)
>  	bdi->client = client;
>  	bdi->dev = dev;
>  	strncpy(bdi->model_name, id->name, I2C_NAME_SIZE);
> +	bdi->info = &bq24190_chip_info_tbl[id->driver_data];
>  	mutex_init(&bdi->f_reg_lock);
>  	bdi->charge_type = POWER_SUPPLY_CHARGE_TYPE_FAST;
>  	bdi->f_reg = 0;
> @@ -1940,7 +2177,7 @@ static void bq24190_shutdown(struct i2c_client *client)
>  	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
>  
>  	/* Turn off 5V boost regulator on shutdown */
> -	bq24190_set_otg_vbus(bdi, false);
> +	bdi->info->set_otg_vbus(bdi, false);
>  }
>  
>  static __maybe_unused int bq24190_runtime_suspend(struct device *dev)
> @@ -2029,10 +2266,11 @@ static const struct dev_pm_ops bq24190_pm_ops = {
>  };
>  
>  static const struct i2c_device_id bq24190_i2c_ids[] = {
> -	{ "bq24190" },
> -	{ "bq24192" },
> -	{ "bq24192i" },
> -	{ "bq24196" },
> +	{ "bq24190", BQ24190 },
> +	{ "bq24192", BQ24192 },
> +	{ "bq24192i", BQ24192i },
> +	{ "bq24196", BQ24196 },
> +	{ "bq24296", BQ24296 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
> @@ -2042,6 +2280,7 @@ static const struct of_device_id bq24190_of_match[] = {
>  	{ .compatible = "ti,bq24192", },
>  	{ .compatible = "ti,bq24192i", },
>  	{ .compatible = "ti,bq24196", },
> +	{ .compatible = "ti,bq24296", },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, bq24190_of_match);

This looks better, but please handle the match_info in the modern
way, like e.g. bq256xx_charger.c, bq24257_charger.c, bq2515x_charger.c.
So instead of using an ID and using that to lookup a device struct
from an array, just directly specify a pointer to the device struct.

Also make sure you specify the pointer in all device tables and use
i2c_get_match_data() in probe() to get the struct.

Thanks,

-- Sebastian

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