[PATCH v2] i2c-imx: Add missing preporcessor directives
andrew.smirnov at gmail.com
Sat Aug 15 16:33:16 PDT 2015
On Sat, Aug 15, 2015 at 1:53 PM, Sam Ravnborg <sam at 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
> 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);
> 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.
More information about the barebox