[PATCH v2 1/5] ACPI: property: Remove dead code

[PATCH v2 1/5] ACPI: property: Remove dead code

[Andy Shevchenko]

After the commit 3a7a2ab839ad couple of functions became a dead code.
Moreover, for all these years nobody used them. Remove.

Fixes: 3a7a2ab839ad ("ACPI / property: Extend fwnode_property_* to data-only subnodes")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/property.c | 20 --------------------
 include/linux/acpi.h    | 21 ---------------------
 2 files changed, 41 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 16b28084c1ca..22ccab4e7c6d 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -841,20 +841,6 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
 	return ret;
 }
 
-int acpi_dev_prop_read_single(struct acpi_device *adev, const char *propname,
-			      enum dev_prop_type proptype, void *val)
-{
-	int ret;
-
-	if (!adev || !val)
-		return -EINVAL;
-
-	ret = acpi_data_prop_read_single(&adev->data, propname, proptype, val);
-	if (ret < 0 || proptype != ACPI_TYPE_STRING)
-		return ret;
-	return 0;
-}
-
 static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val,
 				       size_t nval)
 {
@@ -995,12 +981,6 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
 	return ret;
 }
 
-int acpi_dev_prop_read(const struct acpi_device *adev, const char *propname,
-		       enum dev_prop_type proptype, void *val, size_t nval)
-{
-	return adev ? acpi_data_prop_read(&adev->data, propname, proptype, val, nval) : -EINVAL;
-}
-
 /**
  * acpi_node_prop_read - retrieve the value of an ACPI property with given name.
  * @fwnode: Firmware node to get the property from.
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index ea296289a94c..14ac25165ae1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1121,14 +1121,9 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
 
 int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
 		       void **valptr);
-int acpi_dev_prop_read_single(struct acpi_device *adev,
-			      const char *propname, enum dev_prop_type proptype,
-			      void *val);
 int acpi_node_prop_read(const struct fwnode_handle *fwnode,
 			const char *propname, enum dev_prop_type proptype,
 			void *val, size_t nval);
-int acpi_dev_prop_read(const struct acpi_device *adev, const char *propname,
-		       enum dev_prop_type proptype, void *val, size_t nval);
 
 struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
 					    struct fwnode_handle *child);
@@ -1230,14 +1225,6 @@ static inline int acpi_node_prop_get(const struct fwnode_handle *fwnode,
 	return -ENXIO;
 }
 
-static inline int acpi_dev_prop_read_single(const struct acpi_device *adev,
-					    const char *propname,
-					    enum dev_prop_type proptype,
-					    void *val)
-{
-	return -ENXIO;
-}
-
 static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode,
 				      const char *propname,
 				      enum dev_prop_type proptype,
@@ -1246,14 +1233,6 @@ static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode,
 	return -ENXIO;
 }
 
-static inline int acpi_dev_prop_read(const struct acpi_device *adev,
-				     const char *propname,
-				     enum dev_prop_type proptype,
-				     void *val, size_t nval)
-{
-	return -ENXIO;
-}
-
 static inline struct fwnode_handle *
 acpi_get_next_subnode(const struct fwnode_handle *fwnode,
 		      struct fwnode_handle *child)
-- 
2.30.0

[PATCH v2 2/5] ACPI: property: Make acpi_node_prop_read() static

[Andy Shevchenko]

There is no users outside of property.c. No need to export
acpi_node_prop_read(), hence make it static.

Fixes: 3708184afc77 ("device property: Move FW type specific functionality to FW specific files")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/property.c |  6 +++---
 include/linux/acpi.h    | 11 -----------
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 22ccab4e7c6d..2b65ad9b4c0d 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -993,9 +993,9 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
  * of the property.  Otherwise, read at most @nval values to the array at the
  * location pointed to by @val.
  */
-int acpi_node_prop_read(const struct fwnode_handle *fwnode,
-			const char *propname, enum dev_prop_type proptype,
-			void *val, size_t nval)
+static int acpi_node_prop_read(const struct fwnode_handle *fwnode,
+			       const char *propname, enum dev_prop_type proptype,
+			       void *val, size_t nval)
 {
 	return acpi_data_prop_read(acpi_device_data_of_node(fwnode),
 				   propname, proptype, val, nval);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 14ac25165ae1..3c5757d539ab 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1121,9 +1121,6 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
 
 int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
 		       void **valptr);
-int acpi_node_prop_read(const struct fwnode_handle *fwnode,
-			const char *propname, enum dev_prop_type proptype,
-			void *val, size_t nval);
 
 struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
 					    struct fwnode_handle *child);
@@ -1225,14 +1222,6 @@ static inline int acpi_node_prop_get(const struct fwnode_handle *fwnode,
 	return -ENXIO;
 }
 
