Re: [PATCHv2] power: supply: cpcap-charger: Add minimal CPCAP PMIC battery charger

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Wed, Mar 22, 2017 at 04:42:10PM -0700, Tony Lindgren wrote:
> The custom CPCAP PMIC used on Motorola phones such as Droid 4 has a
> USB battery charger. It can optionally also have a companion chip that
> is used for wireless charging.
> 
> The charger on CPCAP also can feed VBUS for the USB host mode. This
> can be handled by the existing kernel phy_companion interface.
>
> [...]
>
> diff --git a/Documentation/devicetree/bindings/power/supply/cpcap-charger.txt b/Documentation/devicetree/bindings/power/supply/cpcap-charger.txt
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/cpcap-charger.txt
> @@ -0,0 +1,34 @@
> +Motorola CPCAP PMIC battery charger binding
> +
> +Required properties:
> +- compatible: Shall be "motorola,cpcap-charger" or
> +	      "motorola,mapphone-cpcap-charger"
> +- interrupts: Interrupts used on the CPCAP PMIC
> +- interrupt-names: Names of the interrupts

That's usually done like

interrupts: Interrupt specifier for each name in interrupt-names
interrupt-names: Should contain the following entries:
 "bla", "foo", "42"

> +- io-channels: IIO ADC channels used by the charger
> +- io-channel-names: Names of the IIO ADC channels

Same as for interrupts.

> +Optional properties:
> +- mode-gpios: Optionally CPCAP charger can have a companion wireless
> +	      charge controller that is controlled with two GPIOs

DT people prefer to have GPIO activity (high/low) to be specified in the
binding.

> +Example:
> +
> +cpcap_charger: charger {
> +	compatible = "motorola,mapphone-cpcap-charger";
> +	interrupts-extended = <
> +		&cpcap 13 0 &cpcap 12 0 &cpcap 29 0 &cpcap 28 0
> +		&cpcap 22 0 &cpcap 20 0 &cpcap 19 0 &cpcap 54 0
> +	>;
> +	interrupt-names =
> +		"chrg_det", "rvrs_chrg", "chrg_se1b", "se0conn",
> +		"rvrs_mode", "chrgcurr1", "vbusvld", "battdetb";
> +	mode-gpios = <&gpio3 29 GPIO_ACTIVE_LOW
> +		      &gpio3 23 GPIO_ACTIVE_LOW>;
> +	io-channels = <&cpcap_adc 0 &cpcap_adc 1
> +		       &cpcap_adc 2 &cpcap_adc 5
> +		       &cpcap_adc 6>;
> +	io-channel-names = "battdetb", "battp",
> +			   "vbus", "chg_isense",
> +			   "batti";
> +};
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -317,6 +317,13 @@ config BATTERY_RX51
>  	  Say Y here to enable support for battery information on Nokia
>  	  RX-51, also known as N900 tablet.
>  
> +config CHARGER_CPCAP
> +	tristate "CPCAP PMIC Charger Driver"
> +	depends on MFD_CPCAP
> +	help
> +	  Say Y to enable support for CPCAP PMIC charger driver for Motorola
> +	  mobile devices such as Droid 4.
> +

I think adding "default MFD_CPCAP" for CPCPAP's subdrivers would be nice.

>  config CHARGER_ISP1704
>  	tristate "ISP1704 USB Charger Detection"
>  	depends on USB_PHY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> 
> [...]
>
> +#include <linux/gpio/consumer.h>
> +#include <linux/usb/phy_companion.h>
> +#include <linux/phy/omap_usb.h>
> +#include <linux/usb/otg.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/mfd/motorola-cpcap.h>

This looks very entangled with mapphone specific things,
so maybe we should not add "motorola,cpcap-charger" for
now?

> [...]
>
> +static int cpcap_charger_get_ints_state(struct cpcap_charger_ddata *ddata,
> +					struct cpcap_charger_ints_state *s)
> +{
> +	int val, error;
> +
> +	error = regmap_read(ddata->reg, CPCAP_REG_INTS1, &val);
> +	if (error)
> +		return error;
> +
> +	s->chrg_det = val & BIT(13);
> +	s->rvrs_chrg = val & BIT(12);
> +	s->vbusov = val & BIT(11);
> +
> +	error = regmap_read(ddata->reg, CPCAP_REG_INTS2, &val);
> +	if (error)
> +		return error;
> +
> +	s->chrg_se1b = val & BIT(13);
> +	s->rvrs_mode = val & BIT(6);
> +	s->chrgcurr1 = val & BIT(4);
> +	s->vbusvld = val & BIT(3);
> +
> +	error = regmap_read(ddata->reg, CPCAP_REG_INTS4, &val);
> +	if (error)
> +		return error;
> +
> +	s->battdetb = val & BIT(6);

Did you avoid using cpcap_sense_virq() for performance reasons?

s->battdetb = cpcap_sense_virq(ddata->reg, ddata->irq_battdetb);
(and so on)

looks cleaner IMHO.

> +
> +	return 0;
> +}
>
> [...]

-- Sebastian

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux