Re: [PATCH v2 03/15] HID: ft260: add GPIO support

[Bartosz Golaszewski]

On Sat, Oct 31, 2026 at 12:36:25AM +0200, Michael Zaidman wrote:
> Add gpiochip support for the FT260 multifunctional pins, including
> GPIO2/GPIOA/GPIOG function selection via sysfs, and enable the
> available GPIOs according to the active UART mode.
> 
> This supersedes the earlier pre-UART GPIO series:
> https://lore.kernel.org/lkml/20230211115752.26276-2-michael.zaidman@xxxxxxxxx/
> 
> The gpio_chip.set callback returns int to match the current gpiolib
> API, based on the adaptation originally contributed by Rio Liu
> <rio@xxxxxx>.
> 
> Read the current GPIO state before writing so a set does not clobber
> other pins with a zero shadow. Apply direction and value in one
> feature report, and return a failed value write to gpiolib. UART mode
> OFF clears only UART-owned bits. Reject an out-of-range uart_mode
> index (reported by Sashiko).
> 
> Signed-off-by: Michael Zaidman <michael.zaidman@xxxxxxxxx>
> 
> ---

There's a lot of sashiko comments to address but a few additional nits
below.

...

> +
> +static int ft260_gpio_set(struct gpio_chip *gc, u32 offset, int value)
> +{
> +	int ret = 0;
> +	struct ft260_gpio_read_request_report buf;
> +	struct ft260_gpio_write_request_report rep;
> +	struct ft260_device *dev = gpiochip_get_data(gc);
> +	struct hid_device *hdev = dev->hdev;
> +
> +	if (offset >= FT260_GPIO_TOTAL) {
> +		hid_err(hdev, "%s: invalid offset %d\n", __func__, offset);
> +		return -EINVAL;
> +	}
> +
> +	ft260_dbg("offset %d val %d\n", offset, value);
> +
> +	mutex_lock(&dev->lock);

Use lock guards from linux/cleanup.h.

> +
> +	if (!(dev->gpio_en & BIT(offset))) {
> +		hid_err(hdev, "%s: wrong pin function %d\n", __func__, offset);
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	ret = ft260_hid_feature_report_get(hdev, FT260_GPIO, (u8 *)&buf,
> +					   sizeof(buf));
> +	if (unlikely(ret < 0)) {
> +		hid_err(hdev, "%s: cannot get GPIO: %d\n", __func__, ret);
> +		goto exit;
> +	}
> +
> +	rep.report = FT260_GPIO;
> +	rep.gpio = buf.gpio;
> +
> +	if (offset < FT260_GPIO_0_5_COUNT) {
> +		if (value)
> +			rep.gpio.vals |= !!value << offset;
> +		else
> +			rep.gpio.vals &= ~BIT(offset);
> +	} else {
> +		offset = offset - FT260_GPIO_0_5_COUNT;
> +		if (value)
> +			rep.gpio.ex_vals |= !!value << offset;
> +		else
> +			rep.gpio.ex_vals &= ~BIT(offset);
> +	}
> +
> +	ft260_dbg("dirs %#02x vals %#02x ex_dir %#02x ex_vals %#02x\n",
> +		  rep.gpio.dirs, rep.gpio.vals,
> +		  rep.gpio.ex_dirs, rep.gpio.ex_vals);
> +
> +	ret = ft260_hid_feature_report_set(hdev, (u8 *)&rep, sizeof(rep));
> +	if (unlikely(ret < 0)) {
> +		hid_err(hdev, "%s: cannot set GPIO: %d\n", __func__, ret);
> +		goto exit;
> +	}
> +
> +	dev->gpio = rep.gpio;
> +	ret = 0;
> +exit:
> +	mutex_unlock(&dev->lock);

That way you'll avoid this label. Same elsewhere.

> +	return ret;
> +}
> +
> +
> +	hid_info(hdev, "enabled GPIOs: %04x\n", dev->gpio_en);
> +
> +	dev->gc = devm_kzalloc(&hdev->dev, sizeof(*dev->gc), GFP_KERNEL);
> +	if (!dev->gc)
> +		return -ENOMEM;

Why is this dynamically allocated? Make it part of ft260_device.

> +
> +	label_sz = strlen(dev_name(&hdev->dev)) + strlen(prefix) + 1;
> +	label = devm_kzalloc(&hdev->dev, label_sz, GFP_KERNEL);
> +	if (!label) {
> +		ret = -ENOMEM;
> +		goto exit;

Just return here? Why the label?

> +	}
> +	snprintf(label, label_sz, "%s%s", prefix, dev_name(&hdev->dev));

devm_kasprintf()?

> +	hid_info(hdev, "initialize gpio chip on %s\n", label);

Drop logs like this, they add nothing except useless noise. Same
elsewhere.

> +
> +	dev->gc->label			= label;
> +	dev->gc->direction_input	= ft260_gpio_direction_input;
> +	dev->gc->direction_output	= ft260_gpio_direction_output;
> +	dev->gc->get_direction		= ft260_gpio_get_direction;
> +	dev->gc->set			= ft260_gpio_set;
> +	dev->gc->get			= ft260_gpio_get;
> +	dev->gc->base			= -1;
> +	dev->gc->ngpio			= FT260_GPIO_TOTAL;
> +	dev->gc->can_sleep		= true;
> +	dev->gc->parent			= &hdev->dev;
> +
> +	/* Wakeup chip */
> +	(void)ft260_hid_feature_report_get(dev->hdev, FT260_CHIP_VERSION,
> +					(u8 *)&ver, sizeof(ver));
> +
> +	ret = ft260_hid_feature_report_get(hdev, FT260_GPIO, (u8 *)&gbuf,
> +					   sizeof(gbuf));
> +	if (ret < 0)
> +		hid_warn(hdev, "cannot read GPIO state: %d\n", ret);
> +	else
> +		dev->gpio = gbuf.gpio;
> +
> +	ret = devm_gpiochip_add_data(&hdev->dev, dev->gc, dev);
> +	if (ret < 0)
> +		hid_err(hdev, "cannot add GPIO chip %d\n", ret);
> +exit:
> +	return ret;
> +}
> +
>  static int ft260_get_system_config(struct hid_device *hdev,
>  				   struct ft260_get_system_status_report *cfg)
 
Bart