On Tue, Jan 16, 2018 at 02:55:57PM -0500, Lyude Paul wrote: > Thank you for this cleanup, the gotos that were in this code are really > confusing to read through! I'd recommend one very small change described > below. Assuming you add that in the next version: > > Reviewed-by: Lyude Paul <[email protected]> > > On Sun, 2017-12-24 at 13:04 -0800, Guenter Roeck wrote: > > There are too many unnecessary goto statements in sp5100_tco_setupdevice(). > > Rearrange the code and limit goto statements to error handling. > > > > Cc: Zoltán Böszörményi <[email protected]> > > Signed-off-by: Guenter Roeck <[email protected]> > > --- > > drivers/watchdog/sp5100_tco.c | 62 ++++++++++++++++++++------------------ > > ----- > > 1 file changed, 29 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c > > index 0e816f2cdb07..5a13ab483c50 100644 > > --- a/drivers/watchdog/sp5100_tco.c > > +++ b/drivers/watchdog/sp5100_tco.c > > @@ -396,48 +396,44 @@ static int sp5100_tco_setupdevice(void) > > pr_debug("Got 0x%04x from indirect I/O\n", val); > > > > /* Check MMIO address conflict */ > > - if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, > > - dev_name)) > > - goto setup_wdt; > > - else > > + if (!request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, > > + dev_name)) { > > pr_debug("MMIO address 0x%04x already in use\n", val); > > + /* > > + * Secondly, Find the watchdog timer MMIO address > > + * from SBResource_MMIO register. > > + */ > > + if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) { > > + /* Read SBResource_MMIO from PCI config(PCI_Reg: > > 9Ch) */ > > + pci_read_config_dword(sp5100_tco_pci, > > + SP5100_SB_RESOURCE_MMIO_BASE, > > + &val); > > + } else { > > + /* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: > > 24h) */ > > + val = > > sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN); > > + } > > > > - /* > > - * Secondly, Find the watchdog timer MMIO address > > - * from SBResource_MMIO register. > > - */ > > - if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) { > > - /* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */ > > - pci_read_config_dword(sp5100_tco_pci, > > - SP5100_SB_RESOURCE_MMIO_BASE, &val); > > - } else { > > - /* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */ > > - val = sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN); > > - } > > - > > - /* The SBResource_MMIO is enabled and mapped memory space? */ > > - if ((val & (SB800_ACPI_MMIO_DECODE_EN | SB800_ACPI_MMIO_SEL)) == > > + /* The SBResource_MMIO is enabled and mapped memory space? > > */ > > + if ((val & (SB800_ACPI_MMIO_DECODE_EN | > > SB800_ACPI_MMIO_SEL)) != > > SB800_ACPI_MMIO_DECODE_EN > Re-align this line since you're changing the code around here anyway >
This code is changed again in a later patch, and I don't see anything wrong with the final alignment. Can you look at the final code and let me know what alignment you would like to have changed there ? Thanks, Guenter
