On Wed, 10 Jun 2015, Hans de Goede wrote: > Hi, > > Thanks for the quick review I'll do a v2 addressing your concerns soonish. > > On 10-06-15 09:29, Lee Jones wrote: > >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@xxxxxxxxxxxxxxxxx> > >>Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > >>--- > >> .../bindings/power_supply/axp20x_usb_power.txt | 34 +++ > > > >This needs to be submitted in a seperate patch. > > Ok. > > > > >> 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? > > Yes I plan to write drivers for the other 3 power_supply class > cells (ac-power, battery-charger, rtc-bat-charger) in the axp209, > and most of those need the same helper which I why I put it here. Okay. > >>+ unsigned int reg, unsigned int width) > > > >The function name is a bit misleading. > > How about: axp20x_read_multibyte_reg ? axp20x_read_variable_width() ? > >>+{ > >>+ 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 > > Yes, I guess this was done so that you can get all the 8 msb-s > in a single read if you do not care about the lsb-s. Odd, but okay. > >>+ 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html