Re: [PATCH v13 3/3] gpio: gpio-ltc4283: Add support for the LTC4283 Swap Controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware glitch in `ltc4283_gpio_direction_output` because pin direction is set before output value.
- [High] NULL pointer dereference in `ltc4283_init_valid_mask` if platform data is not provided.
- [High] Missing PGIO pin initialization logic contrary to commit message, leading to incorrect state reporting.
--

commit 75b887d21b6291239c2e7e790fcef64a2fef85d7
Author: Nuno Sá <nuno.sa@analog.com>

gpio: gpio-ltc4283: Add support for the LTC4283 Swap Controller

This patch introduces GPIO support for the Analog Devices LTC4283 Swap
Controller. It adds a new driver via the auxiliary bus interface to manage
up to 8 pins configurable as GPIOs.

> Note that PGIO pins are not set as GPIOs by default so if they are
> configured to be used as GPIOs we need to make sure to initialize them
> to a sane default. They are set as inputs by default.

Is the PGIO pin initialization logic missing from the probe sequence?

The commit message mentions initializing them to a sane default, but there
doesn't appear to be any logic in the driver's probe sequence to enforce
this. If the hardware default config for these pins is not
LTC4283_PGIO_DIR_IN, ltc4283_pgio_get_direction() will return
GPIO_LINE_DIRECTION_OUT for any other value, leading the GPIO framework
to mistakenly report the uninitialized pins as outputs.

[ ... ]

> diff --git a/drivers/gpio/gpio-ltc4283.c b/drivers/gpio/gpio-ltc4283.c
> new file mode 100644
> index 000000000000..6609443c5d62
> --- /dev/null
> +++ b/drivers/gpio/gpio-ltc4283.c

[ ... ]

> +static int ltc4283_gpio_direction_output(struct gpio_chip *gc, unsigned int off, int val)
> +{
> +	struct ltc4283_gpio *st = gpiochip_get_data(gc);
> +	int ret;
> +
> +	ret = ltc4283_gpio_direction_set(st, off, false);
> +	if (ret)
> +		return ret;
> +
> +	return __ltc4283_gpio_set_value(st, off, val);
> +}

Does this sequence cause a hardware glitch? 

By calling ltc4283_gpio_direction_set() before __ltc4283_gpio_set_value(),
the pin is configured as an output before the new value is written. As these
are two separate regmap operations, the hardware will briefly drive the old
value present in the output register onto the pin.

Should the intended output value be set first, and only then enable the
output direction to ensure a glitch-free transition?

[ ... ]

> +static int ltc4283_init_valid_mask(struct gpio_chip *gc, unsigned long *valid_mask,
> +				   unsigned int ngpios)
> +{
> +	unsigned long *mask = dev_get_platdata(gc->parent);
> +
> +	bitmap_copy(valid_mask, mask, ngpios);
> +	return 0;
> +}

Can the mask pointer be NULL here if the parent device does not provide
platform data?

If dev_get_platdata() returns NULL, passing it to bitmap_copy() will
result in a NULL pointer dereference, as bitmap_copy() does not internally
check for a NULL source pointer.

Would it be safer to add a check to handle the absence of platform data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260502-ltc4283-support-v13-0-1c206542e652@analog.com?part=3