On 05/20/2010 02:27 AM, Marc Kleine-Budde wrote:
> Hey,
>
> just some short thoughts,....I'll be offline till Monday.
>
> Wolfgang Grandegger wrote:
>> Hi Marc,
>>
>> On 05/19/2010 08:16 PM, Marc Kleine-Budde wrote:
>>> Hello,
>>>
>>> I just notided that the "ctrlmode" variable is handled in most drivers
>>> only in "set_bittiming" function, here the function from the sja:
>>>
>>>> static int sja1000_set_bittiming(struct net_device *dev)
>>>> {
>>>> struct sja1000_priv *priv = netdev_priv(dev);
>>>> struct can_bittiming *bt = &priv->can.bittiming;
>>>> u8 btr0, btr1;
>>>>
>>>> btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
>>>> btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
>>>> (((bt->phase_seg2 - 1) & 0x7) << 4);
>>>> if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
>>>> btr1 |= 0x80;
>>>>
>>>> dev_info(dev->dev.parent,
>>>> "setting BTR0=0x%02x BTR1=0x%02x\n", btr0, btr1);
>>>>
>>>> priv->write_reg(priv, REG_BTR0, btr0);
>>>> priv->write_reg(priv, REG_BTR1, btr1);
>>>>
>>>> return 0;
>>>> }
>>> Consider the following sequence:
>>> $ modprobe sja1000-platform
>>> $ canconfig can0 bitrate 250000 # using pengutronix's canutils
>>> $ ifconfig can0 up
>>>
>>> # do some testing, with a bitrate of 250000
>>>
>>> $ ifconfig can0 down
>>>
>>> # decide to activate tripple samling:
>>>
>>> $ canconfig can0 ctrlmode triple-sampling on
>>> $ ifconfig can0
>>>
>>> Contrary to the expectation the tripple sampling mode isn't activated.
>>
>> Ah, I see. That's needs to be fixed.
>>
>>> All other control modes that are set during bittiming aren't handled as
>>> well. For e.g. the flexcan (I'm just hacking on) it's the tripple
>>> sample, listen only and loopback mode.
>>
>> The latter two are usually set in the open function when the device is
>> started. See:
>>
>> http://lxr.linux.no/#linux+v2.6.34/drivers/net/can/mcp251x.c#L506
>
> hmmm...that if ... else if... else if looks strange...
> are the control modes mutually exclusive?
Well, I does make little sense to select listen-only and loopback at the
same time. Printing an error message might be more transparent but we
regarded it as overkill when the patch was reviewed. Nevertheless, the
checking should be done in common code.
>>> Thinking a bit about the problem...what are the pros and cons of calling
>>> the do_set_bittiming when the bittiming is changed comapred to call it
>>> in the "ndo_open" function, e.g. via "open_candev".
>>
>> Some drivers already do that mainly because it could not be done
>> earlier, e.g. the MPC512x driver. Just do not set the do_set_bittiming
>> callback and call the set_bittiming function in the open. Well, there
>> are not that much cons. Important is that the bittiming parameters are
>> verified when they are set, but that's already the case.
>>
>>> This would simplify the flexcan driver, because it must be in a certain
>>> state (clocks enabled but device in freeze mode) for the bittiming
>>> configuration. For power saving reasons the clock stay off between
>>> _probe and _open.
>>
>> Yep. Maybe it's even more flexible to remove the do_set_bittiming
>> callback completely.
>
> Yes, I think that's a quite good and simple idea. However there's at
> least one driver that has special restrictions, IIRC brp >4 for tripple
> sampling. The driver has to check in it's open() function then. Then
> "ifconfig up" would fail....same goes for "intelligent" can cards with a
> limited set of fixed bitrates.
Yes, that would be bad.
> This means canconfig bitrate does succeed, but ifconfig up does fail. We
> can rename the callback into "do_check_bittiming"....
Good idea. I vote for it.
> I'm not familiar with the netlink interface, but is it possible to set
> the bittiming and control mode atomically? Can you call the
> "do_X_bittiming" accordingly?
>
> I just noticed, it's already possible to not assign "do_set_bittiming",
> it's checked for NULL before calling it. :)
Yep, that's what I explained in my previous mail (see above).
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
