Adding new CAN driver

Adding new CAN driver

[Kołłątaj, Remigiusz]

Hello,

I have created SocketCAN driver for Michrochip CAN BUS Analyzer
(details: https://github.com/rkollataj/mcba_usb). I would like to
upstream it, but as I've never done it before I want to make sure I
will take proper steps.

My understanding is that I should merge my changes with
linux-can-next/master, generate patch and send it to this list. Is
that right?


regards,
Remik
--

Re: Adding new CAN driver

[Oliver Hartkopp]

Hello Remik,

On 10/20/2016 11:11 AM, Kołłątaj, Remigiusz wrote:
> Hello,
>
> I have created SocketCAN driver for Michrochip CAN BUS Analyzer
> (details: https://github.com/rkollataj/mcba_usb). I would like to
> upstream it, but as I've never done it before I want to make sure I
> will take proper steps.
>
> My understanding is that I should merge my changes with
> linux-can-next/master, generate patch and send it to this list. Is
> that right?

Yes :-)

'git send-email' is easy to use and the best way to send stuff to the 
mailing list.

I was just looking through your code and wondered whether we should 
support the termination switching feature via 'ip' tool or via sysfs.

Just added Marc in CC.

Regards,
Oliver

Re: Adding new CAN driver

[Kołłątaj, Remigiusz]

Thanks for you answer Oliver :)

On 21 October 2016 at 18:37, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hello Remik,
>
> On 10/20/2016 11:11 AM, Kołłątaj, Remigiusz wrote:
>>
>> Hello,
>>
>> I have created SocketCAN driver for Michrochip CAN BUS Analyzer
>> (details: https://github.com/rkollataj/mcba_usb). I would like to
>> upstream it, but as I've never done it before I want to make sure I
>> will take proper steps.
>>
>> My understanding is that I should merge my changes with
>> linux-can-next/master, generate patch and send it to this list. Is
>> that right?
>
>
> Yes :-)
>
> 'git send-email' is easy to use and the best way to send stuff to the
> mailing list.
>
> I was just looking through your code and wondered whether we should support
> the termination switching feature via 'ip' tool or via sysfs.
>
> Just added Marc in CC.
>

I tried to use 'ip' for setting up termination, but I couldn't find a
way. If this is possible I will modify the driver as it seems to be
better idea. Could you give me some hints?

> Regards,
> Oliver
>

One more question about the structure of the driver. I have split it
into source and header files as it was easier to run some tests. I do
not expect that the header will be useful for other drivers. Shall I
merge it with source file in final version?

Regards,
Remik

Re: Adding new CAN driver

[Oliver Hartkopp]

Hi Remik,

On 10/21/2016 08:47 PM, Kołłątaj, Remigiusz wrote:
>> I was just looking through your code and wondered whether we should support
>> the termination switching feature via 'ip' tool or via sysfs.
>>
>> Just added Marc in CC.
>>
>
> I tried to use 'ip' for setting up termination, but I couldn't find a
> way. If this is possible I will modify the driver as it seems to be
> better idea. Could you give me some hints?

No - not now :-)

That's why I wanted to start the discussion whether we should add this 
kind of feature to 'ip'.

> One more question about the structure of the driver. I have split it
> into source and header files as it was easier to run some tests. I do
> not expect that the header will be useful for other drivers. Shall I
> merge it with source file in final version?

Yes.

Your patch should contain the changes for Kconfig, Makefile and the 
creation of the mcba_usb.c file.

Regards,
Oliver

Re: Adding new CAN driver

[Kołłątaj, Remigiusz]

On 22 October 2016 at 13:57, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Remik,
>
> On 10/21/2016 08:47 PM, Kołłątaj, Remigiusz wrote:
>>>
>>> I was just looking through your code and wondered whether we should
>>> support
>>> the termination switching feature via 'ip' tool or via sysfs.
>>>
>>> Just added Marc in CC.
>>>
>>
>> I tried to use 'ip' for setting up termination, but I couldn't find a
>> way. If this is possible I will modify the driver as it seems to be
>> better idea. Could you give me some hints?
>
>
> No - not now :-)
>
> That's why I wanted to start the discussion whether we should add this kind
> of feature to 'ip'.

I have misunderstood, sorry. I know it's not democracy, but I vote for
"yes" :) Having termination as a part of "ip" would be more
straightforward. Putting it to sysfs is more like hidden/secret
feature. On the other hand I do not know how many CAN devices has
possibility to configure built-in termination.

If you will find this feature reasonable enough I could probably spend
some effort to work on it.

>
>> One more question about the structure of the driver. I have split it
>> into source and header files as it was easier to run some tests. I do
>> not expect that the header will be useful for other drivers. Shall I
>> merge it with source file in final version?
>
>
> Yes.
>
> Your patch should contain the changes for Kconfig, Makefile and the creation
> of the mcba_usb.c file.
>
> Regards,
> Oliver

Regards,
Remik

Re: Adding new CAN driver

[Uwe Bonnes]

>>>>> "Kołłątaj," == Kołłątaj, Remigiusz <remigiusz.kollataj@mobica.com> writes:

    Kołłątaj,> If you will find this feature reasonable enough I could
    Kołłątaj,> probably spend some effort to work on it.

Any news on adding the Microchip CAN driver?

Thanks
-- 
Uwe Bonnes                bon@elektron.ikp.physik.tu-darmstadt.de

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 1623569 ------- Fax. 06151 1623305 ---------

Re: Adding new CAN driver

[Kołłątaj, Remigiusz]

Hi Uwe,

Not sure if you are asking about termination or about the driver
itself, so let me give two answers.

I'll try to send the patch till end of this week. In the meantime you
may still use the driver from the github repository
(https://github.com/rkollataj/mcba_usb). I am keeping it up to date.

I didn't get a chance to work on adding support for termination to
"ip". Before investing any effort I would prefer to get confirmation
from Oliver of Marc that the direction is right. Anyway setting up
termination via sysfs works just fine. It is described in readme file
in repository.

Regards,
Remik


On 1 November 2016 at 10:48, Uwe Bonnes
<bon@elektron.ikp.physik.tu-darmstadt.de> wrote:
>>>>>> "Kołłątaj," == Kołłątaj, Remigiusz <remigiusz.kollataj@mobica.com> writes:
>
>     Kołłątaj,> If you will find this feature reasonable enough I could
>     Kołłątaj,> probably spend some effort to work on it.
>
> Any news on adding the Microchip CAN driver?
>
> Thanks
> --
> Uwe Bonnes                bon@elektron.ikp.physik.tu-darmstadt.de
>
> Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
> --------- Tel. 06151 1623569 ------- Fax. 06151 1623305 ---------



-- 
________________________

Remigiusz Kołłątaj

Mobica Ltd

Software Consultant, CTO Team

Tel:  +48 91 881 95 18

Skype: remigiusz.kollataj

www.mobica.com


Mobica is a provider of software engineering, testing and consultancy
services based in the UK, Poland and the USA, with a worldwide
customer base. We have a proven track record in delivering innovative
solutions to some of the world’s best-known companies in a range of
sectors including Automotive, Mobile, Semiconductor, Finance, TV,
Marine and Aviation.

Re: Adding new CAN driver

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1065 bytes --]

