On Fri, Oct 16, 2009 at 07:28:08PM +0200, Wolfgang Grandegger wrote:
> 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.
> >
[...]
> > 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.
I know PHY looks similar. Oliver suggested that also.
First trouble: I'm not very familiar with those.
So, I did look at them. My conclusion is that it is not very suitable
for anything other than ethernet. Since we're now evaluating these, I'll
summarize my findings.
* The way PHY is implemented right now is a rather solid unit of both
access methods & functional methods. My interpretation is this:
a PHY is indicated by an address of some MDIO bus (access method).
It provides then a set of registers (access method).
These registers can be layed out as the MII interface (functional
method). As such, a 'generic' PHY is implemented.
CAN transceivers today have no common access method. Therefore, only the
functional part of PHY can be used. And the functional part of PHY is
very ethernet oriented.
The result (my proposal) is rather similar in the functional part. But
by instantiating a new class, which provides no access methods at all,
the overhead per transceiver is lowered.
Another major drawback from reusing PHY is that besides the rather
heavy modifications, ethernet should still keep on working.
* The clean seperation between datalink & physical layer that exists on
etherenet is not respected on CAN. An ethernet controller seems to rely
completely on it's PHY to decide the carrier is off. CAN chips on the
other side do this theirselve (for increased predictability) by going
busooff. So, you'd end up with 2 sources deciding things about
the carrier. This would require some other cooperation model
than what exists for PHY right now.
Also, the PHY's statemachine for autonegotiation is something that would
on CAN typically be performed purely by or at least with cooperation of
the CAN chip. I don't know any transceiver today that could do this on
its own, as happens on ethernet.
*
summarized, I believe the differences are way bigger than the
similarities.
> - SysFS is not an option.
I must admit I do not totally understand this argument during the
bittiming development of yours either.
please note that my transceiver implementation is 'rather' seperated
from the net_devices.
On the other hand, if transceiver control could be implemented via
netlink too, I would not veto that.
> - It's a lot of code for the functionality it provides (that's just my
> first impression).
That's a difficult argument :-).
>
> 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.
sure.
> 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.
I'd avoid an implementation that integrates one single predefined
transceiver (pca82c251) into every driver. That's the bad way.
My SJA1000 patch learned me that I was sure not the only one looking for
that kind of functionality.
Oliver's contribution is that there's a lot more functionality than just
enable the transceiver.
For my system, I still can do with my previous patch, or use my
transciever drivers from now. The advantage of the latter is that it is
quite seperated from CAN chip drivers, and gives SYSFS access on how the
system connected.
>
> 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 ;-).
I see. With 80 columns, I tend to abuse labels to skip a bunch of
functions.
>
> - Use the prefix "can_" or "CAN_", e.g. CAN_TR instead of CAN_TR or even
> better CAN_TRANSCEIVER.
ok (replace CONFIG_CANTR_CORE with CONFIG_CAN_TRANSCEIVER_CORE is what I
think you meant)
>
> - 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.
Those are valid comments. This is however a 'prototype'.
The driver specific code is probably only readable with the datasheet
next to it.
>
> - Use Macro definitions for bit masks:
>
> + if (!(mask & 4))
> + goto no_err;
>
> What is bit 2 good for?
idem.
>
> Wolfgang.
>
Kurt
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
