| Message ID | 20220325113148.588163-4-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | introduce fwnode in the I2C subsystem | expand |
On Fri, Mar 25, 2022 at 12:31:42PM +0100, Clément Léger wrote: > This will allow to read a string array from a specified offset. Support > for this new parameter has been added in both of through the use of > of_property_read_string_array_index() and in swnode though the existing > property_entry_read_string_array() function. ACPI support has not yet > been added and only index == 0 is accepted. ... > static int > acpi_fwnode_property_read_string_array(const struct fwnode_handle *fwnode, > const char *propname, const char **val, > - size_t nval) > + size_t nval, int index) > { > + if (index != 0) > + return -EINVAL; > + > return acpi_node_prop_read(fwnode, propname, DEV_PROP_STRING, > val, nval); I guess it would be nice if some of us (ACPI folks) can provide a proper prerequisite patch. ... > - array_len = min_t(size_t, nval, array_len); > length = array_len * sizeof(*strings); > - Stray change. > pointer = property_entry_find(props, propname, length); > if (IS_ERR(pointer)) > return PTR_ERR(pointer); > + if (index >= array_len) > + return -ENODATA; I was about to ask if we can check this before the property_entry_find() call, but realized that in such case it will shadow possible errors due to wrong or absent property. ... > - of_property_read_string_array(node, propname, val, nval) : > + of_property_read_string_array_index(node, propname, val, nval, > + index) : Dunno about the style there, but I think it can be one line.
Le Fri, 25 Mar 2022 16:30:45 +0200, Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > pointer = property_entry_find(props, propname, length); > > if (IS_ERR(pointer)) > > return PTR_ERR(pointer); > > > + if (index >= array_len) > > + return -ENODATA; > > I was about to ask if we can check this before the > property_entry_find() call, but realized that in such case it will > shadow possible errors due to wrong or absent property. I think you are actually right, the check can be done after property_entry_count_elems_of_size() since it already checks for the property to be present. I'll move that check. > > ... > > > - of_property_read_string_array(node, propname, val, > > nval) : > > + of_property_read_string_array_index(node, > > propname, val, nval, > > + index) : > > Dunno about the style there, but I think it can be one line. Seems like the complete file is strictly applying the 80 columns rules so I thought it was better to keep it like this. However, I think the ternary oeprator is not really readable with such split.
On Mon, Mar 28, 2022 at 4:29 PM Clément Léger <clement.leger@bootlin.com> wrote: > > Le Fri, 25 Mar 2022 16:30:45 +0200, > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > > > pointer = property_entry_find(props, propname, length); > > > if (IS_ERR(pointer)) > > > return PTR_ERR(pointer); > > > > > + if (index >= array_len) > > > + return -ENODATA; > > > > I was about to ask if we can check this before the > > property_entry_find() call, but realized that in such case it will > > shadow possible errors due to wrong or absent property. > > I think you are actually right, the check can be done after > property_entry_count_elems_of_size() since it already checks for the > property to be present. I'll move that check. > > > > > ... > > > > > - of_property_read_string_array(node, propname, val, > > > nval) : > > > + of_property_read_string_array_index(node, > > > propname, val, nval, > > > + index) : > > > > Dunno about the style there, but I think it can be one line. > > Seems like the complete file is strictly applying the 80 columns rules > so I thought it was better to keep it like this. However, I think the > ternary oeprator is not really readable with such split. So FWIW I would entirely change it to if (!val) return of_property_count_strings(node, propname); return of_property_read_string_array_index(node, propname, val, nval, index); which IMO would be way easier to read.
Le Tue, 5 Apr 2022 15:22:51 +0200, "Rafael J. Wysocki" <rafael@kernel.org> a écrit : > On Mon, Mar 28, 2022 at 4:29 PM Clément Léger <clement.leger@bootlin.com> wrote: > > > > Le Fri, 25 Mar 2022 16:30:45 +0200, > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit : > > > > > > pointer = property_entry_find(props, propname, length); > > > > if (IS_ERR(pointer)) > > > > return PTR_ERR(pointer); > > > > > > > + if (index >= array_len) > > > > + return -ENODATA; > > > > > > I was about to ask if we can check this before the > > > property_entry_find() call, but realized that in such case it will > > > shadow possible errors due to wrong or absent property. > > > > I think you are actually right, the check can be done after > > property_entry_count_elems_of_size() since it already checks for the > > property to be present. I'll move that check. > > > > > > > > ... > > > > > > > - of_property_read_string_array(node, propname, val, > > > > nval) : > > > > + of_property_read_string_array_index(node, > > > > propname, val, nval, > > > > + index) : > > > > > > Dunno about the style there, but I think it can be one line. > > > > Seems like the complete file is strictly applying the 80 columns rules > > so I thought it was better to keep it like this. However, I think the > > ternary oeprator is not really readable with such split. > > So FWIW I would entirely change it to > > if (!val) > return of_property_count_strings(node, propname); > > return of_property_read_string_array_index(node, propname, val, > > nval, index); > > which IMO would be way easier to read. Hi Rafael, Agreed, this is way more readable. I'll modify that. Thanks,
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index 12bbfe833609..7847b21c94f7 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -1293,8 +1293,11 @@ acpi_fwnode_property_read_int_array(const struct fwnode_handle *fwnode, static int acpi_fwnode_property_read_string_array(const struct fwnode_handle *fwnode, const char *propname, const char **val, - size_t nval) + size_t nval, int index) { + if (index != 0) + return -EINVAL; + return acpi_node_prop_read(fwnode, propname, DEV_PROP_STRING, val, nval); } diff --git a/drivers/base/property.c b/drivers/base/property.c index e6497f6877ee..d95c949e0a79 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -372,12 +372,12 @@ int fwnode_property_read_string_array(const struct fwnode_handle *fwnode, int ret; ret = fwnode_call_int_op(fwnode, property_read_string_array, propname, - val, nval); + val, nval, 0); if (ret == -EINVAL && !IS_ERR_OR_NULL(fwnode) && !IS_ERR_OR_NULL(fwnode->secondary)) ret = fwnode_call_int_op(fwnode->secondary, property_read_string_array, propname, - val, nval); + val, nval, 0); return ret; } EXPORT_SYMBOL_GPL(fwnode_property_read_string_array); diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 0a482212c7e8..cb80dab041ef 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -184,9 +184,10 @@ static int property_entry_read_int_array(const struct property_entry *props, static int property_entry_read_string_array(const struct property_entry *props, const char *propname, - const char **strings, size_t nval) + const char **strings, size_t nval, + int index) { - const void *pointer; + const char * const *pointer; size_t length; int array_len; @@ -200,13 +201,20 @@ static int property_entry_read_string_array(const struct property_entry *props, if (!strings) return array_len; - array_len = min_t(size_t, nval, array_len); length = array_len * sizeof(*strings); - pointer = property_entry_find(props, propname, length); if (IS_ERR(pointer)) return PTR_ERR(pointer); + if (index >= array_len) + return -ENODATA; + + pointer += index; + array_len -= index; + + array_len = min_t(size_t, nval, array_len); + length = array_len * sizeof(*strings); + memcpy(strings, pointer, length); return array_len; @@ -400,12 +408,13 @@ static int software_node_read_int_array(const struct fwnode_handle *fwnode, static int software_node_read_string_array(const struct fwnode_handle *fwnode, const char *propname, - const char **val, size_t nval) + const char **val, size_t nval, + int index) { struct swnode *swnode = to_swnode(fwnode); return property_entry_read_string_array(swnode->node->properties, - propname, val, nval); + propname, val, nval, index); } static const char * diff --git a/drivers/of/property.c b/drivers/of/property.c index 8e90071de6ed..77e8df4a6267 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -906,12 +906,13 @@ static int of_fwnode_property_read_int_array(const struct fwnode_handle *fwnode, static int of_fwnode_property_read_string_array(const struct fwnode_handle *fwnode, const char *propname, const char **val, - size_t nval) + size_t nval, int index) { const struct device_node *node = to_of_node(fwnode); return val ? - of_property_read_string_array(node, propname, val, nval) : + of_property_read_string_array_index(node, propname, val, nval, + index) : of_property_count_strings(node, propname); } diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h index 3a532ba66f6c..b9e28ba49038 100644 --- a/include/linux/fwnode.h +++ b/include/linux/fwnode.h @@ -91,8 +91,9 @@ struct fwnode_reference_args { * @property_present: Return true if a property is present. * @property_read_int_array: Read an array of integer properties. Return zero on * success, a negative error code otherwise. - * @property_read_string_array: Read an array of string properties. Return zero - * on success, a negative error code otherwise. + * @property_read_string_array: Read an array of string properties starting at + * a specified index. Return zero on success, a + * negative error code otherwise. * @get_name: Return the name of an fwnode. * @get_name_prefix: Get a prefix for a node (for printing purposes). * @get_parent: Return the parent of an fwnode. @@ -122,7 +123,7 @@ struct fwnode_operations { int (*property_read_string_array)(const struct fwnode_handle *fwnode_handle, const char *propname, const char **val, - size_t nval); + size_t nval, int index); const char *(*get_name)(const struct fwnode_handle *fwnode); const char *(*get_name_prefix)(const struct fwnode_handle *fwnode); struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
This will allow to read a string array from a specified offset. Support for this new parameter has been added in both of through the use of of_property_read_string_array_index() and in swnode though the existing property_entry_read_string_array() function. ACPI support has not yet been added and only index == 0 is accepted. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- drivers/acpi/property.c | 5 ++++- drivers/base/property.c | 4 ++-- drivers/base/swnode.c | 21 +++++++++++++++------ drivers/of/property.c | 5 +++-- include/linux/fwnode.h | 7 ++++--- 5 files changed, 28 insertions(+), 14 deletions(-)