-static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode,
-				      const char *propname,
-				      enum dev_prop_type proptype,
-				      void *val, size_t nval)
-{
-	return -ENXIO;
-}
-
 static inline struct fwnode_handle *
 acpi_get_next_subnode(const struct fwnode_handle *fwnode,
 		      struct fwnode_handle *child)
-- 
2.30.0

[PATCH v2 3/5] ACPI: property: Satisfy kernel doc validator (part 1)

[Andy Shevchenko]

CHECK   drivers/acpi/property.c
 warning: Function parameter or member 'data' not described in 'acpi_data_get_property_array'
 warning: Excess function parameter 'adev' description in 'acpi_data_get_property_array'

Fixes: 3a7a2ab839ad ("ACPI / property: Extend fwnode_property_* to data-only subnodes")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 2b65ad9b4c0d..ab4d7c734b0d 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -564,7 +564,7 @@ int acpi_node_prop_get(const struct fwnode_handle *fwnode,
 
 /**
  * acpi_data_get_property_array - return an ACPI array property with given name
- * @adev: ACPI data object to get the property from
+ * @data: ACPI data object to get the property from
  * @name: Name of the property
  * @type: Expected type of array elements
  * @obj: Location to store a pointer to the property value (if not NULL)
-- 
2.30.0

[PATCH v2 4/5] ACPI: property: Satisfy kernel doc validator (part 2)

[Andy Shevchenko]

CHECK   drivers/acpi/property.c
 warning: Function parameter or member '__fwnode' not described in 'acpi_graph_get_remote_endpoint'
 warning: Excess function parameter 'fwnode' description in 'acpi_graph_get_remote_endpoint'
 warning: Excess function parameter 'endpoint' description in 'acpi_graph_get_remote_endpoint'

Fixes: 0ef7478639c5 ("ACPI: property: Make the ACPI graph API private")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/property.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index ab4d7c734b0d..e312ebaed8db 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1212,8 +1212,7 @@ static struct fwnode_handle *acpi_graph_get_child_prop_value(
 
 /**
  * acpi_graph_get_remote_endpoint - Parses and returns remote end of an endpoint
- * @fwnode: Endpoint firmware node pointing to a remote device
- * @endpoint: Firmware node of remote endpoint is filled here if not %NULL
+ * @__fwnode: Endpoint firmware node pointing to a remote device
  *
  * Returns the remote endpoint corresponding to @__fwnode. NULL on error.
  */
-- 
2.30.0

[PATCH v2 5/5] ACPI: property: Refactor acpi_data_prop_read_single()

[Andy Shevchenko]

Refactor acpi_data_prop_read_single() for less LOCs and better maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/property.c | 80 ++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 46 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index e312ebaed8db..494cf283a573 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -785,60 +785,48 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
 				      enum dev_prop_type proptype, void *val)
 {
 	const union acpi_object *obj;
-	int ret;
+	int ret = 0;
 
-	if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) {
+	if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64)
 		ret = acpi_data_get_property(data, propname, ACPI_TYPE_INTEGER, &obj);
-		if (ret)
-			return ret;
-
-		switch (proptype) {
-		case DEV_PROP_U8:
-			if (obj->integer.value > U8_MAX)
-				return -EOVERFLOW;
-
-			if (val)
-				*(u8 *)val = obj->integer.value;
-
-			break;
-		case DEV_PROP_U16:
-			if (obj->integer.value > U16_MAX)
-				return -EOVERFLOW;
-
-			if (val)
-				*(u16 *)val = obj->integer.value;
-
-			break;
-		case DEV_PROP_U32:
-			if (obj->integer.value > U32_MAX)
-				return -EOVERFLOW;
-
-			if (val)
-				*(u32 *)val = obj->integer.value;
-
-			break;
-		default:
-			if (val)
-				*(u64 *)val = obj->integer.value;
-
-			break;
-		}
-
-		if (!val)
-			return 1;
-	} else if (proptype == DEV_PROP_STRING) {
+	else if (proptype == DEV_PROP_STRING)
 		ret = acpi_data_get_property(data, propname, ACPI_TYPE_STRING, &obj);
-		if (ret)
-			return ret;
+	if (ret)
+		return ret;
 
+	switch (proptype) {
+	case DEV_PROP_U8:
+		if (obj->integer.value > U8_MAX)
+			return -EOVERFLOW;
+		if (val)
+			*(u8 *)val = obj->integer.value;
+		break;
+	case DEV_PROP_U16:
+		if (obj->integer.value > U16_MAX)
+			return -EOVERFLOW;
+		if (val)
+			*(u16 *)val = obj->integer.value;
+		break;
+	case DEV_PROP_U32:
+		if (obj->integer.value > U32_MAX)
+			return -EOVERFLOW;
+		if (val)
+			*(u32 *)val = obj->integer.value;
+		break;
+	case DEV_PROP_U64:
+		if (val)
+			*(u64 *)val = obj->integer.value;
+		break;
+	case DEV_PROP_STRING:
 		if (val)
 			*(char **)val = obj->string.pointer;
-
 		return 1;
-	} else {
-		ret = -EINVAL;
+	default:
+		return -EINVAL;
 	}
-	return ret;
+
+	/* When no storage provided return number of available values */
+	return val ? 0 : 1;
 }
 
 static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val,
