[PATCH 0/6] Refactor driver resource allocation code

[PATCH 0/6] Refactor driver resource allocation code

[Andrey Smirnov]

Everyone:

This series is my attempt to minimzie amount of repeating code in
various resource allocating function in drivers/base/driver.c. All
patches are optional, so if some/all changes don't seem like an
improvement, I am more than happy to drop them.

Thanks,
Andrey Smirnov

Andrey Smirnov (6):
  drivers: base: Simplify generic_memmap_ro()
  drivers: base: Drop dev_get_mem_region_by_name()
  drivers: base: Share implementations for dev_get_resource*()
  drivers: base: Simplify dev_request_mem_region_err_null()
  drivers: base: Share code for dev_request_mem_region*()
  drivers: base: Share code for getting and then requesting a region

 drivers/base/driver.c | 120 +++++++++++++++++++-----------------------
 include/driver.h      |   4 --
 2 files changed, 55 insertions(+), 69 deletions(-)

-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/6] drivers: base: Simplify generic_memmap_ro()

[Andrey Smirnov]

Simplify generic_memmap_ro() by re-implementing it using
generic_memmap_rw().

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/base/driver.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 1941a972c..c74fee99f 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -440,29 +440,23 @@ void __iomem *dev_request_mem_region(struct device_d *dev, int num)
 }
 EXPORT_SYMBOL(dev_request_mem_region);
 
-int generic_memmap_ro(struct cdev *cdev, void **map, int flags)
+int generic_memmap_rw(struct cdev *cdev, void **map, int flags)
 {
 	if (!cdev->dev)
 		return -EINVAL;
 
-	if (flags & PROT_WRITE)
-		return -EACCES;
 	*map = dev_get_mem_region(cdev->dev, 0);
 	if (IS_ERR(*map))
 		return PTR_ERR(*map);
 	return 0;
 }
 
-int generic_memmap_rw(struct cdev *cdev, void **map, int flags)
+int generic_memmap_ro(struct cdev *cdev, void **map, int flags)
 {
-	if (!cdev->dev)
-		return -EINVAL;
-
-	*map = dev_get_mem_region(cdev->dev, 0);
-	if (IS_ERR(*map))
-		return PTR_ERR(*map);
+	if (flags & PROT_WRITE)
+		return -EACCES;
 
-	return 0;
+	return generic_memmap_rw(cdev, map, flags);
 }
 
 int dummy_probe(struct device_d *dev)
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/6] drivers: base: Drop dev_get_mem_region_by_name()

[Andrey Smirnov]

Drop dev_get_mem_region_by_name() which doesn't seem to have any users
in the codebase.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/base/driver.c | 12 ------------
 include/driver.h      |  4 ----
 2 files changed, 16 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index c74fee99f..22ca96d7e 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -373,18 +373,6 @@ struct resource *dev_get_resource_by_name(struct device_d *dev,
 	return ERR_PTR(-ENOENT);
 }
 
