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

[Joel Stanley <joel@xxxxxxxxx>]

On Fri, 11 Sep 2020 at 01:10, Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> 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?

I assume anyone using the sgpio driver in anger would need this patch
for it to work properly, so a fix tag will help them there. No need to
break it down any further in my opinion.

Cheers,

Joel

>
> > > -#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
>