* [PATCH v4 0/2] power: supply: max17042: cleanup and more features
@ 2024-11-29 9:09 Dzmitry Sankouski
2024-11-29 9:09 ` [PATCH v4 1/2] power: supply: max17042: make interrupt shared Dzmitry Sankouski
2024-11-29 9:09 ` [PATCH v4 2/2] power: supply: max17042: add platform driver variant Dzmitry Sankouski
0 siblings, 2 replies; 5+ messages in thread
From: Dzmitry Sankouski @ 2024-11-29 9:09 UTC (permalink / raw)
To: Hans de Goede, Krzysztof Kozlowski, Marek Szyprowski,
Sebastian Krzyszkowiak, Purism Kernel Team, Sebastian Reichel,
Rob Herring, Conor Dooley, Alim Akhtar, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Bjorn Andersson,
Konrad Dybcio
Cc: linux-pm, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, imx, linux-arm-msm, Dzmitry Sankouski
Fuelgauge blocks often are incorporated in bigger chip,
which may use only 1 line for interrupts. Shared-irq
handles that case by requesting irq as shared.
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.
Make max17042 interrupt shared, and add platform driver
version.
Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
---
Changes in v4:
- review fixes.
- Link to v3: https://lore.kernel.org/r/20241118-b4-max17042-v3-0-9bcaeda42a06@gmail.com
Changes in v3:
- pass dev pointer to probe
- Link to v2: https://lore.kernel.org/r/20241108-b4-max17042-v2-0-f058f7a16bab@gmail.com
Changes in v2:
- drop NACKed commits
- make shared interrupts unconditionally
- rework descriptions
- add platform driver version
- Link to v1: https://lore.kernel.org/r/20241109-b4-max17042-v1-0-9e2b07e54e76@gmail.com
---
Dzmitry Sankouski (2):
power: supply: max17042: make interrupt shared
power: supply: max17042: add platform driver variant
drivers/power/supply/max17042_battery.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------
1 file changed, 93 insertions(+), 32 deletions(-)
---
base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
change-id: 20241108-b4-max17042-9306fc75afae
Best regards,
--
Dzmitry Sankouski <dsankouski@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v4 1/2] power: supply: max17042: make interrupt shared
2024-11-29 9:09 [PATCH v4 0/2] power: supply: max17042: cleanup and more features Dzmitry Sankouski
@ 2024-11-29 9:09 ` Dzmitry Sankouski
2024-11-29 9:09 ` [PATCH v4 2/2] power: supply: max17042: add platform driver variant Dzmitry Sankouski
1 sibling, 0 replies; 5+ messages in thread
From: Dzmitry Sankouski @ 2024-11-29 9:09 UTC (permalink / raw)
To: Hans de Goede, Krzysztof Kozlowski, Marek Szyprowski,
Sebastian Krzyszkowiak, Purism Kernel Team, Sebastian Reichel,
Rob Herring, Conor Dooley, Alim Akhtar, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Bjorn Andersson,
Konrad Dybcio
Cc: linux-pm, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, imx, linux-arm-msm, Dzmitry Sankouski
Fuelgauge blocks often are incorporated in bigger chip,
which may use only 1 line for interrupts. Make interrupt
shared.
Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
---
drivers/power/supply/max17042_battery.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 496c3e1f2ee6..99bf6915aa23 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -1103,14 +1103,7 @@ static int max17042_probe(struct i2c_client *client)
}
if (client->irq) {
- unsigned int flags = IRQF_ONESHOT;
-
- /*
- * On ACPI systems the IRQ may be handled by ACPI-event code,
- * so we need to share (if the ACPI code is willing to share).
- */
- if (acpi_id)
- flags |= IRQF_SHARED | IRQF_PROBE_SHARED;
+ unsigned int flags = IRQF_ONESHOT | IRQF_SHARED | IRQF_PROBE_SHARED;
ret = devm_request_threaded_irq(&client->dev, client->irq,
NULL,
--
2.39.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v4 2/2] power: supply: max17042: add platform driver variant
2024-11-29 9:09 [PATCH v4 0/2] power: supply: max17042: cleanup and more features Dzmitry Sankouski
2024-11-29 9:09 ` [PATCH v4 1/2] power: supply: max17042: make interrupt shared Dzmitry Sankouski
@ 2024-11-29 9:09 ` Dzmitry Sankouski
2024-11-29 14:03 ` Amit Singh Tomar
1 sibling, 1 reply; 5+ messages in thread
From: Dzmitry Sankouski @ 2024-11-29 9:09 UTC (permalink / raw)
To: Hans de Goede, Krzysztof Kozlowski, Marek Szyprowski,
Sebastian Krzyszkowiak, Purism Kernel Team, Sebastian Reichel,
Rob Herring, Conor Dooley, Alim Akhtar, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Bjorn Andersson,
Konrad Dybcio
Cc: linux-pm, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, imx, linux-arm-msm, Dzmitry Sankouski
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 v4:
- rename module_init and module_exit fuctions
- rework max17042_init
- assign chip_type in probe function
- pass i2c_client as pointer on pointer, to use same pointer created in
MFD. This allows devm_regmap_init_i2c to cleanup gracefully.
Changes in v3:
- pass dev pointer in max17042_probe
- remove prints
---
drivers/power/supply/max17042_battery.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 92 insertions(+), 24 deletions(-)
diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 99bf6915aa23..6a1bfc4a7b13 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,25 @@ 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->chip_type = chip_type;
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 +1086,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 +1109,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 +1118,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 +1130,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 +1227,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 +1244,45 @@ 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 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 max17042_exit(void)
+{
+ i2c_del_driver(&max17042_i2c_driver);
+ platform_driver_unregister(&max17042_platform_driver);
+}
+module_exit(max17042_exit);
MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
MODULE_DESCRIPTION("MAX17042 Fuel Gauge");
--
2.39.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v4 2/2] power: supply: max17042: add platform driver variant
2024-11-29 9:09 ` [PATCH v4 2/2] power: supply: max17042: add platform driver variant Dzmitry Sankouski
@ 2024-11-29 14:03 ` Amit Singh Tomar
2024-11-30 11:38 ` Dzmitry Sankouski
0 siblings, 1 reply; 5+ messages in thread
From: Amit Singh Tomar @ 2024-11-29 14:03 UTC (permalink / raw)
To: Dzmitry Sankouski, Hans de Goede, Krzysztof Kozlowski,
Marek Szyprowski, Sebastian Krzyszkowiak, Purism Kernel Team,
Sebastian Reichel, Rob Herring, Conor Dooley, Alim Akhtar,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Bjorn Andersson, Konrad Dybcio
Cc: linux-pm, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, imx, linux-arm-msm
Hi,
>
> 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 v4:
> - rename module_init and module_exit fuctions
> - rework max17042_init
> - assign chip_type in probe function
> - pass i2c_client as pointer on pointer, to use same pointer created in
> MFD. This allows devm_regmap_init_i2c to cleanup gracefully.
>
> Changes in v3:
> - pass dev pointer in max17042_probe
> - remove prints
> ---
> drivers/power/supply/max17042_battery.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 92 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index 99bf6915aa23..6a1bfc4a7b13 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,25 @@ 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->chip_type = chip_type;
> 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 +1086,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 +1109,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 +1118,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 +1130,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);
This seems a bit unusual; can't we just use:
struct i2c_client *i2c = dev_get_platdata(&pdev->dev); instead?
> + const struct platform_device_id *id = platform_get_device_id(pdev);
> +
> + if (!i2c)
> + return -EINVAL;
> +
> + return max17042_probe(*i2c, dev, id->driver_data);
and then just pass "i2c" here ?
> +}
> +
> #ifdef CONFIG_PM_SLEEP
> static int max17042_suspend(struct device *dev)
> {
> @@ -1204,6 +1227,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 +1244,45 @@ 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 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 max17042_exit(void)
> +{
> + i2c_del_driver(&max17042_i2c_driver);
> + platform_driver_unregister(&max17042_platform_driver);
> +}
> +module_exit(max17042_exit);
>
> MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
> MODULE_DESCRIPTION("MAX17042 Fuel Gauge");
>
> --
> 2.39.5
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 2/2] power: supply: max17042: add platform driver variant
2024-11-29 14:03 ` Amit Singh Tomar
@ 2024-11-30 11:38 ` Dzmitry Sankouski
0 siblings, 0 replies; 5+ messages in thread
From: Dzmitry Sankouski @ 2024-11-30 11:38 UTC (permalink / raw)
To: Amit Singh Tomar
Cc: Hans de Goede, Krzysztof Kozlowski, Marek Szyprowski,
Sebastian Krzyszkowiak, Purism Kernel Team, Sebastian Reichel,
Rob Herring, Conor Dooley, Alim Akhtar, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Bjorn Andersson,
Konrad Dybcio, linux-pm, devicetree, linux-kernel,
linux-arm-kernel, linux-samsung-soc, imx, linux-arm-msm
пт, 29 нояб. 2024 г. в 17:03, Amit Singh Tomar <amitsinght@marvell.com>:
>
> Hi,
>
> >
> > 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 v4:
> > - rename module_init and module_exit fuctions
> > - rework max17042_init
> > - assign chip_type in probe function
> > - pass i2c_client as pointer on pointer, to use same pointer created in
> > MFD. This allows devm_regmap_init_i2c to cleanup gracefully.
> >
> > Changes in v3:
> > - pass dev pointer in max17042_probe
> > - remove prints
> > ---
> > drivers/power/supply/max17042_battery.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
> > 1 file changed, 92 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
(...)
> > +static int max17042_platform_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct i2c_client **i2c = dev_get_platdata(dev);
> This seems a bit unusual; can't we just use:
> struct i2c_client *i2c = dev_get_platdata(&pdev->dev); instead?
> > + const struct platform_device_id *id = platform_get_device_id(pdev);
> > +
> > + if (!i2c)
> > + return -EINVAL;
> > +
> > + return max17042_probe(*i2c, dev, id->driver_data);
> and then just pass "i2c" here ?
This leads to hang on freeing devm resources, when unloading modules.
Platform driver version intended to be used as MFD sub device, where mfd
creates a dummy i2c client, and passes it to max17042 via platform data.
Sequence is: insmod MFD; insmod max17042; rmmod max17042; rmmod MFD; hang here.
My guess is that it is caused by a new pointer to the i2c-client. New pointer
created at `platform_device_add_data` function call in `mfd_add_device`.
Since C is pass by value, new pointer is assigned to platform device data.
--
Best regards,
Dzmitry
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-11-30 11:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-29 9:09 [PATCH v4 0/2] power: supply: max17042: cleanup and more features Dzmitry Sankouski
2024-11-29 9:09 ` [PATCH v4 1/2] power: supply: max17042: make interrupt shared Dzmitry Sankouski
2024-11-29 9:09 ` [PATCH v4 2/2] power: supply: max17042: add platform driver variant Dzmitry Sankouski
2024-11-29 14:03 ` Amit Singh Tomar
2024-11-30 11:38 ` Dzmitry Sankouski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).