Re: [PATCH RFC v2 3/6] gpio: aggregator: export symbols of the gpio-fwd library

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Mar 17, 2025 at 04:38:01PM +0100, Thomas Richard wrote:
> Export all symbols and create header file for the gpio-fwd library.

...

>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
> +#include <linux/gpio/gpio-fwd.h>

Please, name it forwarder.h.

>  #include <linux/gpio/machine.h>

...

> +int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>  
>  	return gpiod_get_direction(fwd->descs[offset]);
>  }
> +EXPORT_SYMBOL_GPL(gpio_fwd_get_direction);

No namespace? Ditto for all exports.

> -static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
> -				 unsigned long *bits)
> +static int gpio_fwd_get_multiple_unlocked(struct gpiochip_fwd *fwd,
> +					  unsigned long *mask, unsigned long *bits)
>  {
>  	struct gpio_desc **descs = fwd_tmp_descs(fwd);
>  	unsigned long *values = fwd_tmp_values(fwd);
> @@ -332,8 +320,8 @@ static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
>  	return 0;
>  }
>  
> -static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
> -					unsigned long *mask, unsigned long *bits)
> +int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +			  unsigned long *bits)
>  {
>  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>  	unsigned long flags;
> @@ -341,16 +329,17 @@ static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
>  
>  	if (chip->can_sleep) {
>  		mutex_lock(&fwd->mlock);
> -		error = gpio_fwd_get_multiple(fwd, mask, bits);
> +		error = gpio_fwd_get_multiple_unlocked(fwd, mask, bits);
>  		mutex_unlock(&fwd->mlock);
>  	} else {
>  		spin_lock_irqsave(&fwd->slock, flags);
> -		error = gpio_fwd_get_multiple(fwd, mask, bits);
> +		error = gpio_fwd_get_multiple_unlocked(fwd, mask, bits);
>  		spin_unlock_irqrestore(&fwd->slock, flags);
>  	}
>  
>  	return error;
>  }
> +EXPORT_SYMBOL_GPL(gpio_fwd_get_multiple);

These two are nicely named. Instead of touching them, just simply add an
exported wrapper. We can optimize it latter if needed, but it reduces a lot
the churn in this patch.

...

> -static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
> -				  unsigned long *bits)
> +static void gpio_fwd_set_multiple_unlocked(struct gpiochip_fwd *fwd,
> +					   unsigned long *mask,
> +					   unsigned long *bits)
>  {
>  	struct gpio_desc **descs = fwd_tmp_descs(fwd);
>  	unsigned long *values = fwd_tmp_values(fwd);
> @@ -404,37 +395,40 @@ static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
>  		gpiod_set_array_value(j, descs, NULL, values);
>  }
>  
> -static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
> -					 unsigned long *mask, unsigned long *bits)
> +void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +			   unsigned long *bits)
>  {
>  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>  	unsigned long flags;
>  
>  	if (chip->can_sleep) {
>  		mutex_lock(&fwd->mlock);
> -		gpio_fwd_set_multiple(fwd, mask, bits);
> +		gpio_fwd_set_multiple_unlocked(fwd, mask, bits);
>  		mutex_unlock(&fwd->mlock);
>  	} else {
>  		spin_lock_irqsave(&fwd->slock, flags);
> -		gpio_fwd_set_multiple(fwd, mask, bits);
> +		gpio_fwd_set_multiple_unlocked(fwd, mask, bits);
>  		spin_unlock_irqrestore(&fwd->slock, flags);
>  	}
>  }
> +EXPORT_SYMBOL_GPL(gpio_fwd_set_multiple);

Ditto.

...

> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_GPIO_FWD_H
> +#define __LINUX_GPIO_FWD_H

This header uses something that is defined in other headers. Please follow IWYU
principle.

...

> +struct gpiochip_fwd_timing {
> +	u32 ramp_up_us;
> +	u32 ramp_down_us;

types.h

> +};

...

> +struct gpiochip_fwd {
> +	struct device *dev;

struct device;

// forward declaration is enough.

> +	struct gpio_chip chip;

Where is this being defined?

> +	struct gpio_desc **descs;

> +	union {
> +		struct mutex mlock;	/* protects tmp[] if can_sleep */
> +		spinlock_t slock;	/* protects tmp[] if !can_sleep */

And these?

> +	};
> +	struct gpiochip_fwd_timing *delay_timings;
> +	unsigned long tmp[];		/* values and descs for multiple ops */
> +};

-- 
With Best Regards,
Andy Shevchenko