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

 
Thanks for the detailed review — your comments are very helpful.
 

quoted

Add low-level operations (llops) to abstract the register access for SGPIO
registers. With this abstraction layer, the driver can separate the
hardware and software logic, making it easier to extend the driver to
support different hardware register layouts.

With a quick look at the code, it appears the register numbers stay
the same? Is that true?
I think you have reinvented regmap.

 
Yes, the register numbers remain unchanged for ASPEED G4 in this patch.
The intent of introducing the llops abstraction is to decouple the driver logic
from the underlying register layout so that we can support SoCs with different
SGPIO register organizations in the future. The actual AST2700-specific support
will be added in a subsequent patch.
 
We did consider regmap. However, llops is intended to abstract not only register
access but also layout-specific bit mapping, which is difficult to express
cleanly with a flat regmap interface.
 

quoted

@@ -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.
 

quoted

@@ -374,16 +318,14 @@ static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
 {

      struct gpio_chip *gc = irq_desc_get_handler_data(desc);
      struct irq_chip *ic = irq_desc_get_chip(desc);
-     struct aspeed_sgpio *data = gpiochip_get_data(gc);
+     struct aspeed_sgpio *gpio = gpiochip_get_data(gc);

This rename does not belong in this patch. You want lots of small
patches, each doing one logical thing, with a good commit message, and
obviously correct. Changes like this make it a lot less obviously
correct.

 
Agreed. I will revert the rename from this patch and handle it separately if
needed.
 

quoted

      /* Disable IRQ and clear Interrupt status registers for all SGPIO Pins. */
-     for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
-             bank =  &aspeed_sgpio_banks[i];
+     for (i = 0; i < gpio->chip.ngpio; i += 2) {

Why are ARRAY_SIZE() gone? There probably is a good reason, so doing
this in a patch of its own, with a commit message explaining "Why?"
would make this easier to review.

 
The change from ARRAY_SIZE(aspeed_sgpio_banks) to gpio->chip.ngpio is required
because AST2700 does not use a fixed bank-based register layout.
 
Using ngpio removes the dependency on a static bank array and allows the IRQ
handling code to work with SoCs that have different SGPIO organizations.
I agree this change deserves a dedicated patch with a commit message explaining
the rationale, and I will split it out accordingly.
 
Thanks again for the review. I’ll send a revised version with the changes above.

Billy Tsai