Re: [PATCH] power: Add ltc3651-charger driver

[Sebastian Reichel]

Hi,

On Tue, May 02, 2017 at 03:48:33PM +0200, Mike Looijmans wrote:
> The LTC3651 reports its status via GPIO lines. This driver translates
> the GPIO levels to battery charger status information via sysfs.
> It relies on devicetree to supply the IO configuration.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
> ---
> I'll send the devicetree binding documentation in a separate patch.

FYI: I will not merge the driver without the docs.

>  drivers/power/supply/Kconfig           |   7 ++
>  drivers/power/supply/Makefile          |   1 +
>  drivers/power/supply/ltc3651-charger.c | 224 +++++++++++++++++++++++++++++++++
>  3 files changed, 232 insertions(+)
>  create mode 100644 drivers/power/supply/ltc3651-charger.c
> [...]
> diff --git a/drivers/power/supply/ltc3651-charger.c b/drivers/power/supply/ltc3651-charger.c
> new file mode 100644
> index 0000000..cf73640
> --- /dev/null
> +++ b/drivers/power/supply/ltc3651-charger.c
> @@ -0,0 +1,224 @@
> +/*
> + *  Copyright (C) 2017, Topic Embedded Products
> + *  Driver for LTC3651 charger IC.
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under  the terms of the GNU General  Public License as published by the
> + *  Free Software Foundation;  either version 2 of the License, or (at your
> + *  option) any later version.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  675 Mass Ave, Cambridge, MA 02139, USA.

Can you drop the second paragraph? That way we do not need to update
their address every now and then.

> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>

Do you really need of_gpio.h?

> +struct ltc3651_charger {
> +	struct power_supply *charger;
> +	struct power_supply_desc charger_desc;
> +	struct gpio_desc *acpr_gpio;
> +	struct gpio_desc *fault_gpio;
> +	struct gpio_desc *chrg_gpio;
> +};

[...]

> +static int ltc3651_charger_get_property(struct power_supply *psy,
> +		enum power_supply_property psp, union power_supply_propval *val)
> +{
> +	struct ltc3651_charger *ltc3651_charger = psy_to_ltc3651_charger(psy);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		if (!ltc3651_charger->chrg_gpio) {
> +			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> +			break;
> +		}
> +		if (gpiod_get_value(ltc3651_charger->chrg_gpio))
> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +		else
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = gpiod_get_value(ltc3651_charger->acpr_gpio);
> +		break;
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		if (!ltc3651_charger->fault_gpio) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> +			break;
> +		}
> +		if (!gpiod_get_value(ltc3651_charger->fault_gpio)) {
> +			val->intval = POWER_SUPPLY_HEALTH_GOOD;
> +			break;
> +		}
> +		/*
> +		 * If the fault pin is active, the chrg pin explains the type
> +		 * of failure.
> +		 */
> +		if (!ltc3651_charger->chrg_gpio) {
> +			val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
> +			break;
> +		}
> +		val->intval = gpiod_get_value(ltc3651_charger->chrg_gpio) ?
> +				POWER_SUPPLY_HEALTH_OVERHEAT :
> +				POWER_SUPPLY_HEALTH_DEAD;
> +		break;

