Messages in this thread | | | Date | Tue, 26 Jan 2016 14:00:07 +0000 | From | Lee Jones <> | Subject | Re: [PATCH V2 4/4] mfd: mediatek: add MT6323 support to MT6397 driver |
| |
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 <blogic@openwrt.org> > >>>>>>>> --- > >>>>>> > >>>>>> [...] > >>>>>> > >>>>>>>> @@ -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
| |