Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757987Ab2EaNBH (ORCPT ); Thu, 31 May 2012 09:01:07 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:40792 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757119Ab2EaNBF convert rfc822-to-8bit (ORCPT ); Thu, 31 May 2012 09:01:05 -0400 MIME-Version: 1.0 In-Reply-To: References: <1337919230-8296-1-git-send-email-jonghwa3.lee@samsung.com> <4FC43018.9040702@samsung.com> <4FC5F866.6030708@samsung.com> <4FC71617.7070801@samsung.com> Date: Thu, 31 May 2012 18:31:04 +0530 Message-ID: Subject: Re: [PATCH v4] regulator: MAX77686: Add Maxim 77686 regulator driver From: Yadwinder Singh Brar To: jonghwa3.lee@samsung.com Cc: linux-kernel@vger.kernel.org, Liam Girdwood , Mark Brown , Chiwoong Byun , Myungjoo Ham , Kyungmin Park Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3870 Lines: 102 PLEASE ignore previous incomplete mails. Sorry for noise by mistake. On Thu, May 31, 2012 at 6:16 PM, Yadwinder Singh Brar wrote: > On Thu, May 31, 2012 at 12:26 PM,   wrote: >> On 2012년 05월 30일 21:08, Yadwinder Singh Brar wrote: >> >>> Hi Jonghwa, >>> >>> On Wed, May 30, 2012 at 4:07 PM,   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(®ulators[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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/