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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>");
> MODULE_DESCRIPTION("MAX17042 Fuel Gauge");
>
Regards,
Hans
