Hi Joel,
Thanks for the review!
> A Fixes: might be a good idea.
OK, given this isn't strictly (just) a fix, should I split that out?
> > -#define MAX_NR_SGPIO 80
> > +#define MAX_NR_HW_SGPIO 80
> > +#define SGPIO_OUTPUT_OFFSET MAX_NR_HW_SGPIO
>
> A short comment explaining what's going on with these defines (as you
> did in your commit message) will help future reviewers.
Sounds good, I'll add one.
>
> > +static void aspeed_sgpio_irq_init_valid_mask(struct gpio_chip *gc,
> > + unsigned long *valid_mask, unsigned int ngpios)
> > +{
> > + struct aspeed_sgpio *sgpio = gpiochip_get_data(gc);
> > + int n = sgpio->n_sgpio;
> > +
> > + WARN_ON(ngpios < MAX_NR_HW_SGPIO * 2);
> > +
> > + /* input GPIOs in the lower range */
> > + bitmap_set(valid_mask, 0, n);
> > + bitmap_clear(valid_mask, n, ngpios - n);
> > +}
> > +
> > +static const bool aspeed_sgpio_is_input(unsigned int offset)
>
> The 0day bot complained about the 'const' here.
ack, will remove.
> > +{
> > + return offset < SGPIO_OUTPUT_OFFSET;
> > +}
> > static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
> > {
> > struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > unsigned long flags;
> > + int rc;
> >
> > - spin_lock_irqsave(&gpio->lock, flags);
> > -
> > - gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
> > - sgpio_set_value(gc, offset, val);
> > + /* No special action is required for setting the direction; we'll
> > + * error-out in sgpio_set_value if this isn't an output GPIO */
> >
> > + spin_lock_irqsave(&gpio->lock, flags);
> > + rc = sgpio_set_value(gc, offset, val);
> > spin_unlock_irqrestore(&gpio->lock, flags);
> >
> > return 0;
>
> I think this should be 'return rc'
Yup. I'll send a v2 with these changes.
Cheers,
Jeremy