On 11/02/2016 08:23 AM, Kołłątaj, Remigiusz wrote:
> Hi Uwe,
> 
> Not sure if you are asking about termination or about the driver
> itself, so let me give two answers.
> 
> I'll try to send the patch till end of this week. In the meantime you
> may still use the driver from the github repository
> (https://github.com/rkollataj/mcba_usb). I am keeping it up to date.
> 
> I didn't get a chance to work on adding support for termination to
> "ip". Before investing any effort I would prefer to get confirmation
> from Oliver of Marc that the direction is right. Anyway setting up
> termination via sysfs works just fine. It is described in readme file
> in repository.

I'd like to see a netlink interface for this. Oliver what about adding
it to CTRL_MODE?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: Adding new CAN driver

[Oliver Hartkopp]

On 11/02/2016 09:48 AM, Marc Kleine-Budde wrote:
> On 11/02/2016 08:23 AM, Kołłątaj, Remigiusz wrote:

>> I didn't get a chance to work on adding support for termination to
>> "ip". Before investing any effort I would prefer to get confirmation
>> from Oliver of Marc that the direction is right. Anyway setting up
>> termination via sysfs works just fine. It is described in readme file
>> in repository.
>
> I'd like to see a netlink interface for this. Oliver what about adding
> it to CTRL_MODE?

I would suggest to create some kind of TRX_MODE configuration instead.

The CTRL_MODE is CAN controller related (layer 2 / bitstream) while the 
transceiver is layer 1.

When creating an interface for transceivers we need to take care about 
the different power saving modes /and/ the termination.

E.g. when you have a wake-up capable transceiver you may power-down the 
entire machine when you put the TRX into sleep mode (and to wait for CAN 
traffic to switch on the power supply again).

Additionally some transceivers have the capability to switch on the 
power supply only when a specific CAN ID is seen on the CAN bus 
(selective wake-up). In fact these transceivers contain a reduced CAN 
controller with on-board clock generation to perform this kind of 
functionality.

So when we create a new netlink interface for that purpose we need to 
take care to provide a proper API.

Best regards,
Oliver

RE: Adding new CAN driver

[Ramesh Shanmugasundaram]

> On 11/02/2016 08:23 AM, Kołłątaj, Remigiusz wrote:
> > Hi Uwe,
> >
> > Not sure if you are asking about termination or about the driver
> > itself, so let me give two answers.
> >
> > I'll try to send the patch till end of this week. In the meantime you
> > may still use the driver from the github repository
> > (https://github.com/rkollataj/mcba_usb). I am keeping it up to date.
> >
> > I didn't get a chance to work on adding support for termination to
> > "ip". Before investing any effort I would prefer to get confirmation
> > from Oliver of Marc that the direction is right. Anyway setting up
> > termination via sysfs works just fine. It is described in readme file
> > in repository.
> 
> I'd like to see a netlink interface for this. Oliver what about adding it
> to CTRL_MODE?

Is it about the CAN bus termination? Why the driver need to be aware of it?
If it needed for some reason shouldn't it be a device tree property of that device/bridge node?

Thanks,
Ramesh

Re: Adding new CAN driver

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 909 bytes --]

On 11/02/2016 02:38 PM, Ramesh Shanmugasundaram wrote:
>> I'd like to see a netlink interface for this. Oliver what about adding it
>> to CTRL_MODE?
> 
> Is it about the CAN bus termination?

Yes.

> Why the driver need to be aware of it?

If the termination is part of the CAN interface, someone has to
configure it, this is typically done by the driver.

> If it needed for some reason shouldn't it be a device tree property
> of that device/bridge node?

No - you want to be able to configure it in the system without needing
to change the DT. The DT is used to descrive the hardware, not configure it.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: Adding new CAN driver

[Alexander Stein]

On Wednesday 02 November 2016 13:38:22, Ramesh Shanmugasundaram wrote:
> > > I didn't get a chance to work on adding support for termination to
> > > "ip". Before investing any effort I would prefer to get confirmation
> > > from Oliver of Marc that the direction is right. Anyway setting up
> > > termination via sysfs works just fine. It is described in readme file
> > > in repository.
> > 
> > 
> > I'd like to see a netlink interface for this. Oliver what about adding it
> > to CTRL_MODE?
> 
> 
> Is it about the CAN bus termination? Why the driver need to be aware of it?
> If it needed for some reason shouldn't it be a device tree property of that
> device/bridge node? 

I would consider this as a feature, which has to be enabled or disabled on 
demand dynamically, depending on current physical connection, especially 
regarding USB devices. In thise cases there is also no device tree applicable.

Best regards,
Alexander

RE: Adding new CAN driver

[Ramesh Shanmugasundaram]

> > > > I didn't get a chance to work on adding support for termination to
> > > > "ip". Before investing any effort I would prefer to get
> > > > confirmation from Oliver of Marc that the direction is right.
> > > > Anyway setting up termination via sysfs works just fine. It is
> > > > described in readme file in repository.
> > >
> > >
> > > I'd like to see a netlink interface for this. Oliver what about
> > > adding it to CTRL_MODE?
> >
> >
> > Is it about the CAN bus termination? Why the driver need to be aware of
> it?
> > If it needed for some reason shouldn't it be a device tree property of
> > that device/bridge node?
> 
> I would consider this as a feature, which has to be enabled or disabled on
> demand dynamically, depending on current physical connection, especially
> regarding USB devices. In thise cases there is also no device tree
> applicable.

OK. My question was on the lines - "Is the termination information is needed on a register of the CAN device that the driver manages?" There are cases where the termination is provided by the D-sub connector. The controller should just see bus error when improperly terminated. I don't know about USB based devices and please forgive me if this is the norm.

But this is physical layer information as Oliver mentioned. The CAN transceiver may even require some start-up state that may be enabled by setting couple of GPIOs (as on my board). Yes, it is part of the CAN interface configuration but it not part of the CAN controller driver. I think we could map it to Ethernet MAC & PHY case - isn't it? 

Thanks,
Ramesh

Re: Adding new CAN driver

[Alexander Stein]

On Wednesday 02 November 2016 15:49:50, Ramesh Shanmugasundaram wrote:
> > > > > I didn't get a chance to work on adding support for termination to
> > > > > "ip". Before investing any effort I would prefer to get
> > > > > confirmation from Oliver of Marc that the direction is right.
> > > > > Anyway setting up termination via sysfs works just fine. It is
> > > > > described in readme file in repository.
> > > > 
> > > > I'd like to see a netlink interface for this. Oliver what about
> > > > adding it to CTRL_MODE?
> > > 
> > > Is it about the CAN bus termination? Why the driver need to be aware of
> > 
> > it?
> > 
> > > If it needed for some reason shouldn't it be a device tree property of
> > > that device/bridge node?
> > 
> > I would consider this as a feature, which has to be enabled or disabled on
> > demand dynamically, depending on current physical connection, especially
> > regarding USB devices. In thise cases there is also no device tree
> > applicable.
> 
> OK. My question was on the lines - "Is the termination information is needed
> on a register of the CAN device that the driver manages?" There are cases
> where the termination is provided by the D-sub connector. The controller
> should just see bus error when improperly terminated. I don't know about
> USB based devices and please forgive me if this is the norm.

No problem, I just wanted to point out that device tree is not suitable in 
this case.

> But this is physical layer information as Oliver mentioned. The CAN
> transceiver may even require some start-up state that may be enabled by
> setting couple of GPIOs (as on my board). Yes, it is part of the CAN
> interface configuration but it not part of the CAN controller driver. I
> think we could map it to Ethernet MAC & PHY case - isn't it?

The transceiver might be managed by regulators as done by e.g. flexcan. This 
is part of the hardware description, so device tree here is fine.
But in my opinion bus termination is an end user configuration which might 
change rather often than not. It might be independent of the controller driver 
as e.g. in flexcan, but it might be part of the device as in USB hardware 
which requires some specific command to change bus termination.
IMHO it makes no sense to split an USB interface driver into two parts using 
the same interface in the end.

Best regards,
Alexander

Re: Adding new CAN driver

[Kołłątaj, Remigiusz]

On 2 November 2016 at 17:08, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:
> On Wednesday 02 November 2016 15:49:50, Ramesh Shanmugasundaram wrote:
>> > > > > I didn't get a chance to work on adding support for termination to
>> > > > > "ip". Before investing any effort I would prefer to get
>> > > > > confirmation from Oliver of Marc that the direction is right.
>> > > > > Anyway setting up termination via sysfs works just fine. It is
>> > > > > described in readme file in repository.
>> > > >
>> > > > I'd like to see a netlink interface for this. Oliver what about
>> > > > adding it to CTRL_MODE?
>> > >
>> > > Is it about the CAN bus termination? Why the driver need to be aware of
>> >
>> > it?
>> >
>> > > If it needed for some reason shouldn't it be a device tree property of
>> > > that device/bridge node?
>> >
>> > I would consider this as a feature, which has to be enabled or disabled on
>> > demand dynamically, depending on current physical connection, especially
>> > regarding USB devices. In thise cases there is also no device tree
>> > applicable.
>>
>> OK. My question was on the lines - "Is the termination information is needed
>> on a register of the CAN device that the driver manages?" There are cases
>> where the termination is provided by the D-sub connector. The controller
>> should just see bus error when improperly terminated. I don't know about
>> USB based devices and please forgive me if this is the norm.
>
> No problem, I just wanted to point out that device tree is not suitable in
> this case.
>
>> But this is physical layer information as Oliver mentioned. The CAN
>> transceiver may even require some start-up state that may be enabled by
>> setting couple of GPIOs (as on my board). Yes, it is part of the CAN
>> interface configuration but it not part of the CAN controller driver. I
>> think we could map it to Ethernet MAC & PHY case - isn't it?
>
> The transceiver might be managed by regulators as done by e.g. flexcan. This
> is part of the hardware description, so device tree here is fine.
> But in my opinion bus termination is an end user configuration which might
> change rather often than not. It might be independent of the controller driver
> as e.g. in flexcan, but it might be part of the device as in USB hardware
> which requires some specific command to change bus termination.
> IMHO it makes no sense to split an USB interface driver into two parts using
> the same interface in the end.

I agree that termination is a physical layer setting but to be
specific in case of Microchip's CAN BUS it is neither related to CAN
controller nor transceiver. It is rather device specific - termination
settings are controlled via GPIO pin that just attaches/deattaches
resistor. You can find schematics here:
http://ww1.microchip.com/downloads/en/DeviceDoc/51848B.pdf (CAN RES
line).

I can see that the topic is not as straight forward as I thought in
the beginning. I don't even know if termination is configurable in
different CAN devices. As far as I recall Vector for example provides
physical DB9 gender changer with built-in terminator. Maybe we should
treat Microchip as "special case" and leave termination settings in
sysfs?

>
> Best regards,
> Alexander

Regards,
Remik

Re: Adding new CAN driver

[Oliver Hartkopp]

On 11/02/2016 07:03 PM, Kołłątaj, Remigiusz wrote:
> On 2 November 2016 at 17:08, Alexander Stein
> <alexander.stein@systec-electronic.com> wrote:
>> On Wednesday 02 November 2016 15:49:50, Ramesh Shanmugasundaram wrote:

>>> But this is physical layer information as Oliver mentioned. The CAN
>>> transceiver may even require some start-up state that may be enabled by
>>> setting couple of GPIOs (as on my board). Yes, it is part of the CAN
>>> interface configuration but it not part of the CAN controller driver. I
>>> think we could map it to Ethernet MAC & PHY case - isn't it?

