Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1463365 > unrolled thread

[PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property. Finlye Xiao <finley.xiao@rock-chips.com> - 2016-08-16 05:00 +0200
    Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property. Heiko Stübner <heiko@sntech.de> - 2016-08-16 14:20 +0200
    Re: [PATCH v1 2/3] of: Add support for reading a s32 from a  multi-value property. David Woodhouse <dwmw2@infradead.org> - 2016-08-19 16:20 +0200
      Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property. Heiko Stuebner <heiko@sntech.de> - 2016-08-19 22:50 +0200
        Re: [PATCH v1 2/3] of: Add support for reading a s32 from a  multi-value property. David Woodhouse <dwmw2@infradead.org> - 2016-08-19 23:00 +0200
          Re: [PATCH v1 2/3] of: Add support for reading a s32 from a  multi-value property. Finley Xiao <finley.xiao@rock-chips.com> - 2016-08-22 16:10 +0200
          Re: [PATCH v1 2/3] of: Add support for reading a s32 from a  multi-value property. Mark Rutland <mark.rutland@arm.com> - 2016-08-22 17:00 +0200

#1463365 — [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

From: Finley Xiao <finley.xiao@rock-chips.com>

This patch adds an of_property_read_s32_index() function to allow
reading a single indexed s32 value from a property containing multiple
s32 values.

Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
---
 drivers/of/base.c  | 23 +++++++++++++++++++++++
 include/linux/of.h | 10 ++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7792266..346457d 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1200,6 +1200,29 @@ int of_property_read_u32_index(const struct device_node *np,
 EXPORT_SYMBOL_GPL(of_property_read_u32_index);
 
 /**
+ * of_property_read_s32_index - Find and read a s32 from a multi-value property.
+ *
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @index:	index of the u32 in the list of values
+ * @out_value:	pointer to return value, modified only if no error.
+ *
+ * Search for a property in a device node and read nth 32-bit value from
+ * it. Returns 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data isn't large enough.
+ *
+ * The out_value is modified only if a valid s32 value can be decoded.
+ */
+int of_property_read_s32_index(const struct device_node *np,
+			       const char *propname, u32 index, s32 *out_value)
+{
+	return of_property_read_u32_index(np, propname, index,
+		(u32 *)out_value);
+}
+EXPORT_SYMBOL_GPL(of_property_read_s32_index);
+
+/**
  * of_property_read_u8_array - Find and read an array of u8 from a property.
  *
  * @np:		device node from which the property value is to be read.
diff --git a/include/linux/of.h b/include/linux/of.h
index 3d9ff8e..cb9a627 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -291,6 +291,9 @@ extern int of_property_count_elems_of_size(const struct device_node *np,
 extern int of_property_read_u32_index(const struct device_node *np,
 				       const char *propname,
 				       u32 index, u32 *out_value);
+extern int of_property_read_s32_index(const struct device_node *np,
+				      const char *propname,
+				      u32 index, s32 *out_value);
 extern int of_property_read_u8_array(const struct device_node *np,
 			const char *propname, u8 *out_values, size_t sz);
 extern int of_property_read_u16_array(const struct device_node *np,
@@ -536,6 +539,13 @@ static inline int of_property_read_u32_index(const struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int of_property_read_s32_index(const struct device_node *np,
+					     const char *propname,
+					     u32 index, s32 *out_value)
+{
+	return -ENOSYS;
+}
+
 static inline int of_property_read_u8_array(const struct device_node *np,
 			const char *propname, u8 *out_values, size_t sz)
 {
-- 
1.9.1

[toc] | [next] | [standalone]

#1463748

Hi Finley,

Am Dienstag, 16. August 2016, 10:38:58 schrieb Finlye Xiao:
> From: Finley Xiao <finley.xiao@rock-chips.com>
> 
> This patch adds an of_property_read_s32_index() function to allow
> reading a single indexed s32 value from a property containing multiple
> s32 values.
>
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>

for people reading along, this simply duplicates what of_property_read_s32 
already does, only for indexed properties.

But I do have one issue below.

> ---
>  drivers/of/base.c  | 23 +++++++++++++++++++++++
>  include/linux/of.h | 10 ++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 7792266..346457d 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1200,6 +1200,29 @@ int of_property_read_u32_index(const struct
> device_node *np, EXPORT_SYMBOL_GPL(of_property_read_u32_index);
> 
>  /**
> + * of_property_read_s32_index - Find and read a s32 from a multi-value
> property. + *
> + * @np:		device node from which the property value is to be read.
> + * @propname:	name of the property to be searched.
> + * @index:	index of the u32 in the list of values
> + * @out_value:	pointer to return value, modified only if no error.
> + *
> + * Search for a property in a device node and read nth 32-bit value from
> + * it. Returns 0 on success, -EINVAL if the property does not exist,
> + * -ENODATA if property does not have a value, and -EOVERFLOW if the
> + * property data isn't large enough.
> + *
> + * The out_value is modified only if a valid s32 value can be decoded.
> + */
> +int of_property_read_s32_index(const struct device_node *np,
> +			       const char *propname, u32 index, s32 *out_value)
> +{
> +	return of_property_read_u32_index(np, propname, index,
> +		(u32 *)out_value);
> +}
> +EXPORT_SYMBOL_GPL(of_property_read_s32_index);

I think this could very well live directly in the of.h header below 
of_property_read_s32 or so.

As this also only calls the more generic u32 variant, it can be just a static 
inline there and does not need to duplicate the whole stub voodoo, as the 
of_property_read_u32_index will also generate the -ENOSYS error in the !OF 
case.


Heiko

[toc] | [prev] | [next] | [standalone]

#1466460 — Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2016-08-16 at 10:38 +0800, Finlye Xiao wrote:
> From: Finley Xiao <finley.xiao@rock-chips.com>
> 
> This patch adds an of_property_read_s32_index() function to allow
> reading a single indexed s32 value from a property containing multiple
> s32 values.
> 
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>

NAK.

Nobody should be using the old of_property_* functions any more anyway.
You should be using the generic device_propery_* functions which work
regardless of where the information comes from (actual DT vs. ACPI
_DSD).

So no, don't *add* any more of these functions. Only add the generic
version. And if your driver isn't using the generic property
functions... fix it.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1466672

Am Freitag, 19. August 2016, 15:15:19 CEST schrieb David Woodhouse:
> On Tue, 2016-08-16 at 10:38 +0800, Finlye Xiao wrote:
> > From: Finley Xiao <finley.xiao@rock-chips.com>
> > 
> > This patch adds an of_property_read_s32_index() function to allow
> > reading a single indexed s32 value from a property containing multiple
> > s32 values.
> > 
> > Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> 
> NAK.
> 
> Nobody should be using the old of_property_* functions any more anyway.
> You should be using the generic device_propery_* functions which work
> regardless of where the information comes from (actual DT vs. ACPI
> _DSD).
>
> So no, don't *add* any more of these functions. Only add the generic
> version. And if your driver isn't using the generic property
> functions... fix it.

As far as I can see, all the device_property_* functions are grounded on their
of_property_*, acpi_property_* etc counterparts and functions reading specific
elements (the _index variants) are currently not available at all.

drivers/base/property.c:
#define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval)				\
	(val) ? of_property_read_##type##_array((node), (propname), (val), (nval))	\
	      : of_property_count_elems_of_size((node), (propname), sizeof(type))

So even if you're using the device_property_* functions you'd still need
a match in the underlying functions or am I missing something?

[toc] | [prev] | [next] | [standalone]

#1466677 — Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2016-08-19 at 22:41 +0200, Heiko Stuebner wrote:
> 
> > So no, don't *add* any more of these functions. Only add the generic
> > version. And if your driver isn't using the generic property
> > functions... fix it.
> 
> As far as I can see, all the device_property_* functions are grounded on their
> of_property_*, acpi_property_* etc counterparts and functions reading specific
> elements (the _index variants) are currently not available at all.
> 
> drivers/base/property.c:
> #define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval)                         \
>         (val) ? of_property_read_##type##_array((node), (propname), (val), (nval))      \
>               : of_property_count_elems_of_size((node), (propname), sizeof(type))
> 
> So even if you're using the device_property_* functions you'd still need
> a match in the underlying functions or am I missing something?

Yes, but the underlying function should never be used directly by
drivers. And should probably be prefixed with __ or marked deprecated
(with an override in its one genuine call site).

-- 
dwmw2

[toc] | [prev] | [next] | [standalone]

#1467651 — Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

在 2016/8/20 4:47, David Woodhouse 写道:
> On Fri, 2016-08-19 at 22:41 +0200, Heiko Stuebner wrote:
>>> So no, don't *add* any more of these functions. Only add the generic
>>> version. And if your driver isn't using the generic property
>>> functions... fix it.
>> As far as I can see, all the device_property_* functions are grounded on their
>> of_property_*, acpi_property_* etc counterparts and functions reading specific
>> elements (the _index variants) are currently not available at all.
>>
>> drivers/base/property.c:
>> #define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval)                         \
>>          (val) ? of_property_read_##type##_array((node), (propname), (val), (nval))      \
>>                : of_property_count_elems_of_size((node), (propname), sizeof(type))
>>
>> So even if you're using the device_property_* functions you'd still need
>> a match in the underlying functions or am I missing something?
> Yes, but the underlying function should never be used directly by
> drivers. And should probably be prefixed with __ or marked deprecated
> (with an override in its one genuine call site).
>

So can I add a device_property_read_s32_array function in property.h?

--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -108,6 +108,13 @@ static inline int device_property_read_u32(struct 
device *dev,
         return device_property_read_u32_array(dev, propname, val, 1);
  }

+static inline int device_property_read_s32_array(struct device *dev, 
const char *propname,
+                                  s32 *val, size_t nval)
+{
+       return device_property_read_u32_array(struct device *dev, const 
char *propname,
+                                  (u32 *)val, size_t nval);
+}

-- 
Finley

[toc] | [prev] | [next] | [standalone]

#1467690 — Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

On Fri, Aug 19, 2016 at 09:47:34PM +0100, David Woodhouse wrote:
> On Fri, 2016-08-19 at 22:41 +0200, Heiko Stuebner wrote:
> > 
> > > So no, don't *add* any more of these functions. Only add the generic
> > > version. And if your driver isn't using the generic property
> > > functions... fix it.
> > 
> > As far as I can see, all the device_property_* functions are grounded on their
> > of_property_*, acpi_property_* etc counterparts and functions reading specific
> > elements (the _index variants) are currently not available at all.
> > 
> > drivers/base/property.c:
> > #define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval)                         \
> >         (val) ? of_property_read_##type##_array((node), (propname), (val), (nval))      \
> >               : of_property_count_elems_of_size((node), (propname), sizeof(type))
> > 
> > So even if you're using the device_property_* functions you'd still need
> > a match in the underlying functions or am I missing something?
> 
> Yes, but the underlying function should never be used directly by
> drivers. 

There are properties which never make sense with ACPI, and there are
drivers which shouldn't exist in the case of ACPI (due to clashes with
core ACPI functionality). So using of_property_* for those, and not
matching any ACPI tables is perfectly fine.

IMO, an of_property_read_s32_index() function is perfectly fine (as is
an ACPI-specific variant, or a unified helper, if that's useful to some
driver).

There is no need to force drivers to superficially appear to support
ACPI when they have not been designed with ACPI in mind, and are
unlikely to function in an ACPI environment.

Thanks,
Mark.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web