On Thu, 27 May 2021, at 12:04, Steven Lee wrote:

The 05/27/2021 09:26, Andrew Jeffery wrote:

quoted

Hi Steven,

On Wed, 26 May 2021, at 19:16, Steven Lee wrote:

quoted

 	struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
-	const struct aspeed_sgpio_bank *bank = to_bank(offset);
 	unsigned long flags;
 	enum aspeed_sgpio_reg reg;
 	int rc = 0;
 
 	spin_lock_irqsave(&gpio->lock, flags);
 
-	reg = aspeed_sgpio_is_input(offset) ? reg_val : reg_rdata;
-	rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset));
+	reg = aspeed_sgpio_is_input(offset, gpio->max_ngpios) ? reg_val : 
reg_rdata;

We should just pass gpio here (i.e. make aspeed_sgpio_is_input take a 
'const struct aspeed_sgpio *' parameter), rather than open-coding 
gpio->max_ngpios. This approach will make it easier to refactor the 
implementation in the future (if necessary).

I will rewrite the function to use aspeed_sgpio struct.

quoted

quoted

 	struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
-	const struct aspeed_sgpio_bank *bank = to_bank(offset);
 	void __iomem *addr_r, *addr_w;
 	u32 reg = 0;
 
-	if (aspeed_sgpio_is_input(offset))
+	if (aspeed_sgpio_is_input(offset, gpio->max_ngpios))
 		return -EINVAL;
 
 	/* Since this is an output, read the cached value from rdata, then

@@ -209,9 +214,9 @@ static int sgpio_set_value(struct gpio_chip *gc, 

unsigned int offset, int val)
 	reg = ioread32(addr_r);
 
 	if (val)
-		reg |= GPIO_BIT(offset);
+		reg |= GPIO_BIT(offset % gpio->max_ngpios);

Pass gpio through GPIO_BIT() too.

I don't understand this comment.
Could you describe more?

Oh, sure, what I was suggesting was to make a similar change to 
GPIO_BIT() as I suggested for aspeed_sgpio_is_input() above.

Hope that helps.

Andrew