| Message ID | 1493495283-55306-1-git-send-email-Bogdan-Stefan_mirea@mentor.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 04/29/2017 09:48 PM, Bogdan Mirea wrote: > Added rcar_can_set_bittiming as can.do_set_bittiming callback that will > be called from can_changelink generic callback when link state is > changed. > > This enables set bittiming support: > ip link set can0 type can bitrate 500000 triple-sampling on Why is this needed? rcar_can_set_bittiming() is called during rcar_can_open(). Marc
Hello Marc, > On 04/29/2017 09:48 PM, Bogdan Mirea wrote: > > Added rcar_can_set_bittiming as can.do_set_bittiming callback that > > will be called from can_changelink generic callback when link state > > is > > changed. > > > > This enables set bittiming support: > > ip link set can0 type can bitrate 500000 triple-sampling on > > Why is this needed? rcar_can_set_bittiming() is called during > rcar_can_open(). I know that rcar_can_set_bittiming() is called during rcar_can_open() and setting bittiming works fine when setting can up with a command like: (1st approah) $ip link set can0 up type can bitrate 500000 But most of tutorials online[1] are presenting as a TODO the following pair of commands: (2nd approah) $ip link set can0 type can bitrate 500000 $ip link set up can0 And in this 2nd approah, the first command will fail since it will call can_changelink which on its turn will try calling .do_set_bittiming callback and the former callback is not defined for rcar_can. I don't say this is a must-have feature, since calling iproute2 with "set bitrate" and "set can up" as in 1st approach will work just fine, will work just fine, but it is a nice to have feature for the second approach of setting bitrate. Also peak_usb pcan[2] uses this second approach (the changelink one) and this patch updates rcar_can for it. Best Regards, Bogdan [1] http://elinux.org/Bringing_CAN_interface_up [2] http://lxr.free-electrons.com/source/drivers/net/can/usb/peak_usb/pcan_usb.c#L900
On 04/30/2017 01:33 PM, Mirea, Bogdan-Stefan wrote: > Hello Marc, > >> On 04/29/2017 09:48 PM, Bogdan Mirea wrote: >>> Added rcar_can_set_bittiming as can.do_set_bittiming callback that >>> will be called from can_changelink generic callback when link state >>> is >>> changed. >>> >>> This enables set bittiming support: >>> ip link set can0 type can bitrate 500000 triple-sampling on >> >> Why is this needed? rcar_can_set_bittiming() is called during >> rcar_can_open(). > > I know that rcar_can_set_bittiming() is called during rcar_can_open() > and setting bittiming works fine when setting can up with a command > like: > (1st approah) > $ip link set can0 up type can bitrate 500000 > > But most of tutorials online[1] are presenting as a TODO the following > pair of commands: > (2nd approah) > $ip link set can0 type can bitrate 500000 > $ip link set up can0 > > And in this 2nd approah, the first command will fail since it will call > can_changelink which on its turn will try calling .do_set_bittiming > callback and the former callback is not defined for rcar_can. Why should it fail? > memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt)); > err = can_get_bittiming(dev, &bt, > priv->bittiming_const, > priv->bitrate_const, > priv->bitrate_const_cnt); > if (err) > return err; > memcpy(&priv->bittiming, &bt, sizeof(bt)); Here the bittiming settings are copied to priv->bittiming > > if (priv->do_set_bittiming) { > /* Finally, set the bit-timing registers */ > err = priv->do_set_bittiming(dev); > if (err) > return err; > } and it will only call the set_bittiming if it's defined. > I don't say this is a must-have feature, since calling iproute2 with > "set bitrate" and "set can up" as in 1st approach will work just fine, > will work just fine, but it is a nice to have feature for the second > approach of setting bitrate. > Also peak_usb pcan[2] uses this second approach (the changelink one) and > this patch updates rcar_can for it. Sorry I still don't get why this is necessary. Marc
On April 30, 2017 10:31 PM Marc Kleine-Budde wrote: > > memcpy(&priv->bittiming, &bt, sizeof(bt)); > > Here the bittiming settings are copied to priv->bittiming Yes Marc, you are right, I don't know how did I noticed an error which it isn't since the bittiming structure is filled here, in can_changelink function... My bad. Best Regards, Bogdan
diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c index 788459f..cfabb95 100644 --- a/drivers/net/can/rcar/rcar_can.c +++ b/drivers/net/can/rcar/rcar_can.c @@ -419,7 +419,7 @@ static irqreturn_t rcar_can_interrupt(int irq, void *dev_id) return IRQ_HANDLED; } -static void rcar_can_set_bittiming(struct net_device *dev) +static int rcar_can_set_bittiming(struct net_device *dev) { struct rcar_can_priv *priv = netdev_priv(dev); struct can_bittiming *bt = &priv->can.bittiming; @@ -433,6 +433,8 @@ static void rcar_can_set_bittiming(struct net_device *dev) * read/write (but not on 8-bit, contrary to the manuals)... */ writel((bcr << 8) | priv->clock_select, &priv->regs->bcr); + + return 0; } static void rcar_can_start(struct net_device *ndev) @@ -809,6 +811,7 @@ static int rcar_can_probe(struct platform_device *pdev) priv->clock_select = clock_select; priv->can.clock.freq = clk_get_rate(priv->can_clk); priv->can.bittiming_const = &rcar_can_bittiming_const; + priv->can.do_set_bittiming = &rcar_can_set_bittiming; priv->can.do_set_mode = rcar_can_do_set_mode; priv->can.do_get_berr_counter = rcar_can_get_berr_counter; priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;