| Message ID | 1471315139-28285-3-git-send-email-finley.xiao@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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?
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).
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.
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) {