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 ;-)
+// 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 ...
This definition of 'struct CPC_SJA1000_PARAMS' might look superfluous but it
isn't. It's just needed to build the correct message to *this* USB adapter.
Regards,
Oliver
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
