Messages in this thread | | | Date | Mon, 20 Mar 2023 08:24:58 +0100 | From | Michal Swiatkowski <> | Subject | Re: [PATCH] can: usb: f81604: add Fintek F81604 support |
| |
On Mon, Mar 20, 2023 at 10:59:33AM +0800, Peter Hong wrote: > Hi, > > Michal Swiatkowski 於 2023/3/17 下午 09:15 寫道: > > On Fri, Mar 17, 2023 at 05:33:52PM +0800, Ji-Ze Hong (Peter Hong) wrote: > > > > --- a/drivers/net/can/usb/Kconfig > > +++ b/drivers/net/can/usb/Kconfig > > @@ -147,4 +147,13 @@ config CAN_UCAN > > from Theobroma Systems like the A31-ÂľQ7 and the RK3399-Q7 > > (https://www.theobroma-systems.com/rk3399-q7) > > Hi, > > > > I am not familiar with CAN, so only style review :) > > Thanks for your reviews :D > > + > > + if (status) { > > + dev_err(&dev->dev, "%s: reg: %x data: %x failed: %d\n", > > + __func__, reg, data, status); > > + } > > The { and } aren't needed as inside if is only one line. > > Could I remove the { and } when the logical line to split multi-line ? >
Yes You can, and You should :)
> > > +static int f81604_set_normal_mode(struct net_device *netdev) > > > +{ > > > + struct f81604_port_priv *priv = netdev_priv(netdev); > > > + int status, i; > > > + u8 mod_reg_val = 0x00; > > RCT, mod_reg should be one line above > > What mean about "RCT"? > > Is this section should change to above like ?? > > u8 mod_reg_val; > ... > > mod_reg_val = 0;
reverse christmas tree, it is about how variable definition should look like. In Your case:
struct f81604_port_priv *priv = netdev_priv(netdev); u8 mod_reg_val = 0x00; int status, i;
instead of
struct f81604_port_priv *priv = netdev_priv(netdev); int status, i; u8 mod_reg_val = 0x00;
Cosmetic Linux style rule
> > > +static int f81604_register_urbs(struct net_device *netdev) > > > +{ > > > + struct f81604_port_priv *priv = netdev_priv(netdev); > > > + int status, i; > > > + > > > + for (i = 0; i < F81604_MAX_RX_URBS; ++i) { > > > + status = usb_submit_urb(priv->read_urb[i], GFP_KERNEL); > > > + if (status) { > > > + netdev_warn(netdev, "%s: submit rx urb failed: %d\n", > > > + __func__, status); > > > + return status; > > Don't know usb subsytem, but shouldn't previously submitted urb be > > killed? > > Yes, I had made kill operations in > f81604_start() > -> f81604_unregister_urbs() >
Ok, thanks
> > > +static void f81604_process_rx_packet(struct urb *urb) > > > +{ > > > + struct net_device_stats *stats; > > > + struct net_device *netdev; > > > + struct can_frame *cf; > > > + struct sk_buff *skb; > > > + u8 *data; > > > + u8 *ptr; > > > + int i; > > > + int count; > > RCT > > > > > + > > > + netdev = urb->context; > > > + stats = &netdev->stats; > > > + data = urb->transfer_buffer; > > netdev and data can be set in declaration > > why only netdev & data ?? Could I set netdev, stats & data in declaration ? >
You can, but it will be hard to still have declaration as RCT (netdev declaration have to be before stats declaration).
> > > > +/* Called by the usb core when driver is unloaded or device is removed */ > > > +static void f81604_disconnect(struct usb_interface *intf) > > > +{ > > > + struct f81604_priv *priv = usb_get_intfdata(intf); > > > + int i; > > > + > > > + for (i = 0; i < F81604_MAX_DEV; ++i) { > > > + if (!priv->netdev[i]) > > > + continue; > > > + > > > + unregister_netdev(priv->netdev[i]); > > > + free_candev(priv->netdev[i]); > > > + } > > What about closing USB device? It is called brefore disconnect or it > > should be done here? > > When candev close in f81604_close(), It will call f81604_set_reset_mode() to > make candev to reset mode. >
Understand, thanks
> Thanks
| |