Hi Arnd, On 29 November 2017 at 18:07, Arnd Bergmann <a...@arndb.de> wrote: > On Mon, Nov 20, 2017 at 7:54 AM, Baolin Wang <baolin.w...@linaro.org> wrote: >> Some system control registers need hardware spinlock to synchronize >> between the multiple subsystems, so we should add hardware spinlock >> support for syscon. >> > >> @@ -87,6 +88,12 @@ static struct syscon *of_syscon_register(struct >> device_node *np) >> if (ret) >> reg_io_width = 4; >> >> + ret = of_hwspin_lock_get_id(np, 0); >> + if (ret > 0) { >> + syscon_config.hwlock_id = ret; >> + syscon_config.hwlock_mode = HWLOCK_IRQSTATE; >> + } > > Hi Baolin, > > The error handling here seems insufficient, I think we have to treat > the following cases separately: > > - ret>0 (this is fine) > - no spinlock in DT: of_hwspin_lock_get_id currently returns -EINVAL, > we probably want to use a different return code (maybe -ESRCH ?) > here so we can tell the difference beween no spinlock, and an > invalid spinlock > - deferred probing: if the spinlock is there but the driver is not > yet available, we may need to propagate the -EPROBE_DEFER > here > - other error: this would probably be a fatal condition here, and > we should print a warning and clean up.
You are correct and I will add some error handling. Thanks for your comments. -- Baolin.wang Best Regards