From mboxrd@z Thu Jan 1 00:00:00 1970 From: maxime.ripard@free-electrons.com (Maxime Ripard) Date: Fri, 21 Nov 2014 12:15:03 +0100 Subject: [PATCH 1/9] clk: sunxi: Give sunxi_factors_register a registers parameter In-Reply-To: <546EFB83.1020806@redhat.com> References: <1416498928-1300-1-git-send-email-hdegoede@redhat.com> <1416498928-1300-2-git-send-email-hdegoede@redhat.com> <20141121083555.GK24143@lukather> <546EFB83.1020806@redhat.com> Message-ID: <20141121111503.GC4752@lukather> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Nov 21, 2014 at 09:44:51AM +0100, Hans de Goede wrote: > Hi, > > On 11/21/2014 09:35 AM, Maxime Ripard wrote: > > Hi Hans, > > > > On Thu, Nov 20, 2014 at 04:55:20PM +0100, Hans de Goede wrote: > >> Before this commit sunxi_factors_register uses of_iomap(node, 0) to get > >> the clk registers. The sun6i prcm has factor clocks, for which we want to > >> use sunxi_factors_register, but of_iomap(node, 0) does not work for the prcm > >> factor clocks, because the prcm uses the mfd framework, so the registers > >> are not part of the dt-node, instead they are added to the platform_device, > >> as platform_device resources. > >> > >> This commit makes getting the registers the callers duty, so that > >> sunxi_factors_register can be used with mfd instantiated platform device too. > >> > >> Signed-off-by: Hans de Goede > > > > Funny, I was thinking of doing exactly the same thing for MMC clocks :) > > > >> --- > >> drivers/clk/sunxi/clk-factors.c | 10 ++++------ > >> drivers/clk/sunxi/clk-factors.h | 7 ++++--- > >> drivers/clk/sunxi/clk-mod0.c | 6 ++++-- > >> drivers/clk/sunxi/clk-sun8i-mbus.c | 2 +- > >> drivers/clk/sunxi/clk-sunxi.c | 3 ++- > >> 5 files changed, 15 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c > >> index f83ba09..fc4f4b5 100644 > >> --- a/drivers/clk/sunxi/clk-factors.c > >> +++ b/drivers/clk/sunxi/clk-factors.c > >> @@ -156,9 +156,10 @@ static const struct clk_ops clk_factors_ops = { > >> .set_rate = clk_factors_set_rate, > >> }; > >> > >> -struct clk * __init sunxi_factors_register(struct device_node *node, > >> - const struct factors_data *data, > >> - spinlock_t *lock) > >> +struct clk *sunxi_factors_register(struct device_node *node, > >> + const struct factors_data *data, > >> + spinlock_t *lock, > >> + void __iomem *reg) > >> { > >> struct clk *clk; > >> struct clk_factors *factors; > >> @@ -168,11 +169,8 @@ struct clk * __init sunxi_factors_register(struct device_node *node, > >> struct clk_hw *mux_hw = NULL; > >> const char *clk_name = node->name; > >> const char *parents[FACTORS_MAX_PARENTS]; > >> - void __iomem *reg; > >> int i = 0; > >> > >> - reg = of_iomap(node, 0); > >> - > >> /* if we have a mux, we will have >1 parents */ > >> while (i < FACTORS_MAX_PARENTS && > >> (parents[i] = of_clk_get_parent_name(node, i)) != NULL) > >> diff --git a/drivers/clk/sunxi/clk-factors.h b/drivers/clk/sunxi/clk-factors.h > >> index 9913840..1f5526d 100644 > >> --- a/drivers/clk/sunxi/clk-factors.h > >> +++ b/drivers/clk/sunxi/clk-factors.h > >> @@ -37,8 +37,9 @@ struct clk_factors { > >> spinlock_t *lock; > >> }; > >> > >> -struct clk * __init sunxi_factors_register(struct device_node *node, > >> - const struct factors_data *data, > >> - spinlock_t *lock); > >> +struct clk *sunxi_factors_register(struct device_node *node, > >> + const struct factors_data *data, > >> + spinlock_t *lock, > >> + void __iomem *reg); > > > > Why are you dropping the __init there? > > Because it is going to be used from mfd instantiation, so from a platform_dev > probe function which is not __init. Ah right. Mentionning it in the commit log would be nice. > > > > >> > >> #endif > >> diff --git a/drivers/clk/sunxi/clk-mod0.c b/drivers/clk/sunxi/clk-mod0.c > >> index 4a56385..9530833 100644 > >> --- a/drivers/clk/sunxi/clk-mod0.c > >> +++ b/drivers/clk/sunxi/clk-mod0.c > >> @@ -78,7 +78,8 @@ static DEFINE_SPINLOCK(sun4i_a10_mod0_lock); > >> > >> static void __init sun4i_a10_mod0_setup(struct device_node *node) > >> { > >> - sunxi_factors_register(node, &sun4i_a10_mod0_data, &sun4i_a10_mod0_lock); > >> + sunxi_factors_register(node, &sun4i_a10_mod0_data, > >> + &sun4i_a10_mod0_lock, of_iomap(node, 0)); > > > > As of_iomap can fail, I'd rather check the returned value before > > calling sunxi_factors_register. > > > > I know it wasn't done before, but it's the right thing to do, as it > > would lead to an instant crash if that fails. > > Ok, I'll wait for you to review the rest of the series and then do a v2 of the > patch-set with this fixed (as time permits). Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: [PATCH 1/9] clk: sunxi: Give sunxi_factors_register a registers parameter Date: Fri, 21 Nov 2014 12:15:03 +0100 Message-ID: <20141121111503.GC4752@lukather> References: <1416498928-1300-1-git-send-email-hdegoede@redhat.com> <1416498928-1300-2-git-send-email-hdegoede@redhat.com> <20141121083555.GK24143@lukather> <546EFB83.1020806@redhat.com> Reply-To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="/e2eDi0V/xtL+Mc8" Return-path: Content-Disposition: inline In-Reply-To: <546EFB83.1020806-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Hans de Goede Cc: Emilio Lopez , Mike Turquette , Linux Media Mailing List , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Id: devicetree@vger.kernel.org --/e2eDi0V/xtL+Mc8 Content-Type: text/plain; charset=UTF-8 Content-Disposition: inline On Fri, Nov 21, 2014 at 09:44:51AM +0100, Hans de Goede wrote: > Hi, > > On 11/21/2014 09:35 AM, Maxime Ripard wrote: > > Hi Hans, > > > > On Thu, Nov 20, 2014 at 04:55:20PM +0100, Hans de Goede wrote: > >> Before this commit sunxi_factors_register uses of_iomap(node, 0) to get > >> the clk registers. The sun6i prcm has factor clocks, for which we want to > >> use sunxi_factors_register, but of_iomap(node, 0) does not work for the prcm > >> factor clocks, because the prcm uses the mfd framework, so the registers > >> are not part of the dt-node, instead they are added to the platform_device, > >> as platform_device resources. > >> > >> This commit makes getting the registers the callers duty, so that > >> sunxi_factors_register can be used with mfd instantiated platform device too. > >> > >> Signed-off-by: Hans de Goede > > > > Funny, I was thinking of doing exactly the same thing for MMC clocks :) > > > >> --- > >> drivers/clk/sunxi/clk-factors.c | 10 ++++------ > >> drivers/clk/sunxi/clk-factors.h | 7 ++++--- > >> drivers/clk/sunxi/clk-mod0.c | 6 ++++-- > >> drivers/clk/sunxi/clk-sun8i-mbus.c | 2 +- > >> drivers/clk/sunxi/clk-sunxi.c | 3 ++- > >> 5 files changed, 15 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c > >> index f83ba09..fc4f4b5 100644 > >> --- a/drivers/clk/sunxi/clk-factors.c > >> +++ b/drivers/clk/sunxi/clk-factors.c > >> @@ -156,9 +156,10 @@ static const struct clk_ops clk_factors_ops = { > >> .set_rate = clk_factors_set_rate, > >> }; > >> > >> -struct clk * __init sunxi_factors_register(struct device_node *node, > >> - const struct factors_data *data, > >> - spinlock_t *lock) > >> +struct clk *sunxi_factors_register(struct device_node *node, > >> + const struct factors_data *data, > >> + spinlock_t *lock, > >> + void __iomem *reg) > >> { > >> struct clk *clk; > >> struct clk_factors *factors; > >> @@ -168,11 +169,8 @@ struct clk * __init sunxi_factors_register(struct device_node *node, > >> struct clk_hw *mux_hw = NULL; > >> const char *clk_name = node->name; > >> const char *parents[FACTORS_MAX_PARENTS]; > >> - void __iomem *reg; > >> int i = 0; > >> > >> - reg = of_iomap(node, 0); > >> - > >> /* if we have a mux, we will have >1 parents */ > >> while (i < FACTORS_MAX_PARENTS && > >> (parents[i] = of_clk_get_parent_name(node, i)) != NULL) > >> diff --git a/drivers/clk/sunxi/clk-factors.h b/drivers/clk/sunxi/clk-factors.h > >> index 9913840..1f5526d 100644 > >> --- a/drivers/clk/sunxi/clk-factors.h > >> +++ b/drivers/clk/sunxi/clk-factors.h > >> @@ -37,8 +37,9 @@ struct clk_factors { > >> spinlock_t *lock; > >> }; > >> > >> -struct clk * __init sunxi_factors_register(struct device_node *node, > >> - const struct factors_data *data, > >> - spinlock_t *lock); > >> +struct clk *sunxi_factors_register(struct device_node *node, > >> + const struct factors_data *data, > >> + spinlock_t *lock, > >> + void __iomem *reg); > > > > Why are you dropping the __init there? > > Because it is going to be used from mfd instantiation, so from a platform_dev > probe function which is not __init. Ah right. Mentionning it in the commit log would be nice. > > > > >> > >> #endif > >> diff --git a/drivers/clk/sunxi/clk-mod0.c b/drivers/clk/sunxi/clk-mod0.c > >> index 4a56385..9530833 100644 > >> --- a/drivers/clk/sunxi/clk-mod0.c > >> +++ b/drivers/clk/sunxi/clk-mod0.c > >> @@ -78,7 +78,8 @@ static DEFINE_SPINLOCK(sun4i_a10_mod0_lock); > >> > >> static void __init sun4i_a10_mod0_setup(struct device_node *node) > >> { > >> - sunxi_factors_register(node, &sun4i_a10_mod0_data, &sun4i_a10_mod0_lock); > >> + sunxi_factors_register(node, &sun4i_a10_mod0_data, > >> + &sun4i_a10_mod0_lock, of_iomap(node, 0)); > > > > As of_iomap can fail, I'd rather check the returned value before > > calling sunxi_factors_register. > > > > I know it wasn't done before, but it's the right thing to do, as it > > would lead to an instant crash if that fails. > > Ok, I'll wait for you to review the rest of the series and then do a v2 of the > patch-set with this fixed (as time permits). Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --/e2eDi0V/xtL+Mc8-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from down.free-electrons.com ([37.187.137.238]:54085 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750734AbaKULUO (ORCPT ); Fri, 21 Nov 2014 06:20:14 -0500 Date: Fri, 21 Nov 2014 12:15:03 +0100 From: Maxime Ripard To: Hans de Goede Cc: Emilio Lopez , Mike Turquette , Linux Media Mailing List , linux-arm-kernel@lists.infradead.org, devicetree , linux-sunxi@googlegroups.com Subject: Re: [PATCH 1/9] clk: sunxi: Give sunxi_factors_register a registers parameter Message-ID: <20141121111503.GC4752@lukather> References: <1416498928-1300-1-git-send-email-hdegoede@redhat.com> <1416498928-1300-2-git-send-email-hdegoede@redhat.com> <20141121083555.GK24143@lukather> <546EFB83.1020806@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="/e2eDi0V/xtL+Mc8" Content-Disposition: inline In-Reply-To: <546EFB83.1020806@redhat.com> Sender: linux-media-owner@vger.kernel.org List-ID: --/e2eDi0V/xtL+Mc8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 21, 2014 at 09:44:51AM +0100, Hans de Goede wrote: > Hi, >=20 > On 11/21/2014 09:35 AM, Maxime Ripard wrote: > > Hi Hans, > >=20 > > On Thu, Nov 20, 2014 at 04:55:20PM +0100, Hans de Goede wrote: > >> Before this commit sunxi_factors_register uses of_iomap(node, 0) to get > >> the clk registers. The sun6i prcm has factor clocks, for which we want= to > >> use sunxi_factors_register, but of_iomap(node, 0) does not work for th= e prcm > >> factor clocks, because the prcm uses the mfd framework, so the registe= rs > >> are not part of the dt-node, instead they are added to the platform_de= vice, > >> as platform_device resources. > >> > >> This commit makes getting the registers the callers duty, so that > >> sunxi_factors_register can be used with mfd instantiated platform devi= ce too. > >> > >> Signed-off-by: Hans de Goede > >=20 > > Funny, I was thinking of doing exactly the same thing for MMC clocks :) > >=20 > >> --- > >> drivers/clk/sunxi/clk-factors.c | 10 ++++------ > >> drivers/clk/sunxi/clk-factors.h | 7 ++++--- > >> drivers/clk/sunxi/clk-mod0.c | 6 ++++-- > >> drivers/clk/sunxi/clk-sun8i-mbus.c | 2 +- > >> drivers/clk/sunxi/clk-sunxi.c | 3 ++- > >> 5 files changed, 15 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-f= actors.c > >> index f83ba09..fc4f4b5 100644 > >> --- a/drivers/clk/sunxi/clk-factors.c > >> +++ b/drivers/clk/sunxi/clk-factors.c > >> @@ -156,9 +156,10 @@ static const struct clk_ops clk_factors_ops =3D { > >> .set_rate =3D clk_factors_set_rate, > >> }; > >> =20 > >> -struct clk * __init sunxi_factors_register(struct device_node *node, > >> - const struct factors_data *data, > >> - spinlock_t *lock) > >> +struct clk *sunxi_factors_register(struct device_node *node, > >> + const struct factors_data *data, > >> + spinlock_t *lock, > >> + void __iomem *reg) > >> { > >> struct clk *clk; > >> struct clk_factors *factors; > >> @@ -168,11 +169,8 @@ struct clk * __init sunxi_factors_register(struct= device_node *node, > >> struct clk_hw *mux_hw =3D NULL; > >> const char *clk_name =3D node->name; > >> const char *parents[FACTORS_MAX_PARENTS]; > >> - void __iomem *reg; > >> int i =3D 0; > >> =20 > >> - reg =3D of_iomap(node, 0); > >> - > >> /* if we have a mux, we will have >1 parents */ > >> while (i < FACTORS_MAX_PARENTS && > >> (parents[i] =3D of_clk_get_parent_name(node, i)) !=3D NULL) > >> diff --git a/drivers/clk/sunxi/clk-factors.h b/drivers/clk/sunxi/clk-f= actors.h > >> index 9913840..1f5526d 100644 > >> --- a/drivers/clk/sunxi/clk-factors.h > >> +++ b/drivers/clk/sunxi/clk-factors.h > >> @@ -37,8 +37,9 @@ struct clk_factors { > >> spinlock_t *lock; > >> }; > >> =20 > >> -struct clk * __init sunxi_factors_register(struct device_node *node, > >> - const struct factors_data *data, > >> - spinlock_t *lock); > >> +struct clk *sunxi_factors_register(struct device_node *node, > >> + const struct factors_data *data, > >> + spinlock_t *lock, > >> + void __iomem *reg); > >=20 > > Why are you dropping the __init there? >=20 > Because it is going to be used from mfd instantiation, so from a platform= _dev > probe function which is not __init. Ah right. Mentionning it in the commit log would be nice. >=20 > >=20 > >> =20 > >> #endif > >> diff --git a/drivers/clk/sunxi/clk-mod0.c b/drivers/clk/sunxi/clk-mod0= =2Ec > >> index 4a56385..9530833 100644 > >> --- a/drivers/clk/sunxi/clk-mod0.c > >> +++ b/drivers/clk/sunxi/clk-mod0.c > >> @@ -78,7 +78,8 @@ static DEFINE_SPINLOCK(sun4i_a10_mod0_lock); > >> =20 > >> static void __init sun4i_a10_mod0_setup(struct device_node *node) > >> { > >> - sunxi_factors_register(node, &sun4i_a10_mod0_data, &sun4i_a10_mod0_l= ock); > >> + sunxi_factors_register(node, &sun4i_a10_mod0_data, > >> + &sun4i_a10_mod0_lock, of_iomap(node, 0)); > >=20 > > As of_iomap can fail, I'd rather check the returned value before > > calling sunxi_factors_register. > >=20 > > I know it wasn't done before, but it's the right thing to do, as it > > would lead to an instant crash if that fails. >=20 > Ok, I'll wait for you to review the rest of the series and then do a v2 o= f the > patch-set with this fixed (as time permits). Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --/e2eDi0V/xtL+Mc8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUbx63AAoJEBx+YmzsjxAg2t0QAIh0p4GIIzXPJn+kj3bV/H3F MQFwPrCBl6/WsbHQDoLx6TwBKCpFrNRyB0LIrQ6GdV0KtH4/6rVfLR2u/W+DnNpu 4qJ9up3Xc9ZS3gYX+Zt1ZylxAPcXGgyzdN9S5X2VY9ypagGaWsR53DJIFuNxFnN/ bqP0GTQJCqI4POfmn8Gag5gXqVL20PlPcTDytlICv0UlloWdA0oNiyGlx5udTAyB fV1Hop3MPfiX8WeXzErktMFeI0b9jX9LwB4JTYF/k3LySSOrtyZH46qbjYkt12BN TgShnOqMi98UjiTWPICHxYrSOi+l+nOpi0VFo747yWAiF/jquEH+mS2AXu7sY+6D f09UUpEjXh7PzIJyivvp9I4k0yUYiqby/b6dQVc3EgVvdw2hLrrWrsW0gEA6I0/J Jjp5LKoL76V1CbKydDljZIDvIjUhAdz+YkYGQYZVly9o0ukaeSQFOb8KNlfexJxP +QXlXltI2z+TZcqqGOkEAY2mRuZW038oHls90MMrFxJFWEEqDx7SrW4c0kAbtig0 ids5bEwbHhnkyntPpD5+UIA9QZzMKdA3onJp7Yf90hBCSu1QXsVBXh9VythxulRU DZIkdTsUMYoHxLhdkfGQl8Yewxt7Eud66nJpL3fNSXQJ9nyy4j3MzSpih1wdPc0Y 1QPVhWca35KR7Mr6QPhf =dVmB -----END PGP SIGNATURE----- --/e2eDi0V/xtL+Mc8--