diff mbox series

[v3,2/2] power: supply: max17042: add platform driver variant

Message ID 20241118-b4-max17042-v3-2-9bcaeda42a06@gmail.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series power: supply: max17042: cleanup and more features | expand

Commit Message

Dzmitry Sankouski Nov. 18, 2024, 10:09 a.m. UTC 
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@gmail.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(-)

Comments

Hans de Goede Nov. 18, 2024, 11:03 a.m. UTC | #1 
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@gmail.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@samsung.com>");
>  MODULE_DESCRIPTION("MAX17042 Fuel Gauge");
> 

Regards,

Hans
diff mbox series

Patch

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;
-	}
 	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)
+{
+	int ret = 0;
+
+	ret = platform_driver_register(&max17042_platform_driver);
+
+	if (ret) {
+		platform_driver_unregister(&max17042_platform_driver);
+		return ret;
+	}
+
+	ret = i2c_add_driver(&max17042_i2c_driver);
+
+	return ret;
+}
+module_init(__driver_max17042_platform_init);
+
+static void __exit __driver_max17042_platform_exit(void)
+{
+	i2c_del_driver(&max17042_i2c_driver);
+	platform_driver_unregister(&max17042_platform_driver);
+}
+module_exit(__driver_max17042_platform_exit);
 
 MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
 MODULE_DESCRIPTION("MAX17042 Fuel Gauge");