From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lukasz Majewski <l.majewski@samsung.com>
Date: Tue, 20 Sep 2011 14:38:07 +0200
Subject: [U-Boot] [RFC 0/2] Generic PMIC driver
In-Reply-To: <4E784D9C.5090708@denx.de>
References: <1314868412-2949-1-git-send-email-l.majewski@samsung.com>
	<1316444766-20550-1-git-send-email-l.majewski@samsung.com>
	<4E784D9C.5090708@denx.de>
Message-ID: <20110920143807.590fc691@lmajewski.digital.local>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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