Bias resistor kernel version check

[CrazyIvan359]

So I learned yesterday that bias resistor settings are a recent addition to the Linux kernel (v5.5). In all my digging to get to the bottom of the ambiguous error 22s I was getting, I found out that ioctl calls to pinctrl should allow control of bias resistors (I have not looked into how yet).

I was going to implement kernel version checks and have my downstream software decide when to use this alternate method, but then I thought why do that when I can contribute it upstream to the library!

For the cdev class it will be pretty easy to add branching to use the alternate method if a bias setting is specified, but I wanted your input on how to do it for sysfs or if it should be done. My thought is just to add a named arg bias to the sysfs class init function. That would maintain backwards compatibility and not break the logic that counts unnamed args to determine if the user is trying to get a sysfs or cdev instance. What are you thoughts on this?

Edit: using pinctrl we could actually expand the sysfs class to offer all of the settings that cdev does with the exception of edge and label.

[CrazyIvan359]

Never mind... :( No ioctl calls are available for pinctrl, it is internal to the kernel. Only way to use it is via device tree.

Want me to add the kernel version check at least? The error 22 invalid argument is extremely vague and unhelpful, I can set it up to log a warning and not add the bias flag to the request if running kernel <5.5.

[vsergeev]

Yeah, a kernel version check via os.uname() would be a good improvement. It should happen once and assign a static variable in the CdevGpio class like _SUPPORTS_LINE_BIAS. We could do something similar in SPI for the 32-bit mode flags as well.

[CrazyIvan359]

I've already got the version into a tuple that can be compared like if KERNEL_VERSION < (5, 5):

KERNEL_VERSION = tuple([int(s) for s in platform.release().split(".")[:2]])

For SPI do you know what version it was added? I'm only using the GPIO portion of this library (so far). Also do you want separate PRs for each module?

[CrazyIvan359]

Actually, a safer way to do the version check would be:

try:
    KERNEL_VERSION = tuple([int(s) for s in platform.release().split(".")[:2]])
except ValueError:
    KERNEL_VERSION = (0, 0)

[CrazyIvan359]

Should attempting to use a bias setting when they are not supported raise an error? There is no use of logging in the library so I'm not sure how else we tell the user. The other option is to silently clear the bias setting, but then should the bias property also silently be a noop or should it throw a ValueError?

[vsergeev]

I've already got the version into a tuple that can be compared like if KERNEL_VERSION < (5, 5):

I think it would still be best to register these checks (there will be one for line drive as well) once at the top of the class, e.g.

class CdevGPIO:
    ...
    _SUPPORTS_LINE_BIAS = KERNEL_VERSION > (...)
    _SUPPORTS_LINE_DRIVE = KERNEL_VERSION > (...)

and then use them below with if CdevGPIO._SUPPORTS_LINE_DRIVE: ....

For SPI do you know what version it was added? I'm only using the GPIO portion of this library (so far). Also do you want separate PRs for each module?

It looks like for SPI, MODE32 was added in v3.15. I can do the SPI one if you prefer, that one is a little tricky since it already tries to use mode32 opportunistically, which will probably need to get refactored with this check.

Actually, a safer way to do the version check would be:

try:
    KERNEL_VERSION = tuple([int(s) for s in platform.release().split(".")[:2]])
except ValueError:
    KERNEL_VERSION = (0, 0)

OK, looks good.

Should attempting to use a bias setting when they are not supported raise an error? There is no use of logging in the library so I'm not sure how else we tell the user. The other option is to silently clear the bias setting, but then should the bias property also silently be a noop or should it throw a ValueError?

There isn't a great native Python error for unsupported features, so I think it should return a GPIOError with a message similar to the other peripheries: https://github.com/vsergeev/c-periphery/blob/master/src/gpio.c#L881 . Ideally, python-periphery would have more descriptive error types that subclass GPIOError, but that can be done in the future.

[CrazyIvan359]

Ok I'll submit a PR tonight with the changes for GPIO and leave SPI up to you since I'm not familiar with the code.

[vsergeev]

Cherry-picked in 9c1a4f3 in devel. This issue should auto-close in the next release to master.