Re: [PATCH] power: supply: Add driver for TI BQ2416X battery charger

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 11123 bytes --]

Hi,

On Tue, Feb 07, 2017 at 01:09:09AM +0000, Wojciech Ziemba wrote:
> There is interest in adding a Linux driver for TI BQ2416X battery
> charger.

This is a strange sentence to introduce a patch. If there wasn't
you wouldn't have sent a patch...

> The driver supports BQ24160 chip, thus can be easily extended
> for other BQ2416X family chargers.

Doesn't it already do? Do you mean "I only tested the driver using
bq24160"?

> Device exposes 'POWER_SUPPLY_PROP_*' properties

ok.

> and a number of knobs for controlling the charging process

missing sysfs ABI documentation. Most of them are probably either
not needed, or should become standard POWER_SUPPLY_PROP_ properties.

> as well as sends power supply change notification via power-supply
> subsystem.

ok.

> Signed-off-by: Wojciech Ziemba <wojciech.ziemba@verifone.com>
> ---
>  .../devicetree/bindings/power/supply/bq2416x.txt   |   71 +
>  drivers/power/supply/Kconfig                       |    7 +
>  drivers/power/supply/Makefile                      |    1 +
>  drivers/power/supply/bq2416x_charger.c             | 1871 ++++++++++++++++++++
>  include/dt-bindings/power/bq2416x_charger.h        |   23 +
>  include/linux/power/bq2416x_charger.h              |   80 +
>  6 files changed, 2053 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/bq2416x.txt
>  create mode 100644 drivers/power/supply/bq2416x_charger.c
>  create mode 100644 include/dt-bindings/power/bq2416x_charger.h
>  create mode 100644 include/linux/power/bq2416x_charger.h
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/bq2416x.txt b/Documentation/devicetree/bindings/power/supply/bq2416x.txt
> new file mode 100644
> index 0000000..8f4b814
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/bq2416x.txt
> @@ -0,0 +1,71 @@
> +Binding for TI bq2416x Li-Ion Charger
> +
> +Required properties:
> +===================
> +- compatible: one of the following:
> + * "ti,bq24160"
> + * "ti,bq24160a"
> + * "ti,bq24161"
> + * "ti,bq24161b"
> + * "ti,bq24163"
> + * "ti,bq24168"
> +
> +- reg:			I2c address of the device.
> +- interrupt-parent:	The irq controller(phandle) connected to INT pin on BQ2416x
> +- interrupts:		The irq number assigned for INT pin.
> +
> +Optional properties:
> +===================
> +- interrupt-names:		Meanigfull irq name.

Drop this, it's not used.

> +- ti,charge-voltage:		Charge volatge [mV].
> +- ti,charge-current:		Charge current [mA].
> +- ti,termination-current:	Termination current [mA}.

These are battery dependent. You should get them from the
battery instead.

> +- ti,in-current-limit:		IN source current limit. enum:
> +				- IN_CURR_LIM_1500MA (0)
> +				- IN_CURR_LIM_2500MA (1)

This is probably needed. Just use an integer with the current
instead of the enum. The driver can just bail out if invalid
value was specified.

> +- ti,usb-current-limit:		USB source current limit. enum:
> +				- USB_CURR_LIM_100MA (0)
> +				- USB_CURR_LIM_150MA (1)
> +				- USB_CURR_LIM_500MA (2)
> +				- USB_CURR_LIM_800MA (3)
> +				- USB_CURR_LIM_900MA (4)
> +				- USB_CURR_LIM_1500MA (5)

Let's not add that to the DT binding. It should be auto-detected.
Additionally you can make the sysfs property writable.

> +- ti,status-pin-enable:		0 or 1. Enable charge status output STAT pin.
> +- ti,current-termination-enable:0 or 1. Enable charge current termination.

If termination current is specified -> enable, otherwise -> disable,
so not needed.

> +- ti,usb-dpm-voltage:		USB dpm voltage [mV]. Refer to datasheet.
> +- ti,in-dpm-voltage:		IN dpm voltage [mV].

I will have to check datasheet before commenting about this one.

> +- ti,safety-timer:		Safety timer. enum:
> +			- TMR_27MIN (0)
> +			- TMR_6H (1)
> +			- TMR_9H (2)
> +			- TMR_OFF (3)

This does not belong into DT. Just always set it to 27 minutes and
properly reset the timer in the driver. You will also need a suspend
handler, that disables the timer (or wakeup every now and then to
reset it).

> +- ti,supplied-to:	string array: max 4. Names of devices to which
> +			the charger supplies.

There is a standard binding for this, which is documented here:

Documentation/devicetree/bindings/power/supply/power_supply.txt

