From: Tomasz Figa <tfiga@chromium.org> To: Sascha Hauer <s.hauer@pengutronix.de> Cc: "Matthias Brugger" <matthias.bgg@gmail.com>, "James Liao" <jamesjj.liao@mediatek.com>, "Mike Turquette" <mturquette@linaro.org>, "YH Chen (陳昱豪)" <yh.chen@mediatek.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "Henry Chen" <henryc.chen@mediatek.com>, "Rob Herring" <robh+dt@kernel.org>, kernel@pengutronix.de, "Yingjoe Chen (陳英洲)" <Yingjoe.Chen@mediatek.com>, "Eddie Huang" <eddie.huang@mediatek.com>, "Lee Jones" <lee.jones@linaro.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH 02/13] clk: mediatek: Add initial common clock support for Mediatek SoCs. Date: Fri, 13 Feb 2015 22:22:02 +0900 [thread overview] Message-ID: <CAAFQd5BxYNjsVpJnS+t=kZdXSdBTpgJii=T_qadRmMEzrR=dng@mail.gmail.com> (raw) In-Reply-To: <20150213120613.GZ12209@pengutronix.de> Hi Sascha, On Fri, Feb 13, 2015 at 9:06 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > Hi Tomasz, > >> > +static void mtk_cg_disable(struct clk_hw *hw) >> > +{ >> > + mtk_cg_set_bit(hw); >> > +} >> > + >> > +static int mtk_cg_enable_inv(struct clk_hw *hw) >> > +{ >> > + mtk_cg_set_bit(hw); >> > + >> > + return 0; >> > +} >> > + >> > +static void mtk_cg_disable_inv(struct clk_hw *hw) >> > +{ >> > + mtk_cg_clr_bit(hw); >> > +} >> >> Instead of duplicating the ops, couldn't you add a flag or something >> to mtk_clk_gate struct and then act appropriately in the ops? Also, >> see below. > > I prefer duplicating the ops. It makes the functions simpler without > ifs. I meant something else. Compared to ifs I'd prefer duplicated ops too. is_enabled() { status = regmap_read() ^ (inverted << shift); return status & BIT(shift); } However I missed the fact that writing uses set and clear registers, which effectively means that this approach can't really be used for writing, so I'm okay with keeping this as is. > >> > diff --git a/drivers/clk/mediatek/clk-gate.h b/drivers/clk/mediatek/clk-gate.h >> > new file mode 100644 >> > index 0000000..a44dcbf >> > --- /dev/null >> > +++ b/drivers/clk/mediatek/clk-gate.h >> > @@ -0,0 +1,49 @@ >> > +/* >> > + * Copyright (c) 2014 MediaTek Inc. >> > + * Author: James Liao <jamesjj.liao@mediatek.com> >> >> Might not be necessary, but maybe the other people (all or some of >> them) from signed-off-by should be added to this and other copyright >> statements? > > I rather do not want to update these copyrights frequently. Otherwise we > would see a lot of patches with an extra hunk changing the copyrights. > I'm glad we left that behind and look at the git history instead. > The above is the original author. I don't want to remove him, but I also > do not want to add every other person who touched that file. Alright. I just wanted to make sure that this is desired state. > > The other stuff will be fixed in the next round. Thanks for reviewing. You're welcome. Looking forward for next revision. Best regards, Tomasz
WARNING: multiple messages have this Message-ID (diff)
From: tfiga@chromium.org (Tomasz Figa) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 02/13] clk: mediatek: Add initial common clock support for Mediatek SoCs. Date: Fri, 13 Feb 2015 22:22:02 +0900 [thread overview] Message-ID: <CAAFQd5BxYNjsVpJnS+t=kZdXSdBTpgJii=T_qadRmMEzrR=dng@mail.gmail.com> (raw) In-Reply-To: <20150213120613.GZ12209@pengutronix.de> Hi Sascha, On Fri, Feb 13, 2015 at 9:06 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > Hi Tomasz, > >> > +static void mtk_cg_disable(struct clk_hw *hw) >> > +{ >> > + mtk_cg_set_bit(hw); >> > +} >> > + >> > +static int mtk_cg_enable_inv(struct clk_hw *hw) >> > +{ >> > + mtk_cg_set_bit(hw); >> > + >> > + return 0; >> > +} >> > + >> > +static void mtk_cg_disable_inv(struct clk_hw *hw) >> > +{ >> > + mtk_cg_clr_bit(hw); >> > +} >> >> Instead of duplicating the ops, couldn't you add a flag or something >> to mtk_clk_gate struct and then act appropriately in the ops? Also, >> see below. > > I prefer duplicating the ops. It makes the functions simpler without > ifs. I meant something else. Compared to ifs I'd prefer duplicated ops too. is_enabled() { status = regmap_read() ^ (inverted << shift); return status & BIT(shift); } However I missed the fact that writing uses set and clear registers, which effectively means that this approach can't really be used for writing, so I'm okay with keeping this as is. > >> > diff --git a/drivers/clk/mediatek/clk-gate.h b/drivers/clk/mediatek/clk-gate.h >> > new file mode 100644 >> > index 0000000..a44dcbf >> > --- /dev/null >> > +++ b/drivers/clk/mediatek/clk-gate.h >> > @@ -0,0 +1,49 @@ >> > +/* >> > + * Copyright (c) 2014 MediaTek Inc. >> > + * Author: James Liao <jamesjj.liao@mediatek.com> >> >> Might not be necessary, but maybe the other people (all or some of >> them) from signed-off-by should be added to this and other copyright >> statements? > > I rather do not want to update these copyrights frequently. Otherwise we > would see a lot of patches with an extra hunk changing the copyrights. > I'm glad we left that behind and look at the git history instead. > The above is the original author. I don't want to remove him, but I also > do not want to add every other person who touched that file. Alright. I just wanted to make sure that this is desired state. > > The other stuff will be fixed in the next round. Thanks for reviewing. You're welcome. Looking forward for next revision. Best regards, Tomasz
next prev parent reply other threads:[~2015-02-13 13:22 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-02-09 10:47 [PATCH v5]: clk: Add common clock support for Mediatek MT8135 and MT8173 Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 01/13] clk: dts: mediatek: add Mediatek MT8135 clock bindings Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 13:35 ` Philipp Zabel 2015-02-09 13:35 ` Philipp Zabel 2015-02-09 10:47 ` [PATCH 02/13] clk: mediatek: Add initial common clock support for Mediatek SoCs Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-13 7:41 ` Tomasz Figa 2015-02-13 7:41 ` Tomasz Figa 2015-02-13 12:06 ` Sascha Hauer 2015-02-13 12:06 ` Sascha Hauer 2015-02-13 13:22 ` Tomasz Figa [this message] 2015-02-13 13:22 ` Tomasz Figa 2015-02-09 10:47 ` [PATCH 03/13] clk: mediatek: Add reset controller support Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 13:35 ` Philipp Zabel 2015-02-09 13:35 ` Philipp Zabel 2015-02-09 10:47 ` [PATCH 04/13] clk: mediatek: Add basic clocks for Mediatek MT8135 Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 05/13] clk: dts: mediatek: add Mediatek MT8173 clock bindings Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 06/13] clk: mediatek: Add basic clocks for Mediatek MT8173 Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-13 9:56 ` Tomasz Figa 2015-02-13 9:56 ` Tomasz Figa 2015-02-19 8:24 ` Sascha Hauer 2015-02-19 8:24 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 07/13] dt: bindings: Add MediaTek MT8135/MT8173 reset controller defines Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 08/13] soc: mediatek: Add PMIC wrapper for MT8135 and MT6397 SoC Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 09/13] ARM: dts: mediatek: Enable clock support for Mediatek MT8135 Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 10:51 ` Russell King - ARM Linux 2015-02-09 10:51 ` Russell King - ARM Linux 2015-02-09 11:25 ` Sascha Hauer 2015-02-09 11:25 ` Sascha Hauer 2015-02-09 11:27 ` Russell King - ARM Linux 2015-02-09 11:27 ` Russell King - ARM Linux 2015-02-09 11:44 ` Sascha Hauer 2015-02-09 11:44 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 10/13] ARM: dts: mt8135: Add pmic wrapper nodes Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 11/13] ARM: dts: mt8135-evbp1: Add PMIC support Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 12/13] mfd: dt-bindings: Add bindings for the MediaTek MT6397 PMIC Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-09 10:47 ` [PATCH 13/13] mfd: Add support " Sascha Hauer 2015-02-09 10:47 ` Sascha Hauer 2015-02-16 9:56 ` Lee Jones 2015-02-16 9:56 ` Lee Jones [not found] ` <20150218181904.421.59675@quantum> [not found] ` <20150219082655.GV12209@pengutronix.de> [not found] ` <20150219084349.GA12212@x1> [not found] ` <20150219120409.GW12209@pengutronix.de> [not found] ` <20150219121304.GH12212@x1> 2015-02-19 21:41 ` Mike Turquette 2015-02-19 21:41 ` Mike Turquette
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAAFQd5BxYNjsVpJnS+t=kZdXSdBTpgJii=T_qadRmMEzrR=dng@mail.gmail.com' \ --to=tfiga@chromium.org \ --cc=Yingjoe.Chen@mediatek.com \ --cc=eddie.huang@mediatek.com \ --cc=henryc.chen@mediatek.com \ --cc=jamesjj.liao@mediatek.com \ --cc=kernel@pengutronix.de \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=matthias.bgg@gmail.com \ --cc=mturquette@linaro.org \ --cc=robh+dt@kernel.org \ --cc=s.hauer@pengutronix.de \ --cc=yh.chen@mediatek.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.