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

[Andrey Smirnov <andrew.smirnov@gmail.com>]

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