IIRC there's some mii-tool to configure, view & manipulate 
media-independent interface status:

http://man7.org/linux/man-pages/man8/mii-tool.8.html

At least the ethernet people found a way to configure their transceiver 
part with this tool.

>> The transceiver might be managed by regulators as done by e.g. flexcan. This
>> is part of the hardware description, so device tree here is fine.
>> But in my opinion bus termination is an end user configuration which might
>> change rather often than not. It might be independent of the controller driver
>> as e.g. in flexcan, but it might be part of the device as in USB hardware
>> which requires some specific command to change bus termination.
>> IMHO it makes no sense to split an USB interface driver into two parts using
>> the same interface in the end.

ack.

> I agree that termination is a physical layer setting but to be
> specific in case of Microchip's CAN BUS it is neither related to CAN
> controller nor transceiver.

So what about creating TWO new netlink configuration options:

diff --git a/include/uapi/linux/can/netlink.h 
b/include/uapi/linux/can/netlink.h
index 94ffe0c..8d7c4b9 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -127,6 +127,8 @@ enum {
         IFLA_CAN_BERR_COUNTER,
         IFLA_CAN_DATA_BITTIMING,
         IFLA_CAN_DATA_BITTIMING_CONST,
+       IFLA_CAN_TRX_CTRL,
+       IFLA_CAN_TERMINATION,
         __IFLA_CAN_MAX
  };

Where IFLA_CAN_TRX_CTRL provides an interface to configure the 
transceiver and IFLA_CAN_TERMINATION to configure the termination.

But for IFLA_CAN_TERMINATION we need some possibility to configure 
either a set of supported termination resistors (e.g. disabled, 60 Ohms, 
120 Ohms, xxx Ohms, ...) or a free termination value (e.g. 1 Ohm - 10 
kOhms) following the hardware capabilities of the CAN device.
Therefore a list of values and ranges has to be made available by the 
netlink interface before userspace can select some of them.

Both IFLA_CAN_TRX_CTRL and IFLA_CAN_TERMINATION would only be supported 
if the CAN device can handle them. Probably a device tree decription 
defines a fixed configuration which can be read but not modified.

Any ideas/comments on this?

Regards,
Oliver

Re: Adding new CAN driver

[Kurt Van Dijck]

> 
> On 11/02/2016 07:03 PM, Kołłątaj, Remigiusz wrote:
> >On 2 November 2016 at 17:08, Alexander Stein
> ><alexander.stein@systec-electronic.com> wrote:
> >>On Wednesday 02 November 2016 15:49:50, Ramesh Shanmugasundaram wrote:
> 
> >>>But this is physical layer information as Oliver mentioned. The CAN
> >>>transceiver may even require some start-up state that may be enabled by
> >>>setting couple of GPIOs (as on my board). Yes, it is part of the CAN
> >>>interface configuration but it not part of the CAN controller driver. I
> >>>think we could map it to Ethernet MAC & PHY case - isn't it?
> 
> IIRC there's some mii-tool to configure, view & manipulate media-independent
> interface status:
> 
> http://man7.org/linux/man-pages/man8/mii-tool.8.html
> 
> At least the ethernet people found a way to configure their transceiver part
> with this tool.
> 
> >>The transceiver might be managed by regulators as done by e.g. flexcan. This
> >>is part of the hardware description, so device tree here is fine.
> >>But in my opinion bus termination is an end user configuration which might
> >>change rather often than not. It might be independent of the controller driver
> >>as e.g. in flexcan, but it might be part of the device as in USB hardware
> >>which requires some specific command to change bus termination.
> >>IMHO it makes no sense to split an USB interface driver into two parts using
> >>the same interface in the end.
> 
> ack.
> 
> >I agree that termination is a physical layer setting but to be
> >specific in case of Microchip's CAN BUS it is neither related to CAN
> >controller nor transceiver.
> 
> So what about creating TWO new netlink configuration options:
> 
> diff --git a/include/uapi/linux/can/netlink.h
> b/include/uapi/linux/can/netlink.h
> index 94ffe0c..8d7c4b9 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -127,6 +127,8 @@ enum {
>         IFLA_CAN_BERR_COUNTER,
>         IFLA_CAN_DATA_BITTIMING,
>         IFLA_CAN_DATA_BITTIMING_CONST,
> +       IFLA_CAN_TRX_CTRL,
> +       IFLA_CAN_TERMINATION,
>         __IFLA_CAN_MAX
>  };
> 
> Where IFLA_CAN_TRX_CTRL provides an interface to configure the transceiver
> and IFLA_CAN_TERMINATION to configure the termination.

termination, like bittiming, depends on the physical layer.
Hard to say if it's a user option of a system requirement,
it is similar and should be configurable from userspace (root probably).

> 
> But for IFLA_CAN_TERMINATION we need some possibility to configure either a
> set of supported termination resistors (e.g. disabled, 60 Ohms, 120 Ohms,
> xxx Ohms, ...) or a free termination value (e.g. 1 Ohm - 10 kOhms) following
> the hardware capabilities of the CAN device.
> Therefore a list of values and ranges has to be made available by the
> netlink interface before userspace can select some of them.

Instead of trying to foresee the future and allowing any resistor value,
I'd propose to stick to common physical layers. I wasn't aware of any
physical that has a 10KOhm terminting resistor, nor did I see any TRX
supporting it. So an ISO 11898 bus termination should not allow _any_
resistor value?
Maybe next year a brand new physical layer appears, then we'll add
support for that layer? And maybe that layer may support random
termintion resistors (really?), we'll see then.

for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).

Kind regards,
Kurt

> 
> Both IFLA_CAN_TRX_CTRL and IFLA_CAN_TERMINATION would only be supported if
> the CAN device can handle them. Probably a device tree decription defines a
> fixed configuration which can be read but not modified.

For IFLA_CAN_TERMINATION, I don't see added value putting constants in
device tree, since device tree describes the board and termination
depends on the bus beyond the board. So you can't describe in devicetree
that you always want the termination activated in the controller.

Besides that remark on the constant, I agree with you, although I'm not
sure what function would be possible with IFLA_CAN_TRX_CTRL?
Wakeup-gpio? maximum-bitrate (controlled by a 0-100KOhm resistor on
popular phy's?, ...).

Kurt

RE: Adding new CAN driver

[Ramesh Shanmugasundaram]

> > On 11/02/2016 07:03 PM, Kołłątaj, Remigiusz wrote:
> > >On 2 November 2016 at 17:08, Alexander Stein
> > ><alexander.stein@systec-electronic.com> wrote:
> > >>On Wednesday 02 November 2016 15:49:50, Ramesh Shanmugasundaram wrote:
> >
> > >>>But this is physical layer information as Oliver mentioned. The CAN
> > >>>transceiver may even require some start-up state that may be
> > >>>enabled by setting couple of GPIOs (as on my board). Yes, it is
> > >>>part of the CAN interface configuration but it not part of the CAN
> > >>>controller driver. I think we could map it to Ethernet MAC & PHY case
> - isn't it?
> >
> > IIRC there's some mii-tool to configure, view & manipulate
> > media-independent interface status:
> >
> > http://man7.org/linux/man-pages/man8/mii-tool.8.html
> >
> > At least the ethernet people found a way to configure their
> > transceiver part with this tool.

:-)

> >
> > >>The transceiver might be managed by regulators as done by e.g.
> > >>flexcan. This is part of the hardware description, so device tree here
> is fine.
> > >>But in my opinion bus termination is an end user configuration which
> > >>might change rather often than not. It might be independent of the
> > >>controller driver as e.g. in flexcan, but it might be part of the
> > >>device as in USB hardware which requires some specific command to
> change bus termination.
> > >>IMHO it makes no sense to split an USB interface driver into two
> > >>parts using the same interface in the end.
> >
> > ack.

OK

> >
> > >I agree that termination is a physical layer setting but to be
> > >specific in case of Microchip's CAN BUS it is neither related to CAN
> > >controller nor transceiver.
> >
> > So what about creating TWO new netlink configuration options:
> >
> > diff --git a/include/uapi/linux/can/netlink.h
> > b/include/uapi/linux/can/netlink.h
> > index 94ffe0c..8d7c4b9 100644
> > --- a/include/uapi/linux/can/netlink.h
> > +++ b/include/uapi/linux/can/netlink.h
> > @@ -127,6 +127,8 @@ enum {
> >         IFLA_CAN_BERR_COUNTER,
> >         IFLA_CAN_DATA_BITTIMING,
> >         IFLA_CAN_DATA_BITTIMING_CONST,
> > +       IFLA_CAN_TRX_CTRL,

Not sure about this.

> > +       IFLA_CAN_TERMINATION,

ACK

> >         __IFLA_CAN_MAX
> >  };
> >
> > Where IFLA_CAN_TRX_CTRL provides an interface to configure the
> > transceiver and IFLA_CAN_TERMINATION to configure the termination.
> 
> termination, like bittiming, depends on the physical layer.

It is. However, bittiming is configured "in" controller registers and I don't know if we can say the same for termination. However, it seems like USB drivers (may be with user space app) provide this info as a feature to the user. Wonder how it is done in PCI based CAN cards? 

