Re: [PATCH 4/8] power: Add an axp20x-usb-power driver

[Lee Jones <lee.jones@linaro.org>]

On Tue, 09 Jun 2015, Hans de Goede wrote:

> This adds a driver for the usb power_supply bits of the axp20x PMICs.
> 
> I initially started writing my own driver, before coming aware of
> Bruno Prémont's excellent earlier RFC with a driver for this.
> 
> My driver was lacking CURRENT_MAX and VOLTAGE_MIN support Bruno's
> drvier has, so I've copied the code for those from his driver.
> 
> Note that the AC-power-supply and battery charger bits will need separate
> drivers. Each one needs its own devictree child-node so that other
> devicetree nodes can reference the right power-supply, and thus each one
> will get its own mfd-cell / platform_device and platform-driver.
> 
> Cc: Bruno Prémont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../bindings/power_supply/axp20x_usb_power.txt     |  34 +++

This needs to be submitted in a seperate patch.

>  drivers/power/Kconfig                              |   7 +
>  drivers/power/Makefile                             |   1 +
>  drivers/power/axp20x_usb_power.c                   | 241 +++++++++++++++++++++
>  include/linux/mfd/axp20x.h                         |  24 ++
>  5 files changed, 307 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power_supply/axp20x_usb_power.txt
>  create mode 100644 drivers/power/axp20x_usb_power.c

[...]

> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index f4290ae..d7adb2e 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -11,6 +11,8 @@
>  #ifndef __LINUX_MFD_AXP20X_H
>  #define __LINUX_MFD_AXP20X_H
>  
> +#include <linux/regmap.h>
> +
>  enum {
>  	AXP202_ID = 0,
>  	AXP209_ID,
> @@ -366,4 +368,26 @@ struct axp20x_fg_pdata {
>  	int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
>  };
>  
> +/* generic helper function for reading 9-16 bit wide regs */
> +static inline int axp20x_read_16bit(struct regmap *regmap,

This is only used twice and only in one file.

Do you know of any other uses of this call that will be upstreamed
shortly?

> +				    unsigned int reg, unsigned int width)

The function name is a bit misleading.

> +{
> +	unsigned int v, raw;
> +	int r;

'v' and 'r' are not good variable names.

> +	r = regmap_read(regmap, reg, &v);
> +	if (r)
> +		return r;
> +
> +	raw = v << (width - 8);

So 'reg' is expected to be top end loaded?

E.g A value of say 0x0101 (257) in 9 bits will look like this:

reg          reg + 1
1000 0000b   0000 0001b

> +	r = regmap_read(regmap, reg + 1, &v);
> +	if (r)
> +		return r;
> +
> +	raw |= v;
> +
> +	return raw;
> +}
> +
>  #endif /* __LINUX_MFD_AXP20X_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog