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

[Stefano Babic <sbabic@denx.de>]

On 09/20/2011 02:38 PM, Lukasz Majewski wrote:

>> 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.
> 
> In my opinion on the beginning we should focus on basic scenarios. 
> Therefore the pmic struct is defined static at pmic_core.c as
> follows:
> static struct pmic pmic;
> 
> It is simple and efficient (since it's scope is limited to the
> translation unit). We shall NOT use malloc() allocation for the first
> version of the driver.

Agree, we must not use malloc.

> 
>>> 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.
> 
> Some work for the iMX target boards (e.g. MX51evk) need to be done to
> comply with new framework.

Yes, of course - but if we improve our code, this is not a drawback ;-)

Do not miss the RTC inside the PMIC (drivers/rtc/mc13783-rtc.c)

> As fair as I looked into the code, only iMX power_init methods need
> to be fixed. 

This depends on the board code. There are 6 boards using the PMIC, plus
the RTC driver. For this reason and to avoid to change all boards, maybe
it is not bad to have a compatibility way.

> 
>>> 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>
>> 	}
>>
> I think, that error checking is always welcome :-).

Nothing against checking. We can also

> 
>>> 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.
>>
> In my opinion we shall reuse fsl_pmic.c code as much as possible 
> (especially the SPI code - since I cannot test it). 

Agree.

>> 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.
>>
> So I will try to use switch-case construct.
> 
> switch (p->interface) {
> 	case PMIC_I2C:
> 
> 	case PMIC_SPI:
> 
> }

There is probably a drawback, that is you must link code you do not need
(for example, a board has no I2C at all, but we call and link i2c_write
or another i2 function. The same if a board has only SPI. But I wait for
your proposal !

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
=====================================================================