[PATCH v2] i2c-imx: Add missing preporcessor directives

[PATCH v2] i2c-imx: Add missing preporcessor directives

[Andrey Smirnov]

On non-PowerPC platforms call to i2c_fsl_set_clk() will try to obtain
I2C clock freqency from i2c_fsl->clk, however that field would not be
initialized if CONFIG_COMMON_CLK is not set. This patch makes sure
that i2c_fls_set_clk() is a no-op on non-PPC targets when
CONFIG_COMMON_CLK is not set

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/i2c/busses/i2c-imx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 4cd03e1..84c6e16 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -338,6 +338,7 @@ static void i2c_fsl_set_clk(struct fsl_i2c_struct *i2c_fsl,
 	i2c_fsl->dfsrr = dfsr;
 }
 #else
+#if defined (CONFIG_COMMON_CLK)
 static void i2c_fsl_set_clk(struct fsl_i2c_struct *i2c_fsl,
 			    unsigned int rate)
 {
@@ -374,6 +375,11 @@ static void i2c_fsl_set_clk(struct fsl_i2c_struct *i2c_fsl,
 	dev_dbg(&i2c_fsl->adapter.dev, "<%s> IFDR[IC]=0x%x, REAL DIV=%d\n",
 		__func__, i2c_clk_div[i][1], i2c_clk_div[i][0]);
 }
+#else
+static void i2c_fsl_set_clk(struct fsl_i2c_struct *i2c_fsl,
+			    unsigned int rate)
+{}
+#endif
 #endif

 static int i2c_fsl_write(struct i2c_adapter *adapter, struct i2c_msg *msgs)
--
2.1.4

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] i2c-imx: Add missing preporcessor directives

[Sam Ravnborg]

Hi Andrey.

On Sat, Aug 15, 2015 at 09:44:31AM -0700, Andrey Smirnov wrote:
> On non-PowerPC platforms call to i2c_fsl_set_clk() will try to obtain
> I2C clock freqency from i2c_fsl->clk, however that field would not be
> initialized if CONFIG_COMMON_CLK is not set. This patch makes sure
> that i2c_fls_set_clk() is a no-op on non-PPC targets when
> CONFIG_COMMON_CLK is not set

Per the other mail we will never hit this case.
So you add an ifdef that never will be used,
because this driver is either for IMX (which uses COMMON_CLK) or PowerPC.

There is this snip from the driver:

#ifdef CONFIG_COMMON_CLK
        i2c_fsl->clk = clk_get(pdev, NULL);
        if (IS_ERR(i2c_fsl->clk))
                return PTR_ERR(i2c_fsl->clk);
#endif

You may have been inspired by that.
To the best of my understanding the ifdef can be dropped,
because clk_dev() is always defined, but retinr NULL if
HAVE_CLK is not defined.
I assume thsi is the case for PowerPC.

So the better fix would be to get rid of this ifdet,
rather than introducing a new one.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] i2c-imx: Add missing preporcessor directives

[Andrey Smirnov]

On Sat, Aug 15, 2015 at 1:53 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
> Hi Andrey.
>
> On Sat, Aug 15, 2015 at 09:44:31AM -0700, Andrey Smirnov wrote:
>> On non-PowerPC platforms call to i2c_fsl_set_clk() will try to obtain
>> I2C clock freqency from i2c_fsl->clk, however that field would not be
>> initialized if CONFIG_COMMON_CLK is not set. This patch makes sure
>> that i2c_fls_set_clk() is a no-op on non-PPC targets when
>> CONFIG_COMMON_CLK is not set
>
> Per the other mail we will never hit this case.
> So you add an ifdef that never will be used,
> because this driver is either for IMX (which uses COMMON_CLK) or PowerPC.
>

IMHO, source code is orthogonal to build and configuration system.
While it is true that the configuration system would prevent this
combination of pre-processor symbols to ever be defined I think not
making this assumption would result in more reliable and robust source
code.

