Messages in this thread | | | Date | Mon, 10 Apr 2023 13:50:30 +0800 | Subject | Re: [PATCH V3] can: usb: f81604: add Fintek F81604 support | From | Peter Hong <> |
| |
Hi Vincent,
Vincent MAILHOL 於 2023/3/30 下午 09:11 寫道: > Hmm, I am still not a fan of setting a mutex for a single concurrency > issue which can only happen during probing. > > What about this: > > static int __f81604_set_termination(struct net_device *netdev, u16 term) > { > struct f81604_port_priv *port_priv = netdev_priv(netdev); > u8 mask, data = 0; > > if (netdev->dev_id == 0) > mask = F81604_CAN0_TERM; > else > mask = F81604_CAN1_TERM; > > if (term == F81604_TERMINATION_ENABLED) > data = mask; > > return f81604_mask_set_register(port_priv->dev, F81604_TERMINATOR_REG, > mask, data); > } > > static int f81604_set_termination(struct net_device *netdev, u16 term) > { > ASSERT_RTNL(); > > return __f81604_set_termination(struct net_device *netdev, u16 term); > } > > static int f81604_init_termination(struct f81604_priv *priv) > { > int i, ret; > > for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) { > ret = __f81604_set_termination(f81604_priv->netdev[i], > F81604_TERMINATION_DISABLED); > if (ret) > return ret; > } > } > > static int f81604_probe(struct usb_interface *intf, > const struct usb_device_id *id) > { > /* ... */ > > err = f81604_init_termination(priv); > if (err) > goto failure_cleanup; > > for (i = 0; i < ARRAY_SIZE(f81604_priv->netdev); i++) { > /* ... */ > } > > /* ... */ > } > > Initialise all resistors with __f81604_set_termination() in probe() > before registering any network device. Use f81604_set_termination() > which has the lock assert elsewhere.
The f81604_set_termination() will transform into the following code:
static int f81604_write(struct usb_device *dev, u16 reg, u8 data); static int f81604_read(struct usb_device *dev, u16 reg, u8 *data); static int f81604_update_bits(struct usb_device *dev, u16 reg, u8 mask, u8 data);
static int __f81604_set_termination(struct usb_device *dev, int idx, u16 term) { u8 mask, data = 0;
if (idx == 0) mask = F81604_CAN0_TERM; else mask = F81604_CAN1_TERM;
if (term) data = mask;
return f81604_update_bits(dev, F81604_TERMINATOR_REG, mask, data); }
static int f81604_set_termination(struct net_device *netdev, u16 term) { struct f81604_port_priv *port_priv = netdev_priv(netdev); struct f81604_priv *priv;
ASSERT_RTNL();
priv = usb_get_intfdata(port_priv->intf);
return __f81604_set_termination(port_priv->dev, netdev->dev_id, term); }
and also due to f81604_write() / f81604_read() / f81604_update_bits() may use in f81604_probe() without port private data, so we'll change their first parameter from "struct f81604_port_priv *priv" to "struct usb_device *dev". Is it OK ?
> Also, looking at your probe() function, in label clean_candev:, if the > second can channel fails its initialization, you do not clean the > first can channel. I suggest adding a f81604_init_netdev() and > handling the netdev issue and cleanup in that function.
When the second can channel failed its initialization, the label "clean_candev" will clear second "netdev" object and the first "netdev" will cleanup in f81604_disconnect().
Could I remain this section of code ?
Thanks
| |