RFC Generic PMIC driver
Get series with b4
b4 am [email protected]Versions newest first
- v1 RFC Latest0 of 2 patches 0 repliesNew
Patches in v1 No applies recorded yet
None of the 2 patches the cover letter announces are in this archive.
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [U-Boot] [RFC 0/2] Generic PMIC driver
Hi Stefano,
Thanks for the reply.
Quoted 9 lines
Quoted 5 lines
In my opinion following issues needs discussion:
1. In proposed
int 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.
For the first version of the pmic_core.c driver we might stick to this proposition.
Quoted 3 lines
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.
Quoted 4 lines
Quoted 1 line
Now at
fsl_pmic.cread 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.
As fair as I looked into the code, only iMX power_init methods need to be fixed.
Quoted 7 lines
Quoted 2 lines
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:
Quoted 9 lines
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 :-).
Quoted 9 lines
Quoted 2 lines
2. Since I haven't got a chance to test SPI part of the
fsl_pmic.cdriver, 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).
Quoted 10 lines
Quoted 3 lines
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
OK.
Quoted 23 lines
Quoted 16 lines
4. Now the
pmic_core.cfile consist of#ifdef PMIC_I2C {Code for handling I2C} #else {Code for handling SPI} #endifThe same approach is used at
fsl_pmic.cI'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) {Quoted 6 lines
} else {
}
It would allow to remove obscure #ifdefs, but on the other hand it will reduce execution speed of the driver.
Quoted 7 lines
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:
}
Quoted 3 lines
Best regards,
Stefano Babic
Signature · 6 lines
Best regards, Lukasz Majewski Samsung Poland R&D Center Platform Group