| Message ID | 20191011230721.206646-11-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | software node: add support for reference properties | expand |
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' > + * &struct property_entry instance. > + * @pointer: Pointer to the property when it is stored separately from > + * the &struct property_entry instance. 'separately from' -> 'outside' ? > + * @value: Value of the property when it is stored inline. 'stored inline' -> 'embedded in the &struct...' ?
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". > > > + * &struct property_entry instance. > > > + * @pointer: Pointer to the property when it is stored separately from > > + * the &struct 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 &struct...' ? I was trying to have a link "stored inline" -> "is_inline". Do we want to change the flag to be "is_embedded"? Thanks.
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". > > > > > > + * &struct property_entry instance. > > > > > + * @pointer: Pointer to the property when it is stored separately from > > > + * the &struct 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 &struct...' ? > > 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.
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". > > > > > > > > > + * &struct property_entry instance. > > > > > > > + * @pointer: Pointer to the property when it is stored separately from > > > > + * the &struct 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 &struct...' ? > > > > 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.
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 &struct...' ? > > > > > > 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.
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 &struct...' ? > > > > > > > > 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.
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 <dmitry.torokhov@gmail.com> > --- > 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 &prop->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 > + * &struct 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 &struct 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.
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 &prop->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 + * &struct 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 &struct 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_ } }, \ }
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 <dmitry.torokhov@gmail.com> --- drivers/base/swnode.c | 9 +++++---- include/linux/property.h | 12 +++++++----- 2 files changed, 12 insertions(+), 9 deletions(-)