add adg1414 driver #2344

kseerp · 2023-11-15T02:23:50Z

the driver was tested on rpi4 with eval-adg1414

datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADG1414.pdf

amiclaus

some comments on my side. I'd suggest sending this upstream first (mainline kernel), since the datasheet is public.

Documentation/devicetree/bindings/gpio/gpio-adg1414.yaml

amiclaus · 2023-11-16T09:29:31Z

Documentation/devicetree/bindings/gpio/gpio-adg1414.yaml

+  spi-max-frequency:
+    maximum: 50000000
+
+  spi-cpha: true


drop this after including $ref: /schemas/spi/spi-peripheral-props.yaml#

i checked other drivers and found out that most of them still include the spi-cpha

amiclaus · 2023-11-16T09:29:53Z

drivers/gpio/gpio-adg1414.c

+	struct gpio_chip		chip;
+	struct mutex			lock; /* protect sensor state */
+	u8				buffer[32];
+


drop extra line

checkpatch will show a warning when removing the extra line. I think i have to retain it

drivers/gpio/gpio-adg1414.c

kseerp · 2023-11-17T02:18:44Z

v2:

dt-bindings
added $ref: /schemas/spi/spi-peripheral-props.yaml#
dropped spi-max-frequency property
replaced additionalProperties to unevaluatedProperties

driver
placed ARCH_KMALLOC_MINALIGN inline
dropped lock/unlock

kseerp · 2023-11-24T01:10:00Z

Hi, just to follow up, is this ready to be sent upstream?

nunojsa · 2023-11-24T08:40:49Z

Hi, just to follow up, is this ready to be sent upstream?

I'll have a look today

nunojsa · 2023-11-24T10:01:05Z

Documentation/devicetree/bindings/gpio/gpio-adg1414.yaml

+    description: GPIO specifier that resets the device.
+    maxItems: 1
+
+  daisy-chained-devices:


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?

AFAIK, it passed the binding check

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

nunojsa · 2023-11-24T10:04:22Z

drivers/gpio/gpio-adg1414.c

+	struct gpio_chip		chip;
+	u8				buffer[32];
+
+	__be32				tx __aligned(ARCH_KMALLOC_MINALIGN);


don't forget to adapt this upstream

nunojsa · 2023-11-24T10:04:45Z

drivers/gpio/gpio-adg1414.c

+
+	st->chip.label = "adg1414";
+	st->chip.parent = dev;
+	st->chip.owner = THIS_MODULE;


no need for it

nunojsa · 2023-11-24T10:04:59Z

drivers/gpio/gpio-adg1414.c