-- 
2.30.0

Re: [PATCH v2 5/5] ACPI: property: Refactor acpi_data_prop_read_single()

[Rafael J. Wysocki]

On Fri, Feb 12, 2021 at 3:14 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
 >
> Refactor acpi_data_prop_read_single() for less LOCs and better maintenance.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/acpi/property.c | 80 ++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index e312ebaed8db..494cf283a573 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -785,60 +785,48 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
>                                       enum dev_prop_type proptype, void *val)
>  {
>         const union acpi_object *obj;
> -       int ret;
> +       int ret = 0;
>
> -       if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) {
> +       if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64)
>                 ret = acpi_data_get_property(data, propname, ACPI_TYPE_INTEGER, &obj);
> -               if (ret)
> -                       return ret;
> -
> -               switch (proptype) {
> -               case DEV_PROP_U8:
> -                       if (obj->integer.value > U8_MAX)
> -                               return -EOVERFLOW;
> -
> -                       if (val)
> -                               *(u8 *)val = obj->integer.value;
> -
> -                       break;

The empty lines of code above are intentional, so please retain them.

> -               case DEV_PROP_U16:
> -                       if (obj->integer.value > U16_MAX)
> -                               return -EOVERFLOW;
> -
> -                       if (val)
> -                               *(u16 *)val = obj->integer.value;
> -
> -                       break;
> -               case DEV_PROP_U32:
> -                       if (obj->integer.value > U32_MAX)
> -                               return -EOVERFLOW;
> -
> -                       if (val)
> -                               *(u32 *)val = obj->integer.value;
> -
> -                       break;
> -               default:
> -                       if (val)
> -                               *(u64 *)val = obj->integer.value;
> -
> -                       break;
> -               }
> -
> -               if (!val)
> -                       return 1;
> -       } else if (proptype == DEV_PROP_STRING) {
> +       else if (proptype == DEV_PROP_STRING)
>                 ret = acpi_data_get_property(data, propname, ACPI_TYPE_STRING, &obj);
> -               if (ret)
> -                       return ret;
> +       if (ret)
> +               return ret;

else if (!val)
        ret = 1;

>
> +       switch (proptype) {
> +       case DEV_PROP_U8:
> +               if (obj->integer.value > U8_MAX)
> +                       return -EOVERFLOW;
> +               if (val)
> +                       *(u8 *)val = obj->integer.value;
> +               break;
> +       case DEV_PROP_U16:
> +               if (obj->integer.value > U16_MAX)
> +                       return -EOVERFLOW;
> +               if (val)
> +                       *(u16 *)val = obj->integer.value;
> +               break;
> +       case DEV_PROP_U32:
> +               if (obj->integer.value > U32_MAX)
> +                       return -EOVERFLOW;
> +               if (val)
> +                       *(u32 *)val = obj->integer.value;
> +               break;
> +       case DEV_PROP_U64:
> +               if (val)
> +                       *(u64 *)val = obj->integer.value;
> +               break;
> +       case DEV_PROP_STRING:
>                 if (val)
>                         *(char **)val = obj->string.pointer;
> -
>                 return 1;
> -       } else {
> -               ret = -EINVAL;
> +       default:
> +               return -EINVAL;
>         }
> -       return ret;

Retain this.

> +
> +       /* When no storage provided return number of available values */
> +       return val ? 0 : 1;

And this is just not looking good to me, sorry.

>  }
>
>  static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val,
> --

Re: [PATCH v2 1/5] ACPI: property: Remove dead code

[Rafael J. Wysocki]

On Fri, Feb 12, 2021 at 3:16 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> After the commit 3a7a2ab839ad couple of functions became a dead code.
> Moreover, for all these years nobody used them. Remove.
>
> Fixes: 3a7a2ab839ad ("ACPI / property: Extend fwnode_property_* to data-only subnodes")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patches [1-4/5] from this series applied as 5.12 material and please
see my comments for the last one sent separately.

Thanks!

