On 05/16/2010 11:43 PM, Marc Kleine-Budde wrote:
> Wolfgang Grandegger wrote:
>> Hello,
>>
>> we have a nasty mistake in the clock setup code of the SJA1000 platform
>> driver. The member "clock" of "struct sja1000_platform_data" is
>> documented as "CAN bus oscillator frequency in Hz" but in the probe
>> function we assign:
>>
>> priv->can.clock.freq = pdata->clock;
>>
>> In fact, it should be "pdata->clock/2", but that would require fixing
>> platform code as well. To avoild confusion, we should at least fix the
>> documentation. A clean fix would rename the member to "osc_freq", assign
>> "pdata->osc_freq/2" and fix the relevant platform code.
>>
>> What do you think?
>
> There's two in tree users of sja1000_platform
> "arch/arm/mach-mx3/mach-pcm037.c"
> "arch/arm/mach-mx2/pcm970-baseboard.c"
>
> and they both use platform data like this:
>
> struct sja1000_platform_data pcm970_sja1000_platform_data = {
> .clock = 16000000 / 2,
> .ocr = 0x40 | 0x18,
> .cdr = 0x40,
> };
>
> This means it's at least _used_ consistent in the kernel.
Yes, otherwise it would not work properly.
> Hmmm...it would be more consistent with the other sja users, e.g. the of
> driver, to do the full fix. Renaming the pdata member should compile
> time break it instead of run-time. Thus the non mainline users will
> notice it, too.
Yep, that was exactly my motivation. Also, people get regularly confused
by the factor of 2. I'm going to prepare a patch later this week.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