+		return dev_err_probe(dev, PTR_ERR(reset), "Failed to get reset gpio");
+
+	if (reset) {
+		ndelay(15);


nunojsa · 2023-11-24T10:05:32Z

drivers/gpio/gpio-adg1414.c

+
+	if (reset) {
+		ndelay(15);
+		gpiod_set_value_cansleep(reset, 1);


gpiod_set_value_cansleep(reset, 0), right?

nunojsa · 2023-11-24T10:06:10Z

drivers/gpio/gpio-adg1414.c

+				       &num_devices);
+	if (!ret) {
+		if (num_devices > 4 || num_devices < 1)
+			return dev_err_probe(dev, ret,


some #defines would be nice instead of the raw numbers

nunojsa · 2023-11-24T10:10:12Z

drivers/gpio/gpio-adg1414.c

+	struct adg1414_state *st = gpiochip_get_data(chip);
+
+	return st->buffer[offset];
+}


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 <[email protected]>

kseerp · 2023-11-29T16:58:46Z

v3

modified daisy-chained-devices property
added mutex lock
replaced ndelay to fsleep
changed gpiod_set_value_cansleep to correct state

nunojsa

alright, check my comments and feel free to send this upstream. Again, I'm not sure if gpio is really the right place for this but it feels to be the only acceptable place I'm aware of...

nunojsa · 2023-12-12T14:38:54Z

drivers/gpio/Kconfig

@@ -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


I'm fairly sure you do not need to depend on GPIOLIB

nunojsa · 2023-12-12T14:39:27Z

drivers/gpio/gpio-adg1414.c

+struct adg1414_state {
+	struct spi_device		*spi;
+	struct gpio_chip		chip;
+	struct mutex			lock;


the mutex needs a comment stating why we need it. Otherwise checkpatch will through a warning

nunojsa · 2023-12-12T14:39:40Z

drivers/gpio/gpio-adg1414.c

+	struct adg1414_state *st = gpiochip_get_data(chip);
+	u32 tx = 0;
+	int i, ret;
+


nit: no extra line

nunojsa · 2023-12-12T14:42:52Z

drivers/gpio/gpio-adg1414.c

+				     "Failed to get reset gpio");
+
+	if (reset) {
+		fsleep(0.015);


ahh? Did you tested this? This is not going to work... do you really want nanoseconds in here? Just do fsleep(1)...

nunojsa · 2023-12-12T14:43:53Z

drivers/gpio/gpio-adg1414.c

+	ret = device_property_read_u32(dev, "#daisy-chained-devices",
+				       &num_devices);
+	if (!ret) {
+		if (num_devices > ADG1414_MAX_DEVICES || num_devices < 1)


use !num_devices instead of num_devices < 1

nunojsa · 2023-12-12T14:48:10Z

drivers/gpio/gpio-adg1414.c

+
+	mutex_lock(&st->lock);
+
+	st->buffer[offset] = value;


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.

nunojsa · 2024-06-05T13:15:50Z

@kseerp, what's the status of this one? Was it upstreamed? Can this be closed?

kseerp · 2024-06-10T07:36:11Z

@kseerp, what's the status of this one? Was it upstreamed? Can this be closed?

patch series sent for v1. Though this was a long time, I'm planning to continue and follow up on the development.

amiclaus reviewed Nov 16, 2023

View reviewed changes

kseerp force-pushed the dev/adg1414 branch from cdb150c to 56060ba Compare November 17, 2023 02:14

nunojsa reviewed Nov 24, 2023

View reviewed changes

kseerp force-pushed the dev/adg1414 branch from c2d2c15 to e97cf30 Compare November 29, 2023 15:16

kseerp closed this Nov 29, 2023

kseerp force-pushed the dev/adg1414 branch from e97cf30 to 14a49f9 Compare November 29, 2023 16:29

kseerp added 2 commits November 30, 2023 00:43

dt-bindings: gpio: add adg1414

1f0bb5b

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 <[email protected]>

gpio: gpio-adg1414: new driver

148efee

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 <[email protected]>

kseerp reopened this Nov 29, 2023

nunojsa reviewed Dec 12, 2023

View reviewed changes

SRaus changed the base branch from master to main January 9, 2024 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add adg1414 driver #2344

add adg1414 driver #2344

kseerp commented Nov 15, 2023

amiclaus left a comment

amiclaus Nov 16, 2023

kseerp Nov 17, 2023

amiclaus Nov 16, 2023

kseerp Nov 17, 2023

kseerp commented Nov 17, 2023

kseerp commented Nov 24, 2023

nunojsa commented Nov 24, 2023

nunojsa Nov 24, 2023

kseerp Nov 29, 2023

kseerp Nov 29, 2023

nunojsa Nov 24, 2023

nunojsa Nov 24, 2023

nunojsa Nov 24, 2023

nunojsa Nov 24, 2023

nunojsa Nov 24, 2023

nunojsa Nov 24, 2023

kseerp commented Nov 29, 2023

nunojsa left a comment

nunojsa Dec 12, 2023

nunojsa Dec 12, 2023

nunojsa Dec 12, 2023

nunojsa Dec 12, 2023

nunojsa Dec 12, 2023

nunojsa Dec 12, 2023

nunojsa commented Jun 5, 2024

kseerp commented Jun 10, 2024

add adg1414 driver #2344

Are you sure you want to change the base?

add adg1414 driver #2344

Conversation

kseerp commented Nov 15, 2023

amiclaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kseerp commented Nov 17, 2023

kseerp commented Nov 24, 2023

nunojsa commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kseerp commented Nov 29, 2023

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nunojsa commented Jun 5, 2024

kseerp commented Jun 10, 2024