[U-Boot] [PATCH 2/6] drivers/power/pmic: Add tps65217 driver

Dan Murphy
2013-07-25T17:21:41Z

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 <gguyotte@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 <gguyotte@ti.com>
>>> [trini: Split and rework Greg's changes into new drivers/power
>>> framework]
>>> Signed-off-by: Tom Rini <trini@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