Re: [PATCH 3/3] power: supply: initial support for TWL6030/32

[Krzysztof Kozlowski <krzk@kernel.org>]

On 18/09/2024 10:41, Andreas Kemnade wrote:
> Add a driver for the charger in the TWL6030/32. For now it does not report
> much in sysfs but parameters are set up for USB, charging is enabled with
> the specified parameters. It stops charging when full and also restarts
> charging.
> This prevents ending up in a system setup where you run out of battery
> although a charger is plugged in after precharge completed.
> 
> Battery voltage behavior was checked via the GPADC.
> 

Few stylistic comments below.

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/power/supply/Kconfig           |  10 +
>  drivers/power/supply/Makefile          |   1 +
>  drivers/power/supply/twl6030_charger.c | 566 +++++++++++++++++++++++++
>  3 files changed, 577 insertions(+)
>  create mode 100644 drivers/power/supply/twl6030_charger.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index bcfa63fb9f1e2..9f2eef6787f7a 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -493,6 +493,16 @@ config CHARGER_TWL4030
>  	help
>  	  Say Y here to enable support for TWL4030 Battery Charge Interface.
>  
> +config CHARGER_TWL6030
> +	tristate "OMAP TWL6030 BCI charger driver"
> +	depends on IIO && TWL4030_CORE

|| COMPILE_TEST, at least for TWL part
(but please test first)

> +	help
> +	  Say Y here to enable support for TWL6030/6032 Battery Charge
> +	  Interface.
> +
> +	  This driver can be build as a module. If so, the module will be
> +	  called twl6030_charger.
> +



> +
> +static int twl6030_charger_probe(struct platform_device *pdev)
> +{
> +	struct twl6030_charger_info *charger;
> +	struct power_supply_config psy_cfg = {};
> +	int ret;
> +	u8 val;
> +
> +	charger = devm_kzalloc(&pdev->dev, sizeof(*charger), GFP_KERNEL);
> +	if (!charger)
> +		return -ENOMEM;
> +
> +	charger->dev = &pdev->dev;
> +	charger->irq_chg = platform_get_irq(pdev, 0);
> +
> +	platform_set_drvdata(pdev, charger);
> +	psy_cfg.drv_data = charger;
> +
> +	charger->channel_vusb = devm_iio_channel_get(&pdev->dev, "vusb");
> +	if (IS_ERR(charger->channel_vusb)) {
> +		ret = PTR_ERR(charger->channel_vusb);
> +		if (ret == -EPROBE_DEFER)
> +			return ret;	/* iio not ready */
> +		dev_warn(&pdev->dev, "could not request vusb iio channel (%d)",
> +			 ret);
> +		charger->channel_vusb = NULL;
> +	}
> +
> +	charger->usb = devm_power_supply_register(&pdev->dev,
> +						  &twl6030_charger_usb_desc,
> +						  &psy_cfg);
> +	if (IS_ERR(charger->usb)) {

Checkpatch...

> +		return dev_err_probe(&pdev->dev, PTR_ERR(charger->usb),
> +				     "Failed to register usb\n");
> +	}
> +
> +	ret = power_supply_get_battery_info(charger->usb, &charger->binfo);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to get battery info\n");
> +
> +	dev_info(&pdev->dev, "battery with vmax %d imax: %d\n",
> +		 charger->binfo->constant_charge_voltage_max_uv,
> +		 charger->binfo->constant_charge_current_max_ua);
> +
> +	if (charger->binfo->constant_charge_voltage_max_uv == -EINVAL) {
> +		ret = twl6030_charger_read(CHARGERUSB_CTRLLIMIT1, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		charger->binfo->constant_charge_voltage_max_uv =
> +			VOREG_TO_UV(val);
> +	}
> +
> +	if (charger->binfo->constant_charge_voltage_max_uv > 4760000 ||
> +	    charger->binfo->constant_charge_voltage_max_uv < 350000)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "Invalid charge voltage\n");
> +
> +	if (charger->binfo->constant_charge_current_max_ua == -EINVAL) {
> +		ret = twl6030_charger_read(CHARGERUSB_CTRLLIMIT2, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		charger->binfo->constant_charge_current_max_ua = VICHRG_TO_UA(val);
> +	}
> +
> +	if (charger->binfo->constant_charge_current_max_ua < 100000 ||
> +	    charger->binfo->constant_charge_current_max_ua > 1500000) {
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +			 "Invalid charge current\n");
> +	}
> +
> +	if ((charger->binfo->charge_term_current_ua != -EINVAL) &&
> +	    (charger->binfo->charge_term_current_ua > 400000 ||
> +	     charger->binfo->charge_term_current_ua < 50000)) {
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +			"Invalid charge termination current\n");
> +	}
> +
> +	ret = devm_delayed_work_autocancel(&pdev->dev,
> +					   &charger->charger_monitor,
> +					   twl6030_charger_wdg);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to register delayed work\n");
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, charger->irq_chg, NULL,
> +					twl6030_charger_interrupt,
> +					IRQF_ONESHOT, pdev->name,
> +					charger);
> +	if (ret < 0) {

Drop {}, see checkpatch.

> +		return dev_err_probe(&pdev->dev, ret,
> +				     "could not request irq %d\n",
> +				     charger->irq_chg);
> +	}
> +
> +	/* turing to charging to configure things */
> +	twl6030_charger_write(CONTROLLER_CTRL1, 0);
> +	twl6030_charger_interrupt(0, charger);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id twl_charger_of_match[] __maybe_unused = {
> +	{.compatible = "ti,twl6030-charger", },
> +	{.compatible = "ti,twl6032-charger", },

So they are compatible? Why two entries in such case?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, twl_charger_of_match);
> +
> +static struct platform_driver twl6030_charger_driver = {
> +	.probe = twl6030_charger_probe,
> +	.driver	= {
> +		.name	= "twl6030_charger",
> +		.of_match_table = of_match_ptr(twl_charger_of_match),

I propose to drop of_match_ptr and maybe_unused, so this won't be
restricted only to OF

> +	},
> +};
> +module_platform_driver(twl6030_charger_driver);
> +
> +MODULE_DESCRIPTION("TWL6030 Battery Charger Interface driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:twl6030_charger");


You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.


Best regards,
Krzysztof