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: off...@denx.de ===================================================================== _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot