On 09/08/2009 03:58 PM, Oliver Hartkopp wrote:
> Wolfgang Grandegger wrote:
>> On 09/08/2009 03:10 PM, Oliver Hartkopp wrote:
>>> Sebastian Haas wrote:
>> [snip]
>>>> + if(msg->type == CPC_MSG_T_RTR || msg->type == CPC_MSG_T_XRTR) {
>>>> + cf->can_id |= CAN_RTR_FLAG;
>>>> + } else {
>>>
>>> *(u64 *) (&cf.data) = 0; /* clear payload */
>>
>> Hm, do we need to clear the payload at all?
>>
>
>
> This is a really good point.
>
> Indeed the can_dlc gives the information about the 'valid' items in data[] ...
>
> So we can leave the invalid data untouched (and uninitialized).
>
> I'll remove it in slcan.c, where i stole the sniplet ;-)
>
>
>> [snip]
>>
>>>> +
>>>> +static struct can_bittiming_const ems_usb_bittiming_const = {
>>>> + .name = "ems_usb",
>>>> + .tseg1_min = 1,
>>>> + .tseg1_max = 16,
>>>> + .tseg2_min = 1,
>>>> + .tseg2_max = 8,
>>>> + .sjw_max = 4,
>>>> + .brp_min = 1,
>>>> + .brp_max = 64,
>>>> + .brp_inc = 1,
>>>> +};
>>>
>>>
>>> maybe:
>>>
>>>
>>> static struct can_bittiming_const ems_usb_sja1000_bittiming_const = {
>>> .name = "ems_usb_sja1000",
>>> .tseg1_min = 1,
>>> (..)
>>> };
>>>
>>> static struct can_bittiming_const ems_usb_m16_bittiming_const = {
>>> .name = "ems_usb_m16",
>>> .tseg1_min = 1,
>>> (..)
>>> };
>>>
>>> @Wolfgang: would this be a correct naming scheme as the ems_usb driver
>>> has
>>> different hardware types?
>>
>> The name should be "sja1000" in case there is no difference to the other
>> SJA1000 CAN controllers.
>
>
> static struct can_bittiming_const sja1000_bittiming_const = {
> .name = "sja1000",
> .tseg1_min = 1,
> (..)
> };
>
> static struct can_bittiming_const m16_bittiming_const = {
> .name = "m16",
> .tseg1_min = 1,
> (..)
> };
>
> Fine.
>
>> What is "m16"?
>
> It's a M16C processor that sits in the ems_usb hardware sometimes.
>
> There are two possible CPUs in the USB adapter:
>
> +static struct usb_device_id ems_usb_table[] = {
> + {USB_DEVICE(USB_CPCUSB_VENDOR_ID, USB_CPCUSB_M16C_PRODUCT_ID)},
> + {USB_DEVICE(USB_CPCUSB_VENDOR_ID, USB_CPCUSB_LPC2119_PRODUCT_ID)},
> + {} /* Terminating entry */
> +};
>
>
> Btw. the SJA1000 is indeed a LPC2119 that has two CAN controllers/makros with
> a SJA1000 layout onboard.
>
> So we could also use 'lpc2119_bittiming_const' which would probably add some
> confusion as everything else looks like a SJA1000 ;-)
OK, the name is mainly used to tell the user what CAN controller is used:
$ ip -details link show can0
2: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UP qlen 10
link/can
can <TRIPLE-SAMPLING> state ERROR-ACTIVE restart-ms 100
bitrate 125000 sample_point 0.875
tq 125 prop-seg 6 phase-seg1 7 phase-seg2 2 sjw 1
sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
clock 8000000
Therefore SJA1000 might be to general, indeed. "m16c" or "lpc2119"
might be better, eventually with the prefix "usb-".
> +// CAN params message representation
> +typedef struct CPC_CAN_PARAMS {
> + u8 cc_type; // represents the controller type
> + union {
> + CPC_M16C_BASIC_PARAMS_T m16c_basic;
> + CPC_SJA1000_PARAMS_T sja1000;
> + CPC_PCA82C200_PARAMS_T pca82c200;
> + } cc_params;
> +} CPC_CAN_PARAMS_T;
>
>
>>
>> There are more issues, e.g. "__u8" -> "u8", no typedefs, lowercase names
>> for structs, ...
>
> Yep.
>
>>
>> Also I don't like that we introduce another set of SJA1000 register
>> definitions and offsets. Furthermore, there are many UBS based SJA1000
>> devices on the market and we should think if we could provide a common
>> interface for these, if feasible. E.g. sja1000_usb.c similar to sja1000.c.
>>
>
> The problem is, that each of them defines a proprietary struct for the usb
> communication as they are always talking to 'helper' CPUs and not directly to
> the SJA1000. So a sja1000_usb.c will never become true ...
Then, as already proposed, we might put USB drivers into /drivers/net/can/usb
as theyare special anyhow.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
