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
