On Thu, 27 May 2021, at 12:04, Steven Lee wrote: > The 05/27/2021 09:26, Andrew Jeffery wrote: > > Hi Steven, > > > > On Wed, 26 May 2021, at 19:16, Steven Lee wrote: > > > 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. > > > 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
