On Sat, Aug 15, 2015 at 1:53 PM, Sam Ravnborg <sam@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
