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

[jonghwa3 . lee]

Hi Yadwinder,

I'm sorry for late reply. I understand the problem you pointed out, but
i don't agree with you all.

On 2012ë 05ì 29ì 13:20, Yadwinder Singh Brar wrote:

> On Tue, May 29, 2012 at 7:40 AM,  <jonghwa3.lee@xxxxxxxxxxx> wrote:
>  Hi Jonghwa,
> 
>>>> +
>>>> +       for (i = 0; i < MAX77686_REGULATORS; i++) {
>>>> +               if (pdata)
>>>> +                       init_data[pdata->regulators[i].id] =
>>>> +                                                pdata->regulators[i].initdata;
>>>
>>> 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
> 
> 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
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
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.

>>>> +

>>>> +               config.init_data = init_data[i];
>>>> +               rdev[i] = regulator_register(&regulators[i], &config);
>>>> +
>>>
> 
> 
> 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/
> 


Thanks,
--
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/