Message-ID: <b05db127-b80e-401a-b93b-dec1907902ba@redhat.com>
Date: Mon, 18 Nov 2024 12:03:23 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Dzmitry Sankouski <dsankouski@...il.com>,
 Krzysztof Kozlowski <krzk@...nel.org>,
 Marek Szyprowski <m.szyprowski@...sung.com>,
 Sebastian Krzyszkowiak <sebastian.krzyszkowiak@...i.sm>,
 Purism Kernel Team <kernel@...i.sm>, Sebastian Reichel <sre@...nel.org>,
 Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Alim Akhtar <alim.akhtar@...sung.com>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>
Cc: linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-samsung-soc@...r.kernel.org, imx@...ts.linux.dev,
 linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v3 2/2] power: supply: max17042: add platform driver
 variant

Hi Dzmitry,

On 18-Nov-24 11:09 AM, Dzmitry Sankouski wrote:
> Maxim PMICs may include fuel gauge with additional features, which is
> out of single Linux power supply driver scope.
> 
> For example, in max77705 PMIC fuelgauge has additional registers,
> like IIN_REG, VSYS_REG, ISYS_REG. Those needed to measure PMIC input
> current, system voltage and current respectively. Those measurements
> cannot be bound to any of fuelgauge properties.
> 
> The solution here add and option to use max17042 driver as a MFD
> sub device, thus allowing any additional functionality be implemented as
> another sub device. This will help to reduce code duplication in MFD
> fuel gauge drivers.
> 
> Signed-off-by: Dzmitry Sankouski <dsankouski@...il.com>
> ---
> Changes in v3:
> - pass dev pointer in max17042_probe
> - remove prints
> ---
>  drivers/power/supply/max17042_battery.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 90 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index 99bf6915aa23..d11bf37aaae2 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -16,6 +16,7 @@
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
> +#include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/power_supply.h>
> @@ -1029,14 +1030,12 @@ static const struct power_supply_desc max17042_no_current_sense_psy_desc = {
>  	.num_properties	= ARRAY_SIZE(max17042_battery_props) - 2,
>  };
>  
> -static int max17042_probe(struct i2c_client *client)
> +static int max17042_probe(struct i2c_client *client, struct device *dev,
> +			  enum max170xx_chip_type chip_type)
>  {
> -	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>  	struct i2c_adapter *adapter = client->adapter;
>  	const struct power_supply_desc *max17042_desc = &max17042_psy_desc;
>  	struct power_supply_config psy_cfg = {};
> -	const struct acpi_device_id *acpi_id = NULL;
> -	struct device *dev = &client->dev;
>  	struct max17042_chip *chip;
>  	int ret;
>  	int i;
> @@ -1045,33 +1044,24 @@ static int max17042_probe(struct i2c_client *client)
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
>  		return -EIO;
>  
> -	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
>  	if (!chip)
>  		return -ENOMEM;
>  
>  	chip->client = client;
> -	if (id) {
> -		chip->chip_type = id->driver_data;
> -	} else {
> -		acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
> -		if (!acpi_id)
> -			return -ENODEV;
> -
> -		chip->chip_type = acpi_id->driver_data;
> -	}

I would expect you to now add a:

	chip->chip_type = chip_type;

line here storing the chip_type probe() function argument, but
that appears to be missing always leaving chip_type at 0.

>  	chip->regmap = devm_regmap_init_i2c(client, &max17042_regmap_config);
>  	if (IS_ERR(chip->regmap)) {
> -		dev_err(&client->dev, "Failed to initialize regmap\n");
> +		dev_err(dev, "Failed to initialize regmap\n");
>  		return -EINVAL;
>  	}
>  
>  	chip->pdata = max17042_get_pdata(chip);
>  	if (!chip->pdata) {
> -		dev_err(&client->dev, "no platform data provided\n");
> +		dev_err(dev, "no platform data provided\n");
>  		return -EINVAL;
>  	}
>  
> -	i2c_set_clientdata(client, chip);
> +	dev_set_drvdata(dev, chip);
>  	psy_cfg.drv_data = chip;
>  	psy_cfg.of_node = dev->of_node;
>  
> @@ -1095,17 +1085,17 @@ static int max17042_probe(struct i2c_client *client)
>  		regmap_write(chip->regmap, MAX17042_LearnCFG, 0x0007);
>  	}
>  
> -	chip->battery = devm_power_supply_register(&client->dev, max17042_desc,
> +	chip->battery = devm_power_supply_register(dev, max17042_desc,
>  						   &psy_cfg);
>  	if (IS_ERR(chip->battery)) {
> -		dev_err(&client->dev, "failed: power supply register\n");
> +		dev_err(dev, "failed: power supply register\n");
>  		return PTR_ERR(chip->battery);
>  	}
>  
>  	if (client->irq) {
>  		unsigned int flags = IRQF_ONESHOT | IRQF_SHARED | IRQF_PROBE_SHARED;
>  
> -		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +		ret = devm_request_threaded_irq(dev, client->irq,
>  						NULL,
>  						max17042_thread_handler, flags,
>  						chip->battery->desc->name,
> @@ -1118,7 +1108,7 @@ static int max17042_probe(struct i2c_client *client)
>  		} else {
>  			client->irq = 0;
>  			if (ret != -EBUSY)
> -				dev_err(&client->dev, "Failed to get IRQ\n");
> +				dev_err(dev, "Failed to get IRQ\n");
>  		}
>  	}
>  	/* Not able to update the charge threshold when exceeded? -> disable */
> @@ -1127,7 +1117,7 @@ static int max17042_probe(struct i2c_client *client)
>  
>  	regmap_read(chip->regmap, MAX17042_STATUS, &val);
>  	if (val & STATUS_POR_BIT) {
> -		ret = devm_work_autocancel(&client->dev, &chip->work,
> +		ret = devm_work_autocancel(dev, &chip->work,
>  					   max17042_init_worker);
>  		if (ret)
>  			return ret;
> @@ -1139,6 +1129,38 @@ static int max17042_probe(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static int max17042_i2c_probe(struct i2c_client *client)
> +{
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	const struct acpi_device_id *acpi_id = NULL;
> +	struct device *dev = &client->dev;
> +	enum max170xx_chip_type chip_type;
> +
> +	if (id) {
> +		chip_type = id->driver_data;
> +	} else {
> +		acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
> +		if (!acpi_id)
> +			return -ENODEV;
> +
> +		chip_type = acpi_id->driver_data;
> +	}
> +
> +	return max17042_probe(client, dev, chip_type);
> +}
> +
> +static int max17042_platform_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct i2c_client *i2c = dev_get_platdata(dev);
> +	const struct platform_device_id *id = platform_get_device_id(pdev);
> +
> +	if (!i2c)
> +		return -EINVAL;
> +
> +	return max17042_probe(i2c, dev, id->driver_data);
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static int max17042_suspend(struct device *dev)
>  {
> @@ -1204,6 +1226,16 @@ static const struct i2c_device_id max17042_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, max17042_id);
>  
> +static const struct platform_device_id max17042_platform_id[] = {
> +	{ "max17042", MAXIM_DEVICE_TYPE_MAX17042 },
> +	{ "max17047", MAXIM_DEVICE_TYPE_MAX17047 },
> +	{ "max17050", MAXIM_DEVICE_TYPE_MAX17050 },
> +	{ "max17055", MAXIM_DEVICE_TYPE_MAX17055 },
> +	{ "max77849-battery", MAXIM_DEVICE_TYPE_MAX17047 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, max17042_platform_id);
> +
>  static struct i2c_driver max17042_i2c_driver = {
>  	.driver	= {
>  		.name	= "max17042",
> @@ -1211,10 +1243,44 @@ static struct i2c_driver max17042_i2c_driver = {
>  		.of_match_table = of_match_ptr(max17042_dt_match),
>  		.pm	= &max17042_pm_ops,
>  	},
> -	.probe		= max17042_probe,
> +	.probe		= max17042_i2c_probe,
>  	.id_table	= max17042_id,
>  };
> -module_i2c_driver(max17042_i2c_driver);
> +
> +static struct platform_driver max17042_platform_driver = {
> +	.driver	= {
> +		.name	= "max17042",
> +		.acpi_match_table = ACPI_PTR(max17042_acpi_match),
> +		.of_match_table = of_match_ptr(max17042_dt_match),
> +		.pm	= &max17042_pm_ops,
> +	},
> +	.probe		= max17042_platform_probe,
> +	.id_table	= max17042_platform_id,
> +};
> +
> +static int __init __driver_max17042_platform_init(void)

I would name this just max17042_init() no reason to prefix with __
and using platform in the name is weird since it registers both
the i2c and platform drivers.

> +{
> +	int ret = 0;

No need to initialize ret to 0, since you immediately assign
a value to it after declaring it.

> +	ret = platform_driver_register(&max17042_platform_driver);

missing if (ret) return ret.

> +	if (ret) {
> +		platform_driver_unregister(&max17042_platform_driver);
> +		return ret;
> +	}
> +
> +	ret = i2c_add_driver(&max17042_i2c_driver);

This needs to be above if (ret) which unregisters the platform_driver
again.

> +
> +	return ret;

and this should be return 0;

> +}
> +module_init(__driver_max17042_platform_init);

Basically the whole function should look like this:

static int __init max17042_init(void)
{
	int ret;

	ret = platform_driver_register(&max17042_platform_driver);
	if (ret)
		return ret;

	ret = i2c_add_driver(&max17042_i2c_driver);
	if (ret) {
		platform_driver_unregister(&max17042_platform_driver);
		return ret;
	}

	return 0;
}
module_init(max17042_init);


> +
> +static void __exit __driver_max17042_platform_exit(void)

Please name this one just max17042_exit()

> +{
> +	i2c_del_driver(&max17042_i2c_driver);
> +	platform_driver_unregister(&max17042_platform_driver);
> +}
> +module_exit(__driver_max17042_platform_exit);
>  
>  MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@...sung.com>");
>  MODULE_DESCRIPTION("MAX17042 Fuel Gauge");
> 

Regards,

Hans

Powered by blists - more mailing lists