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

[Lucas Stach <l.stach@pengutronix.de>]

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