On 05/30/2014 06:28 PM, Mike Turquette wrote: > Quoting Alex Elder (2014-05-30 13:53:02) >> Use a counter rather than a Boolean to track whether write access to >> a CCU has been enabled or not. This will allow more than one of >> these requests to be nested. >> >> Note that __ccu_write_enable() and __ccu_write_disable() calls all >> come in pairs, and they are always surrounded immediately by calls >> to ccu_lock() and ccu_unlock(). >> >> Signed-off-by: Alex Elder <el...@linaro.org> >> --- >> drivers/clk/bcm/clk-kona.c | 14 ++++---------- >> drivers/clk/bcm/clk-kona.h | 2 +- >> 2 files changed, 5 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c >> index 95af2e6..ee8e988 100644 >> --- a/drivers/clk/bcm/clk-kona.c >> +++ b/drivers/clk/bcm/clk-kona.c >> @@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, >> unsigned long flags) >> */ >> static inline void __ccu_write_enable(struct ccu_data *ccu) > > Per Documentation/CodingStyle, chapter 15, "the inline disease", it > might be best to not inline these functions.
This was not intentional. I normally only inline things defined in header files, and maybe this is an artifact of having been in a header at one time. I don't know, I'll get rid of the inline. > >> { >> - if (ccu->write_enabled) { >> - pr_err("%s: access already enabled for %s\n", __func__, >> - ccu->name); >> - return; >> - } >> - ccu->write_enabled = true; >> - __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1); >> + if (!ccu->write_enabled++) >> + __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1); >> } >> >> static inline void __ccu_write_disable(struct ccu_data *ccu) >> @@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data >> *ccu) >> ccu->name); >> return; >> } >> - >> - __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD); >> - ccu->write_enabled = false; >> + if (!--ccu->write_enabled) >> + __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD); > > What happens if calls to __ccu_write_enable and __ccu_write_disable are > unbalanced? It would be better to catch that case and throw a WARN: You can't see it in the diff, but that's what happens (well, it's a pr_err(), not a WARN()). I think a WARN() is probably right in this case though. > if (WARN_ON(ccu->write_enabled == 0)) > return; > > if (--ccu->write_enabled > 0) > return; > > __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD); > >> } >> >> /* >> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h >> index 2537b30..e9a8466 100644 >> --- a/drivers/clk/bcm/clk-kona.h >> +++ b/drivers/clk/bcm/clk-kona.h >> @@ -478,7 +478,7 @@ struct ccu_policy { >> struct ccu_data { >> void __iomem *base; /* base of mapped address space */ >> spinlock_t lock; /* serialization lock */ >> - bool write_enabled; /* write access is currently enabled */ >> + u32 write_enabled; /* write access enable count */ > > Why u32? An unsigned int will do just nicely here. That's a preference of mine. I almost always favor using u32, etc. because they are compact, and explicit about the size and signedness. I "know" an int is 32 bits, but I still prefer being explicit. I'll interpret this as a preference on your part for unsigned int, and I have no problem making that change. -Alex > Regards, > Mike > >> struct ccu_policy policy; >> struct list_head links; /* for ccu_list */ >> struct device_node *node; >> -- >> 1.9.1 >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/