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

[Dan Murphy <dmurphy@ti.com>]

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