| Message ID | 1401483188-5395-2-git-send-email-elder@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <elder@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. > { > - 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: 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. Regards, Mike > struct ccu_policy policy; > struct list_head links; /* for ccu_list */ > struct device_node *node; > -- > 1.9.1 >
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 <elder@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 >>
Quoting Alex Elder (2014-05-30 20:46:46) > 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 <elder@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. It's not a big deal, I was just curious why. Feel free to use whatever solution you prefer here. Regards, Mike > > -Alex > > > Regards, > > Mike > > > >> struct ccu_policy policy; > >> struct list_head links; /* for ccu_list */ > >> struct device_node *node; > >> -- > >> 1.9.1 > >> >
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) { - 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); } /* 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 */ struct ccu_policy policy; struct list_head links; /* for ccu_list */ struct device_node *node;
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 <elder@linaro.org> --- drivers/clk/bcm/clk-kona.c | 14 ++++---------- drivers/clk/bcm/clk-kona.h | 2 +- 2 files changed, 5 insertions(+), 11 deletions(-)