diff mbox

[Cocci,v13,3/6] clk: Make clk API return per-user struct clk instances

Message ID 54E3BA20.3080205@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Stephen Boyd Feb. 17, 2015, 10:01 p.m. UTC 
On 02/05/15 18:15, Stephen Boyd wrote:
> On 02/05/15 07:45, Quentin Lambert wrote:
>> On 05/02/2015 00:26, Stephen Boyd wrote:
>>>> If you want me to I can enlarge the search to other directories.
>>> Yes please do. And if you could share the coccinelle patch that would be
>>> great. Thanks.
>>>
>> structclk.cocci is the coccinelle patch
>> structclk-arm.patch is the result I got when applying it to the
>> arch/arm directory
>>
>> Is there anything else I can do to help?
>>
>>
> Thanks for the coccinelle patch. Thinking more about it, I don't think
> we care if the pointer is dereferenced because that would require a
> definition of struct clk and that is most likely not the case outside of
> the clock framework. Did you scan the entire kernel? I'm running it now
> but it seems to be taking a while.
>

I ran the script on all files that include <linux/clk.h>. I've also
trimmed out mips and unicore32 because they're not using the common
clock framework.

sound/soc/kirkwood/kirkwood-i2s.c:582:6-18: WARNING trying to compare or dereference struct clk pointers.

Comments

Sebastian Andrzej Siewior March 12, 2015, 5:20 p.m. UTC | #1 
On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
> diff = 
> --- arch/arm/mach-imx/mach-imx6q.c
> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>  	 * (external OSC), and we need to clear the bit.
>  	 */
> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>  	if (!IS_ERR(gpr))

Any idea how to do the comparison here? Or should we rely that the bootloader
sets this properly (it managed already to select a frequency)? The phy has no
clock node in current DT's so we can check this.

Sebastian
Stephen Boyd March 12, 2015, 7:43 p.m. UTC | #2 
On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
> On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
>> diff = 
>> --- arch/arm/mach-imx/mach-imx6q.c
>> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
>> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>>  	 * (external OSC), and we need to clear the bit.
>>  	 */
>> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>>  	if (!IS_ERR(gpr))
> Any idea how to do the comparison here? Or should we rely that the bootloader
> sets this properly (it managed already to select a frequency)? The phy has no
> clock node in current DT's so we can check this.
>

This has been fixed by adding a clk_is_match() helper and using that to
compare instead of comparing raw pointers. It would be nice if we could
replace the patch with something else that doesn't require this helper
though. It looks like this is static board configuration, so I wonder
why we didn't just have a DT property that indicates how the gpr should
be configured for this particular board.
Shawn Guo March 13, 2015, 3:29 a.m. UTC | #3 
On Thu, Mar 12, 2015 at 12:43:40PM -0700, Stephen Boyd wrote:
> On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
> > On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
> >> diff = 
> >> --- arch/arm/mach-imx/mach-imx6q.c
> >> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
> >> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
> >>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
> >>  	 * (external OSC), and we need to clear the bit.
> >>  	 */
> >> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
> >>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
> >>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> >>  	if (!IS_ERR(gpr))
> > Any idea how to do the comparison here? Or should we rely that the bootloader
> > sets this properly (it managed already to select a frequency)? The phy has no
> > clock node in current DT's so we can check this.
> >
> 
> This has been fixed by adding a clk_is_match() helper and using that to
> compare instead of comparing raw pointers. It would be nice if we could
> replace the patch with something else that doesn't require this helper
> though. It looks like this is static board configuration, so I wonder
> why we didn't just have a DT property that indicates how the gpr should
> be configured for this particular board.

We did not add a DT property for it, because there was already enough
info (clock configuration) in DT for kernel to figure it out.

Shawn
Sebastian Andrzej Siewior March 13, 2015, 8:20 a.m. UTC | #4 
Hi Shawn,

On Fri, Mar 13, 2015 at 11:29:32AM +0800, Shawn Guo wrote:

> We did not add a DT property for it, because there was already enough
> info (clock configuration) in DT for kernel to figure it out.
Correct. My understanding is whatever can be figured out without DT should
be done that way.

Is there a way to get this clock-select bit set without
enable_fec_anatop_clock() in u-boot? Because this one selects the clock and
frequency and also sets the proper bit in gpr1. My question is simply why do
we need to do this here as well.

> Shawn

Sebastian
Shawn Guo March 13, 2015, 1:42 p.m. UTC | #5 
On Fri, Mar 13, 2015 at 09:20:10AM +0100, Sebastian Andrzej Siewior wrote:
> Hi Shawn,
> 
> On Fri, Mar 13, 2015 at 11:29:32AM +0800, Shawn Guo wrote:
> 
> > We did not add a DT property for it, because there was already enough
> > info (clock configuration) in DT for kernel to figure it out.
> Correct. My understanding is whatever can be figured out without DT should
> be done that way.
> 
> Is there a way to get this clock-select bit set without
> enable_fec_anatop_clock() in u-boot? Because this one selects the clock and
> frequency and also sets the proper bit in gpr1. My question is simply why do
> we need to do this here as well.

I'm not sure I follow your question.  But we had been doing this setup
in kernel function imx6q_1588_init() for a while.  The problem we're
having now is that the clk core change broke it since v4.0-rc1.  And the
fix is already queued there [1].

Shawn