> ---
>  drivers/acpi/property.c | 20 --------------------
>  include/linux/acpi.h    | 21 ---------------------
>  2 files changed, 41 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 16b28084c1ca..22ccab4e7c6d 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -841,20 +841,6 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
>         return ret;
>  }
>
> -int acpi_dev_prop_read_single(struct acpi_device *adev, const char *propname,
> -                             enum dev_prop_type proptype, void *val)
> -{
> -       int ret;
> -
> -       if (!adev || !val)
> -               return -EINVAL;
> -
> -       ret = acpi_data_prop_read_single(&adev->data, propname, proptype, val);
> -       if (ret < 0 || proptype != ACPI_TYPE_STRING)
> -               return ret;
> -       return 0;
> -}
> -
>  static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val,
>                                        size_t nval)
>  {
> @@ -995,12 +981,6 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
>         return ret;
>  }
>
> -int acpi_dev_prop_read(const struct acpi_device *adev, const char *propname,
> -                      enum dev_prop_type proptype, void *val, size_t nval)
> -{
> -       return adev ? acpi_data_prop_read(&adev->data, propname, proptype, val, nval) : -EINVAL;
> -}
> -
>  /**
>   * acpi_node_prop_read - retrieve the value of an ACPI property with given name.
>   * @fwnode: Firmware node to get the property from.
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index ea296289a94c..14ac25165ae1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1121,14 +1121,9 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
>
>  int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
>                        void **valptr);
> -int acpi_dev_prop_read_single(struct acpi_device *adev,
> -                             const char *propname, enum dev_prop_type proptype,
> -                             void *val);
>  int acpi_node_prop_read(const struct fwnode_handle *fwnode,
>                         const char *propname, enum dev_prop_type proptype,
>                         void *val, size_t nval);
> -int acpi_dev_prop_read(const struct acpi_device *adev, const char *propname,
> -                      enum dev_prop_type proptype, void *val, size_t nval);
>
>  struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
>                                             struct fwnode_handle *child);
> @@ -1230,14 +1225,6 @@ static inline int acpi_node_prop_get(const struct fwnode_handle *fwnode,
>         return -ENXIO;
>  }
>
> -static inline int acpi_dev_prop_read_single(const struct acpi_device *adev,
> -                                           const char *propname,
> -                                           enum dev_prop_type proptype,
> -                                           void *val)
> -{
> -       return -ENXIO;
> -}
> -
>  static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode,
>                                       const char *propname,
>                                       enum dev_prop_type proptype,
> @@ -1246,14 +1233,6 @@ static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode,
>         return -ENXIO;
>  }
>
> -static inline int acpi_dev_prop_read(const struct acpi_device *adev,
> -                                    const char *propname,
> -                                    enum dev_prop_type proptype,
> -                                    void *val, size_t nval)
> -{
> -       return -ENXIO;
> -}
> -
>  static inline struct fwnode_handle *
>  acpi_get_next_subnode(const struct fwnode_handle *fwnode,
>                       struct fwnode_handle *child)
> --
> 2.30.0
>

Re: [PATCH v2 5/5] ACPI: property: Refactor acpi_data_prop_read_single()

[Andy Shevchenko]

On Fri, Feb 12, 2021 at 03:31:24PM +0100, Rafael J. Wysocki wrote:
> On Fri, Feb 12, 2021 at 3:14 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

> > Refactor acpi_data_prop_read_single() for less LOCs and better maintenance.

Thanks for review, my answers below.

...

> > +       if (ret)
> > +               return ret;
> 
> else if (!val)
>         ret = 1;

But then it become a bug again :-)

...

> And this is just not looking good to me, sorry.

Yeah, I think this patch is not needed right now. At least it seems no gain
from it.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] ACPI: property: Refactor acpi_data_prop_read_single()

[Rafael J. Wysocki]

On Fri, Feb 12, 2021 at 5:01 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Feb 12, 2021 at 03:31:24PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Feb 12, 2021 at 3:14 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
>
> > > Refactor acpi_data_prop_read_single() for less LOCs and better maintenance.
>
> Thanks for review, my answers below.
>
> ...
>
> > > +       if (ret)
> > > +               return ret;
> >
> > else if (!val)
> >         ret = 1;
>
> But then it become a bug again :-)

I'm not sure why?  The checks below will still happen and they may
cause an error to be returned.

> ...
>
> > And this is just not looking good to me, sorry.
>
> Yeah, I think this patch is not needed right now. At least it seems no gain
> from it.

OK

Re: [PATCH v2 5/5] ACPI: property: Refactor acpi_data_prop_read_single()

[Andy Shevchenko]

On Fri, Feb 12, 2021 at 6:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Feb 12, 2021 at 5:01 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Feb 12, 2021 at 03:31:24PM +0100, Rafael J. Wysocki wrote:
> > > On Fri, Feb 12, 2021 at 3:14 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > +       if (ret)
> > > > +               return ret;
> > >
> > > else if (!val)
> > >         ret = 1;
> >
> > But then it become a bug again :-)
>
> I'm not sure why?  The checks below will still happen and they may
> cause an error to be returned.

Oh, I misinterpreted ret = as plain return. Right. Seems okay.

-- 
With Best Regards,
Andy Shevchenko