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 <lyude@xxxxxxxxxx>
>
> 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 <zboszor@xxxxx>
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
