[U-Boot] [PATCH V7 2/3] PMIC: Add dialog pmic support

[Stefano Babic]

On 05/13/2011 05:08 AM, Jason Liu wrote:
> Hi, Stefano,

Hi Jason,

>> This is not what I meant. You have duplicated the code, instead of merge
>> the two functions together. And I think the switch is wrong.
>> This file provides a general access to PMIc using SPI/I2C. There should
>> not be #ifdef related to a single PMIC. Instead of that, You need
>> additional CONFIG_ to select how to access the PMIC (8 bit or 32 bit).
> 
> Then can we have the CONFIG_SYS_DIALOG_PMIC_I2C_ADDR or
> CONFIG_SYS_FSL_PMIC_I2C_ADDR in this file?
> 
> Please tell clear about your mean, don't let me guess what you mean?

I thought it was already clear, but probably not enough. We do not need
special CONFIG_ to select the same attribute for different PMICs. What
is the functional difference between CONFIG_SYS_DIALOG_PMIC_I2C_ADDR and
CONFIG_SYS_FSL_PMIC_I2C_ADDR ? They set exactly the same thing. If we
add several other PMIC, should each of them have a separate CONFIG_ for
the I2C address ? Surely not.

We need only one of them. It seems to me you get confused due to the
original name, that contains the FSL_ string. But think at what this
driver provides: an API to access the PMICs using SPI or I2C, unrelated
to the specific chosen chip.

By the way, you can still use the old name CONFIG_SYS_FSL_PMIC_I2C_ADDR
for your patch. I understand that changing names here means also to
change other boards, that are not related to your patch.

I will then send a patch to reorganize the names after integrating your
patch in the tree.

> 
>>
>> IMHO we can rid of the check_param() function, as this is a constraint
>> to have an implementation independent from a single PMIC.
> 
> If you think we don't need check_param, then It's ok. I can remove it.
> Since this function is added by you before.

The function seemed ok when I write the first version, but it seems
overkilling now.

> 
>>
>> I would prefer something like this:
>>
>> u32 pmic_reg(u32 reg, u32 val, u32 write) {
>>
>>        ........
>> #ifdef CONFIG_SYS_PMIC_8BIT
>>
>>        <read / write 8 bit register>
>> #else
>>
>>        <actual implementation for fsl PMICs>
>> #endif
> 
> Then what you prefer is function pointer or something else?
> Please tell clear, otherwise, it will cost another time to do it.

To be more clearer: this driver should not have inside different
implementations based on a chip type, but based on the specific
*features* or options that are needed to implement. As you see in my
example, it should be possible to access to a PMIC with 8bit with your
implementation also with other PMICs. If we have to add several other
chips, the code (that is real simple) becomes easy unreadable.

As I said, do not change the CONFIG_ names in your patch, as this can
create some confusion, as I see. I will do it later.

>>> +#if defined(CONFIG_FSL_PMIC_I2C) || defined(CONFIG_DIALOG_PMIC_I2C)
>>
>> Same comments apply here. We should select only between I2C and SPI, not
>> the chip.
>>
>>> +#if defined(CONFIG_FSL_PMIC)
>>> +#define PMIC_NAME "Freescale PMIC (Atlas)"
>>> +#elif defined(CONFIG_DIALOG_PMIC)
>>> +#define PMIC_NAME "Dialog PMIC (DA905x)"
>>> +#else
>>> +#error "Unkown PMIC name"
>>> +#endif
>>
>> Instead of that, we can set a general name or put the PMIC_NAME inside
>> the specific PMIC header file.
> 
> Then what general name you think it's good?

It should be unrelated from the specific chip (if you chose this
solution), such as "SPI/I2C PMIC", or dropped from this file and put
into the PMIC header file if you select solution 2).

> 
> I have seen the painful for restructure with the second time, another
> is the make imximage.

If the code in a review is considered wrong, it must be changed. I
cannot avoid it.

> Can we avoid it in the future?

Check in the archive, and you see that changes were requested by several
reviewers an issue was discovered. And for the LOCO the patches were
removed after beeing accepted due to issues found later in the code,
that are considered unaccetable for mainline. I cannot foresee how many
reviews one patch requires, and how many issues a single patch raises.

On the other side, feel free to ask if something is not enough clear
*before* implementing. On my site, I will do my best to explain my
position, as all other reviewers in the list.

> 
> And BTW, do you have any comments about the 1/3 clock patch? If you
> have, please tell it
> as early as possible and I don't want to let the patch version goes up
> very bigger and the time endless.

The patch adds several functions that are strictly related to the
processor and I am checking with the reference manuals to understand
them. I need more time for it.

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