From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965911AbcAZOAP (ORCPT ); Tue, 26 Jan 2016 09:00:15 -0500 Received: from mail-wm0-f41.google.com ([74.125.82.41]:36887 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965102AbcAZOAM (ORCPT ); Tue, 26 Jan 2016 09:00:12 -0500 Date: Tue, 26 Jan 2016 14:00:07 +0000 From: Lee Jones To: John Crispin Cc: Henry Chen , Steven Liu , Sascha Hauer , linux-kernel@vger.kernel.org, Flora Fu , linux-mediatek@lists.infradead.org, Matthias Brugger , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH V2 4/4] mfd: mediatek: add MT6323 support to MT6397 driver Message-ID: <20160126140007.GZ3368@x1> References: <1453716887-38442-1-git-send-email-blogic@openwrt.org> <20160125124112.GG3368@x1> <56A64108.7020807@openwrt.org> <3476571.0yU9yvPsKF@linux-gy6r.site> <56A670A3.5030100@openwrt.org> <1453777643.26374.11.camel@mtksdaap41> <56A71CE5.4080201@openwrt.org> <20160126083433.GX3368@x1> <56A76100.1020307@openwrt.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <56A76100.1020307@openwrt.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 26 Jan 2016, John Crispin wrote: > > > On 26/01/2016 09:34, Lee Jones wrote: > > On Tue, 26 Jan 2016, John Crispin wrote: > >> On 26/01/2016 04:07, Henry Chen wrote: > >>> On Mon, 2016-01-25 at 19:59 +0100, John Crispin wrote: > >>>> > >>>> On 25/01/2016 19:44, Matthias Brugger wrote: > >>>>> On Monday 25 Jan 2016 16:36:40 John Crispin wrote: > >>>>>> Hi, > >>>>>> > >>>>>> On 25/01/2016 13:41, Lee Jones wrote: > >>>>>>> Please honour the subject format of the subsystem you are contributing > >>>>>>> to. > >>>>>>> > >>>>>>> `git log --oneline -- $subsystem` gives you this. > >>>>>>> > >>>>>>> On Mon, 25 Jan 2016, John Crispin wrote: > >>>>>>>> Signed-off-by: John Crispin > >>>>>>>> --- > >>>>>> > >>>>>> [...] > >>>>>> > >>>>>>>> @@ -261,6 +271,15 @@ static int mt6397_probe(struct platform_device > >>>>>>>> *pdev) > >>>>>>>> > >>>>>>>> } > >>>>>>>> > >>>>>>>> switch (id & 0xff) { > >>>>>>>> > >>>>>>>> + case MT6323_CID_CODE: > >>>>>>>> + mt6397->int_con[0] = MT6323_INT_CON0; > >>>>>>> > >>>>>>> This is confusing. You're still using memory allocated for a mt6397 > >>>>>>> device. > >>>>>> > >>>>>> the variable is currently defined as struct mt6397_chip *mt6397; > >>>>>> shall i only change the name or also create a patch to rename the struct ? > >>>>>> > >>>>> > >>>>> I think we should rename the struct and the file as well. > >>>>> > >>>>> Cheers, > >>>>> Matthias > >>>> > >>>> Hi, > >>>> > >>>> that would have been my next question. renaming the struct would imply > >>>> renaming the driver and the whole namespace contained within. We would > >>>> then also need to change the Kconfig and Makefile. I am happy to do this > >>>> but want to be sure that is is actually wanted. > >>>> > >>>> John > >>> Hi, > >>> > >>> Since mt6323 was similar with mt6397, I think we can reuse the > >>> mt6397_chip without duplicate code. > >>> > >>> Maybe we can rename the local variable name to avoid confusing. > >>> > >>> struct mt6397_chip *mt_pmic; > >>> ... > >>> ... > >>> switch (id & 0xff) { > >>> case MT6323_CID_CODE: > >>> mt_pmic->int_con[0] = MT6323_INT_CON0; > >>> mt_pmic->int_con[1] = MT6323_INT_CON1; > >>> ... > >>> ... > >>> > >>> Henry > >> > >> Hi, > >> > >> IMHO we should either rename the namespace or not. renaming some > >> variables seems weird as that will just move the confusion/inconsistency > >> to another place in the code. I am however rather indifferent on this > >> matter. > > > > It's common to name a driver after the device which was enabled first, > > so no need to rename the files or CONFIGs; however, it does seem > > prudent to generify the struct (both parts). > > > > Hi, > > renaming struct mt6397_chip is not as straightforward as it seems as > other drivers such as pinctrl and rtc share the data structure with the > mfd driver. > > i have kept the structs name as is for now and only renamed the instance. That's fine. > If you want i can create a separate series that addresses only the > renaming of the structure. The "rename struct mt6397" patch would need > to do a change to drivers from 3 subsystems in 1 single commit as not to > break compile and merge order. I'll leave this up to you. The real problem from my PoV was the instance name. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog