On 22.05.2023 21:53:54, Peter Seiderer wrote:
> > > +static int ixxat_usb_init_ctrl(struct ixxat_usb_device *dev)
> > > +{
> > > + const struct can_bittiming *bt = &dev->can.bittiming;
> > > + const u16 port = dev->ctrl_index;
> > > + int err;
> > > + struct ixxat_usb_init_cl1_cmd *cmd;
> > > + const u32 rcv_size = sizeof(cmd->res);
> > > + const u32 snd_size = sizeof(*cmd);
> >
> > Remove those size variables and directly use sizeof(cmd->res) and
> > sizeof(*cmd) in the code.
>
> O.k.
>
> >
> > > + u8 opmode = IXXAT_USB_OPMODE_EXTENDED | IXXAT_USB_OPMODE_STANDARD;
> > > + u8 btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
> >
> > Add a macro definition for the 0x3f and 0x3 masks using GENMASK() and
> > then calculate the value using FIELD_PREP().
>
> O.k., define names o.k.?
ACK
> +#define IXXAT_USB_CAN_BTR0_BRP_MASK GENMASK(5, 0)
> +#define IXXAT_USB_CAN_BTR0_SJW_MASK GENMASK(7, 6)
Now use:
btr0 = FIELD_PREP(IXXAT_USB_CAN_BTR0_BRP_MASK, bt->brp - 1) |
FIELD_PREP(IXXAT_USB_CAN_BTR0_SJW_MASK, bt->sjw - 1);
[...]
> > > +static void ixxat_usb_update_ts_now(struct ixxat_usb_device *dev, u32 ts_now)
> > > +{
> > > + u32 *ts_dev = &dev->time_ref.ts_dev_0;
> > > + ktime_t *kt_host = &dev->time_ref.kt_host_0;
> > > + u64 timebase = (u64)0x00000000FFFFFFFF - (u64)(*ts_dev) + (u64)ts_now;
> >
> > Replace 0x00000000FFFFFFFF by U32_MAX from linux/limits.h
>
> O.k.
Can you read the free running timer register a USB transfer? If so, it's
better to use the cyclecounter/timecounter framework.
[...]
> > > +static void ixxat_usb_decode_buf(struct urb *urb)
> > > +{
> > > + struct ixxat_usb_device *dev = urb->context;
> > > + struct net_device *netdev = dev->netdev;
> > > + struct ixxat_can_msg *can_msg;
> > > + int err = 0;
> > > + u32 pos = 0;
> > > + u8 *data = urb->transfer_buffer;
> > > +
> > > + while (pos < urb->actual_length) {
> > > + u32 time;
> > > + u8 size;
> > > + u8 type;
> > > +
> > > + can_msg = (struct ixxat_can_msg *)&data[pos];
> > > + if (!can_msg || !can_msg->base.size) {
> > > + err = -ENOTSUPP;
>
> checkpatch.pl warns about this one:
>
> WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> #1045: FILE: drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c:644:
> + err = -ENOTSUPP;
>
> Is EOPNOTSUPP appropriate (or EBADMSG as below)?
EBADMSG sounds good.
[...]
> > > +static void ixxat_usb_read_bulk_callback(struct urb *urb)
> > > +{
> > > + struct ixxat_usb_device *dev = urb->context;
> > > + const struct ixxat_usb_adapter *adapter = dev->adapter;
> > > + struct net_device *netdev = dev->netdev;
> > > + struct usb_device *udev = dev->udev;
> > > + int err;
> > > +
> > > + if (!netif_device_present(netdev))
> > > + return;
> > > +
> > > + switch (urb->status) {
> > > + case 0: /* success */
> > > + break;
> > > + case -EPROTO:
> > > + case -EILSEQ:
> > > + case -ENOENT:
> > > + case -ECONNRESET:
> > > + case -ESHUTDOWN:
> > > + return;
> > > + default:
> > > + netdev_err(netdev, "Rx urb aborted /(%d)\n", urb->status);
> >
> > Do not use parenthesis in log messages.
> >
> > Ref: https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages
> >
> > Printing numbers in parentheses (%d) adds no value and should be avoided.
> >
>
> O.k., seems a common pattern, at least the same logging used in peak_usb,
> change it there as well (but seems you answered this in another thread
> already '...but bonus points if you send a clean-up patch to fix
> the existing log messages...')?
IIRC the %pe is quite new compared to the peak_usb driver. Code evolves
and new code should not be based on outdated examples/drivers.
[...]
> > > +/**
> > > + * struct ixxat_canbtp Bittiming parameters (CL2)
> > > + * @mode: Operation mode
> > > + * @bps: Bits per second
> > > + * @ts1: First time segment
> > > + * @ts2: Second time segment
> > > + * @sjw: Synchronization jump width
> > > + * @tdo: Transmitter delay offset
> > > + *
> > > + * Bittiming parameters of a CL2 initialization request
> > > + */
> > > +struct ixxat_canbtp {
> > > + __le32 mode;
> > > + __le32 bps;
> > > + __le16 ts1;
> > > + __le16 ts2;
> > > + __le16 sjw;
> > > + __le16 tdo;
> >
> > It seems that your device supports TDC. What is the reason to not configure it?
> >
> > Please have a look at struct can_tdc:
> >
> > https://elixir.bootlin.com/linux/v6.2/source/include/linux/can/bittiming.h#L21
> >
> > Please refer to this patch if you want an example of how to use TDC:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1010a8fa9608
> >
>
> Not sure about this one (and I have limited knowledge of the hardware details),
> seems I need to find more (spare) time to look into this one, or maybe
> something better for an follow-up patch?
We usually need TDC for bit rates > 1..2 MBit/s
ACK.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Attachment:
signature.asc
Description: PGP signature