-void *dev_get_mem_region_by_name(struct device_d *dev, const char *name)
-{
-	struct resource *res;
-
-	res = dev_get_resource_by_name(dev, IORESOURCE_MEM, name);
-	if (IS_ERR(res))
-		return ERR_CAST(res);
-
-	return (void __force *)res->start;
-}
-EXPORT_SYMBOL(dev_get_mem_region_by_name);
-
 void __iomem *dev_request_mem_region_by_name(struct device_d *dev, const char *name)
 {
 	struct resource *res;
diff --git a/include/driver.h b/include/driver.h
index db844aed3..7da184d3a 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -201,10 +201,6 @@ struct resource *dev_get_resource(struct device_d *dev, unsigned long type,
 struct resource *dev_get_resource_by_name(struct device_d *dev,
 					  unsigned long type,
 					  const char *name);
-/*
- * get register base 'name' for a device
- */
-void *dev_get_mem_region_by_name(struct device_d *dev, const char *name);
 
 /*
  * exlusively request register base 'name' for a device
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/6] drivers: base: Share implementations for dev_get_resource*()

[Andrey Smirnov]

Both dev_get_resource() and dev_get_resource_by_name() implement the
same algorithm and differ only in matching criteria. Move that
algoritm into a separate generic function and implement those two
functions using it.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/base/driver.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 22ca96d7e..4c1b6110a 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -325,14 +325,27 @@ int register_driver(struct driver_d *drv)
 }
 EXPORT_SYMBOL(register_driver);
 
-struct resource *dev_get_resource(struct device_d *dev, unsigned long type,
-				  int num)
+static struct resource *
+dev_get_resource_by_name_or_id(struct device_d *dev, unsigned long type,
+			       const char *name, int num)
 {
 	int i, n = 0;
 
 	for (i = 0; i < dev->num_resources; i++) {
 		struct resource *res = &dev->resource[i];
-		if (resource_type(res) == type) {
+		if (resource_type(res) != type)
+			continue;
+
+		if (name) {
+			/*
+			 * If name is proveded we search using it
+			 */
+			if (res->name && !strcmp(name, res->name))
+				return res;
+		} else {
+			/*
+			 * Otherwise we match against provided id
+			 */
 			if (n == num)
 				return res;
 			n++;
@@ -342,6 +355,12 @@ struct resource *dev_get_resource(struct device_d *dev, unsigned long type,
 	return ERR_PTR(-ENOENT);
 }
 
+struct resource *dev_get_resource(struct device_d *dev, unsigned long type,
+				  int num)
+{
+	return dev_get_resource_by_name_or_id(dev, type, NULL, num);
+}
+
 void *dev_get_mem_region(struct device_d *dev, int num)
 {
 	struct resource *res;
@@ -358,19 +377,7 @@ struct resource *dev_get_resource_by_name(struct device_d *dev,
 					  unsigned long type,
 					  const char *name)
 {
-	int i;
-
-	for (i = 0; i < dev->num_resources; i++) {
-		struct resource *res = &dev->resource[i];
-		if (resource_type(res) != type)
-			continue;
-		if (!res->name)
-			continue;
-		if (!strcmp(name, res->name))
-			return res;
-	}
-
-	return ERR_PTR(-ENOENT);
+	return dev_get_resource_by_name_or_id(dev, type, name, -1);
 }
 
 void __iomem *dev_request_mem_region_by_name(struct device_d *dev, const char *name)
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/6] drivers: base: Simplify dev_request_mem_region_err_null()

[Andrey Smirnov]

Simplify dev_request_mem_region_err_null() by making use of
dev_request_mem_resource() to implement it.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/base/driver.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 4c1b6110a..c687afd75 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -396,32 +396,28 @@ void __iomem *dev_request_mem_region_by_name(struct device_d *dev, const char *n
 }
 EXPORT_SYMBOL(dev_request_mem_region_by_name);
 
-void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num)
+struct resource *dev_request_mem_resource(struct device_d *dev, int num)
 {
 	struct resource *res;
 
 	res = dev_get_resource(dev, IORESOURCE_MEM, num);
 	if (IS_ERR(res))
-		return NULL;
-
-	res = request_iomem_region(dev_name(dev), res->start, res->end);
-	if (IS_ERR(res))
-		return NULL;
+		return ERR_CAST(res);
 
-	return IOMEM(res->start);
+	return request_iomem_region(dev_name(dev), res->start, res->end);
 }
-EXPORT_SYMBOL(dev_request_mem_region_err_null);
 
-struct resource *dev_request_mem_resource(struct device_d *dev, int num)
+void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num)
 {
 	struct resource *res;
 
-	res = dev_get_resource(dev, IORESOURCE_MEM, num);
+	res = dev_request_mem_resource(dev, num);
 	if (IS_ERR(res))
-		return ERR_CAST(res);
+		return NULL;
 
-	return request_iomem_region(dev_name(dev), res->start, res->end);
+	return IOMEM(res->start);
 }
+EXPORT_SYMBOL(dev_request_mem_region_err_null);
 
 void __iomem *dev_request_mem_region(struct device_d *dev, int num)
 {
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/6] drivers: base: Share code for dev_request_mem_region*()

[Andrey Smirnov]

