[U-Boot] [RFC 0/2] Generic PMIC driver

[Stefano Babic <sbabic@denx.de>]

On 09/19/2011 05:06 PM, Lukasz Majewski wrote:
> Dear all,

Hi Lukasz,

> 
> I'd like to propose a new approach for PMIC generic driver.

Fine !

> 
> In my opinion following issues needs discussion:
> 1. In proposed 
> int pmic_reg_read(struct pmic *p, u32 *val) the val is returned by pointer.

And the register to be read/write is embedded in the structure. For
readness I will let it as separate parameter, but this is my personal taste.

Who is responsible to allocate the pmic structure ? It could be (there
is no use case at the moment) that the pmic can be programmed before the
relocation, when malloc() is not available.

This is the main reason because everything is decided at compile time
with CONFIG_ macros. However, we can also decide if it is *really*
required that PMIC can be accessed before relocation.

> Now at fsl_pmic.c read value is returned by return clause.

Yes, we can change - of course, we need to update in one-shot also the
boards already using a pmic.

>  
> I think, that passing pointer is a better approach,since errors from i2c_read/spi_read can be
> caught in upper layers.

Using a pointer is the common way to implement a read routine, so I
cannot argue. The side effect is that the calling code will be filled
with the same and boring check:

	ret = pmic_read_reg(....);
	if (ret) {
		<...error handling, most case only print>
	}
	ret = pmic_read_reg(....);
	if (ret) {
		<...error handling, most case only print>
	}


> 
> 2. Since I haven't got a chance to test SPI part of the fsl_pmic.c driver, I've focused
> mainly on I2C and place stubs for SPI.

Ok, right - this is a RFC. I will do no not review the details in your
sample code, we are discussing the interface ;-).

For the "real" patch, I think we should merge this new code with
fsl_pmic.c, too.

> 
> 3. Suggestions for struct pmic's additional fields for SPI are more than welcome :-) 

We should have the parameters to setup a SPUI connection:
	- SPI chipselect
	- SPI frequency
	- SPI mode
	- SPI bus


> 
> 4. Now the pmic_core.c file consist of
>    #ifdef PMIC_I2C
>    {Code for handling I2C}
>    #else
>    {Code for handling SPI}
>    #endif
> 
> The same approach is used at fsl_pmic.c   
> 
> I'm wondering if this approach shouldn't be replaced with on-time checking if
> SPI or I2C interface is available.
> This check can be performed by:
> 
> struct pmic *p;
> if (p->interface == PMIC_I2C) {
> 
> } else {
> 
> }
> 
> It would allow to remove obscure #ifdefs, but on the other hand it will reduce 
> execution speed of the driver.

Well, I do not worry about time execution. The time to execute the if
branch is maybe negligible compared to the time to make a I2C/SPI transfer.

However, we increase the footprint, and surely only one mechanism is
required on the same board.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================