> Hard to say if it's a user option of a system requirement, it is similar
> and should be configurable from userspace (root probably).
> 
> >
> > But for IFLA_CAN_TERMINATION we need some possibility to configure
> > either a set of supported termination resistors (e.g. disabled, 60
> > Ohms, 120 Ohms, xxx Ohms, ...) or a free termination value (e.g. 1 Ohm
> > - 10 kOhms) following the hardware capabilities of the CAN device.
> > Therefore a list of values and ranges has to be made available by the
> > netlink interface before userspace can select some of them.
> 
> Instead of trying to foresee the future and allowing any resistor value,
> I'd propose to stick to common physical layers. I wasn't aware of any
> physical that has a 10KOhm terminting resistor, nor did I see any TRX
> supporting it. So an ISO 11898 bus termination should not allow _any_
> resistor value?
> Maybe next year a brand new physical layer appears, then we'll add support
> for that layer? And maybe that layer may support random termintion
> resistors (really?), we'll see then.
> 
> for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).

ACK. We could start simple with a bool.

> 
> Kind regards,
> Kurt
> 
> >
> > Both IFLA_CAN_TRX_CTRL and IFLA_CAN_TERMINATION would only be
> > supported if the CAN device can handle them.

ACK

 Probably a device tree
> > decription defines a fixed configuration which can be read but not
> modified.

May be we don't need to report/fill these details if the CAN controller does not manage them?

> 
> For IFLA_CAN_TERMINATION, I don't see added value putting constants in
> device tree, since device tree describes the board and termination depends
> on the bus beyond the board. So you can't describe in devicetree that you
> always want the termination activated in the controller.

If the board supports on-board termination and if it is modifiable (on/off - e.g. with GPIO/regulator) for whatever reason, I think it is OK to define the initial state in dev tree. Netlink could report this and modify if it is allowed.

> 
> Besides that remark on the constant, I agree with you, although I'm not
> sure what function would be possible with IFLA_CAN_TRX_CTRL?
> Wakeup-gpio? maximum-bitrate (controlled by a 0-100KOhm resistor on
> popular phy's?, ...).

ACK. I think TRX_CTRL could be anything and I wonder if we need to expose this under netdev? It could be as simple as wakeup-gpio to complex policies. As you mentioned there could be a mini-can controller behind the transceiver running own firmware with some default config. So not sure about this option.

Thanks,
Ramesh

Re: Adding new CAN driver

[Oliver Hartkopp]

On 11/04/2016 12:06 PM, Ramesh Shanmugasundaram wrote:
>> for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
>
> ACK. We could start simple with a bool.

I would suggest to support a variable resistor (varistor) ranging from 1 
to 65534 Ohms and a list of discrete termination values as suggested.

diff --git a/include/uapi/linux/can/netlink.h 
b/include/uapi/linux/can/netlink.h
index 94ffe0c..a07601a 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -91,6 +91,32 @@ struct can_ctrlmode {
  	__u32 flags;
  };

+/*
+ * CAN hardware-dependent termination resistor constants
+ *
+ * Capabilities shown by ip tool if supported by CAN hardware
+ * Unsupported values are filled with zero (CAN_RESISTOR_INVALID)
+ */
+struct can_termination_const {
+
+	/* range for programmable resistor (if supported) */
+	__u16 range_low;	/* lowermost resistor value (Ohm) */
+	__u16 range_high;	/* highest resistor value (Ohm) */
+
+	/* array of supported discrete resistor values (Ohm) */
+	__u16 term[8];
+};
+
+/*
+ * CAN termination
+ */
+struct can_termination {
+	__u16 term;
+};
+
+#define CAN_RESISTOR_INVALID	0x0000U	/* indicates no value / unsupported */
+#define CAN_RESISTOR_DISABLED	0xFFFFU	/* indicates disabled termination */
+
  #define CAN_CTRLMODE_LOOPBACK		0x01	/* Loopback mode */
  #define CAN_CTRLMODE_LISTENONLY		0x02	/* Listen-only mode */
  #define CAN_CTRLMODE_3_SAMPLES		0x04	/* Triple sampling mode */
@@ -127,6 +153,8 @@ enum {
  	IFLA_CAN_BERR_COUNTER,
  	IFLA_CAN_DATA_BITTIMING,
  	IFLA_CAN_DATA_BITTIMING_CONST,
+	IFLA_CAN_TERMINATION,
+	IFLA_CAN_TERMINATION_CONST,
  	__IFLA_CAN_MAX
  };

With this interface the ip tool can get the capabilities of the 
termination analogue to the BITTIMING_CONST values.
And it can set the termination with IFLA_CAN_TERMINATION.

The CAN driver would provide a struct can_termination_const - in the 
case the CAN interface supports termination control.

E.g. the mcba_usb driver would provide this:

static struct can_termination_const = {
	.range_low = CAN_RESISTOR_INVALID;
	.range_high = CAN_RESISTOR_INVALID;
	.term = { 120, CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID };
}

Another driver supporting 60 and 120 Ohms but no possibility to disable 
the termination:

static struct can_termination_const = {
	.range_low = CAN_RESISTOR_INVALID;
	.range_high = CAN_RESISTOR_INVALID;
	.term = { 60, 120, CAN_RESISTOR_INVALID };
}

Another driver supporting 60 to 5600 Ohms varistor and the possibility 
to disable the termination:

static struct can_termination_const = {
	.range_low = 60;
	.range_high = 5600;
	.term = { CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID };
}

These termination capabilities might be provided by device tree 
configuration too - although I don't know how this would be done.

What do you think about this kind of configuration interface?

Regards,
Oliver

RE: Adding new CAN driver

[Ramesh Shanmugasundaram]

> >> for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
> >
> > ACK. We could start simple with a bool.
> 
> I would suggest to support a variable resistor (varistor) ranging from 1
> to 65534 Ohms and a list of discrete termination values as suggested.

If there are use cases for such variable resistor configs, it is OK.
 
> 
> diff --git a/include/uapi/linux/can/netlink.h
> b/include/uapi/linux/can/netlink.h
> index 94ffe0c..a07601a 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -91,6 +91,32 @@ struct can_ctrlmode {
>   	__u32 flags;
>   };
> 
> +/*
> + * CAN hardware-dependent termination resistor constants
> + *
> + * Capabilities shown by ip tool if supported by CAN hardware

"Capabilities shown by ip tool if managed by CAN driver" - would that be apt?
As termination is provided on the CAN bus by somebody (DB9 connector/fixed on board termination/gpio managed etc.), this would clarify the driver's role in managing it.

> + * Unsupported values are filled with zero (CAN_RESISTOR_INVALID)  */
> +struct can_termination_const {
> +
> +	/* range for programmable resistor (if supported) */
> +	__u16 range_low;	/* lowermost resistor value (Ohm) */
> +	__u16 range_high;	/* highest resistor value (Ohm) */
> +
> +	/* array of supported discrete resistor values (Ohm) */
> +	__u16 term[8];
> +};
> +
> +/*
> + * CAN termination
> + */
> +struct can_termination {
> +	__u16 term;
> +};
> +
> +#define CAN_RESISTOR_INVALID	0x0000U	/* indicates no value /
> unsupported */
> +#define CAN_RESISTOR_DISABLED	0xFFFFU	/* indicates disabled
> termination */
> +
>   #define CAN_CTRLMODE_LOOPBACK		0x01	/* Loopback mode */
>   #define CAN_CTRLMODE_LISTENONLY		0x02	/* Listen-only mode */
>   #define CAN_CTRLMODE_3_SAMPLES		0x04	/* Triple sampling mode */
> @@ -127,6 +153,8 @@ enum {
>   	IFLA_CAN_BERR_COUNTER,
>   	IFLA_CAN_DATA_BITTIMING,
>   	IFLA_CAN_DATA_BITTIMING_CONST,
> +	IFLA_CAN_TERMINATION,
> +	IFLA_CAN_TERMINATION_CONST,
>   	__IFLA_CAN_MAX
>   };
> 
> With this interface the ip tool can get the capabilities of the
> termination analogue to the BITTIMING_CONST values.
> And it can set the termination with IFLA_CAN_TERMINATION.

ACK.

> The CAN driver would provide a struct can_termination_const - in the case
> the CAN interface supports termination control.
> 
> E.g. the mcba_usb driver would provide this:
> 
> static struct can_termination_const = {
> 	.range_low = CAN_RESISTOR_INVALID;
> 	.range_high = CAN_RESISTOR_INVALID;
> 	.term = { 120, CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID }; }
> 
> Another driver supporting 60 and 120 Ohms but no possibility to disable
> the termination:
> 
> static struct can_termination_const = {
> 	.range_low = CAN_RESISTOR_INVALID;
> 	.range_high = CAN_RESISTOR_INVALID;
> 	.term = { 60, 120, CAN_RESISTOR_INVALID }; }
> 
> Another driver supporting 60 to 5600 Ohms varistor and the possibility to
> disable the termination:
> 
> static struct can_termination_const = {
> 	.range_low = 60;
> 	.range_high = 5600;
> 	.term = { CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID }; }
> 
> These termination capabilities might be provided by device tree
> configuration too - although I don't know how this would be done.

I think it can be something like this 

Board DTS file
&can0 {
	...
	can-termination = <120>;	/* default value */
	
	/* GPIO managed */
	can-term-gpio = <&gpio0 10 GPIO_ACTIVE_HIGH>;
	...

	/* Regulator managed */
	can-term-supply = <&phandle to regulator node>
};

The driver could manage the respective GPIO/regulator from the phandle. The MMC subsystem code has examples of such config.

Complex cases could use a driver specific hook (as with bittiming) to set TERM values.

> 
> What do you think about this kind of configuration interface?
> 

ACK. 
USB, PCI based CAN designers may have some suggestions/requirements?

Thanks,
Ramesh

Re: Adding new CAN driver

[Oliver Hartkopp]

On 11/08/2016 10:36 AM, Ramesh Shanmugasundaram wrote:
>>>> for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
>>>
>>> ACK. We could start simple with a bool.
>>
>> I would suggest to support a variable resistor (varistor) ranging from 1
>> to 65534 Ohms and a list of discrete termination values as suggested.
>
> If there are use cases for such variable resistor configs, it is OK.
>

I double checked the varistors currently used in CAN context.
They are used to provide ESD (electrostatic discharge) protection %-)