Both dev_request_mem_region() and dev_request_mem_region_err_null()
implement exactly the same functionality different only in error
reporting value. Change the code to make use of a common generic
function whose return type can be specified via an argument.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/base/driver.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index c687afd75..476f844be 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -407,27 +407,27 @@ struct resource *dev_request_mem_resource(struct device_d *dev, int num)
 	return request_iomem_region(dev_name(dev), res->start, res->end);
 }
 
-void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num)
+static void __iomem *__dev_request_mem_region(struct device_d *dev, int num,
+					      bool null_on_error)
 {
 	struct resource *res;
 
 	res = dev_request_mem_resource(dev, num);
 	if (IS_ERR(res))
-		return NULL;
+		return null_on_error ? NULL : ERR_CAST(res);
 
 	return IOMEM(res->start);
 }
+
+void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num)
+{
+	return __dev_request_mem_region(dev, num, true);
+}
 EXPORT_SYMBOL(dev_request_mem_region_err_null);
 
 void __iomem *dev_request_mem_region(struct device_d *dev, int num)
 {
-	struct resource *res;
-
-	res = dev_request_mem_resource(dev, num);
-	if (IS_ERR(res))
-		return ERR_CAST(res);
-
-	return IOMEM(res->start);
+	return __dev_request_mem_region(dev, num, false);
 }
 EXPORT_SYMBOL(dev_request_mem_region);
 
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/6] drivers: base: Share code for getting and then requesting a region

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/base/driver.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 476f844be..687e36e1b 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -355,6 +355,19 @@ dev_get_resource_by_name_or_id(struct device_d *dev, unsigned long type,
 	return ERR_PTR(-ENOENT);
 }
 
