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@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. > 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@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
