<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:DengXian;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:"\@DengXian";
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;
mso-ligatures:none;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style>
</head>
<body lang="en-TW" link="#0563C1" vlink="#954F72" style="word-wrap:break-word">
<div class="WordSection1">
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks for the detailed review — your comments are very helpful.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">> > Add low-level operations (llops) to abstract the register access for SGPIO<o:p></o:p></p>
<p class="MsoNormal">> > registers. With this abstraction layer, the driver can separate the<o:p></o:p></p>
<p class="MsoNormal">> > hardware and software logic, making it easier to extend the driver to<o:p></o:p></p>
<p class="MsoNormal">> > support different hardware register layouts.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">> With a quick look at the code, it appears the register numbers stay<o:p></o:p></p>
<p class="MsoNormal">> the same? Is that true?<o:p></o:p></p>
<p class="MsoNormal">> I think you have reinvented regmap.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Yes, the register numbers remain unchanged for ASPEED G4 in this patch.<o:p></o:p></p>
<p class="MsoNormal">The intent of introducing the llops abstraction is to decouple the driver logic<o:p></o:p></p>
<p class="MsoNormal">from the underlying register layout so that we can support SoCs with different<o:p></o:p></p>
<p class="MsoNormal">SGPIO register organizations in the future. The actual AST2700-specific support<o:p></o:p></p>
<p class="MsoNormal">will be added in a subsequent patch.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">We did consider regmap. However, llops is intended to abstract not only register<o:p></o:p></p>
<p class="MsoNormal">access but also layout-specific bit mapping, which is difficult to express<o:p></o:p></p>
<p class="MsoNormal">cleanly with a flat regmap interface.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">> > @@ -318,30 +278,25 @@ static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)<o:p></o:p></p>
<p class="MsoNormal">> > u32 type0 = 0;<o:p></o:p></p>
<p class="MsoNormal">> > u32 type1 = 0;<o:p></o:p></p>
<p class="MsoNormal">> > u32 type2 = 0;<o:p></o:p></p>
<p class="MsoNormal">> > - u32 bit, reg;<o:p></o:p></p>
<p class="MsoNormal">> > - const struct aspeed_sgpio_bank *bank;<o:p></o:p></p>
<p class="MsoNormal">> > irq_flow_handler_t handler;<o:p></o:p></p>
<p class="MsoNormal">> > - struct aspeed_sgpio *gpio;<o:p></o:p></p>
<p class="MsoNormal">> > - void __iomem *addr;<o:p></o:p></p>
<p class="MsoNormal">> > - int offset;<o:p></o:p></p>
<p class="MsoNormal">> > -<o:p></o:p></p>
<p class="MsoNormal">> > - irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);<o:p></o:p></p>
<p class="MsoNormal">> > + struct aspeed_sgpio *gpio = irq_data_get_irq_chip_data(d);<o:p></o:p></p>
<p class="MsoNormal">> > + int offset = irqd_to_hwirq(d);<o:p></o:p></p>
<p class="MsoNormal">> ><o:p></o:p></p>
<p class="MsoNormal">> > switch (type & IRQ_TYPE_SENSE_MASK) {<o:p></o:p></p>
<p class="MsoNormal">> > case IRQ_TYPE_EDGE_BOTH:<o:p></o:p></p>
<p class="MsoNormal">> > - type2 |= bit;<o:p></o:p></p>
<p class="MsoNormal">> > + type2 = 1;<o:p></o:p></p>
<p class="MsoNormal">> > fallthrough;<o:p></o:p></p>
<p class="MsoNormal">> > case IRQ_TYPE_EDGE_RISING:<o:p></o:p></p>
<p class="MsoNormal">> > - type0 |= bit;<o:p></o:p></p>
<p class="MsoNormal">> > + type0 = 1;<o:p></o:p></p>
<p class="MsoNormal">> > fallthrough;<o:p></o:p></p>
<p class="MsoNormal">> > case IRQ_TYPE_EDGE_FALLING:<o:p></o:p></p>
<p class="MsoNormal">> > handler = handle_edge_irq;<o:p></o:p></p>
<p class="MsoNormal">> > break;<o:p></o:p></p>
<p class="MsoNormal">> > case IRQ_TYPE_LEVEL_HIGH:<o:p></o:p></p>
<p class="MsoNormal">> > - type0 |= bit;<o:p></o:p></p>
<p class="MsoNormal">> > + type0 = 1;<o:p></o:p></p>
<p class="MsoNormal">> > fallthrough;<o:p></o:p></p>
<p class="MsoNormal">> > case IRQ_TYPE_LEVEL_LOW:<o:p></o:p></p>
<p class="MsoNormal">> > - type1 |= bit;<o:p></o:p></p>
<p class="MsoNormal">> > + type1 = 1;<o:p></o:p></p>
<p class="MsoNormal">> > handler = handle_level_irq;<o:p></o:p></p>
<p class="MsoNormal">> > break;<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">> This change is not obviously correct to me. It is not about<o:p></o:p></p>
<p class="MsoNormal">> abstracting register accesses, what you actually write to the<o:p></o:p></p>
<p class="MsoNormal">> registers appears to of changed. Maybe you could add a refactoring<o:p></o:p></p>
<p class="MsoNormal">> patch first which does this change, with a commit message explaining<o:p></o:p></p>
<p class="MsoNormal">> it, and then insert the register abstraction?<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">You’re right — viewed together, this change is not obviously correct and makes<o:p></o:p></p>
<p class="MsoNormal">the refactoring harder to review.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">While the llops interface is designed to handle bit positioning internally<o:p></o:p></p>
<p class="MsoNormal">(changing the semantics from passing a bitmask to passing a value), combining<o:p></o:p></p>
<p class="MsoNormal">this semantic change with the abstraction refactoring increases review<o:p></o:p></p>
<p class="MsoNormal">complexity.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">To address this, I will respin the series and split it into:<o:p></o:p></p>
<p class="MsoNormal"> 1. a preparatory refactoring patch that introduces the llops helpers without<o:p></o:p></p>
<p class="MsoNormal">changing behavior, and<o:p></o:p></p>
<p class="MsoNormal"> 2. a follow-up patch that switches callers to the new value-based interface,<o:p></o:p></p>
<p class="MsoNormal">with a commit message explicitly explaining the semantic change.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">> > @@ -374,16 +318,14 @@ static void aspeed_sgpio_irq_handler(struct irq_desc *desc)<o:p></o:p></p>
<p class="MsoNormal">> > {<o:p></o:p></p>
<p class="MsoNormal">> > struct gpio_chip *gc = irq_desc_get_handler_data(desc);<o:p></o:p></p>
<p class="MsoNormal">> > struct irq_chip *ic = irq_desc_get_chip(desc);<o:p></o:p></p>
<p class="MsoNormal">> > - struct aspeed_sgpio *data = gpiochip_get_data(gc);<o:p></o:p></p>
<p class="MsoNormal">> > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">> This rename does not belong in this patch. You want lots of small<o:p></o:p></p>
<p class="MsoNormal">> patches, each doing one logical thing, with a good commit message, and<o:p></o:p></p>
<p class="MsoNormal">> obviously correct. Changes like this make it a lot less obviously<o:p></o:p></p>
<p class="MsoNormal">> correct.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Agreed. I will revert the rename from this patch and handle it separately if<o:p></o:p></p>
<p class="MsoNormal">needed.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">> > /* Disable IRQ and clear Interrupt status registers for all SGPIO Pins. */<o:p></o:p></p>
<p class="MsoNormal">> > - for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {<o:p></o:p></p>
<p class="MsoNormal">> > - bank = &aspeed_sgpio_banks[i];<o:p></o:p></p>
<p class="MsoNormal">> > + for (i = 0; i < gpio->chip.ngpio; i += 2) {<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">> Why are ARRAY_SIZE() gone? There probably is a good reason, so doing<o:p></o:p></p>
<p class="MsoNormal">> this in a patch of its own, with a commit message explaining "Why?"<o:p></o:p></p>
<p class="MsoNormal">> would make this easier to review.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">The change from ARRAY_SIZE(aspeed_sgpio_banks) to gpio->chip.ngpio is required<o:p></o:p></p>
<p class="MsoNormal">because AST2700 does not use a fixed bank-based register layout.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Using ngpio removes the dependency on a static bank array and allows the IRQ<o:p></o:p></p>
<p class="MsoNormal">handling code to work with SoCs that have different SGPIO organizations.<o:p></o:p></p>
<p class="MsoNormal">I agree this change deserves a dedicated patch with a commit message explaining<o:p></o:p></p>
<p class="MsoNormal">the rationale, and I will split it out accordingly.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks again for the review. I’ll send a revised version with the changes above.<br>
<br>
<span lang="EN-US">Billy Tsai</span><span lang="EN-US"><o:p></o:p></span></p>
</div>
</div>
</body>
</html>