+static struct resource *
+dev_request_resource_by_name_or_id(struct device_d *dev, unsigned long type,
+				   const char *name, int num)
+{
+	struct resource *res;
+
+	res = dev_get_resource_by_name_or_id(dev, type, name, num);
+	if (IS_ERR(res))
+		return ERR_CAST(res);
+
+	return request_iomem_region(dev_name(dev), res->start, res->end);
+}
+
 struct resource *dev_get_resource(struct device_d *dev, unsigned long type,
 				  int num)
 {
@@ -384,11 +397,8 @@ void __iomem *dev_request_mem_region_by_name(struct device_d *dev, const char *n
 {
 	struct resource *res;
 
-	res = dev_get_resource_by_name(dev, IORESOURCE_MEM, name);
-	if (IS_ERR(res))
-		return ERR_CAST(res);
-
-	res = request_iomem_region(dev_name(dev), res->start, res->end);
+	res = dev_request_resource_by_name_or_id(dev, IORESOURCE_MEM,
+						 name, -1);
 	if (IS_ERR(res))
 		return ERR_CAST(res);
 
@@ -398,13 +408,8 @@ EXPORT_SYMBOL(dev_request_mem_region_by_name);
 
 struct resource *dev_request_mem_resource(struct device_d *dev, int num)
 {
-	struct resource *res;
-
-	res = dev_get_resource(dev, IORESOURCE_MEM, num);
-	if (IS_ERR(res))
-		return ERR_CAST(res);
-
-	return request_iomem_region(dev_name(dev), res->start, res->end);
+	return dev_request_resource_by_name_or_id(dev, IORESOURCE_MEM,
+						  NULL, num);
 }
 
 static void __iomem *__dev_request_mem_region(struct device_d *dev, int num,
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/6] drivers: base: Simplify generic_memmap_ro()

[Sam Ravnborg]

Hi Andrey.

On Fri, Oct 26, 2018 at 06:32:25PM -0700, Andrey Smirnov wrote:
> Simplify generic_memmap_ro() by re-implementing it using
> generic_memmap_rw().
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/base/driver.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 1941a972c..c74fee99f 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -440,29 +440,23 @@ void __iomem *dev_request_mem_region(struct device_d *dev, int num)
>  }
>  EXPORT_SYMBOL(dev_request_mem_region);
>  
> -int generic_memmap_ro(struct cdev *cdev, void **map, int flags)
> +int generic_memmap_rw(struct cdev *cdev, void **map, int flags)
>  {
>  	if (!cdev->dev)
>  		return -EINVAL;
>  
> -	if (flags & PROT_WRITE)
> -		return -EACCES;
>  	*map = dev_get_mem_region(cdev->dev, 0);


>  	if (IS_ERR(*map))
>  		return PTR_ERR(*map);
>  	return 0;

Use PTR_ERR_OR_ZERO()?
The same result, just a bit shorter.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/6] drivers: base: Share code for dev_request_mem_region*()

[Sam Ravnborg]

Hi Andrey

On Fri, Oct 26, 2018 at 06:32:29PM -0700, Andrey Smirnov wrote:
> Both dev_request_mem_region() and dev_request_mem_region_err_null()
> implement exactly the same functionality different only in error
> reporting value. Change the code to make use of a common generic
> function whose return type can be specified via an argument.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Using functions that takes a boolean to do two different things
is not an improvement.
At least not when the functions are this small and obvious.

This is not in the public interface, only a few static
functions which helps a lot.

Well, thats my personal preference, and in this case
not something I feel strong about.

	Sam


> ---
>  drivers/base/driver.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index c687afd75..476f844be 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -407,27 +407,27 @@ struct resource *dev_request_mem_resource(struct device_d *dev, int num)
>  	return request_iomem_region(dev_name(dev), res->start, res->end);
>  }
>  
> -void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num)
> +static void __iomem *__dev_request_mem_region(struct device_d *dev, int num,
> +					      bool null_on_error)
>  {
>  	struct resource *res;
>  
>  	res = dev_request_mem_resource(dev, num);
>  	if (IS_ERR(res))
> -		return NULL;
> +		return null_on_error ? NULL : ERR_CAST(res);
>  
>  	return IOMEM(res->start);
>  }
> +
> +void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num)
> +{
> +	return __dev_request_mem_region(dev, num, true);
> +}
>  EXPORT_SYMBOL(dev_request_mem_region_err_null);
>  
>  void __iomem *dev_request_mem_region(struct device_d *dev, int num)
>  {
> -	struct resource *res;
> -
> -	res = dev_request_mem_resource(dev, num);
> -	if (IS_ERR(res))
> -		return ERR_CAST(res);
> -
> -	return IOMEM(res->start);
> +	return __dev_request_mem_region(dev, num, false);
>  }
>  EXPORT_SYMBOL(dev_request_mem_region);
>  
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/6] drivers: base: Share code for dev_request_mem_region*()

[Sam Ravnborg]

Hi Andrey,

On Fri, Oct 26, 2018 at 06:32:29PM -0700, Andrey Smirnov wrote:
> Both dev_request_mem_region() and dev_request_mem_region_err_null()
> implement exactly the same functionality different only in error
> reporting value. Change the code to make use of a common generic
> function whose return type can be specified via an argument.

Same story as PATCH 6/6 where a helper functions does
two things depending on a bool.
For simple functions like this my personal preference
is to keep the small functions that are more explicit.

	Sam

> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/base/driver.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index c687afd75..476f844be 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -407,27 +407,27 @@ struct resource *dev_request_mem_resource(struct device_d *dev, int num)
>  	return request_iomem_region(dev_name(dev), res->start, res->end);
>  }
>  
> -void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num)
> +static void __iomem *__dev_request_mem_region(struct device_d *dev, int num,
> +					      bool null_on_error)
>  {
>  	struct resource *res;
>  
>  	res = dev_request_mem_resource(dev, num);
>  	if (IS_ERR(res))
> -		return NULL;
> +		return null_on_error ? NULL : ERR_CAST(res);
>  
>  	return IOMEM(res->start);
>  }
> +
> +void __iomem *dev_request_mem_region_err_null(struct device_d *dev, int num)
> +{
> +	return __dev_request_mem_region(dev, num, true);
> +}
>  EXPORT_SYMBOL(dev_request_mem_region_err_null);
>  
>  void __iomem *dev_request_mem_region(struct device_d *dev, int num)
>  {
> -	struct resource *res;
> -
> -	res = dev_request_mem_resource(dev, num);
> -	if (IS_ERR(res))
> -		return ERR_CAST(res);
> -
> -	return IOMEM(res->start);
> +	return __dev_request_mem_region(dev, num, false);
>  }
>  EXPORT_SYMBOL(dev_request_mem_region);
>  
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/6] drivers: base: Share code for dev_request_mem_region*()

