| Message ID | 20240108092042.16949-3-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | platform_device: add new devres function | expand |
Hello Philipp, the Subject is incomprehensible (to me). Maybe make it: platform_device: Add devm function to simplify mem and io requests ? On Mon, Jan 08, 2024 at 10:20:42AM +0100, Philipp Stanner wrote: > Some drivers want to use (request) a region exclusively but nevertheless > create several mappings within that region. > > Currently, there is no managed devres function to request a region > without mapping it. > > Add the function devm_platform_get_resource(). ^ Still the old function name -' Other than that I indifferent if this is a good idea. There are so many helpers around these functions ... Best regards Uwe
On Mon, 2024-01-08 at 10:37 +0100, Uwe Kleine-König wrote: > Hello Philipp, > > the Subject is incomprehensible (to me). Maybe make it: > > platform_device: Add devm function to simplify mem and io > requests > > ? > > On Mon, Jan 08, 2024 at 10:20:42AM +0100, Philipp Stanner wrote: > > Some drivers want to use (request) a region exclusively but > > nevertheless > > create several mappings within that region. > > > > Currently, there is no managed devres function to request a region > > without mapping it. > > > > Add the function devm_platform_get_resource(). > ^ > Still the old function name -' ACK. Monday morning... > > Other than that I indifferent if this is a good idea. There are so > many > helpers around these functions ... Around which, the devres functions in general? There are, but that's kind of the point, unless we'd want everyone to call into the lowest level region-request functions with their own devres callbacks. In any case: What would your suggestion be, should parties who can't (without restructuring very large parts of their code) ioremap() and request() simultaneously just not use devres? See my patch #2 Or is there another way to reach that goal that I'm not aware of? P. > > Best regards > Uwe > > >
On Mon, Jan 08, 2024 at 10:45:31AM +0100, Philipp Stanner wrote: > On Mon, 2024-01-08 at 10:37 +0100, Uwe Kleine-König wrote: > > Other than that I indifferent if this is a good idea. There are so many > > helpers around these functions ... > > Around which, the devres functions in general? There are, but that's > kind of the point, unless we'd want everyone to call into the lowest > level region-request functions with their own devres callbacks. > > In any case: What would your suggestion be, should parties who can't > (without restructuring very large parts of their code) ioremap() and > request() simultaneously just not use devres? See my patch #2 > Or is there another way to reach that goal that I'm not aware of? This wasn't a constructive feedback unfortunately and more a feeling than a measurable criticism. To actually improve the state, maybe first check what helpers are actually there, how they are used and if they are suitable to what they are used for. Having many helpers is a hint that the usage is complicated. Is that because the situation is complicated, or is this just a big pile of inconsistency that can be simplified and consolidated? Also I think there are helpers that take a resource type parameter (as your function) and others hard code it in the function name. Maybe unifying that would be nice, too. Best regards Uwe
Yo! On Mon, 2024-01-08 at 12:46 +0100, Uwe Kleine-König wrote: > On Mon, Jan 08, 2024 at 10:45:31AM +0100, Philipp Stanner wrote: > > On Mon, 2024-01-08 at 10:37 +0100, Uwe Kleine-König wrote: > > > Other than that I indifferent if this is a good idea. There are > > > so many > > > helpers around these functions ... > > > > Around which, the devres functions in general? There are, but > > that's > > kind of the point, unless we'd want everyone to call into the > > lowest > > level region-request functions with their own devres callbacks. > > > > In any case: What would your suggestion be, should parties who > > can't > > (without restructuring very large parts of their code) ioremap() > > and > > request() simultaneously just not use devres? See my patch #2 > > Or is there another way to reach that goal that I'm not aware of? > > This wasn't a constructive feedback unfortunately and more a feeling > than a measurable criticism. To actually improve the state, maybe > first > check what helpers are actually there, how they are used and if they > are > suitable to what they are used for. > > Having many helpers is a hint that the usage is complicated. Is that > because the situation is complicated, or is this just a big pile of > inconsistency that can be simplified and consolidated? I thought about that and tend to believe that you are right in this case. The reason being that there'd be very few callers to such a wrapper. We have the functions for doing pure requests and pure ioremaps, so that should be sufficient. I think we can do sth like this in the rare cases where someone needs to request without (immediately) mapping: struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output) { struct platform_device *pdev = to_platform_device(dev); int ret; struct resource *res; struct dcss_dev *dcss; const struct dcss_type_data *devtype; resource_size_t res_len; devtype = of_device_get_match_data(dev); if (!devtype) { dev_err(dev, "no device match found\n"); return ERR_PTR(-ENODEV); } res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) { dev_err(dev, "cannot get memory resource\n"); return ERR_PTR(-EINVAL); } res_len = res->end - res->start; if (!devm_request_mem_region(pdev->dev, res->start, res_len, "dcss")) { dev_err(dev, "cannot request memory region\n"); return ERR_PTR(-EBUSY); } And then do the associated devm_ioremap()s where they're needed. So I'd 'close' this patch series and handle it entirely through my dcss patch-series. Thx for the feedback P. > > Also I think there are helpers that take a resource type parameter > (as > your function) and others hard code it in the function name. Maybe > unifying that would be nice, too. > > Best regards > Uwe >
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 10c577963418..7b29e6da31ae 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -82,6 +82,44 @@ struct resource *platform_get_mem_or_io(struct platform_device *dev, } EXPORT_SYMBOL_GPL(platform_get_mem_or_io); +/** + * devm_platform_get_and_request_resource - get and request a resource + * + * @pdev: the platform device to get the resource from + * @type: resource type (either IORESOURCE_MEM or IORESOURCE_IO) + * @num: resource index + * @name: name to be associated with the request + * + * Return: a pointer to the resource on success, an ERR_PTR on failure. + * + * Gets a resource and requests it. Use this instead of + * devm_platform_ioremap_resource() only if you have to create several single + * mappings with devm_ioremap(). + */ +struct resource *devm_platform_get_and_request_resource( + struct platform_device *pdev, unsigned int type, + unsigned int num, const char *name) +{ + struct resource *res; + + res = platform_get_resource(pdev, type, num); + if (!res) + return ERR_PTR(-EINVAL); + + if (type & IORESOURCE_MEM) + res = devm_request_mem_region(&pdev->dev, res->start, res->end, name); + else if (type & IORESOURCE_IO) + res = devm_request_region(&pdev->dev, res->start, res->end, name); + else + return ERR_PTR(-EINVAL); + + if (!res) + return ERR_PTR(-EBUSY); + + return res; +} +EXPORT_SYMBOL_GPL(devm_platform_get_and_request_resource); + #ifdef CONFIG_HAS_IOMEM /** * devm_platform_get_and_ioremap_resource - call devm_ioremap_resource() for a diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 7a41c72c1959..44e4ba930d5f 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -59,6 +59,9 @@ extern struct resource *platform_get_resource(struct platform_device *, unsigned int, unsigned int); extern struct resource *platform_get_mem_or_io(struct platform_device *, unsigned int); +extern struct resource *devm_platform_get_and_request_resource( + struct platform_device *pdev, unsigned int type, + unsigned int num, const char *name); extern struct device * platform_find_device_by_driver(struct device *start,
Some drivers want to use (request) a region exclusively but nevertheless create several mappings within that region. Currently, there is no managed devres function to request a region without mapping it. Add the function devm_platform_get_resource(). Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- drivers/base/platform.c | 38 +++++++++++++++++++++++++++++++++ include/linux/platform_device.h | 3 +++ 2 files changed, 41 insertions(+)