> On 18.01.2013 15:24, Bernd Krumboeck wrote: > >> Add device driver for USB2CAN interface from "8 devices" >> (http://www.8devices.com). > >> > >> Signed-off-by: Bernd Krumboeck <[email protected]> >> Acked-by: Wolfgang Grandegger <[email protected]> > > > > You can add my > > Tested-by: Oliver Hartkopp <[email protected]> > > in the v11 post :-) > > I have only one small cosmetic change request: > > Please remove/split up this #define block. > > >> + >> +/* bittiming constants */ >> +#define USB_8DEV_ABP_CLOCK 32000000 > > > /* device CAN clock */ > #define USB_8DEV_ABP_CLOCK 32000000 > >> +#define USB_8DEV_BAUD_MANUAL 0x09 > > > -> moved down, see below > >> +#define USB_8DEV_TSEG1_MIN 1 >> +#define USB_8DEV_TSEG1_MAX 16 >> +#define USB_8DEV_TSEG2_MIN 1 >> +#define USB_8DEV_TSEG2_MAX 8 >> +#define USB_8DEV_SJW_MAX 4 >> +#define USB_8DEV_BRP_MIN 1 >> +#define USB_8DEV_BRP_MAX 1024 >> +#define USB_8DEV_BRP_INC 1 > > > It makes no sense to define these values when you can specify them > directly in > the const struct can_bittiming_const below. > > This is usual and does not reduce the readability. > >> + >> +/* setup flags */ >> +#define USB_8DEV_SILENT 0x01 >> +#define USB_8DEV_LOOPBACK 0x02 >> +#define USB_8DEV_DISABLE_AUTO_RESTRANS 0x04 >> +#define USB_8DEV_STATUS_FRAME 0x08 >> + >> +/* commands */ >> +enum usb_8dev_cmd { >> + USB_8DEV_RESET = 1, >> + USB_8DEV_OPEN, >> + USB_8DEV_CLOSE, >> + USB_8DEV_SET_SPEED, >> + USB_8DEV_SET_MASK_FILTER, >> + USB_8DEV_GET_STATUS, >> + USB_8DEV_GET_STATISTICS, >> + USB_8DEV_GET_SERIAL, >> + USB_8DEV_GET_SOFTW_VER, >> + USB_8DEV_GET_HARDW_VER, >> + USB_8DEV_RESET_TIMESTAMP, >> + USB_8DEV_GET_SOFTW_HARDW_VER >> +}; > > > /* command options */ > #define USB_8DEV_BAUD_MANUAL 0x09 > >> + >> +#define USB_8DEV_CMD_START 0x11 >> +#define USB_8DEV_CMD_END 0x22 >> + > > (..) > >> + >> +static const struct can_bittiming_const usb_8dev_bittiming_const = { >> + .name = "usb_8dev", >> + .tseg1_min = USB_8DEV_TSEG1_MIN, >> + .tseg1_max = USB_8DEV_TSEG1_MAX, >> + .tseg2_min = USB_8DEV_TSEG2_MIN, >> + .tseg2_max = USB_8DEV_TSEG2_MAX, >> + .sjw_max = USB_8DEV_SJW_MAX, >> + .brp_min = USB_8DEV_BRP_MIN, >> + .brp_max = USB_8DEV_BRP_MAX, >> + .brp_inc = USB_8DEV_BRP_INC, >> +}; >> + > > > insert values directly here. > > Regards, > Oliver > >
Thank you. regards, Bernd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
