linux-kernel - Re: [PATCH v2 2/2] gpio: gpio-adg1414: New driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZR8X17Bn-i2anqjxf0Gk60V175F7Xfwytkhy7_K+LsSA@mail.gmail.com>
Date: Fri, 14 Feb 2025 00:25:06 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Kim Seer Paller <kimseer.paller@...log.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-gpio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] gpio: gpio-adg1414: New driver

Hi Kim,

thanks for your patch!

On Thu, Feb 13, 2025 at 2:17 PM Kim Seer Paller
<kimseer.paller@...log.com> wrote:

> 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@...log.com>

OK so I looked at the data sheet and it looks like this:

A  o-------/ --------o B

It'a a switch.

Why is this switch a "gpio", other than that it is convenient
to use the GPIO abstraction to control it?

GPIO is usually devices that can drive a line high or low.
This is very far from that. This could switch some analog
line or whatever, right?

Now, the kernel does not have switch subsystem I think,
so this is something like a special case, so we might be
compelled to make an exception, if the users will all be in
say userspace and make use of this switch for factory lines
or similar.

Otherwise there is also drivers/mux, but maybe a 1:1
mux is an odd type of switch...

> +static int adg1414_spi_write(void *context, const void *data, size_t count)
> +{
> +       struct adg1414_state *st = context;
> +
> +       struct spi_transfer xfer = {
> +               .tx_buf = &st->tx,
> +               .len = count,
> +       };
> +
> +       return spi_sync_transfer(st->spi, &xfer, 1);
> +}
> +
> +static int adg1414_spi_read(void *context, const void *reg, size_t reg_size,
> +                           void *val, size_t val_size)
> +{
> +       return 0;
> +}
> +
> +static int adg1414_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct adg1414_state *st = gpiochip_get_data(chip);
> +
> +       guard(mutex)(&st->lock);
> +
> +       return st->buf & BIT(offset);
> +}
> +
> +static void adg1414_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +       struct adg1414_state *st = gpiochip_get_data(chip);
> +
> +       guard(mutex)(&st->lock);
> +
> +       if (value)
> +               st->buf |= BIT(offset);
> +       else
> +               st->buf &= ~BIT(offset);
> +
> +       st->tx = cpu_to_be32(st->buf << (32 - st->chip.ngpio));
> +
> +       adg1414_spi_write(st, 0, st->chip.ngpio / 8);
> +}
> +
> +static const struct regmap_bus adg1414_regmap_bus = {
> +       .write = adg1414_spi_write,
> +       .read = adg1414_spi_read,
> +       .reg_format_endian_default = REGMAP_ENDIAN_BIG,
> +       .val_format_endian_default = REGMAP_ENDIAN_BIG,
> +};
> +
> +static const struct regmap_config adg1414_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +};

This is not how regmap works. Your get/set callbacks for GPIO
should call regmap_get() and regmap_update_bits() to get/set
the individual bits.

So the adg1414_spi_write() needs to be done from inside the
regmap abstraction.

Yours,
Linus Walleij