So reporting of status is incorrect with fault_gpio being set?

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum power_supply_property ltc3651_charger_properties[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_HEALTH,
> +};
> +
> +static int ltc3651_charger_probe(struct platform_device *pdev)
> +{
> +	struct power_supply_config psy_cfg = {};
> +	struct ltc3651_charger *ltc3651_charger;
> +	struct power_supply_desc *charger_desc;
> +	int ret;
> +
> +	ltc3651_charger = devm_kzalloc(&pdev->dev, sizeof(*ltc3651_charger),
> +					GFP_KERNEL);
> +	if (!ltc3651_charger) {
> +		dev_err(&pdev->dev, "Failed to alloc driver structure\n");

Drop this. It won't happen on real systems and the memory allocation
will print warnings itself.

> +		return -ENOMEM;
> +	}
> +
> +	ltc3651_charger->acpr_gpio = devm_gpiod_get(&pdev->dev,
> +					"acpr", GPIOD_IN);
> +	if (IS_ERR(ltc3651_charger->acpr_gpio)) {
> +		ret = PTR_ERR(ltc3651_charger->charger);
> +		dev_err(&pdev->dev, "Failed to acquire acpr GPIO: %d\n", ret);
> +		return ret;
> +	}
> +	ltc3651_charger->fault_gpio = devm_gpiod_get_optional(&pdev->dev,
> +					"fault", GPIOD_IN);
> +	if (IS_ERR(ltc3651_charger->fault_gpio)) {
> +		ret = PTR_ERR(ltc3651_charger->charger);
> +		dev_err(&pdev->dev, "Failed to acquire fault GPIO: %d\n", ret);
> +		return ret;
> +	}
> +	ltc3651_charger->chrg_gpio = devm_gpiod_get_optional(&pdev->dev,
> +					"chrg", GPIOD_IN);
> +	if (IS_ERR(ltc3651_charger->chrg_gpio)) {
> +		ret = PTR_ERR(ltc3651_charger->charger);
> +		dev_err(&pdev->dev, "Failed to acquire chrg GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
> +	charger_desc = &ltc3651_charger->charger_desc;
> +	charger_desc->name = pdev->dev.of_node->name;
> +	charger_desc->type = POWER_SUPPLY_TYPE_MAINS;
> +	charger_desc->properties = ltc3651_charger_properties;
> +	charger_desc->num_properties = ARRAY_SIZE(ltc3651_charger_properties);
> +	charger_desc->get_property = ltc3651_charger_get_property;
> +	psy_cfg.of_node = pdev->dev.of_node;
> +	psy_cfg.drv_data = ltc3651_charger;
> +
> +	ltc3651_charger->charger = power_supply_register(&pdev->dev,
> +						      charger_desc, &psy_cfg);
> +	if (IS_ERR(ltc3651_charger->charger)) {
> +		ret = PTR_ERR(ltc3651_charger->charger);
> +		dev_err(&pdev->dev, "Failed to register power supply: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	/* Acquire IRQs for the GPIO pins if possible */
> +	if (ltc3651_charger->acpr_gpio) {
> +		ret = gpiod_to_irq(ltc3651_charger->acpr_gpio);
> +		if (ret >= 0)
> +			ret = devm_request_any_context_irq(&pdev->dev, ret,
> +				ltc3651_charger_irq,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				dev_name(&pdev->dev), ltc3651_charger->charger);
> +		if (ret < 0)
> +			dev_warn(&pdev->dev, "Failed to request acpr irq\n");
> +	}
> +	if (ltc3651_charger->fault_gpio) {
> +		ret = gpiod_to_irq(ltc3651_charger->fault_gpio);
> +		if (ret >= 0)
> +			ret = devm_request_any_context_irq(&pdev->dev, ret,
> +				ltc3651_charger_irq,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				dev_name(&pdev->dev), ltc3651_charger->charger);
> +		if (ret < 0)
> +			dev_warn(&pdev->dev, "Failed to request fault irq\n");
> +	}
> +	if (ltc3651_charger->chrg_gpio) {
> +		ret = gpiod_to_irq(ltc3651_charger->chrg_gpio);
> +		if (ret >= 0)
> +			ret = devm_request_any_context_irq(&pdev->dev, ret,
> +				ltc3651_charger_irq,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				dev_name(&pdev->dev), ltc3651_charger->charger);
> +		if (ret < 0)
> +			dev_warn(&pdev->dev, "Failed to request chrg irq\n");
> +	}

I think we should either require irq support or provide
some kind of polling fallback.

> +	platform_set_drvdata(pdev, ltc3651_charger);
> +
> +	return 0;
> +}
> +
> +static int ltc3651_charger_remove(struct platform_device *pdev)
> +{
> +	struct ltc3651_charger *ltc3651_charger = platform_get_drvdata(pdev);
> +
> +	power_supply_unregister(ltc3651_charger->charger);
> +
> +	return 0;
> +}

Use devm_power_supply_register() and kill remove function :)

> +static const struct of_device_id ltc3651_charger_match[] = {
> +	{ .compatible = "ltc3651-charger" },

Missing vendor prefix.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ltc3651_charger_match);
> +
> +static struct platform_driver ltc3651_charger_driver = {
> +	.probe = ltc3651_charger_probe,
> +	.remove = ltc3651_charger_remove,
> +	.driver = {
> +		.name = "ltc3651-charger",
> +		.of_match_table = ltc3651_charger_match,
> +	},
> +};
> +
> +module_platform_driver(ltc3651_charger_driver);
> +
> +MODULE_AUTHOR("Mike Looijmans <mike.looijmans@xxxxxxxx>");
> +MODULE_DESCRIPTION("Driver for LTC3651 charger");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ltc3651-charger");

-- Sebastian

Attachment: signature.asc
Description: PGP signature