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

[Yadwinder Singh Brar]

PLEASE

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 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.
>
>>>>
>>>> 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.
>>>>
>>>
>>>
>>> I think we have same variable num_regulators but use differently. In my
>>> code, it represents number of regulators to be used actually, but in
>>> your code it equals to total number of regulators. Since it has
>>
>> not exactly.
>>
>>> different meaning, it doesn't have to same with MAX77686_REGULATORS.
>>> MAX77686_REGULATORS is macro which indicates total number of regulators
>>> in max77686, and it equals to ARRAY_SIZE(regulators). Even if they are
>>> not same, it's not a bug because we want to register all regulators
>>> whether it will be used or not.
>>>
>>>
>>>> If we consider a case pdata->num_regulators is
>>>> equal to MAX77686_REGULATORS and initdata is
>>>> not their(i.e. NULL) than I think it will initialise
>>>> init_data[pdata->regulators[i].id to NULL, which again will be a bug.
>>>>
>>>>> those state, i think just using temporary array which satisfies
>>>>> regulator's id order is fine while it can't use pdata's initdata directly.
>>>>>
>>>>
>>>> If I am not wrong, I think we can also sort pdata's initdata also using
>>>> kernel's sort api and use one instance of (default)initdata for
>>>> all unused or uninitialized regulators in platform file.
>>>>
>>>
>>>
>>> If init_data references to NULL, it will be ignored while
>>> register_regulators() does initialize. Thus it doesn't make any problem.
>>>
>>> I'm afraid of using Kernel's sort API because of its overhead. Do you
>>
>> I don't think it's overhead will matter more than that of allocating a
>> new array and than
>> sorting it here.
>>
>>> think it will be better to use them? If you mind that init_data has been
>>> dynamic allocated, it can be modified to a static pointer array.
>>>
>>
>> No, their is no problem with dynamic.
>> Anyways, I had just suggested you to use pdata->regulators[i].initdata.
>>
>
>
> 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.
>
>
>> Â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/
>>
>
>
--
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/