Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752603AbdFVGMw (ORCPT ); Thu, 22 Jun 2017 02:12:52 -0400 Received: from nbd.name ([46.4.11.11]:60373 "EHLO nbd.name" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750924AbdFVGMv (ORCPT ); Thu, 22 Jun 2017 02:12:51 -0400 Subject: Re: [PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support To: Zhi Mao References: <1498032672-7172-1-git-send-email-zhi.mao@mediatek.com> <1498032672-7172-5-git-send-email-zhi.mao@mediatek.com> <4600cf23-5e59-df02-37dc-056cb4a9744c@phrozen.org> <1498111776.18841.10.camel@mhfsdcap03> Cc: Thierry Reding , Rob Herring , Mark Rutland , Matthias Brugger , "linux-pwm@vger.kernel.org" , =?UTF-8?B?WmhlbmJhbyBMaXUgKOWImOaMr+WunSk=?= , "devicetree@vger.kernel.org" , srv_heupstream , =?UTF-8?B?U2VhbiBXYW5nICjnjovlv5fkupgp?= , "linux-kernel@vger.kernel.org" , "linux-mediatek@lists.infradead.org" , =?UTF-8?B?WVQgU2hlbiAo5rKI5bKz6ZyGKQ==?= , =?UTF-8?B?WWluZ2pvZSBDaGVuICg/P+iLsea0sik=?= , "linux-arm-kernel@lists.infradead.org" From: John Crispin Message-ID: <05b53007-44de-a236-f592-aea8e75ced44@phrozen.org> Date: Thu, 22 Jun 2017 08:12:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.6.0 MIME-Version: 1.0 In-Reply-To: <1498111776.18841.10.camel@mhfsdcap03> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2955 Lines: 99 On 22/06/17 08:09, Zhi Mao wrote: > Hi John, > > Thanks for your review the code and feedback. > There are 3 issues in this patch: > 1.adds PWM_CLK_DIV_MAX which really should go into its own patch > 2.adds mtk_pwm_com_reg which should also go into its own patch > 3.remove comments inline /*===*/ > > for #1 and #3, I will modify them in the next release. > but for #2, I want to discuss with you, > adding the structure "mtk_pwm_com_reg" is only for the registers of > MT2712 PWM8, > so we want to keep this modification in this patch. > > what's your opinion about it? > Any reply is welcome. > > > Regards > Zhi > > > > Hi Zhi, I just had another look and noticed that the CON registers are not at a fixed offset of 0x40 for the new pwm8 register so having 2) inside this patch makes sense. please explain in the description that this is the case John > > On Wed, 2017-06-21 at 20:22 +0800, John Crispin wrote: >> On 21/06/17 10:11, Zhi Mao wrote: >> > support multiple chip(MT2712, MT7622, MT7623) >> This patch does more than add extra SoC support. It also >> * adds PWM_CLK_DIV_MAX which really should go into its own patch >> * adds mtk_pwm_com_reg which should also go into its own patch >> >> more comments inline >> >> > >> > Signed-off-by: Zhi Mao > >> > --- >> > drivers/pwm/pwm-mediatek.c | 63 +++++++++++++++++++++++++++++++++++--------- >> > 1 file changed, 51 insertions(+), 12 deletions(-) >> > >> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c >> > index c803ff6..d520356 100644 >> > --- a/drivers/pwm/pwm-mediatek.c >> > +++ b/drivers/pwm/pwm-mediatek.c >> > @@ -16,6 +16,7 @@ >> > #include >> > #include >> > #include >> > +#include >> > #include >> > #include >> > #include >> [...] >> > @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev) >> > return pwmchip_remove(&pc->chip); >> > } >> > >> > +/*==========================================*/ >> >> please remove these comment lines >> >> John >> > +static const struct mtk_com_pwm_data mt2712_pwm_data = { >> > + .pwm_nums = 8, >> > +}; >> > + >> > +static const struct mtk_com_pwm_data mt7622_pwm_data = { >> > + .pwm_nums = 6, >> > +}; >> > + >> > +static const struct mtk_com_pwm_data mt7623_pwm_data = { >> > + .pwm_nums = 5, >> > +}; >> > +/*==========================================*/ >> > + >> > static const struct of_device_id mtk_pwm_of_match[] = { >> > - { .compatible = "mediatek,mt7623-pwm" }, >> > - { } >> > + {.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data}, >> > + {.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data}, >> > + {.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data}, >> > + {}, >> > }; >> > MODULE_DEVICE_TABLE(of, mtk_pwm_of_match); >> > >> >