Hi Kurt,
Kurt Van Dijck wrote:
> Hi,
>
> This is an effort to provide a CAN transceiver subsystem for use with
> socketcan.
> The goal is to allow different functions of different CAN transceivers
> to nicely cooperate with socketCAN devices, without socketCAN devices
> needing to know thing of their corresponding transceivers.
>
> I did take these arguments into account during developing this:
> 1) the intrusion in socketcan device code should be minimal.
> 2) the registration of CAN transceivers should be optional, not required
> 3) CAN transceiver core should provide meaningfull defaults for
> functions that the particular transceiver does not support.
> 4) the matching of transceivers to net_devices should be predictable
> 5) beside the regular device scheme, a board with on-board transceiver
> should not depend on transceiver core device matching. It should easily
> pre-match a transceiver to a net device.
>
>
> I'll shortly describe the changes:
> 1) a new class is made: can-transceiver.
> 2) this class receives net_device register/unregister notifications.
> 3) this class will match netdevs to transceivers. The matching itself is
> done with the netdevice's parent name (like sja1000_platform.0)
> These names are not changed by using udev or similar, whereas ifnames
> like 'can0' can.
> 4) The match between transceiver & net device can change during device
> lifetime.
> 5) a can_transceiver_alert entry is provided that could handle common
> transceiver fault handling. Currently, I sends a can_frame upstream,
> and brings down the iface.
> 6) A new bit in error can_frame's can_id is occupied to indicate
> CAN transceiver notifications.
>
> I probably forgot to tell a lot more :-).
> Probably, the locking is not yet correct (I still must run with
> PROVE_LOCKING).
> At least, it can act as a starting point.
I'm not yet familiar with the features and technical details of CAN
transceivers but at a first glance I have the following comments:
- I would follow more closely the existing PHY implementation.
- SysFS is not an option.
- It's a lot of code for the functionality it provides (that's just my
first impression).
When time permits, I will have a closer look. Hopefully before Oliver is
back from holiday. This new interface will require some time to mature.
If you need a quick solution, resend your original patch for the SJA1000
similar to the one of the AT91. It should also not be a big deal to push
it upstream.
In general, there are various coding style issues:
- Use labels just for the usual cleanup purposes:
+#ifdef CONFIG_CANTR_CORE
+ if (!priv->cantr)
+ goto no_transceiver;
+ max_bitrate = can_transceiver_get_max_bitrate(priv->cantr);
+ if (priv->bittiming.bitrate <= max_bitrate)
+ goto transceiver_bitrate_ok;
+ dev_err(ND2D(dev), "bitrate over transceiver's range\n");
+ return -EINVAL;
+transceiver_bitrate_ok:
+ ret = can_transceiver_set_mode(priv->cantr, CANTR_MODE_ON);
+ if (ret < 0)
+ return ret;
+no_transceiver:
+#endif
This is not Fortran4 ;-).
- Use the prefix "can_" or "CAN_", e.g. CAN_TR instead of CAN_TR or even
better CAN_TRANSCEIVER.
- Use long names for members of structures:
+struct tja1041_platform_data {
+ /* gpio's */
+ int stb;
+ int en;
+ int err;
+
+ const char *match;
+};
Or at least document them.
- Use Macro definitions for bit masks:
+ if (!(mask & 4))
+ goto no_err;
What is bit 2 good for?
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
