Hi Stefano,
Thanks for the reply.
> >
> > 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.
> 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.
> > 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.
As fair as I looked into the code, only iMX power_init methods need
to be fixed.
> > 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 :-).
> > 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).
> >
> > 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.
> > 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.
>
So I will try to use switch-case construct.
switch (p->interface) {
case PMIC_I2C:
case PMIC_SPI:
}
> Best regards,
> Stefano Babic
>
--
Best regards,
Lukasz Majewski
Samsung Poland R&D Center
Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