So having a discrete list of discrete termination resistors would 
obviously be sufficient for our use-case.

>>
>> diff --git a/include/uapi/linux/can/netlink.h
>> b/include/uapi/linux/can/netlink.h
>> index 94ffe0c..a07601a 100644
>> --- a/include/uapi/linux/can/netlink.h
>> +++ b/include/uapi/linux/can/netlink.h
>> @@ -91,6 +91,32 @@ struct can_ctrlmode {
>>   	__u32 flags;
>>   };
>>
>> +/*
>> + * CAN hardware-dependent termination resistor constants
>> + *
>> + * Capabilities shown by ip tool if supported by CAN hardware
>
> "Capabilities shown by ip tool if managed by CAN driver" - would that be apt?
> As termination is provided on the CAN bus by somebody (DB9 connector/fixed on board termination/gpio managed etc.), this would clarify the driver's role in managing it.
>
>> + * Unsupported values are filled with zero (CAN_RESISTOR_INVALID)  */
>> +struct can_termination_const {
>> +
>> +	/* range for programmable resistor (if supported) */
>> +	__u16 range_low;	/* lowermost resistor value (Ohm) */
>> +	__u16 range_high;	/* highest resistor value (Ohm) */

Will remove this resistor range stuff next time.

>> +
>> +	/* array of supported discrete resistor values (Ohm) */
>> +	__u16 term[8];

Do you think 8 resistor values are enough?

>> +};
>> +
>> +/*
>> + * CAN termination
>> + */
>> +struct can_termination {
>> +	__u16 term;
>> +};
>> +
>> +#define CAN_RESISTOR_INVALID	0x0000U	/* indicates no value /
>> unsupported */
>> +#define CAN_RESISTOR_DISABLED	0xFFFFU	/* indicates disabled
>> termination */
>> +
>>   #define CAN_CTRLMODE_LOOPBACK		0x01	/* Loopback mode */
>>   #define CAN_CTRLMODE_LISTENONLY		0x02	/* Listen-only mode */
>>   #define CAN_CTRLMODE_3_SAMPLES		0x04	/* Triple sampling mode */
>> @@ -127,6 +153,8 @@ enum {
>>   	IFLA_CAN_BERR_COUNTER,
>>   	IFLA_CAN_DATA_BITTIMING,
>>   	IFLA_CAN_DATA_BITTIMING_CONST,
>> +	IFLA_CAN_TERMINATION,
>> +	IFLA_CAN_TERMINATION_CONST,
>>   	__IFLA_CAN_MAX
>>   };
>>
>> With this interface the ip tool can get the capabilities of the
>> termination analogue to the BITTIMING_CONST values.
>> And it can set the termination with IFLA_CAN_TERMINATION.
>
> ACK.
>
>> The CAN driver would provide a struct can_termination_const - in the case
>> the CAN interface supports termination control.
>>
>> E.g. the mcba_usb driver would provide this:
>>
>> static struct can_termination_const = {
>> 	.range_low = CAN_RESISTOR_INVALID;
>> 	.range_high = CAN_RESISTOR_INVALID;
>> 	.term = { 120, CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID }; }
>>
>> Another driver supporting 60 and 120 Ohms but no possibility to disable
>> the termination:
>>
>> static struct can_termination_const = {
>> 	.range_low = CAN_RESISTOR_INVALID;
>> 	.range_high = CAN_RESISTOR_INVALID;
>> 	.term = { 60, 120, CAN_RESISTOR_INVALID }; }
>>
>> Another driver supporting 60 to 5600 Ohms varistor and the possibility to
>> disable the termination:
>>
>> static struct can_termination_const = {
>> 	.range_low = 60;
>> 	.range_high = 5600;
>> 	.term = { CAN_RESISTOR_DISABLED, CAN_RESISTOR_INVALID }; }
>>
>> These termination capabilities might be provided by device tree
>> configuration too - although I don't know how this would be done.
>
> I think it can be something like this
>
> Board DTS file
> &can0 {
> 	...
> 	can-termination = <120>;	/* default value */
> 	
> 	/* GPIO managed */
> 	can-term-gpio = <&gpio0 10 GPIO_ACTIVE_HIGH>;
> 	...
>
> 	/* Regulator managed */
> 	can-term-supply = <&phandle to regulator node>
> };
>
> The driver could manage the respective GPIO/regulator from the phandle. The MMC subsystem code has examples of such config.
>
> Complex cases could use a driver specific hook (as with bittiming) to set TERM values.
>
>>
>> What do you think about this kind of configuration interface?
>>
>
> ACK.
> USB, PCI based CAN designers may have some suggestions/requirements?

Yep.

Anyone?

Regards,
Oliver

Re: Adding new CAN driver

[Kurt Van Dijck]

> On 11/08/2016 10:36 AM, Ramesh Shanmugasundaram wrote:
> >>>>for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
> >>>
> >>>ACK. We could start simple with a bool.
> >>
> >>I would suggest to support a variable resistor (varistor) ranging from 1
> >>to 65534 Ohms and a list of discrete termination values as suggested.
> >
> >If there are use cases for such variable resistor configs, it is OK.
> >
> 
> I double checked the varistors currently used in CAN context.
> They are used to provide ESD (electrostatic discharge) protection %-)
> 
> So having a discrete list of discrete termination resistors would obviously
> be sufficient for our use-case.

Up to now, I only saw systems that allow to enable 120Ohm, and no
other values are coming AFAIK.
This can be addressed as a 'list of discrete values' indeed.

[...]

> >
> >I think it can be something like this
> >
> >Board DTS file
> >&can0 {
> >	...
> >	can-termination = <120>;	/* default value */
> >	
> >	/* GPIO managed */
> >	can-term-gpio = <&gpio0 10 GPIO_ACTIVE_HIGH>;
> >	...
> >
> >	/* Regulator managed */
> >	can-term-supply = <&phandle to regulator node>
> >};

This GPIO you're using to activate the termination resistor is/should be
controlled independantly of the rest of your CAN driver.
Take this scenario: you activate the bus termination from the CAN
driver. Up to that point, the bus is not properly terminated, and other
nodes on the bus do not communicate (properly).
If you really depended on that termination, and it took that long in
order to enable, you would place the termination elsewhere.

It seems we're adding quite a lot just to make the system activate a GPIO that
does not want any interaction to the CAN subsystem (at least, I have not
seen any use case up to now justifying this).
Or am I missing something here?

Note that this applies to the CAN bus termination.
The transceiver settings on the other hand do interact with the CAN
driver (like limiting the max baudrate as an example, or activating the
TRX via a GPIO).

Kind regards,
Kurt

Re: Adding new CAN driver

[Tom Evans]

On 10/11/2016 8:18 PM, Kurt Van Dijck wrote:
> Up to now, I only saw systems that allow to enable 120Ohm,
 > and no other values are coming AFAIK.

ISO/CD 11898-3?

If you don't have the ISO standards, check the second picture in:

https://en.wikipedia.org/wiki/CAN_bus#Architecture

Maybe some 11898-3 transceivers have internal resistors.

Tom

RE: Adding new CAN driver

[Ramesh Shanmugasundaram]

> Subject: Re: Adding new CAN driver
> 
> > On 11/08/2016 10:36 AM, Ramesh Shanmugasundaram wrote:
> > >>>>for now, IFLA_CAN_TERMINATION may be a boolean (or enumeration).
> > >>>
> > >>>ACK. We could start simple with a bool.
> > >>
> > >>I would suggest to support a variable resistor (varistor) ranging
> > >>from 1 to 65534 Ohms and a list of discrete termination values as
> suggested.
> > >
> > >If there are use cases for such variable resistor configs, it is OK.
> > >
> >
> > I double checked the varistors currently used in CAN context.
> > They are used to provide ESD (electrostatic discharge) protection %-)
> >
> > So having a discrete list of discrete termination resistors would
> > obviously be sufficient for our use-case.
> 
> Up to now, I only saw systems that allow to enable 120Ohm, and no other
> values are coming AFAIK.
> This can be addressed as a 'list of discrete values' indeed.

ACK.

