| Message ID | 20240925093807.1026949-3-wenst@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add of_regulator_get_optional() and Fix MTK Power Domain Driver | expand |
On Wed, Sep 25, 2024 at 05:38:05PM +0800, Chen-Yu Tsai wrote: > There are existing uses for a devres version of of_regulator_get_optional() > in power domain drivers. On MediaTek platforms, power domains may have > regulator supplies tied to them. The driver currently tries to use > devm_regulator_get() to not have to manage the lifecycle, but ends up > doing it in a very hacky way by replacing the device node of the power > domain controller device to the device node of the power domain that is > currently being registered, getting the supply, and reverting the device > node. > > Provide a better API so that the hack can be replaced. ... > +#if IS_ENABLED(CONFIG_OF) Do we really need this? > +static struct regulator *_devm_of_regulator_get(struct device *dev, struct device_node *node, > + const char *id, int get_type) > +{ > + struct regulator **ptr, *regulator; > + > + ptr = devres_alloc(devm_regulator_release, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return ERR_PTR(-ENOMEM); > + > + regulator = _of_regulator_get(dev, node, id, get_type); > + if (!IS_ERR(regulator)) { > + *ptr = regulator; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + } > + > + return regulator; Why not using devm_add_action() / devm_add_action_or_reset() (whichever suits better here)? > +} > +#endif ... > +static inline struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev, > + struct device_node *node, > + const char *id) I don't know the conventions here, but I find better to have it as static inline __must_check struct regulator * devm_of_regulator_get_optional(struct device *dev, struct device_node *node, const char *id) Similar to other stubs and declarations.
On Wed, Sep 25, 2024 at 6:56 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Sep 25, 2024 at 05:38:05PM +0800, Chen-Yu Tsai wrote: > > There are existing uses for a devres version of of_regulator_get_optional() > > in power domain drivers. On MediaTek platforms, power domains may have > > regulator supplies tied to them. The driver currently tries to use > > devm_regulator_get() to not have to manage the lifecycle, but ends up > > doing it in a very hacky way by replacing the device node of the power > > domain controller device to the device node of the power domain that is > > currently being registered, getting the supply, and reverting the device > > node. > > > > Provide a better API so that the hack can be replaced. > > ... > > > +#if IS_ENABLED(CONFIG_OF) > > Do we really need this? What's the point of going through devres_* stuff if we already know _of_regulator_get() is going to fail anyway? Also, _of_regulator_get() does not have a stub version for !CONFIG_OF. > > +static struct regulator *_devm_of_regulator_get(struct device *dev, struct device_node *node, > > + const char *id, int get_type) > > +{ > > + struct regulator **ptr, *regulator; > > + > > + ptr = devres_alloc(devm_regulator_release, sizeof(*ptr), GFP_KERNEL); > > + if (!ptr) > > + return ERR_PTR(-ENOMEM); > > + > > + regulator = _of_regulator_get(dev, node, id, get_type); > > + if (!IS_ERR(regulator)) { > > + *ptr = regulator; > > + devres_add(dev, ptr); > > + } else { > > + devres_free(ptr); > > + } > > + > > + return regulator; > > Why not using devm_add_action() / devm_add_action_or_reset() > (whichever suits better here)? Cargo cult from _devm_regulator_get() in this file. However since this is meant to share the same release function, both functions need to use the same mechanism. I could also argue that this is not an action, but an allocation, and so devres_alloc() seems to make more sense. ChenYu > > +} > > > +#endif > > ... > > > +static inline struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev, > > + struct device_node *node, > > + const char *id) > > I don't know the conventions here, but I find better to have it as > > static inline __must_check struct regulator * > devm_of_regulator_get_optional(struct device *dev, struct device_node *node, const char *id) > > Similar to other stubs and declarations. I don't think there are any conventions. This file already has three types: 1. Wrap the line with the function name on the second line 2. Wrap the arguments; wrapped arguments aligned to the left parenthesis. 3. Wrap the arguments; wrapped arguments aligned with aribtrary number of tabs. I prefer the way I have put them.
On Thu, Sep 26, 2024 at 04:43:52PM +0800, Chen-Yu Tsai wrote: > On Wed, Sep 25, 2024 at 6:56 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Sep 25, 2024 at 05:38:05PM +0800, Chen-Yu Tsai wrote: ... > > > +#if IS_ENABLED(CONFIG_OF) > > > > Do we really need this? > > What's the point of going through devres_* stuff if we already know > _of_regulator_get() is going to fail anyway? With devm_add_action*() this will be other way around and there are plenty of APIs done this way. The ifdeffery is simply ugly in the code. > Also, _of_regulator_get() does not have a stub version for !CONFIG_OF. So, what prevents us from adding it? > > > +static struct regulator *_devm_of_regulator_get(struct device *dev, struct device_node *node, > > > + const char *id, int get_type) > > > +{ > > > + struct regulator **ptr, *regulator; > > > + > > > + ptr = devres_alloc(devm_regulator_release, sizeof(*ptr), GFP_KERNEL); > > > + if (!ptr) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + regulator = _of_regulator_get(dev, node, id, get_type); > > > + if (!IS_ERR(regulator)) { > > > + *ptr = regulator; > > > + devres_add(dev, ptr); > > > + } else { > > > + devres_free(ptr); > > > + } > > > + > > > + return regulator; > > > > Why not using devm_add_action() / devm_add_action_or_reset() > > (whichever suits better here)? > > Cargo cult from _devm_regulator_get() in this file. However since this is > meant to share the same release function, both functions need to use the > same mechanism. > > I could also argue that this is not an action, but an allocation, and so > devres_alloc() seems to make more sense. It's rather matter of the naming of the devm_add_action*() APIs, but again, we have plenty of APIs using it when it's allocation and not strictly speaking an action. > > > +} > > > > > +#endif ... > > > +static inline struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev, > > > + struct device_node *node, > > > + const char *id) > > > > I don't know the conventions here, but I find better to have it as > > > > static inline __must_check struct regulator * > > devm_of_regulator_get_optional(struct device *dev, struct device_node *node, const char *id) > > > > Similar to other stubs and declarations. > > I don't think there are any conventions. This file already has three types: > > 1. Wrap the line with the function name on the second line > 2. Wrap the arguments; wrapped arguments aligned to the left parenthesis. > 3. Wrap the arguments; wrapped arguments aligned with aribtrary number of > tabs. > > I prefer the way I have put them. The way you put it despite relaxed limit is slightly harder to read. I don't remember many headers that do so-o indented parameters. Besides your way defers the burden of resplit to the future in case one more parameter needs to be added which will excess the 100 limit. Also __must_check is somehow misplaced in my opinion (talking from my experience and this can be simply checked by grepping other headers). That said, I prefer the way I suggested or something alike.
On Thu, Sep 26, 2024 at 8:26 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Sep 26, 2024 at 04:43:52PM +0800, Chen-Yu Tsai wrote: > > On Wed, Sep 25, 2024 at 6:56 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, Sep 25, 2024 at 05:38:05PM +0800, Chen-Yu Tsai wrote: > > ... > > > > > +#if IS_ENABLED(CONFIG_OF) > > > > > > Do we really need this? > > > > What's the point of going through devres_* stuff if we already know > > _of_regulator_get() is going to fail anyway? > > With devm_add_action*() this will be other way around and there are plenty of > APIs done this way. The ifdeffery is simply ugly in the code. It's still extra code that doesn't get compiled out. > > Also, _of_regulator_get() does not have a stub version for !CONFIG_OF. > > So, what prevents us from adding it? Because there's no need if the only internal user isn't using it. I could also move them over to of_regulator.c. _of_regulator_get() stays internal to that file, and devm_regulator_release() gets exposed instead. Does that sound better? > > > > +static struct regulator *_devm_of_regulator_get(struct device *dev, struct device_node *node, > > > > + const char *id, int get_type) > > > > +{ > > > > + struct regulator **ptr, *regulator; > > > > + > > > > + ptr = devres_alloc(devm_regulator_release, sizeof(*ptr), GFP_KERNEL); > > > > + if (!ptr) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + regulator = _of_regulator_get(dev, node, id, get_type); > > > > + if (!IS_ERR(regulator)) { > > > > + *ptr = regulator; > > > > + devres_add(dev, ptr); > > > > + } else { > > > > + devres_free(ptr); > > > > + } > > > > + > > > > + return regulator; > > > > > > Why not using devm_add_action() / devm_add_action_or_reset() > > > (whichever suits better here)? > > > > Cargo cult from _devm_regulator_get() in this file. However since this is > > meant to share the same release function, both functions need to use the > > same mechanism. > > > > I could also argue that this is not an action, but an allocation, and so > > devres_alloc() seems to make more sense. > > It's rather matter of the naming of the devm_add_action*() APIs, but again, > we have plenty of APIs using it when it's allocation and not strictly speaking > an action. OK. Still the mechanism used needs to match that of the existing API. So devres_add() it is for now. > > > > +} > > > > > > > +#endif > > ... > > > > > +static inline struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev, > > > > + struct device_node *node, > > > > + const char *id) > > > > > > I don't know the conventions here, but I find better to have it as > > > > > > static inline __must_check struct regulator * > > > devm_of_regulator_get_optional(struct device *dev, struct device_node *node, const char *id) > > > > > > Similar to other stubs and declarations. > > > > I don't think there are any conventions. This file already has three types: > > > > 1. Wrap the line with the function name on the second line > > 2. Wrap the arguments; wrapped arguments aligned to the left parenthesis. > > 3. Wrap the arguments; wrapped arguments aligned with aribtrary number of > > tabs. > > > > I prefer the way I have put them. > > The way you put it despite relaxed limit is slightly harder to read. > I don't remember many headers that do so-o indented parameters. Besides > your way defers the burden of resplit to the future in case one more parameter > needs to be added which will excess the 100 limit. > > Also __must_check is somehow misplaced in my opinion (talking from my > experience and this can be simply checked by grepping other headers). Seems correct to me. It's between the return type and the function name. From the coding style doc: __init void * __must_check action(enum magic value, size_t size, u8 count, char *fmt, ...) __printf(4, 5) __malloc; The preferred order of elements for a function prototype is: - storage class (below, ``static __always_inline``, noting that ``__always_inline`` is technically an attribute but is treated like ``inline``) - storage class attributes (here, ``__init`` -- i.e. section declarations, but also things like ``__cold``) - return type (here, ``void *``) - return type attributes (here, ``__must_check``) - function name (here, ``action``) - function parameters (here, ``(enum magic value, size_t size, u8 count, char *fmt, ...)``, noting that parameter names should always be included) - function parameter attributes (here, ``__printf(4, 5)``) - function behavior attributes (here, ``__malloc``) > That said, I prefer the way I suggested or something alike. Two people arguing over style that is not clearly specified in the coding style doc is probably wasting time. I'll use what `clang-format` gave: static inline struct regulator *__must_check of_regulator_get_optional( struct device *dev, struct device_node *node, const char *id) static inline struct regulator *__must_check devm_of_regulator_get_optional( struct device *dev, struct device_node *node, const char *id) ChenYu
On Fri, Sep 27, 2024 at 12:38:35PM +0800, Chen-Yu Tsai wrote: > On Thu, Sep 26, 2024 at 8:26 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Sep 26, 2024 at 04:43:52PM +0800, Chen-Yu Tsai wrote: > > > On Wed, Sep 25, 2024 at 6:56 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Wed, Sep 25, 2024 at 05:38:05PM +0800, Chen-Yu Tsai wrote: ... > > > > > +#if IS_ENABLED(CONFIG_OF) > > > > > > > > Do we really need this? > > > > > > What's the point of going through devres_* stuff if we already know > > > _of_regulator_get() is going to fail anyway? > > > > With devm_add_action*() this will be other way around and there are plenty of > > APIs done this way. The ifdeffery is simply ugly in the code. > > It's still extra code that doesn't get compiled out. Are you sure? In case of the stub the compiler should go with a "dead code elimination" optimisation and get rid of most of it (yes, I admit that it might be the overhead for the exporting a function which returns a constant). > > > Also, _of_regulator_get() does not have a stub version for !CONFIG_OF. > > > > So, what prevents us from adding it? > > Because there's no need if the only internal user isn't using it. > > I could also move them over to of_regulator.c. > > _of_regulator_get() stays internal to that file, and devm_regulator_release() > gets exposed instead. > > Does that sound better? This sounds good to me! ... > > > > > +static inline struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev, > > > > > + struct device_node *node, > > > > > + const char *id) > > > > > > > > I don't know the conventions here, but I find better to have it as > > > > > > > > static inline __must_check struct regulator * > > > > devm_of_regulator_get_optional(struct device *dev, struct device_node *node, const char *id) > > > > > > > > Similar to other stubs and declarations. > > > > > > I don't think there are any conventions. This file already has three types: > > > > > > 1. Wrap the line with the function name on the second line > > > 2. Wrap the arguments; wrapped arguments aligned to the left parenthesis. > > > 3. Wrap the arguments; wrapped arguments aligned with aribtrary number of > > > tabs. > > > > > > I prefer the way I have put them. > > > > The way you put it despite relaxed limit is slightly harder to read. > > I don't remember many headers that do so-o indented parameters. Besides > > your way defers the burden of resplit to the future in case one more parameter > > needs to be added which will excess the 100 limit. > > > > Also __must_check is somehow misplaced in my opinion (talking from my > > experience and this can be simply checked by grepping other headers). > > Seems correct to me. It's between the return type and the function name. > From the coding style doc: > > __init void * __must_check action(enum magic value, size_t size, u8 count, > char *fmt, ...) __printf(4, 5) __malloc; > > The preferred order of elements for a function prototype is: > > - storage class (below, ``static __always_inline``, noting that > ``__always_inline`` > is technically an attribute but is treated like ``inline``) > - storage class attributes (here, ``__init`` -- i.e. section > declarations, but also > things like ``__cold``) > - return type (here, ``void *``) > - return type attributes (here, ``__must_check``) > - function name (here, ``action``) > - function parameters (here, ``(enum magic value, size_t size, u8 > count, char *fmt, ...)``, > noting that parameter names should always be included) > - function parameter attributes (here, ``__printf(4, 5)``) > - function behavior attributes (here, ``__malloc``) > > > That said, I prefer the way I suggested or something alike. > > Two people arguing over style that is not clearly specified in the coding > style doc is probably wasting time. I'll use what `clang-format` gave: > > static inline struct regulator *__must_check of_regulator_get_optional( > struct device *dev, struct device_node *node, const char *id) > static inline struct regulator *__must_check devm_of_regulator_get_optional( > struct device *dev, struct device_node *node, const char *id) With all my hatred towards this clang-format "feature", i.e. open ended parenthesis, this looks better than your original variant.
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c index 1b893cdd1aad..36164aec30e8 100644 --- a/drivers/regulator/devres.c +++ b/drivers/regulator/devres.c @@ -749,3 +749,42 @@ void *devm_regulator_irq_helper(struct device *dev, return ptr; } EXPORT_SYMBOL_GPL(devm_regulator_irq_helper); + +#if IS_ENABLED(CONFIG_OF) +static struct regulator *_devm_of_regulator_get(struct device *dev, struct device_node *node, + const char *id, int get_type) +{ + struct regulator **ptr, *regulator; + + ptr = devres_alloc(devm_regulator_release, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + regulator = _of_regulator_get(dev, node, id, get_type); + if (!IS_ERR(regulator)) { + *ptr = regulator; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return regulator; +} + +/** + * devm_of_regulator_get_optional - Resource managed of_regulator_get_optional() + * @dev: device used for dev_printk() messages and resource lifetime management + * @node: device node for regulator "consumer" + * @id: supply name or regulator ID. + * + * Managed regulator_get_optional(). Regulators returned from this + * function are automatically regulator_put() on driver detach. See + * of_regulator_get_optional() for more information. + */ +struct regulator *devm_of_regulator_get_optional(struct device *dev, struct device_node *node, + const char *id) +{ + return _devm_of_regulator_get(dev, node, id, OPTIONAL_GET); +} +EXPORT_SYMBOL_GPL(devm_of_regulator_get_optional); +#endif diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h index f62cacbbc729..b3d48dc38bc4 100644 --- a/drivers/regulator/internal.h +++ b/drivers/regulator/internal.h @@ -65,6 +65,13 @@ static inline struct regulator_dev *dev_to_rdev(struct device *dev) return container_of(dev, struct regulator_dev, dev); } +enum regulator_get_type { + NORMAL_GET, + EXCLUSIVE_GET, + OPTIONAL_GET, + MAX_GET_TYPE +}; + #ifdef CONFIG_OF struct regulator_dev *of_regulator_dev_lookup(struct device *dev, struct device_node *np, @@ -74,6 +81,9 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev, struct regulator_config *config, struct device_node **node); +struct regulator *_of_regulator_get(struct device *dev, struct device_node *node, + const char *id, enum regulator_get_type get_type); + struct regulator_dev *of_parse_coupled_regulator(struct regulator_dev *rdev, int index); @@ -116,12 +126,6 @@ static inline bool of_check_coupling_data(struct regulator_dev *rdev) } #endif -enum regulator_get_type { - NORMAL_GET, - EXCLUSIVE_GET, - OPTIONAL_GET, - MAX_GET_TYPE -}; int _regulator_get_common_check(struct device *dev, const char *id, enum regulator_get_type get_type); diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c index 358c3ed791db..3d85762beda6 100644 --- a/drivers/regulator/of_regulator.c +++ b/drivers/regulator/of_regulator.c @@ -668,8 +668,8 @@ struct regulator_dev *of_regulator_dev_lookup(struct device *dev, struct device_ return ERR_PTR(-ENODEV); } -static struct regulator *_of_regulator_get(struct device *dev, struct device_node *node, - const char *id, enum regulator_get_type get_type) +struct regulator *_of_regulator_get(struct device *dev, struct device_node *node, + const char *id, enum regulator_get_type get_type) { struct regulator_dev *r; int ret; diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h index 2b22f07e491c..8c3c372ad735 100644 --- a/include/linux/regulator/consumer.h +++ b/include/linux/regulator/consumer.h @@ -172,6 +172,9 @@ void devm_regulator_put(struct regulator *regulator); struct regulator *__must_check of_regulator_get_optional(struct device *dev, struct device_node *node, const char *id); +struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev, + struct device_node *node, + const char *id); #else static inline struct regulator *__must_check of_regulator_get_optional(struct device *dev, struct device_node *node, @@ -179,6 +182,13 @@ static inline struct regulator *__must_check of_regulator_get_optional(struct de { return ERR_PTR(-ENODEV); } + +static inline struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev, + struct device_node *node, + const char *id) +{ + return ERR_PTR(-ENODEV); +} #endif int regulator_register_supply_alias(struct device *dev, const char *id, @@ -370,6 +380,13 @@ static inline struct regulator *__must_check of_regulator_get_optional(struct de return ERR_PTR(-ENODEV); } +static inline struct regulator *__must_check devm_of_regulator_get_optional(struct device *dev, + struct device_node *node, + const char *id) +{ + return ERR_PTR(-ENODEV); +} + static inline void regulator_put(struct regulator *regulator) { }
There are existing uses for a devres version of of_regulator_get_optional() in power domain drivers. On MediaTek platforms, power domains may have regulator supplies tied to them. The driver currently tries to use devm_regulator_get() to not have to manage the lifecycle, but ends up doing it in a very hacky way by replacing the device node of the power domain controller device to the device node of the power domain that is currently being registered, getting the supply, and reverting the device node. Provide a better API so that the hack can be replaced. Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- Changes since v7: - New patch --- drivers/regulator/devres.c | 39 ++++++++++++++++++++++++++++++ drivers/regulator/internal.h | 16 +++++++----- drivers/regulator/of_regulator.c | 4 +-- include/linux/regulator/consumer.h | 17 +++++++++++++ 4 files changed, 68 insertions(+), 8 deletions(-)