Please keep the patch subject within reasonable length.
It sould describe what you have done. The patch description should describe the why.
On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@xxxxxxxxxxxxxxxx>
> ---
> drivers/net/can/usb/ems_usb.c | 66 +++++++++++++++++++++++++----------
> 1 file changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 4ed0d681a68c..a3943042b8c8 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -266,7 +266,6 @@ static struct usb_device_id ems_usb_table[] = {
>
> MODULE_DEVICE_TABLE(usb, ems_usb_table);
>
> -#define RX_BUFFER_SIZE 64
Can you keep this define instead of using a "64" below? Give it a proper
prefix/postfix if needed.
> #define CPC_HEADER_SIZE 4
> #define INTR_IN_BUFFER_SIZE 4
>
> @@ -290,6 +289,8 @@ struct ems_usb {
> struct usb_device *udev;
> struct net_device *netdev;
>
> + u32 bulk_rd_buf_size;
> +
> atomic_t active_tx_urbs;
> struct usb_anchor tx_submitted;
> struct ems_tx_urb_context tx_contexts[MAX_TX_URBS];
> @@ -301,7 +302,9 @@ struct ems_usb {
> u8 *tx_msg_buffer;
>
> u8 *intr_in_buffer;
> - unsigned int free_slots; /* remember number of available slots */
> + u32 free_slots; /* remember number of available slots */
nitpick
Why this change?
> +
> + int (*ems_usb_write_mode)(struct ems_usb *dev, u32 mode);
>
> struct ems_cpc_msg active_params; /* active controller parameters */
> };
> @@ -522,7 +525,7 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
>
> resubmit_urb:
> usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 2),
> - urb->transfer_buffer, RX_BUFFER_SIZE,
> + urb->transfer_buffer, dev->bulk_rd_buf_size,
> ems_usb_read_bulk_callback, dev);
>
> retval = usb_submit_urb(urb, GFP_ATOMIC);
> @@ -596,9 +599,18 @@ static int ems_usb_command_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
>
> /* Change CAN controllers' mode register
> */
> -static int ems_usb_write_mode(struct ems_usb *dev, u8 mode)
> +static int ems_usb_write_mode_arm7(struct ems_usb *dev, u32 mode)
> {
> - dev->active_params.msg.can_params.cc_params.sja1000.mode = mode;
> + struct cpc_sja1000_params *sja1000 =
> + &dev->active_params.msg.can_params.cc_params.sja1000;
> +
> + if (mode == CPC_USB_RESET_MODE)
> + sja1000->mode |= SJA1000_MOD_RM;
> + else if (mode == CPC_USB_RUN_MODE)
> + sja1000->mode &= ~SJA1000_MOD_RM;
> +
> + else
> + return -EINVAL;
>
> return ems_usb_command_msg(dev, &dev->active_params);
> }
> @@ -641,7 +653,7 @@ static int ems_usb_start(struct ems_usb *dev)
> break;
> }
>
> - buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
> + buf = usb_alloc_coherent(dev->udev, dev->bulk_rd_buf_size, GFP_KERNEL,
> &urb->transfer_dma);
> if (!buf) {
> netdev_err(netdev, "No memory left for USB buffer\n");
> @@ -651,7 +663,7 @@ static int ems_usb_start(struct ems_usb *dev)
> }
>
> usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 2),
> - buf, RX_BUFFER_SIZE,
> + buf, dev->bulk_rd_buf_size,
> ems_usb_read_bulk_callback, dev);
> urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> usb_anchor_urb(urb, &dev->rx_submitted);
> @@ -659,7 +671,7 @@ static int ems_usb_start(struct ems_usb *dev)
> err = usb_submit_urb(urb, GFP_KERNEL);
> if (err) {
> usb_unanchor_urb(urb);
> - usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
> + usb_free_coherent(dev->udev, dev->bulk_rd_buf_size, buf,
> urb->transfer_dma);
> usb_free_urb(urb);
> break;
> @@ -708,7 +720,7 @@ static int ems_usb_start(struct ems_usb *dev)
> if (err)
> goto failed;
>
> - err = ems_usb_write_mode(dev, SJA1000_MOD_NORMAL);
> + err = dev->can.do_set_mode(netdev, CAN_MODE_START);
> if (err)
> goto failed;
>
> @@ -742,7 +754,7 @@ static int ems_usb_open(struct net_device *netdev)
> struct ems_usb *dev = netdev_priv(netdev);
> int err;
>
> - err = ems_usb_write_mode(dev, SJA1000_MOD_RM);
> + err = dev->ems_usb_write_mode(dev, CPC_USB_RESET_MODE);
> if (err)
> return err;
>
> @@ -900,7 +912,7 @@ static int ems_usb_close(struct net_device *netdev)
> netif_stop_queue(netdev);
>
> /* Set CAN controller to reset mode */
> - if (ems_usb_write_mode(dev, SJA1000_MOD_RM))
> + if (dev->ems_usb_write_mode(dev, CPC_USB_RESET_MODE))
> netdev_warn(netdev, "couldn't stop device");
>
> close_candev(netdev);
> @@ -915,8 +927,8 @@ static const struct net_device_ops ems_usb_netdev_ops = {
> .ndo_change_mtu = can_change_mtu,
> };
>
> -static const struct can_bittiming_const ems_usb_bittiming_const = {
> - .name = "ems_usb",
> +static const struct can_bittiming_const ems_usb_bittiming_const_arm7 = {
> + .name = "ems_usb_arm7",
You are changing the user space visible name of the CAN device. Is this needed?
> .tseg1_min = 1,
> .tseg1_max = 16,
> .tseg2_min = 1,
> @@ -933,7 +945,7 @@ static int ems_usb_set_mode(struct net_device *netdev, enum can_mode mode)
>
> switch (mode) {
> case CAN_MODE_START:
> - if (ems_usb_write_mode(dev, SJA1000_MOD_NORMAL))
> + if (dev->ems_usb_write_mode(dev, CPC_USB_RUN_MODE))
> netdev_warn(netdev, "couldn't start device");
>
> if (netif_queue_stopped(netdev))
> @@ -947,7 +959,7 @@ static int ems_usb_set_mode(struct net_device *netdev, enum can_mode mode)
> return 0;
> }
>
> -static int ems_usb_set_bittiming(struct net_device *netdev)
> +static int ems_usb_set_bittiming_arm7(struct net_device *netdev)
> {
> struct ems_usb *dev = netdev_priv(netdev);
> struct can_bittiming *bt = &dev->can.bittiming;
> @@ -1018,11 +1030,29 @@ static int ems_usb_probe(struct usb_interface *intf,
> dev->netdev = netdev;
>
> dev->can.state = CAN_STATE_STOPPED;
> +
> + /* Select CPC-USB/ARM7 or CPC-USB/FD */
> + switch (dev->udev->descriptor.idProduct) {
> + case USB_CPCUSB_ARM7_PRODUCT_ID:
please indent one level after the case
> +
> dev->can.clock.freq = EMS_USB_ARM7_CLOCK;
> - dev->can.bittiming_const = &ems_usb_bittiming_const;
> - dev->can.do_set_bittiming = ems_usb_set_bittiming;
> + dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
> + dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
> dev->can.do_set_mode = ems_usb_set_mode;
> dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
> + init_params_sja1000(&dev->active_params);
> + dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
> + dev->bulk_rd_buf_size = 64;
> + break;
> +
> + case USB_CPCUSB_FD_PRODUCT_ID:
> + // Placeholder for next patchess
Add this case in the patch where you fill the placeholder.
> + break;
> +
> + default:
> + err = -ENODEV;
> + goto cleanup_candev;
> + }
>
> netdev->netdev_ops = &ems_usb_netdev_ops;
>
> @@ -1053,8 +1083,6 @@ static int ems_usb_probe(struct usb_interface *intf,
>
> SET_NETDEV_DEV(netdev, &intf->dev);
>
> - init_params_sja1000(&dev->active_params);
> -
> err = ems_usb_command_msg(dev, &dev->active_params);
> if (err) {
> netdev_err(netdev, "couldn't initialize controller: %d\n", err);
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: OpenPGP digital signature