> There is this snip from the driver:
>
> #ifdef CONFIG_COMMON_CLK
>         i2c_fsl->clk = clk_get(pdev, NULL);
>         if (IS_ERR(i2c_fsl->clk))
>                 return PTR_ERR(i2c_fsl->clk);
> #endif
>
> You may have been inspired by that.
> To the best of my understanding the ifdef can be dropped,
> because clk_dev() is always defined, but retinr NULL if
> HAVE_CLK is not defined.
> I assume thsi is the case for PowerPC.

It doesn't really matter if this snip is present or not, since
i2c_fls->clk would either be NULLed by the value returned by clk_get()
or it would be zero from the time the memory for i2c_fls was
kzalloc'ed. The point is that i2c_fsl->ifdr(AFAIU a clock divider)
would be populated with a bogus value.

>
> So the better fix would be to get rid of this ifdet,
> rather than introducing a new one.

IMHO, the patch is very trivial yet between the two of us we already
exchanged 4 e-mails discussing it, so this whole thing is rapidly
descending into a bike-shedding exercise. Since I agree with you that
this bug is very unlikely to be triggered, let's just drop this patch.

Andrey

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] i2c-imx: Add missing preporcessor directives

[Lucas Stach]

Hi Andrey,

Am Samstag, den 15.08.2015, 16:33 -0700 schrieb Andrey Smirnov:
> On Sat, Aug 15, 2015 at 1:53 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
> > Hi Andrey.
> >
> > On Sat, Aug 15, 2015 at 09:44:31AM -0700, Andrey Smirnov wrote:
> >> On non-PowerPC platforms call to i2c_fsl_set_clk() will try to obtain
> >> I2C clock freqency from i2c_fsl->clk, however that field would not be
> >> initialized if CONFIG_COMMON_CLK is not set. This patch makes sure
> >> that i2c_fls_set_clk() is a no-op on non-PPC targets when
> >> CONFIG_COMMON_CLK is not set
> >
> > Per the other mail we will never hit this case.
> > So you add an ifdef that never will be used,
> > because this driver is either for IMX (which uses COMMON_CLK) or PowerPC.
> >
> 
> IMHO, source code is orthogonal to build and configuration system.
> While it is true that the configuration system would prevent this
> combination of pre-processor symbols to ever be defined I think not
> making this assumption would result in more reliable and robust source
> code.
> 
> > There is this snip from the driver:
> >
> > #ifdef CONFIG_COMMON_CLK
> >         i2c_fsl->clk = clk_get(pdev, NULL);
> >         if (IS_ERR(i2c_fsl->clk))
> >                 return PTR_ERR(i2c_fsl->clk);
> > #endif
> >
> > You may have been inspired by that.
> > To the best of my understanding the ifdef can be dropped,
> > because clk_dev() is always defined, but retinr NULL if
> > HAVE_CLK is not defined.
> > I assume thsi is the case for PowerPC.
> 
> It doesn't really matter if this snip is present or not, since
> i2c_fls->clk would either be NULLed by the value returned by clk_get()
> or it would be zero from the time the memory for i2c_fls was
> kzalloc'ed. The point is that i2c_fsl->ifdr(AFAIU a clock divider)
> would be populated with a bogus value.
> 
> >
> > So the better fix would be to get rid of this ifdet,
> > rather than introducing a new one.
> 
> IMHO, the patch is very trivial yet between the two of us we already
> exchanged 4 e-mails discussing it, so this whole thing is rapidly
> descending into a bike-shedding exercise. Since I agree with you that
> this bug is very unlikely to be triggered, let's just drop this patch.
> 
Even if you are dropping this patch, let me add a little remark.

If you are going to send similar patches in the future, please take a
look at the IS_ENABLED macro. We try to get rid of those #ifdef
constructs in barebox and replace it with the above macro, which will
expand to 0 if the config option isn't enabled and the resulting code
will the be removed by the optimizer.

Regards,
Lucas 
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox