On 07/25/2013 09:37 AM, Tom Rini wrote: > On Tue, Jul 23, 2013 at 01:34:15PM -0500, Dan Murphy wrote: >> On 07/19/2013 02:00 PM, Tom Rini wrote: >>> From: Greg Guyotte <gguyo...@ti.com> >>> >>> Add a driver for the TPS65217 PMIC that is found in the Beaglebone >>> family of boards.
Can we add the public reference to the technical manual in the commit message? I found <http://www.ti.com/lit/ug/slvu580b/slvu580b.pdf>http://www.ti.com/lit/ds/symlink/tps65217b.pdf >>> >>> Signed-off-by: Greg Guyotte <gguyo...@ti.com> >>> [trini: Split and rework Greg's changes into new drivers/power >>> framework] >>> Signed-off-by: Tom Rini <tr...@ti.com> >>> --- >>> drivers/power/pmic/Makefile | 1 + >>> drivers/power/pmic/pmic_tps65217.c | 108 >>> ++++++++++++++++++++++++++++++++++++ >>> include/power/tps65217.h | 92 ++++++++++++++++++++++++++++++ >>> 3 files changed, 201 insertions(+) >>> create mode 100644 drivers/power/pmic/pmic_tps65217.c >>> create mode 100644 include/power/tps65217.h >>> >>> diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile >>> index 14d426f..473cb80 100644 >>> --- a/drivers/power/pmic/Makefile >>> +++ b/drivers/power/pmic/Makefile >>> @@ -29,6 +29,7 @@ COBJS-$(CONFIG_POWER_MAX8998) += pmic_max8998.o >>> COBJS-$(CONFIG_POWER_MAX8997) += pmic_max8997.o >>> COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o >>> COBJS-$(CONFIG_POWER_MAX77686) += pmic_max77686.o >>> +COBJS-$(CONFIG_POWER_TPS65217) += pmic_tps65217.o >>> >>> COBJS := $(COBJS-y) >>> SRCS := $(COBJS:.o=.c) >>> diff --git a/drivers/power/pmic/pmic_tps65217.c >>> b/drivers/power/pmic/pmic_tps65217.c >>> new file mode 100644 >>> index 0000000..c84bbcd >>> --- /dev/null >>> +++ b/drivers/power/pmic/pmic_tps65217.c >>> @@ -0,0 +1,108 @@ >>> +/* >>> + * (C) Copyright 2011-2013 >> Curious if this is the first time in why does it have a 2011 copyright? > Because the code was written in 2011 (and has been whacked around a few > times every year. Got it thanks for the clarification > > [snip] >>> +/** >>> + * tps65217_reg_read() - Generic function that can read a TPS65217 register >>> + * @src_reg: Source register address >>> + * @src_val: Address of destination variable >> No return defined here in the brief > Fixed. > >>> + */ >>> +uchar tps65217_reg_read(uchar src_reg, uchar *src_val) >>> +{ >>> + if (i2c_read(TPS65217_CHIP_PM, src_reg, 1, src_val, 1)) >>> + return 1; >> This may be nit picky but generally in error cases we return negative. >> Also why not return an error from errno? > Because we're following i2c which is 0 or not 0, updated to use ret = > i2c_read(...); if (ret) return ret here and throughout. > >> Also why an uchar when you are returning an int? > Fixed. > > [snip] >>> +int tps65217_reg_write(uchar prot_level, uchar dest_reg, uchar dest_val, >> is prot_level a uchar or int? > It's 0/1/2. I don't have a strong preference on if we type this out as > an int or uchar. > >> Also would it not be better to have an interface that will check for >> mask and do the read and just have a dedicated write function? > I don't see the benefit, especially given the usage we have of just > updating certain bitfields at a time. > > [snip] >>> +int tps65217_voltage_update(uchar dc_cntrl_reg, uchar volt_sel) >> No header for the interface > Fixed. > >>> +{ >>> + if ((dc_cntrl_reg != DEFDCDC1) && (dc_cntrl_reg != DEFDCDC2) && >>> + (dc_cntrl_reg != DEFDCDC3)) >> What do these magic numbers mean? Are these HEX numbers or a string? > OK, it took me a minute to understand your question here. These are > defines to register names, matching the TRM for the part. The register > names are however annoyingly and easily confused as hex values. Maybe we can rename the #defines so they are not so confusing. Maybe something like #define TPS65217_<register name> <register offset> So these will become #define TPS65217_DEFDCDC1 0xe #define TPS65217_DEFDCDC2 0xf #define TPS65217_DEFDCDC3 0x10 This will at least keep other defines like CHIPID and SEQ unique as well. > >>> +#define PROT_LEVEL_NONE 0x00 >> Are these registers or a mask now? >>> +#define PROT_LEVEL_1 0x01 >>> +#define PROT_LEVEL_2 0x02 > These are values as to what level of protection the chip has the > register under. > >>> +uchar tps65217_reg_read(uchar src_reg, uchar *src_val); >>> +int tps65217_reg_write(uchar prot_level, uchar dest_reg, uchar dest_val, >>> + uchar mask); >>> +int tps65217_voltage_update(uchar dc_cntrl_reg, uchar volt_sel); >> Are these interfaces supposed to be accessed by an outside object? >> >> Typically there should be no direct register access from other objects. > We can evaluate if there's consolidation to be done here once other > boards go and adapt MPU clock frequency scaling. What registers need to > be whacked on what board are going to decide if we can hide more > details, or not. > -- ------------------ Dan Murphy
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
