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. > > > > 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. [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. > > +#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. -- Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot