Message-ID: <07fd6855ef46f4815940187bd85c9b191e9054c2.camel@codeconstruct.com.au>
Date: Mon, 16 Sep 2024 12:13:15 +0930
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Billy Tsai <billy_tsai@...eedtech.com>, linus.walleij@...aro.org, 
	brgl@...ev.pl, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	joel@....id.au, linux-gpio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-aspeed@...ts.ozlabs.org, 
	linux-kernel@...r.kernel.org, BMC-SW@...eedtech.com, Peter.Yin@...ntatw.com
Subject: Re: [PATCH v3 3/6] gpio: aspeed: Create llops to handle hardware
 access

On Fri, 2024-09-13 at 15:43 +0800, Billy Tsai wrote:
> Add low-level operations (llops) to abstract the register access for GPIO
> registers and the coprocessor request/release. With this abstraction
> layer, the driver can separate the hardware and software logic, making it
> easier to extend the driver to support different hardware register
> layouts.
> 
> Signed-off-by: Billy Tsai <billy_tsai@...eedtech.com>
> ---
>  drivers/gpio/gpio-aspeed.c | 366 ++++++++++++++++++-------------------
>  1 file changed, 178 insertions(+), 188 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index d20e15b2079d..40ca308054c6 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -39,6 +39,10 @@ struct aspeed_bank_props {
>  struct aspeed_gpio_config {
>  	unsigned int nr_gpios;
>  	const struct aspeed_bank_props *props;
> +	const struct aspeed_gpio_llops *llops;
> +	const int *debounce_timers_array;
> +	int debounce_timers_num;
> +	bool cmd_source_supoort;

Typo: s/cmd_source_supoort/cmd_source_support/

However, this is a derived value. It can be inferred like:

   static bool aspeed_gpio_supports_cmd_source(const struct aspeed_gpio_config *config)
   {
       return config->llops->copro_request && config->llops->coproc_release;
   }

Can we do that instead? That removes the opportunity for the member
values to become unsynchronised in bad ways.

>  };
>  
>  /*
> @@ -178,6 +182,15 @@ enum aspeed_gpio_reg {
>  	reg_cmdsrc1,
>  };
>  
> +struct aspeed_gpio_llops {
> +	bool (*copro_request)(struct aspeed_gpio *gpio, unsigned int offset);
> +	void (*copro_release)(struct aspeed_gpio *gpio, unsigned int offset);
> +	void (*reg_bits_set)(struct aspeed_gpio *gpio, unsigned int offset,
> +			     const enum aspeed_gpio_reg reg, u32 val);
> +	u32 (*reg_bits_read)(struct aspeed_gpio *gpio, unsigned int offset,

Guess I should have commented on this on v2, but maybe call this
`reg_bits_get()` for symmetry with `reg_bits_set()`?

I also have a comment on the use of "bits" in the member names below.

> +			     const enum aspeed_gpio_reg reg);
> +};
> +
>  #define GPIO_VAL_VALUE	0x00
>  #define GPIO_VAL_DIR	0x04
>  
> @@ -237,10 +250,6 @@ static inline void __iomem *bank_reg(struct aspeed_gpio *gpio,
>  #define GPIO_OFFSET(x)	((x) & 0x1f)
>  #define GPIO_BIT(x)	BIT(GPIO_OFFSET(x))
>  
> -#define _GPIO_SET_DEBOUNCE(t, o, i) ((!!((t) & BIT(i))) << GPIO_OFFSET(o))
> -#define GPIO_SET_DEBOUNCE1(t, o) _GPIO_SET_DEBOUNCE(t, o, 1)
> -#define GPIO_SET_DEBOUNCE2(t, o) _GPIO_SET_DEBOUNCE(t, o, 0)
> -
>  static const struct aspeed_gpio_bank *to_bank(unsigned int offset)
>  {
>  	unsigned int bank = GPIO_BANK(offset);
> @@ -296,107 +305,56 @@ static inline bool have_output(struct aspeed_gpio *gpio, unsigned int offset)
>  }
>  
>  static void aspeed_gpio_change_cmd_source(struct aspeed_gpio *gpio,
> -					  const struct aspeed_gpio_bank *bank,
> -					  int bindex, int cmdsrc)
> +					  unsigned int offset,
> +					  int cmdsrc)
>  {
> -	void __iomem *c0 = bank_reg(gpio, bank, reg_cmdsrc0);
> -	void __iomem *c1 = bank_reg(gpio, bank, reg_cmdsrc1);
> -	u32 bit, reg;
> -
>  	/*
> -	 * Each register controls 4 banks, so take the bottom 2
> -	 * bits of the bank index, and use them to select the
> -	 * right control bit (0, 8, 16 or 24).
> +	 * The command source register is only valid in bits 0, 8, 16, and 24, so we use
> +	 * (offset & ~(0x7)) to ensure that reg_bits_set always targets a valid bit.
>  	 */
> -	bit = BIT((bindex & 3) << 3);
> -
>  	/* Source 1 first to avoid illegal 11 combination */
> -	reg = ioread32(c1);
> -	if (cmdsrc & 2)
> -		reg |= bit;
> -	else
> -		reg &= ~bit;
> -	iowrite32(reg, c1);
> -
> +	gpio->config->llops->reg_bits_set(gpio, offset & ~(0x7), reg_cmdsrc1, !!(cmdsrc & BIT(1)));
>  	/* Then Source 0 */
> -	reg = ioread32(c0);
> -	if (cmdsrc & 1)
> -		reg |= bit;
> -	else
> -		reg &= ~bit;
> -	iowrite32(reg, c0);
> +	gpio->config->llops->reg_bits_set(gpio, offset & ~(0x7), reg_cmdsrc0, !!(cmdsrc & BIT(0)));
>  }
>  
>  static bool aspeed_gpio_copro_request(struct aspeed_gpio *gpio,
>  				      unsigned int offset)
>  {
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
> -
> -	if (!copro_ops || !gpio->cf_copro_bankmap)
> -		return false;
> -	if (!gpio->cf_copro_bankmap[offset >> 3])
> -		return false;
> -	if (!copro_ops->request_access)
> -		return false;
> +	if (gpio->config->llops->copro_request)
> +		return gpio->config->llops->copro_request(gpio, offset);
>  
> -	/* Pause the coprocessor */
> -	copro_ops->request_access(copro_data);
> -
> -	/* Change command source back to ARM */
> -	aspeed_gpio_change_cmd_source(gpio, bank, offset >> 3, GPIO_CMDSRC_ARM);
> -
> -	/* Update cache */
> -	gpio->dcache[GPIO_BANK(offset)] = ioread32(bank_reg(gpio, bank, reg_rdata));
> -
> -	return true;
> +	return false;
>  }
>  
>  static void aspeed_gpio_copro_release(struct aspeed_gpio *gpio,
>  				      unsigned int offset)
>  {
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
> -
> -	if (!copro_ops || !gpio->cf_copro_bankmap)
> -		return;
> -	if (!gpio->cf_copro_bankmap[offset >> 3])
> -		return;
> -	if (!copro_ops->release_access)
> -		return;
> -
> -	/* Change command source back to ColdFire */
> -	aspeed_gpio_change_cmd_source(gpio, bank, offset >> 3,
> -				      GPIO_CMDSRC_COLDFIRE);
> -
> -	/* Restart the coprocessor */
> -	copro_ops->release_access(copro_data);
> +	if (gpio->config->llops->copro_release)
> +		gpio->config->llops->copro_release(gpio, offset);
>  }
>  
>  static int aspeed_gpio_get(struct gpio_chip *gc, unsigned int offset)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
>  
> -	return !!(ioread32(bank_reg(gpio, bank, reg_val)) & GPIO_BIT(offset));
> +	return gpio->config->llops->reg_bits_read(gpio, offset, reg_val);
>  }
>  
>  static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
>  			      int val)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
> -	void __iomem *addr;
>  	u32 reg;
>  
> -	addr = bank_reg(gpio, bank, reg_val);
>  	reg = gpio->dcache[GPIO_BANK(offset)];
> -
>  	if (val)
>  		reg |= GPIO_BIT(offset);
>  	else
>  		reg &= ~GPIO_BIT(offset);
>  	gpio->dcache[GPIO_BANK(offset)] = reg;
>  
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_val, val);
>  }
>  
>  static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
> @@ -404,7 +362,7 @@ static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
>  	unsigned long flags;
> -	bool copro;
> +	bool copro = 0;

Isn't this a dead store? If it's not, can we use `false` instead to
align with the `bool` type?

If it is a dead store can you please address the rest of the instances
throughout the patch?

>  
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
> @@ -419,22 +377,16 @@ static void aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
>  static int aspeed_gpio_dir_in(struct gpio_chip *gc, unsigned int offset)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
> -	void __iomem *addr = bank_reg(gpio, bank, reg_dir);
>  	unsigned long flags;
> -	bool copro;
> -	u32 reg;
> +	bool copro = 0;
>  
>  	if (!have_input(gpio, offset))
>  		return -ENOTSUPP;
>  
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  
> -	reg = ioread32(addr);
> -	reg &= ~GPIO_BIT(offset);
> -
>  	copro = aspeed_gpio_copro_request(gpio, offset);
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_dir, 0);
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
>  
> @@ -447,23 +399,17 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
>  			       unsigned int offset, int val)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
> -	void __iomem *addr = bank_reg(gpio, bank, reg_dir);
>  	unsigned long flags;
> -	bool copro;
> -	u32 reg;
> +	bool copro = 0;
>  
>  	if (!have_output(gpio, offset))
>  		return -ENOTSUPP;
>  
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  
> -	reg = ioread32(addr);
> -	reg |= GPIO_BIT(offset);
> -
>  	copro = aspeed_gpio_copro_request(gpio, offset);
>  	__aspeed_gpio_set(gc, offset, val);
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_dir, 1);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -475,7 +421,6 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,
>  static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
>  	unsigned long flags;
>  	u32 val;
>  
> @@ -487,7 +432,7 @@ static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
>  
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  
> -	val = ioread32(bank_reg(gpio, bank, reg_dir)) & GPIO_BIT(offset);
> +	val = gpio->config->llops->reg_bits_read(gpio, offset, reg_dir);
>  
>  	raw_spin_unlock_irqrestore(&gpio->lock, flags);
>  
> @@ -496,8 +441,7 @@ static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
>  
>  static inline int irqd_to_aspeed_gpio_data(struct irq_data *d,
>  					   struct aspeed_gpio **gpio,
> -					   const struct aspeed_gpio_bank **bank,
> -					   u32 *bit, int *offset)
> +					   int *offset)
>  {
>  	struct aspeed_gpio *internal;
>  
> @@ -510,32 +454,25 @@ static inline int irqd_to_aspeed_gpio_data(struct irq_data *d,
>  		return -ENOTSUPP;
>  
>  	*gpio = internal;
> -	*bank = to_bank(*offset);
> -	*bit = GPIO_BIT(*offset);
>  
>  	return 0;
>  }
>  
>  static void aspeed_gpio_irq_ack(struct irq_data *d)
>  {
> -	const struct aspeed_gpio_bank *bank;
>  	struct aspeed_gpio *gpio;
>  	unsigned long flags;
> -	void __iomem *status_addr;
>  	int rc, offset;
> -	bool copro;
> -	u32 bit;
> +	bool copro = 0;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
>  	if (rc)
>  		return;
>  
> -	status_addr = bank_reg(gpio, bank, reg_irq_status);
> -
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
>  
> -	iowrite32(bit, status_addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_status, 1);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -544,20 +481,15 @@ static void aspeed_gpio_irq_ack(struct irq_data *d)
>  
>  static void aspeed_gpio_irq_set_mask(struct irq_data *d, bool set)
>  {
> -	const struct aspeed_gpio_bank *bank;
>  	struct aspeed_gpio *gpio;
>  	unsigned long flags;
> -	u32 reg, bit;
> -	void __iomem *addr;
>  	int rc, offset;
> -	bool copro;
> +	bool copro = 0;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
>  	if (rc)
>  		return;
>  
> -	addr = bank_reg(gpio, bank, reg_irq_enable);
> -
>  	/* Unmasking the IRQ */
>  	if (set)
>  		gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(d));
> @@ -565,12 +497,7 @@ static void aspeed_gpio_irq_set_mask(struct irq_data *d, bool set)
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
>  
> -	reg = ioread32(addr);
> -	if (set)
> -		reg |= bit;
> -	else
> -		reg &= ~bit;
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_enable, set);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -596,34 +523,31 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
>  	u32 type0 = 0;
>  	u32 type1 = 0;
>  	u32 type2 = 0;
> -	u32 bit, reg;
> -	const struct aspeed_gpio_bank *bank;
>  	irq_flow_handler_t handler;
>  	struct aspeed_gpio *gpio;
>  	unsigned long flags;
> -	void __iomem *addr;
>  	int rc, offset;
> -	bool copro;
> +	bool copro = 0;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
>  	if (rc)
>  		return -EINVAL;
>  
>  	switch (type & IRQ_TYPE_SENSE_MASK) {
>  	case IRQ_TYPE_EDGE_BOTH:
> -		type2 |= bit;
> +		type2 = 1;
>  		fallthrough;
>  	case IRQ_TYPE_EDGE_RISING:
> -		type0 |= bit;
> +		type0 = 1;
>  		fallthrough;
>  	case IRQ_TYPE_EDGE_FALLING:
>  		handler = handle_edge_irq;
>  		break;
>  	case IRQ_TYPE_LEVEL_HIGH:
> -		type0 |= bit;
> +		type0 = 1;
>  		fallthrough;
>  	case IRQ_TYPE_LEVEL_LOW:
> -		type1 |= bit;
> +		type1 = 1;
>  		handler = handle_level_irq;
>  		break;
>  	default:
> @@ -633,20 +557,9 @@ static int aspeed_gpio_set_type(struct irq_data *d, unsigned int type)
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
>  
> -	addr = bank_reg(gpio, bank, reg_irq_type0);
> -	reg = ioread32(addr);
> -	reg = (reg & ~bit) | type0;
> -	iowrite32(reg, addr);
> -
> -	addr = bank_reg(gpio, bank, reg_irq_type1);
> -	reg = ioread32(addr);
> -	reg = (reg & ~bit) | type1;
> -	iowrite32(reg, addr);
> -
> -	addr = bank_reg(gpio, bank, reg_irq_type2);
> -	reg = ioread32(addr);
> -	reg = (reg & ~bit) | type2;
> -	iowrite32(reg, addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_type0, type0);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_type1, type1);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_irq_type2, type2);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -661,7 +574,6 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
>  {
>  	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
>  	struct irq_chip *ic = irq_desc_get_chip(desc);
> -	struct aspeed_gpio *data = gpiochip_get_data(gc);
>  	unsigned int i, p, banks;
>  	unsigned long reg;
>  	struct aspeed_gpio *gpio = gpiochip_get_data(gc);
> @@ -670,9 +582,7 @@ static void aspeed_gpio_irq_handler(struct irq_desc *desc)
>  
>  	banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
>  	for (i = 0; i < banks; i++) {
> -		const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
> -
> -		reg = ioread32(bank_reg(data, bank, reg_irq_status));
> +		reg = gpio->config->llops->reg_bits_read(gpio, i, reg_irq_status);
>  
>  		for_each_set_bit(p, &reg, 32)
>  			generic_handle_domain_irq(gc->irq.domain, i * 32 + p);
> @@ -711,23 +621,12 @@ static int aspeed_gpio_reset_tolerance(struct gpio_chip *chip,
>  {
>  	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
>  	unsigned long flags;
> -	void __iomem *treg;
> -	bool copro;
> -	u32 val;
> -
> -	treg = bank_reg(gpio, to_bank(offset), reg_tolerance);
> +	bool copro = 0;
>  
>  	raw_spin_lock_irqsave(&gpio->lock, flags);
>  	copro = aspeed_gpio_copro_request(gpio, offset);
>  
> -	val = readl(treg);
> -
> -	if (enable)
> -		val |= GPIO_BIT(offset);
> -	else
> -		val &= ~GPIO_BIT(offset);
> -
> -	writel(val, treg);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_tolerance, enable);
>  
>  	if (copro)
>  		aspeed_gpio_copro_release(gpio, offset);
> @@ -821,21 +720,11 @@ static inline bool timer_allocation_registered(struct aspeed_gpio *gpio,
>  static void configure_timer(struct aspeed_gpio *gpio, unsigned int offset,
>  		unsigned int timer)
>  {
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
> -	const u32 mask = GPIO_BIT(offset);
> -	void __iomem *addr;
> -	u32 val;
> -
>  	/* Note: Debounce timer isn't under control of the command
>  	 * source registers, so no need to sync with the coprocessor
>  	 */
> -	addr = bank_reg(gpio, bank, reg_debounce_sel1);
> -	val = ioread32(addr);
> -	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE1(timer, offset), addr);
> -
> -	addr = bank_reg(gpio, bank, reg_debounce_sel2);
> -	val = ioread32(addr);
> -	iowrite32((val & ~mask) | GPIO_SET_DEBOUNCE2(timer, offset), addr);
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_debounce_sel1, !!(timer & BIT(1)));
> +	gpio->config->llops->reg_bits_set(gpio, offset, reg_debounce_sel2, !!(timer & BIT(0)));
>  }
>  
>  static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
> @@ -866,15 +755,15 @@ static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
>  	}
>  
>  	/* Try to find a timer already configured for the debounce period */
> -	for (i = 1; i < ARRAY_SIZE(debounce_timers); i++) {
> +	for (i = 1; i < gpio->config->debounce_timers_num; i++) {
>  		u32 cycles;
>  
> -		cycles = ioread32(gpio->base + debounce_timers[i]);
> +		cycles = ioread32(gpio->base + gpio->config->debounce_timers_array[i]);
>  		if (requested_cycles == cycles)
>  			break;
>  	}
>  
> -	if (i == ARRAY_SIZE(debounce_timers)) {
> +	if (i == gpio->config->debounce_timers_num) {
>  		int j;
>  
>  		/*
> @@ -888,8 +777,8 @@ static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
>  
>  		if (j == ARRAY_SIZE(gpio->timer_users)) {
>  			dev_warn(chip->parent,
> -					"Debounce timers exhausted, cannot debounce for period %luus\n",
> -					usecs);
> +				 "Debounce timers exhausted, cannot debounce for period %luus\n",
> +				 usecs);
>  
>  			rc = -EPERM;
>  
> @@ -905,7 +794,7 @@ static int enable_debounce(struct gpio_chip *chip, unsigned int offset,
>  
>  		i = j;
>  
> -		iowrite32(requested_cycles, gpio->base + debounce_timers[i]);
> +		iowrite32(requested_cycles, gpio->base + gpio->config->debounce_timers_array[i]);
>  	}
>  
>  	if (WARN(i == 0, "Cannot register index of disabled timer\n")) {
> @@ -1008,6 +897,9 @@ int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc,
>  	const struct aspeed_gpio_bank *bank = to_bank(offset);
>  	unsigned long flags;
>  
> +	if (!gpio->config->cmd_source_supoort)
> +		return -EOPNOTSUPP;
> +
>  	if (!gpio->cf_copro_bankmap)
>  		gpio->cf_copro_bankmap = kzalloc(gpio->chip.ngpio >> 3, GFP_KERNEL);
>  	if (!gpio->cf_copro_bankmap)
> @@ -1027,7 +919,7 @@ int aspeed_gpio_copro_grab_gpio(struct gpio_desc *desc,
>  
>  	/* Switch command source */
>  	if (gpio->cf_copro_bankmap[bindex] == 1)
> -		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
> +		aspeed_gpio_change_cmd_source(gpio, offset,
>  					      GPIO_CMDSRC_COLDFIRE);
>  
>  	if (vreg_offset)
> @@ -1051,9 +943,11 @@ int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc)
>  	struct gpio_chip *chip = gpiod_to_chip(desc);
>  	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
>  	int rc = 0, bindex, offset = gpio_chip_hwgpio(desc);
> -	const struct aspeed_gpio_bank *bank = to_bank(offset);
>  	unsigned long flags;
>  
> +	if (!gpio->config->cmd_source_supoort)
> +		return -EOPNOTSUPP;
> +
>  	if (!gpio->cf_copro_bankmap)
>  		return -ENXIO;
>  
> @@ -1072,7 +966,7 @@ int aspeed_gpio_copro_release_gpio(struct gpio_desc *desc)
>  
>  	/* Switch command source */
>  	if (gpio->cf_copro_bankmap[bindex] == 0)
> -		aspeed_gpio_change_cmd_source(gpio, bank, bindex,
> +		aspeed_gpio_change_cmd_source(gpio, offset,
>  					      GPIO_CMDSRC_ARM);
>   bail:
>  	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> @@ -1082,12 +976,10 @@ EXPORT_SYMBOL_GPL(aspeed_gpio_copro_release_gpio);
>  
>  static void aspeed_gpio_irq_print_chip(struct irq_data *d, struct seq_file *p)
>  {
> -	const struct aspeed_gpio_bank *bank;
>  	struct aspeed_gpio *gpio;
> -	u32 bit;
>  	int rc, offset;
>  
> -	rc = irqd_to_aspeed_gpio_data(d, &gpio, &bank, &bit, &offset);
> +	rc = irqd_to_aspeed_gpio_data(d, &gpio, &offset);
>  	if (rc)
>  		return;
>  
> @@ -1104,6 +996,78 @@ static const struct irq_chip aspeed_gpio_irq_chip = {
>  	GPIOCHIP_IRQ_RESOURCE_HELPERS,
>  };
>  
> +static void aspeed_g4_reg_bits_set(struct aspeed_gpio *gpio, unsigned int offset,
> +				   const enum aspeed_gpio_reg reg, u32 val)
> +{
> +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +	void __iomem *addr = bank_reg(gpio, bank, reg);
> +	u32 temp;
> +
> +	temp = ioread32(addr);
> +	if (val)
> +		temp |= GPIO_BIT(offset);
> +	else
> +		temp &= ~GPIO_BIT(offset);

Perhaps the type of `val` should be `bool` for now to indicate it can
only handle a single bit? Maybe "_bits_" in the function names is
unhelpful too, and that should be just "_bit_"?

> +
> +	iowrite32(temp, addr);
> +}
> +
> +static u32 aspeed_g4_reg_bits_read(struct aspeed_gpio *gpio, unsigned int offset,
> +				   const enum aspeed_gpio_reg reg)
> +{
> +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +	void __iomem *addr = bank_reg(gpio, bank, reg);
> +
> +	if (reg == reg_rdata)
> +		return ioread32(addr);
> +	return !!(ioread32(addr) & GPIO_BIT(offset));
> +}
> +
> +static bool aspeed_g4_copro_request(struct aspeed_gpio *gpio, unsigned int offset)
> +{
> +	const struct aspeed_gpio_bank *bank = to_bank(offset);
> +
> +	if (!copro_ops || !gpio->cf_copro_bankmap)
> +		return false;
> +	if (!gpio->cf_copro_bankmap[offset >> 3])
> +		return false;
> +	if (!copro_ops->request_access)
> +		return false;
> +
> +	/* Pause the coprocessor */
> +	copro_ops->request_access(copro_data);
> +
> +	/* Change command source back to ARM */
> +	aspeed_gpio_change_cmd_source(gpio, offset, GPIO_CMDSRC_ARM);
> +
> +	/* Update cache */
> +	gpio->dcache[GPIO_BANK(offset)] = ioread32(bank_reg(gpio, bank, reg_rdata));
> +
> +	return true;
> +}
> +
> +static void aspeed_g4_copro_release(struct aspeed_gpio *gpio, unsigned int offset)
> +{
> +	if (!copro_ops || !gpio->cf_copro_bankmap)
> +		return;
> +	if (!gpio->cf_copro_bankmap[offset >> 3])
> +		return;
> +	if (!copro_ops->release_access)
> +		return;
> +
> +	/* Change command source back to ColdFire */
> +	aspeed_gpio_change_cmd_source(gpio, offset, GPIO_CMDSRC_COLDFIRE);
> +
> +	/* Restart the coprocessor */
> +	copro_ops->release_access(copro_data);
> +}
> +
> +static const struct aspeed_gpio_llops aspeed_g4_llops = {
> +	.copro_request = aspeed_g4_copro_request,
> +	.copro_release = aspeed_g4_copro_release,
> +	.reg_bits_set = aspeed_g4_reg_bits_set,
> +	.reg_bits_read = aspeed_g4_reg_bits_read,
> +};
>  /*
>   * Any banks not specified in a struct aspeed_bank_props array are assumed to
>   * have the properties:
> @@ -1120,7 +1084,14 @@ static const struct aspeed_bank_props ast2400_bank_props[] = {
>  
>  static const struct aspeed_gpio_config ast2400_config =
>  	/* 220 for simplicity, really 216 with two 4-GPIO holes, four at end */
> -	{ .nr_gpios = 220, .props = ast2400_bank_props, };
> +	{
> +		.nr_gpios = 220,
> +		.props = ast2400_bank_props,
> +		.llops = &aspeed_g4_llops,
> +		.debounce_timers_array = debounce_timers,
> +		.debounce_timers_num = ARRAY_SIZE(debounce_timers),
> +		.cmd_source_supoort = true,
> +	};
>  
>  static const struct aspeed_bank_props ast2500_bank_props[] = {
>  	/*     input	  output   */
> @@ -1132,7 +1103,14 @@ static const struct aspeed_bank_props ast2500_bank_props[] = {
>  
>  static const struct aspeed_gpio_config ast2500_config =
>  	/* 232 for simplicity, actual number is 228 (4-GPIO hole in GPIOAB) */
> -	{ .nr_gpios = 232, .props = ast2500_bank_props, };
> +	{
> +		.nr_gpios = 232,
> +		.props = ast2500_bank_props,
> +		.llops = &aspeed_g4_llops,
> +		.debounce_timers_array = debounce_timers,
> +		.debounce_timers_num = ARRAY_SIZE(debounce_timers),
> +		.cmd_source_supoort = true,
> +	};
>  
>  static const struct aspeed_bank_props ast2600_bank_props[] = {
>  	/*     input	  output   */
> @@ -1148,7 +1126,14 @@ static const struct aspeed_gpio_config ast2600_config =
>  	 * We expect ngpio being set in the device tree and this is a fallback
>  	 * option.
>  	 */
> -	{ .nr_gpios = 208, .props = ast2600_bank_props, };
> +	{
> +		.nr_gpios = 208,
> +		.props = ast2600_bank_props,
> +		.llops = &aspeed_g4_llops,
> +		.debounce_timers_array = debounce_timers,
> +		.debounce_timers_num = ARRAY_SIZE(debounce_timers),
> +		.cmd_source_supoort = true,
> +	};
>  
>  static const struct of_device_id aspeed_gpio_of_table[] = {
>  	{ .compatible = "aspeed,ast2400-gpio", .data = &ast2400_config, },
> @@ -1191,6 +1176,9 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
>  
>  	gpio->config = gpio_id->data;
>  
> +	if (!gpio->config->llops->reg_bits_set || !gpio->config->llops->reg_bits_read)
> +		return -EOPNOTSUPP;
> +

Hmm, not quite sure about -EOPNOTSUPP. -EINVAL might be better. Given
it's a programming error I think it deserves a log message as well, as
ultimately it should never be hit, and if it is we want to clear up the
confusion quickly.

Andrew

Powered by blists - more mailing lists