From mboxrd@z Thu Jan 1 00:00:00 1970 From: hdegoede@redhat.com (Hans de Goede) Date: Sun, 31 Jan 2016 10:12:15 +0100 Subject: [PATCH v3 3/3] reset: Add support for shared reset controls In-Reply-To: <20160130113837.GA25528@pengutronix.de> References: <1453918516-3078-1-git-send-email-hdegoede@redhat.com> <1453918516-3078-4-git-send-email-hdegoede@redhat.com> <20160130113837.GA25528@pengutronix.de> Message-ID: <56ADCFEF.3040707@redhat.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi, On 01/30/2016 12:38 PM, Philipp Zabel wrote: > Hi Hans, > > On Wed, Jan 27, 2016 at 07:15:16PM +0100, Hans de Goede wrote: >> In some SoCs some hw-blocks share a reset control. Add support for this >> setup by adding new: >> >> reset_control_get_shared() >> devm_reset_control_get_shared() >> devm_reset_control_get_shared_by_index() >> >> methods to get a reset_control. Note that this patch omits adding of_ >> variants, if these are needed later they can be easily added. >> >> This patch also changes the behavior of the existing exclusive >> reset_control_get() variants, if these are now called more then once >> for the same reset_control they will return -EBUSY. To catch existing >> drivers triggering this error (there should not be any) a WARN_ON(1) >> is added in this path. > > Which is a good thing. > >> When a reset_control is shared, the behavior of reset_control_assert / >> deassert is changed, for shared reset_controls these will work like the >> clock-enable/disable and regulator-on/off functions. They will keep a >> deassert_count, and only (re-)assert the reset after reset_control_assert >> has been called as many times as reset_control_deassert was called. >> >> Calling reset_control_assert without first calling reset_control_deassert >> is not allowed on a shared reset control. Calling reset_control_reset is >> also not allowed on a shared reset control. >> >> Signed-off-by: Hans de Goede > > All three patches look very nice. I'll give them a test-drive next week. > So far I have one small issue, and I like Stephens suggestion of > elaborating on how shared resets are to be used a bit more. I can do that, where would you like me to put this, a doxygen style comment in include/linux/reset.h ? Regards, Hans > >> --- >> drivers/reset/core.c | 61 ++++++++++++++++++++++++++++++++------- >> include/linux/reset.h | 79 +++++++++++++++++++++++++++++++++++++++++---------- >> 2 files changed, 114 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/reset/core.c b/drivers/reset/core.c >> index e439ae2..5554585 100644 >> --- a/drivers/reset/core.c >> +++ b/drivers/reset/core.c >> @@ -8,6 +8,7 @@ >> * the Free Software Foundation; either version 2 of the License, or >> * (at your option) any later version. >> */ >> +#include >> #include >> #include >> #include >> @@ -29,12 +30,16 @@ static LIST_HEAD(reset_controller_list); >> * @id: ID of the reset controller in the reset >> * controller device >> * @refcnt: Number of gets of this reset_control >> + * @shared: Is this a shared (1), or an exclusive (0) reset_control? >> + * @deassert_cnt: Number of times this reset line has been deasserted >> */ >> struct reset_control { >> struct reset_controller_dev *rcdev; >> struct list_head list; >> unsigned int id; >> unsigned int refcnt; >> + int shared; > > Could we make this an > enum reset_control_type type; > > enum reset_control_type { > RESET_CONTROL_EXCLUSIVE, > RESET_CONTROL_SHARED, > }; > instead? > > [...] >> @@ -147,7 +180,7 @@ EXPORT_SYMBOL_GPL(reset_control_status); >> >> static struct reset_control *__reset_control_get( >> struct reset_controller_dev *rcdev, >> - unsigned int index) >> + unsigned int index, int shared) >> { >> struct reset_control *rstc; >> >> @@ -155,6 +188,10 @@ static struct reset_control *__reset_control_get( >> >> list_for_each_entry(rstc, &rcdev->reset_control_head, list) { >> if (rstc->id == index) { >> + if (!rstc->shared || !shared) { > > Then this would have to be > > if ((rstc->type == RESET_CONTROL_EXCLUSIVE) || > (type == RESET_CONTROL_EXCLUSIVE)) { > ... > > [...] >> @@ -78,7 +78,8 @@ static inline struct reset_control *__devm_reset_control_get( >> #endif /* CONFIG_RESET_CONTROLLER */ >> >> /** >> - * reset_control_get - Lookup and obtain a reference to a reset controller. >> + * reset_control_get - Lookup and obtain an exclusive reference to a >> + * reset controller. >> * @dev: device to be reset by the controller >> * @id: reset line name >> * >> @@ -92,17 +93,34 @@ static inline struct reset_control *__must_check reset_control_get( >> #ifndef CONFIG_RESET_CONTROLLER >> WARN_ON(1); >> #endif >> - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0); >> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); > > ... but these would't be as opaque: > > return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, > RESET_CONTROL_EXCLUSIVE); > > [...] >> /** >> - * of_reset_control_get - Lookup and obtain a reference to a reset controller. >> + * reset_control_get_shared - Lookup and obtain a shared reference to a >> + * reset controller. >> + * @dev: device to be reset by the controller >> + * @id: reset line name >> + * >> + * Returns a struct reset_control or IS_ERR() condition containing errno. > > How about addressing Stephen's suggestion by extending this kerneldoc comment a > bit? > >> + * Use of id names is optional. >> + */ >> +static inline struct reset_control *reset_control_get_shared( >> + struct device *dev, const char *id) >> +{ >> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 1); > > return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, > RESET_CONTROL_SHARED); > > [...] > > best regards > Philipp >