[Sam Ravnborg]

On Sat, Oct 27, 2018 at 10:26:56AM +0200, Sam Ravnborg wrote:
> Hi Andrey,
> 
> On Fri, Oct 26, 2018 at 06:32:29PM -0700, Andrey Smirnov wrote:
> > Both dev_request_mem_region() and dev_request_mem_region_err_null()
> > implement exactly the same functionality different only in error
> > reporting value. Change the code to make use of a common generic
> > function whose return type can be specified via an argument.
> 
> Same story as PATCH 6/6 where a helper functions does
> two things depending on a bool.
> For simple functions like this my personal preference
> is to keep the small functions that are more explicit.
Hmm, seems coffee did not kick in yet.
There is only this single patch with the boolean argument.
Sorry for the noise.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/6] drivers: base: Share implementations for dev_get_resource*()

[Sascha Hauer]

On Fri, Oct 26, 2018 at 06:32:27PM -0700, Andrey Smirnov wrote:
> Both dev_get_resource() and dev_get_resource_by_name() implement the
> same algorithm and differ only in matching criteria. Move that
> algoritm into a separate generic function and implement those two
> functions using it.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/base/driver.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 22ca96d7e..4c1b6110a 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -325,14 +325,27 @@ int register_driver(struct driver_d *drv)
>  }
>  EXPORT_SYMBOL(register_driver);
>  
> -struct resource *dev_get_resource(struct device_d *dev, unsigned long type,
> -				  int num)
> +static struct resource *
> +dev_get_resource_by_name_or_id(struct device_d *dev, unsigned long type,
> +			       const char *name, int num)

The behaviour of this function is a bit ambiguous: type must always match,
name must match if given, num must match if name is not given but is otherwise
ignored. Given that dev_get_resource() and dev_get_resource_by_name()
both implement a simpler behaviour and both are straight forward to read
I don't think this is an improvement.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/6] drivers: base: Share code for dev_request_mem_region*()

[Sascha Hauer]

On Sat, Oct 27, 2018 at 10:20:39AM +0200, Sam Ravnborg wrote:
> Hi Andrey
> 
> On Fri, Oct 26, 2018 at 06:32:29PM -0700, Andrey Smirnov wrote:
> > Both dev_request_mem_region() and dev_request_mem_region_err_null()
> > implement exactly the same functionality different only in error
> > reporting value. Change the code to make use of a common generic
> > function whose return type can be specified via an argument.
> > 
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> 
> Using functions that takes a boolean to do two different things
> is not an improvement.
> At least not when the functions are this small and obvious.

+1

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/6] Refactor driver resource allocation code

[Sascha Hauer]

On Fri, Oct 26, 2018 at 06:32:24PM -0700, Andrey Smirnov wrote:
> Everyone:
> 
> This series is my attempt to minimzie amount of repeating code in
> various resource allocating function in drivers/base/driver.c. All
> patches are optional, so if some/all changes don't seem like an
> improvement, I am more than happy to drop them.
> 
> Thanks,
> Andrey Smirnov
> 
> Andrey Smirnov (6):
>   drivers: base: Simplify generic_memmap_ro()
>   drivers: base: Drop dev_get_mem_region_by_name()
>   drivers: base: Share implementations for dev_get_resource*()
>   drivers: base: Simplify dev_request_mem_region_err_null()
>   drivers: base: Share code for dev_request_mem_region*()
>   drivers: base: Share code for getting and then requesting a region

Ok, I took:

[PATCH 1/6] drivers: base: Simplify generic_memmap_ro()
[PATCH 2/6] drivers: base: Drop dev_get_mem_region_by_name()
[PATCH 4/6] drivers: base: Simplify dev_request_mem_region_err_null()

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox