[PATCH v2] i2c-imx: Add missing preporcessor directives
l.stach at pengutronix.de
Mon Aug 17 01:39:02 PDT 2015
Am Samstag, den 15.08.2015, 16:33 -0700 schrieb Andrey Smirnov:
> 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);
> > #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.
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
More information about the barebox