lkml.org 
[lkml]   [2023]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V3] can: usb: f81604: add Fintek F81604 support
From
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

\
 
 \ /
  Last update: 2023-04-10 07:53    [W:0.278 / U:0.540 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site