[PATCH v2 0/2] platform_device: add new devres function

[PATCH v2 0/2] platform_device: add new devres function

[Philipp Stanner]

Changes in v2:
- Fix wrong function name in docstring (Uwe)
- Change devres function name so it becomes obvious that it's requesting

Patch #1 adds a new devres function that I found could be useful for the
driver dcss in drm. Patch #2 makes that driver use the new function.

I compiled this successfully but unfortunately don't have the hardware
to test it for dcss.
So you might want to have a closer look.

Greetings,
P.

Philipp Stanner (2):
  platform_device: add devres function region-reqs
  drm/dcss: request memory region

 drivers/base/platform.c             | 38 +++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/dcss/dcss-dev.c |  9 ++++---
 include/linux/platform_device.h     |  3 +++
 3 files changed, 46 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] platform_device: add devres function region-reqs

[Philipp Stanner]

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(+)

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,
-- 
2.43.0

[PATCH v2 2/2] drm/dcss: request memory region

[Philipp Stanner]

The driver's memory regions are currently just ioremap()ed, but not
reserved through a request. That's not a bug, but having the request is
a little more robust.

Implement the region-request through the corresponding managed
devres-function.

Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/imx/dcss/dcss-dev.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/imx/dcss/dcss-dev.c b/drivers/gpu/drm/imx/dcss/dcss-dev.c
index 4f3af0dfb344..63dbb8d9c1b0 100644
--- a/drivers/gpu/drm/imx/dcss/dcss-dev.c
+++ b/drivers/gpu/drm/imx/dcss/dcss-dev.c
@@ -177,10 +177,11 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output)
 		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 = devm_platform_get_and_request_resource(pdev, IORESOURCE_MEM, 
+						     0, "dcss");
+	if (IS_ERR(res)) {
+		dev_err(dev, "cannot get / request memory resource\n");
+		return res;
 	}
 
 	dcss = kzalloc(sizeof(*dcss), GFP_KERNEL);
-- 
2.43.0

Re: [PATCH v2 1/2] platform_device: add devres function region-reqs

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

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



-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] platform_device: add devres function region-reqs

[Philipp Stanner]

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
> 
> 
> 

Re: [PATCH v2 1/2] platform_device: add devres function region-reqs

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] platform_device: add devres function region-reqs

[Philipp Stanner]

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
>