> 
> [...]
> 
> > >
> > >I think it can be something like this
> > >
> > >Board DTS file
> > >&can0 {
> > >	...
> > >	can-termination = <120>;	/* default value */
> > >
> > >	/* GPIO managed */
> > >	can-term-gpio = <&gpio0 10 GPIO_ACTIVE_HIGH>;
> > >	...
> > >
> > >	/* Regulator managed */
> > >	can-term-supply = <&phandle to regulator node> };
> 
> This GPIO you're using to activate the termination resistor is/should be
> controlled independantly of the rest of your CAN driver.

I agree. This is an e.g. config only and in general I too prefer the CAN driver (exposing a netdev) limits it to managing the controller alone. But as we are introducing TERM as a netlink config we could try to isolate the config "action" with a driver specific callback. In USB case, it gets the config and the action is sending a USB message. In GPIO or regulator case, the action is upto the driver "if" it wants that to be part of it - like opening a gate that switches termination paths onboard. It could be a onetime config or run-time manageable - its up to the driver.

There are other things we need to decide for this netlink config - what should be the state of netdev(UP/DOWN) when TERM is configured? Does it matter? It depends on the end solution and I would vote for not having any policy within CAN core. Any thoughts?

> Take this scenario: you activate the bus termination from the CAN driver.
> Up to that point, the bus is not properly terminated, and other nodes on
> the bus do not communicate (properly).
> If you really depended on that termination, and it took that long in order
> to enable, you would place the termination elsewhere.
> 
> It seems we're adding quite a lot just to make the system activate a GPIO
> that does not want any interaction to the CAN subsystem (at least, I have
> not seen any use case up to now justifying this).
> Or am I missing something here?
> 
> Note that this applies to the CAN bus termination.
> The transceiver settings on the other hand do interact with the CAN driver
> (like limiting the max baudrate as an example, or activating the TRX via a
> GPIO).
> 

Thanks,
Ramesh

CAN Termination API - was Re: Adding new CAN driver

[Oliver Hartkopp]

Hi all,

I would like to pick up the termination API again.

>>> +
>>> +    /* array of supported discrete resistor values (Ohm) */
>>> +    __u16 term[8];
>
> Do you think 8 resistor values are enough?
>

see below ...

>>> +};
>>> +
>>> +/*
>>> + * CAN termination
>>> + */
>>> +struct can_termination {
>>> +    __u16 term;
>>> +};
>>> +
>>> +#define CAN_RESISTOR_INVALID    0x0000U    /* indicates no value /
>>> unsupported */
>>> +#define CAN_RESISTOR_DISABLED    0xFFFFU    /* indicates disabled
>>> termination */
>>> +
>>>   #define CAN_CTRLMODE_LOOPBACK        0x01    /* Loopback mode */
>>>   #define CAN_CTRLMODE_LISTENONLY        0x02    /* Listen-only mode */
>>>   #define CAN_CTRLMODE_3_SAMPLES        0x04    /* Triple sampling
>>> mode */
>>> @@ -127,6 +153,8 @@ enum {
>>>       IFLA_CAN_BERR_COUNTER,
>>>       IFLA_CAN_DATA_BITTIMING,
>>>       IFLA_CAN_DATA_BITTIMING_CONST,
>>> +    IFLA_CAN_TERMINATION,
>>> +    IFLA_CAN_TERMINATION_CONST,
>>>       __IFLA_CAN_MAX
>>>   };
>>>
>>> With this interface the ip tool can get the capabilities of the
>>> termination analogue to the BITTIMING_CONST values.
>>> And it can set the termination with IFLA_CAN_TERMINATION.
>>
>> ACK.

I tried to get into the netlink stuff and I wonder how to make this API 
really simple.

By now the netlink interface in linux/drivers/net/can/dev.c looks like:

/*
  * CAN netlink interface
  */
static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
         [IFLA_CAN_STATE]        = { .type = NLA_U32 },
         [IFLA_CAN_CTRLMODE]     = { .len = sizeof(struct can_ctrlmode) },
         [IFLA_CAN_RESTART_MS]   = { .type = NLA_U32 },
         [IFLA_CAN_RESTART]      = { .type = NLA_U32 },
         [IFLA_CAN_BITTIMING]    = { .len = sizeof(struct can_bittiming) },
         [IFLA_CAN_BITTIMING_CONST]
                                 = { .len = sizeof(struct 
can_bittiming_const) },
         [IFLA_CAN_CLOCK]        = { .len = sizeof(struct can_clock) },
         [IFLA_CAN_BERR_COUNTER] = { .len = sizeof(struct 
can_berr_counter) },
         [IFLA_CAN_DATA_BITTIMING]
                                 = { .len = sizeof(struct can_bittiming) },
         [IFLA_CAN_DATA_BITTIMING_CONST]
                                 = { .len = sizeof(struct 
can_bittiming_const) },
};

To add IFLA_CAN_TERMINATION we would just need

	[IFLA_CAN_TERMINATION]   = { .type = NLA_U16 },

to enable a defined termination value from 1 to 65534 Ohms where 65535 
would be CAN_RESISTOR_DISABLED. In the CAN driver this given value is 
checked against the supported list of termination values and switched on 
if supported.

To provide a list of possible termination values with 
IFLA_CAN_TERMINATION_CONST we need to pass an array of u16 constants to 
the userspace.

In opposite to our current netlink messages this array make contain 1..n 
constant values - so defining a fixed struct of e.g. 8 entries looks 
like a bad design.

But how to provide the array to the netlink interface correctly?

With the netlink type NLA_UNSPEC, NLA_NESTED, NLA_NESTED_COMPAT or 
NLA_BINARY??

See http://lxr.linux.no/#linux+v4.8.10/include/net/netlink.h#L160

The current

[IFLA_CAN_DATA_BITTIMING]  = { .len = sizeof(struct can_bittiming_const) },

seems to use NLA_UNSPEC which means the .len value is the _minimum_ 
length value.

Would this mean we can define

[IFLA_CAN_TERMINATION_CONST]   = { .len = sizeof(__u16) },

as a minimum length value and provide as much termination values as the 
CAN interface supports?

Regards,
Oliver

Re: CAN Termination API - was Re: Adding new CAN driver

[Kołłątaj, Remigiusz]

Hi,

What about keeping possibility to set termination value as u16 but
without defining supported values? This will give us opportunity to
define every value and it will be up to driver to verify it. I know
such approach will be less convenient for a user as he will not get
list of supported terminations. On the other hand these values can be
simply logged (i.e. netdev_info/warn/err).

This is pretty similar to setting up bitrate in USB devices. They are
usually predefined and there is no way to learn supported values with
ip command

Regards,
Remik

On 25 November 2016 at 15:16, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi all,
>
> I would like to pick up the termination API again.
>
>>>> +
>>>> +    /* array of supported discrete resistor values (Ohm) */
>>>> +    __u16 term[8];
>>
>>
>> Do you think 8 resistor values are enough?
>>
>
> see below ...
>
>>>> +};
>>>> +
>>>> +/*
>>>> + * CAN termination
>>>> + */
>>>> +struct can_termination {
>>>> +    __u16 term;
>>>> +};
>>>> +
>>>> +#define CAN_RESISTOR_INVALID    0x0000U    /* indicates no value /
>>>> unsupported */
>>>> +#define CAN_RESISTOR_DISABLED    0xFFFFU    /* indicates disabled
>>>> termination */
>>>> +
>>>>   #define CAN_CTRLMODE_LOOPBACK        0x01    /* Loopback mode */
>>>>   #define CAN_CTRLMODE_LISTENONLY        0x02    /* Listen-only mode */
>>>>   #define CAN_CTRLMODE_3_SAMPLES        0x04    /* Triple sampling
>>>> mode */
>>>> @@ -127,6 +153,8 @@ enum {
>>>>       IFLA_CAN_BERR_COUNTER,
>>>>       IFLA_CAN_DATA_BITTIMING,
>>>>       IFLA_CAN_DATA_BITTIMING_CONST,
>>>> +    IFLA_CAN_TERMINATION,
>>>> +    IFLA_CAN_TERMINATION_CONST,
>>>>       __IFLA_CAN_MAX
>>>>   };
>>>>
>>>> With this interface the ip tool can get the capabilities of the
>>>> termination analogue to the BITTIMING_CONST values.
>>>> And it can set the termination with IFLA_CAN_TERMINATION.
>>>
>>>
>>> ACK.
>
>
> I tried to get into the netlink stuff and I wonder how to make this API
> really simple.
>
> By now the netlink interface in linux/drivers/net/can/dev.c looks like:
>
> /*
>  * CAN netlink interface
>  */
> static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
>         [IFLA_CAN_STATE]        = { .type = NLA_U32 },
>         [IFLA_CAN_CTRLMODE]     = { .len = sizeof(struct can_ctrlmode) },
>         [IFLA_CAN_RESTART_MS]   = { .type = NLA_U32 },
>         [IFLA_CAN_RESTART]      = { .type = NLA_U32 },
>         [IFLA_CAN_BITTIMING]    = { .len = sizeof(struct can_bittiming) },
>         [IFLA_CAN_BITTIMING_CONST]
>                                 = { .len = sizeof(struct
> can_bittiming_const) },
>         [IFLA_CAN_CLOCK]        = { .len = sizeof(struct can_clock) },
>         [IFLA_CAN_BERR_COUNTER] = { .len = sizeof(struct can_berr_counter)
> },
>         [IFLA_CAN_DATA_BITTIMING]
>                                 = { .len = sizeof(struct can_bittiming) },
>         [IFLA_CAN_DATA_BITTIMING_CONST]
>                                 = { .len = sizeof(struct
> can_bittiming_const) },
> };
>
> To add IFLA_CAN_TERMINATION we would just need
>
>         [IFLA_CAN_TERMINATION]   = { .type = NLA_U16 },
>
> to enable a defined termination value from 1 to 65534 Ohms where 65535 would
> be CAN_RESISTOR_DISABLED. In the CAN driver this given value is checked
> against the supported list of termination values and switched on if
> supported.
>
> To provide a list of possible termination values with
> IFLA_CAN_TERMINATION_CONST we need to pass an array of u16 constants to the
> userspace.
>
> In opposite to our current netlink messages this array make contain 1..n
> constant values - so defining a fixed struct of e.g. 8 entries looks like a
> bad design.
>
> But how to provide the array to the netlink interface correctly?
>
> With the netlink type NLA_UNSPEC, NLA_NESTED, NLA_NESTED_COMPAT or
> NLA_BINARY??
>
> See http://lxr.linux.no/#linux+v4.8.10/include/net/netlink.h#L160
>
> The current
>
> [IFLA_CAN_DATA_BITTIMING]  = { .len = sizeof(struct can_bittiming_const) },
>
> seems to use NLA_UNSPEC which means the .len value is the _minimum_ length
> value.
>
> Would this mean we can define
>
> [IFLA_CAN_TERMINATION_CONST]   = { .len = sizeof(__u16) },
>
> as a minimum length value and provide as much termination values as the CAN
> interface supports?
>
> Regards,
> Oliver
>



