add adg1414 driver #2344
add adg1414 driver #2344
Conversation
| spi-max-frequency: | ||
| maximum: 50000000 | ||
|
|
||
| spi-cpha: true |
There was a problem hiding this comment.
drop this after including $ref: /schemas/spi/spi-peripheral-props.yaml#
There was a problem hiding this comment.
i checked other drivers and found out that most of them still include the spi-cpha
| struct gpio_chip chip; | ||
| struct mutex lock; /* protect sensor state */ | ||
| u8 buffer[32]; | ||
|
|
There was a problem hiding this comment.
checkpatch will show a warning when removing the extra line. I think i have to retain it
|
v2: dt-bindings driver |
|
Hi, just to follow up, is this ready to be sent upstream? |
I'll have a look today |
| description: GPIO specifier that resets the device. | ||
| maxItems: 1 | ||
|
|
||
| daisy-chained-devices: |
There was a problem hiding this comment.
I see this in Documentation/devicetree/bindings/common-properties.txt as #daisy-chained-devices. However I'm not sure if there's any schema for it? Are your bindings passsing validation?
There was a problem hiding this comment.
AFAIK, it passed the binding check
There was a problem hiding this comment.
i'm not sure if this is how to properly define this
'#daisy-chained-devices':
description: The number of daisy-chained devices.
default: 1
minimum: 1
maximum: 4
| struct gpio_chip chip; | ||
| u8 buffer[32]; | ||
|
|
||
| __be32 tx __aligned(ARCH_KMALLOC_MINALIGN); |
There was a problem hiding this comment.
don't forget to adapt this upstream
drivers/gpio/gpio-adg1414.c
Outdated
|
|
||
| st->chip.label = "adg1414"; | ||
| st->chip.parent = dev; | ||
| st->chip.owner = THIS_MODULE; |
drivers/gpio/gpio-adg1414.c
Outdated
| return dev_err_probe(dev, PTR_ERR(reset), "Failed to get reset gpio"); | ||
|
|
||
| if (reset) { | ||
| ndelay(15); |
drivers/gpio/gpio-adg1414.c
Outdated
|
|
||
| if (reset) { | ||
| ndelay(15); | ||
| gpiod_set_value_cansleep(reset, 1); |
There was a problem hiding this comment.
gpiod_set_value_cansleep(reset, 0), right?
| &num_devices); | ||
| if (!ret) { | ||
| if (num_devices > 4 || num_devices < 1) | ||
| return dev_err_probe(dev, ret, |
There was a problem hiding this comment.
some #defines would be nice instead of the raw numbers
| struct adg1414_state *st = gpiochip_get_data(chip); | ||
|
|
||
| return st->buffer[offset]; | ||
| } |
There was a problem hiding this comment.
AFAICT, there's no guaranteed locking in gpiolib and you're accessing device shared data in adg1414_get() and adg1414_st(). Your buffer and tx should likely be properly protected.
The ADG1414 is a 9.5 Ω RON ±15 V/+12 V/±5 V iCMOS Serially-Controlled Octal SPST Switches Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
The ADG1414 is a 9.5 Ω RON ±15 V/+12 V/±5 V iCMOS Serially-Controlled Octal SPST Switches Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
|
v3 modified daisy-chained-devices property |
| @@ -1606,6 +1606,16 @@ config GPIO_74X164 | |||
| shift registers. This driver can be used to provide access | |||
| to more GPIO outputs. | |||
|
|
|||
| config GPIO_ADG1414 | |||
| tristate "ADG1414 SPST Switch Driver" | |||
| depends on GPIOLIB && SPI | |||
There was a problem hiding this comment.
I'm fairly sure you do not need to depend on GPIOLIB
| struct adg1414_state { | ||
| struct spi_device *spi; | ||
| struct gpio_chip chip; | ||
| struct mutex lock; |
There was a problem hiding this comment.
the mutex needs a comment stating why we need it. Otherwise checkpatch will through a warning
| struct adg1414_state *st = gpiochip_get_data(chip); | ||
| u32 tx = 0; | ||
| int i, ret; | ||
|
|
| "Failed to get reset gpio"); | ||
|
|
||
| if (reset) { | ||
| fsleep(0.015); |
There was a problem hiding this comment.
ahh? Did you tested this? This is not going to work... do you really want nanoseconds in here? Just do fsleep(1)...
| ret = device_property_read_u32(dev, "#daisy-chained-devices", | ||
| &num_devices); | ||
| if (!ret) { | ||
| if (num_devices > ADG1414_MAX_DEVICES || num_devices < 1) |
There was a problem hiding this comment.
use !num_devices instead of num_devices < 1
|
|
||
| mutex_lock(&st->lock); | ||
|
|
||
| st->buffer[offset] = value; |
There was a problem hiding this comment.
maybe your buffer could just be a bitmap... then you can use things like set_bit() or clear_bit() to make the code simpler. Also take a look here:
https://elixir.bootlin.com/linux/latest/source/include/linux/bitmap.h
see if you find anything you can use to make the code simpler... Basically I'm fairly sure that you can likely refactor things so that for() loop is not really needed.
the driver was tested on rpi4 with eval-adg1414
datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADG1414.pdf