> +Example:
> +=======
> +#include <dt-bindings/power/bq2416x_charger.h>
> +
> +bq24160@6b {
> +	compatible = "ti,bq24160";
> +	reg = <0x6b>;
> +
> +	interrupt-parent = <&gpio5>;
> +	interrupts = <31 IRQ_TYPE_EDGE_RISING>;
> +	interrupt-names = "bq24160-charge-status-change";
> +
> +	ti,charge-voltage = <4300>;
> +	ti,charge-current = <1300>;
> +	ti,termination-current = <100>;
> +	ti,in-current-limit = <IN_CURR_LIM_1500MA>;
> +	ti,usb-current-limit = <USB_CURR_LIM_1500MA>;
> +	ti,status-pin-enable = <1>;
> +	ti,current-termination-enable = <1>;
> +	ti,usb-dpm-voltage = <4300>;
> +	ti,in-dpm-voltage = <4300>;
> +	ti,safety-timer = <TMR_6H>; /* charge max 6h */
> +	ti,supplied-to = "bq27621-0";
> +};
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 76806a0..575096e 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -413,6 +413,13 @@ config CHARGER_BQ2415X
>  	  You'll need this driver to charge batteries on e.g. Nokia
>  	  RX-51/N900.
>  
> +config CHARGER_BQ2416X
> +	tristate "TI BQ2416x Dual-Input, Single Cell Switch-Mode Li-Ion charger"
> +	depends on I2C
> +	help
> +	  Say Y here to enable support for the TI BQ2416x battery charger.
> +	  The driver is configured to operate with a single lithium cell.
> +
>  config CHARGER_BQ24190
>  	tristate "TI BQ24190 battery charger driver"
>  	depends on I2C
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 36c599d..73711e0 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_CHARGER_MAX8997)	+= max8997_charger.o
>  obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
>  obj-$(CONFIG_CHARGER_QCOM_SMBB)	+= qcom_smbb.o
>  obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
> +obj-$(CONFIG_CHARGER_BQ2416X)	+= bq2416x_charger.o
>  obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
>  obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
>  obj-$(CONFIG_CHARGER_BQ24735)	+= bq24735-charger.o
> diff --git a/drivers/power/supply/bq2416x_charger.c b/drivers/power/supply/bq2416x_charger.c
> new file mode 100644
> index 0000000..fa13e55
> --- /dev/null
> +++ b/drivers/power/supply/bq2416x_charger.c
> @@ -0,0 +1,1871 @@
> +/*
> + * Driver for BQ2416X Li-Ion Battery Charger
> + *
> + * Copyright (C) 2015 Verifone, Inc.
> + *
> + * Author: Wojciech Ziemba <wojciech.ziemba@verifone.com>
> + *
> + * This package is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * THIS PACKAGE IS PROVIDED AS IS AND WITHOUT ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> + *
> + * The bq2416x series is a 2.5A, Dual-Input, Single-Cell Switched-Mode
> + * Li-Ion Battery Charger with Power
> + * Path Management and I2C Interface

strange line wrapping above.

> + * This driver was tested on BQ24160.
> + *
> + * Datasheets:
> + * http://www.ti.com/product/bq24160
> + * http://www.ti.com/product/bq24160a
> + * http://www.ti.com/product/bq24161
> + * http://www.ti.com/product/bq24161b
> + * http://www.ti.com/product/bq24163
> + * http://www.ti.com/product/bq24168
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/regmap.h>
> +#include <linux/power_supply.h>
> +#include <linux/power/bq2416x_charger.h>

[...]

> diff --git a/include/linux/power/bq2416x_charger.h b/include/linux/power/bq2416x_charger.h
> new file mode 100644
> index 0000000..c561666
> --- /dev/null
> +++ b/include/linux/power/bq2416x_charger.h
> @@ -0,0 +1,80 @@
> +/*
> + * Driver for BQ2416X Li-Ion Battery Charger
> + *
> + * Copyright (C) 2015 Verifone, Inc.
> + *
> + * Author: Wojciech Ziemba <wojciech.ziemba@verifone.com>
> + *
> + * This package is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * THIS PACKAGE IS PROVIDED AS IS AND WITHOUT ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> + *
> + * The bq2416x series is a 2.5A, Dual-Input, Single-Cell Switched-Mode
> + * Li-Ion Battery Charger with Power
> + * Path Management and I2C Interface
> + *
> + */
> +
> +#ifndef _BQ2416X_CHARGER_H
> +#define _BQ2416X_CHARGER_H
> +
> +/* IN(Wall) source limit */
> +enum in_curr_lim {
> +	IN_CURR_LIM_1500MA,
> +	IN_CURR_LIM_2500MA,
> +};
> +
> +/* USB source current limit */
> +enum usb_curr_lim {
> +	USB_CURR_LIM_100MA,
> +	USB_CURR_LIM_150MA,
> +	USB_CURR_LIM_500MA,
> +	USB_CURR_LIM_800MA,
> +	USB_CURR_LIM_900MA,
> +	USB_CURR_LIM_1500MA,
> +};
> +
> +/* Safety timer settings */
> +enum safe_tmr {
> +	TMR_27MIN,
> +	TMR_6H,
> +	TMR_9H,
> +	TMR_OFF,
> +};
> +
> +/**
> + * struct bq2416x_pdata - Platform data for bq2416x chip. It contains default
> + *			  board voltages and currents.
> + * @charge_voltage: charge voltage in [mV]
> + * @charge_current: charge current in [mA]
> + * @in_curr_limit: Current limit for IN source . Enum 1.5A or 2.5A
> + * @usb_curr_limit: Current limit for USB source Enum 100mA - 1500mA
> + * @curr_term_en: enable charge terination by current
> + * @term_current: charge termination current in [mA]
> + * @usb_dpm_voltage: USB DPM voltage [mV]
> + * @in_dpm_voltage: IN DPM voltage [mV]
> + * @stat_pin_en: status pin enable
> + * @safety_timer: safety timer enum: 27min, 6h, 9h, off.
> + * @num_supplicants: number of notify devices. Max 4.
> + * @supplied_to: array of names of supplied to devices
> + */
> +struct bq2416x_pdata {
> +	int charge_voltage;
> +	int charge_current;
> +	enum in_curr_lim in_curr_limit;
> +	enum usb_curr_lim usb_curr_limit;
> +	int curr_term_en;
> +	int term_current;
> +	int usb_dpm_voltage;
> +	int in_dpm_voltage;
> +	int stat_pin_en;
> +	enum safe_tmr safety_timer;
> +	int num_supplicants;
> +	const char *supplied_to[4];
> +};
> +
> +#endif /* _BQ2416X_CHARGER_H */

Please use device properties instead of platform_data (especially if
this is not yet used).

Apart from the comments I only skipped quickly over the driver. I
will have a more detailed look once the basic things are fixed.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]