-- 
________________________

Remigiusz Kołłątaj

Mobica Ltd

Software Consultant, CTO Team

Skype: remigiusz.kollataj

www.mobica.com


Mobica is a provider of software engineering, testing and consultancy
services based in the UK, Poland and the USA, with a worldwide
customer base. We have a proven track record in delivering innovative
solutions to some of the world’s best-known companies in a range of
sectors including Automotive, Mobile, Semiconductor, Finance, TV,
Marine and Aviation.

RE: CAN Termination API - was Re: Adding new CAN driver

[Ramesh Shanmugasundaram]

> Subject: CAN Termination API - was Re: Adding new CAN driver
> 
> Hi all,
> 
> I would like to pick up the termination API again.
> 
> >>> +
> >>> +    /* array of supported discrete resistor values (Ohm) */
> >>> +    __u16 term[8];
> >
> > Do you think 8 resistor values are enough?
> >
> 
> see below ...
> 
> >>> +};
> >>> +
> >>> +/*
> >>> + * CAN termination
> >>> + */
> >>> +struct can_termination {
> >>> +    __u16 term;
> >>> +};
> >>> +
> >>> +#define CAN_RESISTOR_INVALID    0x0000U    /* indicates no value /
> >>> unsupported */
> >>> +#define CAN_RESISTOR_DISABLED    0xFFFFU    /* indicates disabled
> >>> termination */
> >>> +
> >>>   #define CAN_CTRLMODE_LOOPBACK        0x01    /* Loopback mode */
> >>>   #define CAN_CTRLMODE_LISTENONLY        0x02    /* Listen-only mode
> */
> >>>   #define CAN_CTRLMODE_3_SAMPLES        0x04    /* Triple sampling
> >>> mode */
> >>> @@ -127,6 +153,8 @@ enum {
> >>>       IFLA_CAN_BERR_COUNTER,
> >>>       IFLA_CAN_DATA_BITTIMING,
> >>>       IFLA_CAN_DATA_BITTIMING_CONST,
> >>> +    IFLA_CAN_TERMINATION,
> >>> +    IFLA_CAN_TERMINATION_CONST,
> >>>       __IFLA_CAN_MAX
> >>>   };
> >>>
> >>> With this interface the ip tool can get the capabilities of the
> >>> termination analogue to the BITTIMING_CONST values.
> >>> And it can set the termination with IFLA_CAN_TERMINATION.
> >>
> >> ACK.
> 
> I tried to get into the netlink stuff and I wonder how to make this API
> really simple.
> 
> By now the netlink interface in linux/drivers/net/can/dev.c looks like:
> 
> /*
>   * CAN netlink interface
>   */
> static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
>          [IFLA_CAN_STATE]        = { .type = NLA_U32 },
>          [IFLA_CAN_CTRLMODE]     = { .len = sizeof(struct can_ctrlmode) },
>          [IFLA_CAN_RESTART_MS]   = { .type = NLA_U32 },
>          [IFLA_CAN_RESTART]      = { .type = NLA_U32 },
>          [IFLA_CAN_BITTIMING]    = { .len = sizeof(struct can_bittiming)
> },
>          [IFLA_CAN_BITTIMING_CONST]
>                                  = { .len = sizeof(struct
> can_bittiming_const) },
>          [IFLA_CAN_CLOCK]        = { .len = sizeof(struct can_clock) },
>          [IFLA_CAN_BERR_COUNTER] = { .len = sizeof(struct
> can_berr_counter) },
>          [IFLA_CAN_DATA_BITTIMING]
>                                  = { .len = sizeof(struct can_bittiming)
> },
>          [IFLA_CAN_DATA_BITTIMING_CONST]
>                                  = { .len = sizeof(struct
> can_bittiming_const) },
> };
> 
> To add IFLA_CAN_TERMINATION we would just need
> 
> 	[IFLA_CAN_TERMINATION]   = { .type = NLA_U16 },
> 
> to enable a defined termination value from 1 to 65534 Ohms where 65535
> would be CAN_RESISTOR_DISABLED. In the CAN driver this given value is
> checked against the supported list of termination values and switched on
> if supported.
> To provide a list of possible termination values with
> IFLA_CAN_TERMINATION_CONST we need to pass an array of u16 constants to
> the userspace.
> 
> In opposite to our current netlink messages this array make contain 1..n
> constant values - so defining a fixed struct of e.g. 8 entries looks like
> a bad design.
> 
> But how to provide the array to the netlink interface correctly?
> 
> With the netlink type NLA_UNSPEC, NLA_NESTED, NLA_NESTED_COMPAT or
> NLA_BINARY??
> 
> See http://lxr.linux.no/#linux+v4.8.10/include/net/netlink.h#L160
> 
> The current
> 
> [IFLA_CAN_DATA_BITTIMING]  = { .len = sizeof(struct can_bittiming_const)
> },
> 
> seems to use NLA_UNSPEC which means the .len value is the _minimum_ length
> value.
> 
> Would this mean we can define
> 
> [IFLA_CAN_TERMINATION_CONST]   = { .len = sizeof(__u16) },
> 
> as a minimum length value and provide as much termination values as the
> CAN interface supports?

OK. This would be a simpler option.

Optionally, the can core could also check the given value against a list of valid termination values & round it to the nearest possible value. Something like below

possible_termination_values = { 60, 120, 240  } /* some random numbers */

- if user_val < 60 -> use 60
- if user_val > 240 -> use 240
- if user_val falls within a range of the possible values
    if ((range_low + (range_high - range_low) / 2)) <= user_val)
	user_val = range_low;
    else
      user_val = range_high;

   ret = driver_callback(user_val)

I felt the error reporting would be consistent. It is up to you.

Thanks,
Ramesh.

Re: CAN Termination API - was Re: Adding new CAN driver

[Kurt Van Dijck]