[1] https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-fixes
Stephen Boyd March 13, 2015, 5:42 p.m. UTC | #6 
On 03/12/15 20:29, Shawn Guo wrote:
> On Thu, Mar 12, 2015 at 12:43:40PM -0700, Stephen Boyd wrote:
>> On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
>>> On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
>>>> diff = 
>>>> --- arch/arm/mach-imx/mach-imx6q.c
>>>> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
>>>> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>>>>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>>>>  	 * (external OSC), and we need to clear the bit.
>>>>  	 */
>>>> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>>>>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>>>>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>>>>  	if (!IS_ERR(gpr))
>>> Any idea how to do the comparison here? Or should we rely that the bootloader
>>> sets this properly (it managed already to select a frequency)? The phy has no
>>> clock node in current DT's so we can check this.
>>>
>> This has been fixed by adding a clk_is_match() helper and using that to
>> compare instead of comparing raw pointers. It would be nice if we could
>> replace the patch with something else that doesn't require this helper
>> though. It looks like this is static board configuration, so I wonder
>> why we didn't just have a DT property that indicates how the gpr should
>> be configured for this particular board.
> We did not add a DT property for it, because there was already enough
> info (clock configuration) in DT for kernel to figure it out.

Ok well then if everything is in DT we don't need to be comparing clock
pointers at all, right? We should be able to write up some DT parsing
logic to figure it out?
diff mbox

Patch

diff = 
--- arch/arm/mach-imx/mach-imx6q.c
+++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
@@ -211,7 +211,6 @@  static void __init imx6q_1588_init(void)
 	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
 	 * (external OSC), and we need to clear the bit.
 	 */
-	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
 				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
 	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
 	if (!IS_ERR(gpr))
diff = 
--- drivers/gpu/drm/armada/armada_510.c
+++ /tmp/cocci-output-12321-a5f298-armada_510.c
@@ -53,7 +53,6 @@  static int armada510_crtc_compute_clock(
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-	if (dcrtc->clk != clk) {
 		ret = clk_prepare_enable(clk);
 		if (ret)
 			return ret;
drivers/gpu/drm/armada/armada_510.c:56:5-15: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- drivers/pwm/pwm-atmel-hlcdc.c
+++ /tmp/cocci-output-12679-3c5195-pwm-atmel-hlcdc.c
@@ -91,7 +91,6 @@  static int atmel_hlcdc_pwm_config(struct
 
 	pwmcfg = ATMEL_HLCDC_PWMPS(pres);
 
-	if (new_clk != chip->cur_clk) {
 		u32 gencfg = 0;
 		int ret;
 
drivers/pwm/pwm-atmel-hlcdc.c:94:5-12: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- drivers/tty/serial/samsung.c
+++ /tmp/cocci-output-12827-715e72-samsung.c
@@ -750,7 +750,6 @@  static void s3c24xx_serial_set_termios(s
 
 	/* check to see if we need  to change clock source */
 
-	if (ourport->baudclk != clk) {
 		s3c24xx_serial_setsource(port, clk_sel);
 
 		if (!IS_ERR(ourport->baudclk)) {
drivers/tty/serial/samsung.c:753:5-21: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- sound/soc/fsl/fsl_esai.c
+++ /tmp/cocci-output-13020-d518c3-fsl_esai.c
@@ -269,7 +269,6 @@  static int fsl_esai_set_dai_sysclk(struc
 	}
 
 	/* Only EXTAL source can be output directly without using PSR and PM */
-	if (ratio == 1 && clksrc == esai_priv->extalclk) {
 		/* Bypass all the dividers if not being needed */
 		ecr |= tx ? ESAI_ECR_ETO : ESAI_ECR_ERO;
 		goto out;
sound/soc/fsl/fsl_esai.c:272:19-25: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- sound/soc/fsl/fsl_spdif.c
+++ /tmp/cocci-output-13024-7acb1d-fsl_spdif.c
@@ -1054,7 +1054,6 @@  static u32 fsl_spdif_txclk_caldiv(struct
 				enum spdif_txrate index, bool round)
 {
 	const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 };
-	bool is_sysclk = clk == spdif_priv->sysclk;
 	u64 rate_ideal, rate_actual, sub;
 	u32 sysclk_dfmin, sysclk_dfmax;
 	u32 txclk_df, sysclk_df, arate;
@@ -1148,7 +1147,6 @@  static int fsl_spdif_probe_txclk(struct
 			spdif_priv->txclk_src[index], rate[index]);
 	dev_dbg(&pdev->dev, "use txclk df %d for %dHz sample rate\n",
 			spdif_priv->txclk_df[index], rate[index]);
-	if (spdif_priv->txclk[index] == spdif_priv->sysclk)
 		dev_dbg(&pdev->dev, "use sysclk df %d for %dHz sample rate\n",
 				spdif_priv->sysclk_df[index], rate[index]);
 	dev_dbg(&pdev->dev, "the best rate for %dHz sample rate is %dHz\n",
sound/soc/fsl/fsl_spdif.c:1151:5-29: WARNING trying to compare or dereference struct clk pointers.
sound/soc/fsl/fsl_spdif.c:1057:18-21: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- sound/soc/kirkwood/kirkwood-i2s.c
+++ /tmp/cocci-output-13041-3200a6-kirkwood-i2s.c
@@ -579,7 +579,6 @@  static int kirkwood_i2s_dev_probe(struct
 		if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
 	} else {
-		if (priv->extclk == priv->clk) {
 			devm_clk_put(&pdev->dev, priv->extclk);
 			priv->extclk = ERR_PTR(-EINVAL);
 		} else {