Re: [PATCH v4] regulator: MAX77686: Add Maxim 77686 regulator driver

[Yadwinder Singh Brar]

PLEASE ignore previous incomplete mails. Sorry for noise by mistake.

On Thu, May 31, 2012 at 6:16 PM, Yadwinder Singh Brar
<yadi.brar01@xxxxxxxxx> wrote:
> On Thu, May 31, 2012 at 12:26 PM, Â<jonghwa3.lee@xxxxxxxxxxx> wrote:
>> On 2012ë 05ì 30ì 21:08, Yadwinder Singh Brar wrote:
>>
>>> Hi Jonghwa,
>>>
>>> On Wed, May 30, 2012 at 4:07 PM, Â<jonghwa3.lee@xxxxxxxxxxx> wrote:
>>>> Hi Yadwinder,
>>>>
>>>> I'm sorry for late reply. I understand the problem you pointed out, but
>>>> i don't agree with you all.
>>>
>>> Sorry,I think you didn't get my points. Lets forget my code and talk
>>> about this code now.
>>>
>>>>>>>> +
>>>>>>>> + Â Â Â for (i = 0; i < MAX77686_REGULATORS; i++) {
>>>>>>>> + Â Â Â Â Â Â Â if (pdata)
>>>>>>>> + Â Â Â Â Â Â Â Â Â Â Â init_data[pdata->regulators[i].id] =
>>>>>>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âpdata->regulators[i].initdata;
>>>
>>> In case we have a list of 5 regulators only in pdata, than what will
>>> happen here when i > 5 ???
>>>
>>
>>
>> You're right, it has bug. How do you think that change the condition to
>> (pdata && i < pdata->num_regulators)?
>>
>
 I think this stuff is not at right place, It should be moved out of this loop
 to solve this both problems.
 Please try to do all this stuff related to platform file at starting while
 checking if(pdata) {...}
>
>>>>>>>
>>>>>>> I Âthink we can directly use Âpdata->regulators[i].initdata instead of
>>>>>>> init_data[i].
>>>>>>> In case if pdata is not their we can use same instance of
>>>>>>> init_data(default) Âfor all regulators.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> This if for some situation that pdata's initdata doensn't line up. When
>>>
>>>>>>>> + Â Â Â Â Â Â Â config.init_data = init_data[i];
>>>>>>>> + Â Â Â Â Â Â Â rdev[i] = regulator_register(&regulators[i], &config);
>>>
>>> In case pdata->regulators[0] is not the first regulator(i.e id > 0), then
>>> will it get proper initdata for regulators[0] before registering ????
>>>
>>
>>
>> Yes, because above code replaces pdata->regalator's initdata to proper
>> position of initdata array referencing regulator's id.
>>
>
> No, sorry i think you again missed the point.
> Anyways, moving above stuff out of this loop
> will also sove this problem.
>
>>>>>
>>>>> Ok, but I think this not right place for sorting (sorting is not taking
>>>>> place.) You have to sort it before entering in loop for registering
>>>>> regulators.
>>>>>
>>>>>> user sets only initdata considered it being used, there may be
>>>>>> regulators not having initdata, also its order is not clear. So for
>>>>>
>>>>> Ok, I think this is a bug in present driver also, because
>>>>> without checking pdata->num_regulators, you are running in
>>>>> loop Âfor (i = 0; i < MAX77686_REGULATORS; i++)
>>>>> where MAX77686_REGULATORS should be equal to
>>>>> pdata->num_regulators for this driver to work fine.
>>>>>

>> So, to sum up to this, you think it is better to sort pdata->regulators
>> by its id before entering loop and just use pdata->regulators directly,
>> right? Okay, I'll do modify it.
>>
>
 Yes, this will allow us to add device tree support without any
 modifications to this code in future and keeping our code simple
 and compact.

 In my opinion since we are unconditionally registering all the regulators
to keep our code simple, as mark said, we should also populate also
all regulators in pdata list in platform file in sorted order to keep our code
simple here.
So that here we need to verify only if (pdata->num_regulators ==
MAX77686_REGULATORS).

>>> ÂRegards,
>>> ÂYadwinder.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/