--- Original message ---
> Date: Thu, 22 Dec 2016 09:39:00 +0000
> From: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> To: Oliver Hartkopp <socketcan@hartkopp.net>, Kurt Van Dijck
>  <dev.kurt@vandijck-laurijssen.be>
> CC: "Kołłątaj, Remigiusz" <remigiusz.kollataj@mobica.com>, Alexander
>  Stein <alexander.stein@systec-electronic.com>, Marc Kleine-Budde
>  <mkl@pengutronix.de>, Uwe Bonnes
>  <bon@elektron.ikp.physik.tu-darmstadt.de>, "linux-can@vger.kernel.org"
>  <linux-can@vger.kernel.org>
> Subject: RE: CAN Termination API - was Re: Adding new CAN driver
> 
> > Subject: CAN Termination API - was Re: Adding new CAN driver
> > 
> > Hi all,
> > 
> > I would like to pick up the termination API again.
> > 
> > >>> +
> > >>> +    /* array of supported discrete resistor values (Ohm) */
> > >>> +    __u16 term[8];
> > >
> > > Do you think 8 resistor values are enough?
> > >
> > 
> > see below ...
> > 
> > >>> +};
> > >>> +
> > >>> +/*
> > >>> + * CAN termination
> > >>> + */
> > >>> +struct can_termination {
> > >>> +    __u16 term;
> > >>> +};
> > >>> +
> > >>> +#define CAN_RESISTOR_INVALID    0x0000U    /* indicates no value /
> > >>> unsupported */
> > >>> +#define CAN_RESISTOR_DISABLED    0xFFFFU    /* indicates disabled
> > >>> termination */
> > >>> +
> > >>>   #define CAN_CTRLMODE_LOOPBACK        0x01    /* Loopback mode */
> > >>>   #define CAN_CTRLMODE_LISTENONLY        0x02    /* Listen-only mode
> > */
> > >>>   #define CAN_CTRLMODE_3_SAMPLES        0x04    /* Triple sampling
> > >>> mode */
> > >>> @@ -127,6 +153,8 @@ enum {
> > >>>       IFLA_CAN_BERR_COUNTER,
> > >>>       IFLA_CAN_DATA_BITTIMING,
> > >>>       IFLA_CAN_DATA_BITTIMING_CONST,
> > >>> +    IFLA_CAN_TERMINATION,
> > >>> +    IFLA_CAN_TERMINATION_CONST,
> > >>>       __IFLA_CAN_MAX
> > >>>   };
> > >>>
> > >>> With this interface the ip tool can get the capabilities of the
> > >>> termination analogue to the BITTIMING_CONST values.
> > >>> And it can set the termination with IFLA_CAN_TERMINATION.
> > >>
> > >> ACK.
> > 
> > I tried to get into the netlink stuff and I wonder how to make this API
> > really simple.
> > 
> > By now the netlink interface in linux/drivers/net/can/dev.c looks like:
> > 
> > /*
> >   * CAN netlink interface
> >   */
> > static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
> >          [IFLA_CAN_STATE]        = { .type = NLA_U32 },
> >          [IFLA_CAN_CTRLMODE]     = { .len = sizeof(struct can_ctrlmode) },
> >          [IFLA_CAN_RESTART_MS]   = { .type = NLA_U32 },
> >          [IFLA_CAN_RESTART]      = { .type = NLA_U32 },
> >          [IFLA_CAN_BITTIMING]    = { .len = sizeof(struct can_bittiming)
> > },
> >          [IFLA_CAN_BITTIMING_CONST]
> >                                  = { .len = sizeof(struct
> > can_bittiming_const) },
> >          [IFLA_CAN_CLOCK]        = { .len = sizeof(struct can_clock) },
> >          [IFLA_CAN_BERR_COUNTER] = { .len = sizeof(struct
> > can_berr_counter) },
> >          [IFLA_CAN_DATA_BITTIMING]
> >                                  = { .len = sizeof(struct can_bittiming)
> > },
> >          [IFLA_CAN_DATA_BITTIMING_CONST]
> >                                  = { .len = sizeof(struct
> > can_bittiming_const) },
> > };
> > 
> > To add IFLA_CAN_TERMINATION we would just need
> > 
> > 	[IFLA_CAN_TERMINATION]   = { .type = NLA_U16 },

Why would you limit the API to u16?
I understand current common values fit perfectly in an u16, but limiting
the API to what is common today, seems like a bad plan.
the _only_ known value, 120, fits in an u8 even.

u32 does not seem to add significant overhead IMHO, and protects against
future developments?

> > 
> > to enable a defined termination value from 1 to 65534 Ohms where 65535
> > would be CAN_RESISTOR_DISABLED. In the CAN driver this given value is
> > checked against the supported list of termination values and switched on
> > if supported.
> > To provide a list of possible termination values with
> > IFLA_CAN_TERMINATION_CONST we need to pass an array of u16 constants to
> > the userspace.
> > 
> > In opposite to our current netlink messages this array make contain 1..n
> > constant values - so defining a fixed struct of e.g. 8 entries looks like
> > a bad design.
> > 
> > But how to provide the array to the netlink interface correctly?
> > 
> > With the netlink type NLA_UNSPEC, NLA_NESTED, NLA_NESTED_COMPAT or
> > NLA_BINARY??
> > 
> > See http://lxr.linux.no/#linux+v4.8.10/include/net/netlink.h#L160
> > 
> > The current
> > 
> > [IFLA_CAN_DATA_BITTIMING]  = { .len = sizeof(struct can_bittiming_const)
> > },
> > 
> > seems to use NLA_UNSPEC which means the .len value is the _minimum_ length
> > value.
> > 
> > Would this mean we can define
> > 
> > [IFLA_CAN_TERMINATION_CONST]   = { .len = sizeof(__u16) },
> > 
> > as a minimum length value and provide as much termination values as the
> > CAN interface supports?
> 
> OK. This would be a simpler option.
> 
> Optionally, the can core could also check the given value against a list of valid termination values & round it to the nearest possible value. Something like below
> 
> possible_termination_values = { 60, 120, 240  } /* some random numbers */
> 
> - if user_val < 60 -> use 60
> - if user_val > 240 -> use 240
> - if user_val falls within a range of the possible values
>     if ((range_low + (range_high - range_low) / 2)) <= user_val)
> 	user_val = range_low;
>     else
>       user_val = range_high;
> 
>    ret = driver_callback(user_val)
> 
> I felt the error reporting would be consistent. It is up to you.

I think that it is a bad idea to implement rounding in kernel space.
The algorithm you specify is rather trivial, and I'd do the same, but
is subject to change and belongs to userspace.

When rounding to the nearest is done in the kernel, that happens on
places where the nearest is not communicated to userspace, which has
been solved in this case by providing a list of possible values.

Kurt

Re: CAN Termination API - was Re: Adding new CAN driver

[Oliver Hartkopp]

On 12/30/2016 08:20 PM, Kurt Van Dijck wrote:

>>> To add IFLA_CAN_TERMINATION we would just need
>>>
>>> 	[IFLA_CAN_TERMINATION]   = { .type = NLA_U16 },
>
> Why would you limit the API to u16?
> I understand current common values fit perfectly in an u16, but limiting
> the API to what is common today, seems like a bad plan.
> the _only_ known value, 120, fits in an u8 even.
>
> u32 does not seem to add significant overhead IMHO, and protects against
> future developments?

There are some usual termination values for the distributed termination 
in the CAN Low Speed physical layer (ISO 11898-3).

E.g. http://www.peak-system.com/PCAN-TJA1054.219.0.html?&L=1
provides a switch to set the termination to 560 Ohm / 5.66 kOhm.

But beyond 20 kOhm the CAN termination seems to be really pointless.

You are that u32 values are no big deal in todays APIs - but when the 
netlink interface provides an u16 data structure and it fits the needed 
values: Why not use u16 then?

>>>
>>> to enable a defined termination value from 1 to 65534 Ohms where 65535
>>> would be CAN_RESISTOR_DISABLED. In the CAN driver this given value is
>>> checked against the supported list of termination values and switched on
>>> if supported.
>>> To provide a list of possible termination values with
>>> IFLA_CAN_TERMINATION_CONST we need to pass an array of u16 constants to
>>> the userspace.
>>>
>>> In opposite to our current netlink messages this array make contain 1..n
>>> constant values - so defining a fixed struct of e.g. 8 entries looks like
>>> a bad design.
>>>
>>> But how to provide the array to the netlink interface correctly?
>>>
>>> With the netlink type NLA_UNSPEC, NLA_NESTED, NLA_NESTED_COMPAT or
>>> NLA_BINARY??
>>>
>>> See http://lxr.linux.no/#linux+v4.8.10/include/net/netlink.h#L160
>>>
>>> The current
>>>
>>> [IFLA_CAN_DATA_BITTIMING]  = { .len = sizeof(struct can_bittiming_const)
>>> },
>>>
>>> seems to use NLA_UNSPEC which means the .len value is the _minimum_ length
>>> value.
>>>
>>> Would this mean we can define
>>>
>>> [IFLA_CAN_TERMINATION_CONST]   = { .len = sizeof(__u16) },
>>>
>>> as a minimum length value and provide as much termination values as the
>>> CAN interface supports?
>>
>> OK. This would be a simpler option.
>>
>> Optionally, the can core could also check the given value against a list of valid termination values & round it to the nearest possible value. Something like below
>>
>> possible_termination_values = { 60, 120, 240  } /* some random numbers */
>>
>> - if user_val < 60 -> use 60
>> - if user_val > 240 -> use 240
>> - if user_val falls within a range of the possible values
>>     if ((range_low + (range_high - range_low) / 2)) <= user_val)
>> 	user_val = range_low;
>>     else
>>       user_val = range_high;
>>
>>    ret = driver_callback(user_val)
>>
>> I felt the error reporting would be consistent. It is up to you.
>
> I think that it is a bad idea to implement rounding in kernel space.

Yes - I feel the same.

> The algorithm you specify is rather trivial, and I'd do the same, but
> is subject to change and belongs to userspace.
>
> When rounding to the nearest is done in the kernel, that happens on
> places where the nearest is not communicated to userspace, which has
> been solved in this case by providing a list of possible values.

ACK. As the plan is to provide this list userspace has to pick one of 
these values and the kernel can easily check for matches.

Regards,
Oliver

Re: termination?

[Kurt Van Dijck]

> Hi Remik,
> 
> On 10/21/2016 08:47 PM, Kołłątaj, Remigiusz wrote:
> >>I was just looking through your code and wondered whether we should support
> >>the termination switching feature via 'ip' tool or via sysfs.
> >>
> >>Just added Marc in CC.
> >>
> >
> >I tried to use 'ip' for setting up termination, but I couldn't find a
> >way. If this is possible I will modify the driver as it seems to be
> >better idea. Could you give me some hints?
> 
> No - not now :-)
> 
> That's why I wanted to start the discussion whether we should add this kind
> of feature to 'ip'.

selectable 120Ohm termination sounds very familiar, I'm sure I encountered >1
devices in the past offering this, so it's a good thing to discuss.

Looking to the (dis)advantages of both methods, I'd vote for extending
the netlink interface.
Keeping the sysfs interface semantically equal between drivers is hard,
not speaking on the kernel internal issues.

Kurt