[PATCH 3/5] gpio: aspeed-sgpio: Create llops to handle hardware access

[Billy Tsai]

> > > @@ -318,30 +278,25 @@ static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
> > >       u32 type0 = 0;
> > >       u32 type1 = 0;
> > >       u32 type2 = 0;
> > > -     u32 bit, reg;
> > > -     const struct aspeed_sgpio_bank *bank;
> > >       irq_flow_handler_t handler;
> > > -     struct aspeed_sgpio *gpio;
> > > -     void __iomem *addr;
> > > -     int offset;
> > > -
> > > -     irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > > +     struct aspeed_sgpio *gpio = irq_data_get_irq_chip_data(d);
> > > +     int offset = irqd_to_hwirq(d);
> > >
> > >       switch (type & IRQ_TYPE_SENSE_MASK) {
> > >       case IRQ_TYPE_EDGE_BOTH:
> > > -             type2 |= bit;
> > > +             type2 = 1;
> > >               fallthrough;
> > >       case IRQ_TYPE_EDGE_RISING:
> > > -             type0 |= bit;
> > > +             type0 = 1;
> > >               fallthrough;
> > >       case IRQ_TYPE_EDGE_FALLING:
> > >               handler = handle_edge_irq;
> > >               break;
> > >       case IRQ_TYPE_LEVEL_HIGH:
> > > -             type0 |= bit;
> > > +             type0 = 1;
> > >               fallthrough;
> > >       case IRQ_TYPE_LEVEL_LOW:
> > > -             type1 |= bit;
> > > +             type1 = 1;
> > >               handler = handle_level_irq;
> > >               break;

> > This change is not obviously correct to me. It is not about
> > abstracting register accesses, what you actually write to the
> > registers appears to of changed. Maybe you could add a refactoring
> > patch first which does this change, with a commit message explaining
> > it, and then insert the register abstraction?

> You’re right — viewed together, this change is not obviously correct and makes
> the refactoring harder to review.
> 
> While the llops interface is designed to handle bit positioning internally
> (changing the semantics from passing a bitmask to passing a value), combining
> this semantic change with the abstraction refactoring increases review
> complexity.
> 
> To address this, I will respin the series and split it into:
>                 1.            a preparatory refactoring patch that introduces the llops helpers without
> changing behavior, and
>                 2.            a follow-up patch that switches callers to the new value-based interface,
> with a commit message explicitly explaining the semantic change.

Follow-up clarification

I’d like to clarify this part after double-checking the logic.

You’re right that, when viewed in isolation, this change is not obviously correct.

In the llops-based design, the semantics intentionally change from passing a
bitmask to passing an offset plus a value. The llops helpers are responsible for
deriving the correct bit position internally. The same model is used on the
other side as well, so functionally this does not change the behavior.

Thanks again for pointing this out.

Billy Tsai