Re: [PATCH 1/2] gpio/aspeed-sgpio: enable access to all 80 input & output sgpios

[Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx>]

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