Re: [PATCH v5 10/14] software node: rename is_array to is_inline
On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> We do not need a special flag to know if we are dealing with an array,
> as we can get that data from ratio between element length and the data
> size, however we do need a flag to know whether the data is stored
> directly inside property_entry or separately.
>
> Signed-off-by: Dmitry Torokhov
> ---
> drivers/base/swnode.c| 9 +
> include/linux/property.h | 12 +++-
> 2 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 8826c9040c80..ae4b24ee2a54 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -108,7 +108,7 @@ static const void *property_get_pointer(const struct
> property_entry *prop)
> if (!prop->length)
> return NULL;
>
> - if (prop->is_array)
> + if (!prop->is_inline)
> return prop->pointer;
>
> return >value;
> @@ -205,7 +205,7 @@ static void property_entry_free_data(const struct
> property_entry *p)
> const char * const *src_str;
> size_t i, nval;
>
> - if (p->is_array) {
> + if (!p->is_inline) {
> if (p->type == DEV_PROP_STRING && p->pointer) {
> src_str = p->pointer;
> nval = p->length / sizeof(const char *);
> @@ -250,7 +250,7 @@ static int property_entry_copy_data(struct property_entry
> *dst,
> const void *pointer = property_get_pointer(src);
> const void *new;
>
> - if (src->is_array) {
> + if (!src->is_inline) {
> if (!src->length)
> return -ENODATA;
>
> @@ -264,15 +264,16 @@ static int property_entry_copy_data(struct
> property_entry *dst,
> return -ENOMEM;
> }
>
> - dst->is_array = true;
> dst->pointer = new;
> } else if (src->type == DEV_PROP_STRING) {
> new = kstrdup(src->value.str, GFP_KERNEL);
> if (!new && src->value.str)
> return -ENOMEM;
>
> + dst->is_inline = true;
> dst->value.str = new;
> } else {
> + dst->is_inline = true;
> dst->value = src->value;
> }
>
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 238e1507925f..ac7823d58cfe 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -222,15 +222,17 @@ static inline int fwnode_property_count_u64(const
> struct fwnode_handle *fwnode,
> * struct property_entry - "Built-in" device property representation.
> * @name: Name of the property.
> * @length: Length of data making up the value.
> - * @is_array: True when the property is an array.
> + * @is_inline: True when the property value is stored directly in
> + * property_entry instance.
> * @type: Type of the data in unions.
> - * @pointer: Pointer to the property (an array of items of the given type).
> - * @value: Value of the property (when it is a single item of the given
> type).
> + * @pointer: Pointer to the property when it is stored separately from
> + * the property_entry instance.
> + * @value: Value of the property when it is stored inline.
> */
> struct property_entry {
> const char *name;
> size_t length;
> - bool is_array;
> + bool is_inline;
> enum dev_prop_type type;
> union {
> const void *pointer;
> @@ -257,7 +259,6 @@ struct property_entry {
> (struct property_entry) {\
> .name = _name_, \
> .length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_), \
> - .is_array = true, \
> .type = DEV_PROP_##_Type_, \
> { .pointer = _val_ }, \
> }
> @@ -288,6 +289,7 @@ struct property_entry {
> (struct property_entry) {\
> .name = _name_, \
> .length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),\
> + .is_inline = true, \
> .type = DEV_PROP_##_Type_, \
> { .value = { ._elem_ = _val_ } }, \
> }
The patch also needs to set is_inline in PROPERTY_ENTRY_BOOL(), I'll
send updated version after I get more feedback.
Thanks.
--
Dmitry
Re: [PATCH v5 10/14] software node: rename is_array to is_inline
On Thu, Oct 17, 2019 at 10:16:28AM +0300, Andy Shevchenko wrote: > On Wed, Oct 16, 2019 at 09:54:30AM -0700, Dmitry Torokhov wrote: > > On Wed, Oct 16, 2019 at 10:59:40AM +0300, Andy Shevchenko wrote: > > > On Tue, Oct 15, 2019 at 11:22:06AM -0700, Dmitry Torokhov wrote: > > > > On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote: > > > > > On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote: > > > > > > 'stored inline' -> 'embedded in the ' ? > > > > > > > > I was trying to have a link "stored inline" -> "is_inline". > > > > > > > > Do we want to change the flag to be "is_embedded"? > > > > > > In dictionaries I have > > > > > > embedded <-> unilateral > > > > Are you trying to show synonym or antonym here? But I am pretty sure > > "unilateral" is either. > > Antonyms. The 'unilateral' is marked as so in the dictionary. OK, that is not something that I would ever think of as an antonym, so even though I am not a native speaker I do not think we should be using it here as documentation and comments are supposed to be understood by all people from around the world and not by English majors only. Out of curiosity, is this dictionary available online? I would really want to see to what particular meaning of "embedded" they assign "unilateral" as antonym so that I know better next time I see it used. > > > Antonyms for our use of "embedded" are likely "detached" or > > "disconnected". > > > > > inline <-> ??? > > > > "out of line" but I still believe "stored separately" explains precisely > > what we have here. > > No, 'out of line' is idiom with a special meaning. Yes, and it is also a well defined term in CS. Thanks. -- Dmitry
Re: [PATCH v5 10/14] software node: rename is_array to is_inline
On Wed, Oct 16, 2019 at 09:54:30AM -0700, Dmitry Torokhov wrote: > On Wed, Oct 16, 2019 at 10:59:40AM +0300, Andy Shevchenko wrote: > > On Tue, Oct 15, 2019 at 11:22:06AM -0700, Dmitry Torokhov wrote: > > > On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote: > > > > On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote: > > > > 'stored inline' -> 'embedded in the ' ? > > > > > > I was trying to have a link "stored inline" -> "is_inline". > > > > > > Do we want to change the flag to be "is_embedded"? > > > > In dictionaries I have > > > > embedded <-> unilateral > > Are you trying to show synonym or antonym here? But I am pretty sure > "unilateral" is either. Antonyms. The 'unilateral' is marked as so in the dictionary. > Antonyms for our use of "embedded" are likely "detached" or > "disconnected". > > > inline <-> ??? > > "out of line" but I still believe "stored separately" explains precisely > what we have here. No, 'out of line' is idiom with a special meaning. -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 10/14] software node: rename is_array to is_inline
On Wed, Oct 16, 2019 at 10:59:40AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 15, 2019 at 11:22:06AM -0700, Dmitry Torokhov wrote:
> > On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> > > > We do not need a special flag to know if we are dealing with an array,
> > > > as we can get that data from ratio between element length and the data
> > > > size, however we do need a flag to know whether the data is stored
> > > > directly inside property_entry or separately.
> > >
> > > > - if (prop->is_array)
> > > > + if (!prop->is_inline)
> > >
> > > > - if (p->is_array) {
> > > > + if (!p->is_inline) {
> > >
> > > > - if (src->is_array) {
> > > > + if (!src->is_inline) {
> > >
> > > May we have positive conditionals instead?
> >
> > I was trying to limit the context churn. I can definitely change
> > property_get_pointer(), but the other 2 I think are better in the
> > current form.
> >
> > >
> > > > + * @is_inline: True when the property value is stored directly in
> > >
> > > I think word 'directly' is superfluous here.
> > > Or, perhaps, 'stored directly' -> 'embedded'
> >
> > I'm OK with "embedded".
> >
> > >
> > > > + * property_entry instance.
> > >
> > > > + * @pointer: Pointer to the property when it is stored separately from
> > > > + * the property_entry instance.
> > >
> > > 'separately from' -> 'outside' ?
> >
> > Umm, I think I prefer "separately" actually.
> >
> > >
> > > > + * @value: Value of the property when it is stored inline.
> > >
> > > 'stored inline' -> 'embedded in the ' ?
> >
> > I was trying to have a link "stored inline" -> "is_inline".
> >
> > Do we want to change the flag to be "is_embedded"?
>
> In dictionaries I have
>
> embedded <-> unilateral
Are you trying to show synonym or antonym here? But I am pretty sure
"unilateral" is either.
Antonyms for our use of "embedded" are likely "detached" or
"disconnected".
> inline <-> ???
"out of line" but I still believe "stored separately" explains precisely
what we have here.
Thanks.
--
Dmitry
Re: [PATCH v5 10/14] software node: rename is_array to is_inline
On Tue, Oct 15, 2019 at 11:22:06AM -0700, Dmitry Torokhov wrote:
> On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> > > We do not need a special flag to know if we are dealing with an array,
> > > as we can get that data from ratio between element length and the data
> > > size, however we do need a flag to know whether the data is stored
> > > directly inside property_entry or separately.
> >
> > > - if (prop->is_array)
> > > + if (!prop->is_inline)
> >
> > > - if (p->is_array) {
> > > + if (!p->is_inline) {
> >
> > > - if (src->is_array) {
> > > + if (!src->is_inline) {
> >
> > May we have positive conditionals instead?
>
> I was trying to limit the context churn. I can definitely change
> property_get_pointer(), but the other 2 I think are better in the
> current form.
>
> >
> > > + * @is_inline: True when the property value is stored directly in
> >
> > I think word 'directly' is superfluous here.
> > Or, perhaps, 'stored directly' -> 'embedded'
>
> I'm OK with "embedded".
>
> >
> > > + * property_entry instance.
> >
> > > + * @pointer: Pointer to the property when it is stored separately from
> > > + * the property_entry instance.
> >
> > 'separately from' -> 'outside' ?
>
> Umm, I think I prefer "separately" actually.
>
> >
> > > + * @value: Value of the property when it is stored inline.
> >
> > 'stored inline' -> 'embedded in the ' ?
>
> I was trying to have a link "stored inline" -> "is_inline".
>
> Do we want to change the flag to be "is_embedded"?
In dictionaries I have
embedded <-> unilateral
inline <-> ???
Perhaps native speaker can jump in and help here.
My point is to be consistent in the terms we are using.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v5 10/14] software node: rename is_array to is_inline
On Mon, Oct 14, 2019 at 10:37:20AM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> > We do not need a special flag to know if we are dealing with an array,
> > as we can get that data from ratio between element length and the data
> > size, however we do need a flag to know whether the data is stored
> > directly inside property_entry or separately.
>
> > - if (prop->is_array)
> > + if (!prop->is_inline)
>
> > - if (p->is_array) {
> > + if (!p->is_inline) {
>
> > - if (src->is_array) {
> > + if (!src->is_inline) {
>
> May we have positive conditionals instead?
I was trying to limit the context churn. I can definitely change
property_get_pointer(), but the other 2 I think are better in the
current form.
>
> > + * @is_inline: True when the property value is stored directly in
>
> I think word 'directly' is superfluous here.
> Or, perhaps, 'stored directly' -> 'embedded'
I'm OK with "embedded".
>
> > + * property_entry instance.
>
> > + * @pointer: Pointer to the property when it is stored separately from
> > + * the property_entry instance.
>
> 'separately from' -> 'outside' ?
Umm, I think I prefer "separately" actually.
>
> > + * @value: Value of the property when it is stored inline.
>
> 'stored inline' -> 'embedded in the ' ?
I was trying to have a link "stored inline" -> "is_inline".
Do we want to change the flag to be "is_embedded"?
Thanks.
--
Dmitry
Re: [PATCH v5 10/14] software node: rename is_array to is_inline
On Fri, Oct 11, 2019 at 04:07:17PM -0700, Dmitry Torokhov wrote:
> We do not need a special flag to know if we are dealing with an array,
> as we can get that data from ratio between element length and the data
> size, however we do need a flag to know whether the data is stored
> directly inside property_entry or separately.
> - if (prop->is_array)
> + if (!prop->is_inline)
> - if (p->is_array) {
> + if (!p->is_inline) {
> - if (src->is_array) {
> + if (!src->is_inline) {
May we have positive conditionals instead?
> + * @is_inline: True when the property value is stored directly in
I think word 'directly' is superfluous here.
Or, perhaps, 'stored directly' -> 'embedded'
> + * property_entry instance.
> + * @pointer: Pointer to the property when it is stored separately from
> + * the property_entry instance.
'separately from' -> 'outside' ?
> + * @value: Value of the property when it is stored inline.
'stored inline' -> 'embedded in the ' ?
--
With Best Regards,
Andy Shevchenko
[PATCH v5 10/14] software node: rename is_array to is_inline
We do not need a special flag to know if we are dealing with an array,
as we can get that data from ratio between element length and the data
size, however we do need a flag to know whether the data is stored
directly inside property_entry or separately.
Signed-off-by: Dmitry Torokhov
---
drivers/base/swnode.c| 9 +
include/linux/property.h | 12 +++-
2 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 8826c9040c80..ae4b24ee2a54 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -108,7 +108,7 @@ static const void *property_get_pointer(const struct
property_entry *prop)
if (!prop->length)
return NULL;
- if (prop->is_array)
+ if (!prop->is_inline)
return prop->pointer;
return >value;
@@ -205,7 +205,7 @@ static void property_entry_free_data(const struct
property_entry *p)
const char * const *src_str;
size_t i, nval;
- if (p->is_array) {
+ if (!p->is_inline) {
if (p->type == DEV_PROP_STRING && p->pointer) {
src_str = p->pointer;
nval = p->length / sizeof(const char *);
@@ -250,7 +250,7 @@ static int property_entry_copy_data(struct property_entry
*dst,
const void *pointer = property_get_pointer(src);
const void *new;
- if (src->is_array) {
+ if (!src->is_inline) {
if (!src->length)
return -ENODATA;
@@ -264,15 +264,16 @@ static int property_entry_copy_data(struct property_entry
*dst,
return -ENOMEM;
}
- dst->is_array = true;
dst->pointer = new;
} else if (src->type == DEV_PROP_STRING) {
new = kstrdup(src->value.str, GFP_KERNEL);
if (!new && src->value.str)
return -ENOMEM;
+ dst->is_inline = true;
dst->value.str = new;
} else {
+ dst->is_inline = true;
dst->value = src->value;
}
diff --git a/include/linux/property.h b/include/linux/property.h
index 238e1507925f..ac7823d58cfe 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -222,15 +222,17 @@ static inline int fwnode_property_count_u64(const struct
fwnode_handle *fwnode,
* struct property_entry - "Built-in" device property representation.
* @name: Name of the property.
* @length: Length of data making up the value.
- * @is_array: True when the property is an array.
+ * @is_inline: True when the property value is stored directly in
+ * property_entry instance.
* @type: Type of the data in unions.
- * @pointer: Pointer to the property (an array of items of the given type).
- * @value: Value of the property (when it is a single item of the given type).
+ * @pointer: Pointer to the property when it is stored separately from
+ * the property_entry instance.
+ * @value: Value of the property when it is stored inline.
*/
struct property_entry {
const char *name;
size_t length;
- bool is_array;
+ bool is_inline;
enum dev_prop_type type;
union {
const void *pointer;
@@ -257,7 +259,6 @@ struct property_entry {
(struct property_entry) { \
.name = _name_, \
.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_), \
- .is_array = true, \
.type = DEV_PROP_##_Type_, \
{ .pointer = _val_ }, \
}
@@ -288,6 +289,7 @@ struct property_entry {
(struct property_entry) { \
.name = _name_, \
.length = __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),\
+ .is_inline = true, \
.type = DEV_PROP_##_Type_, \
{ .value = { ._elem_ = _val_ } }, \
}
--
2.23.0.700.g56cf767bdb-goog
