[v5,10/14] software node: rename is_array to is_inline
diff mbox series

Message ID 20191011230721.206646-11-dmitry.torokhov@gmail.com
State Superseded, archived
Headers show
Series
  • software node: add support for reference properties
Related show

Commit Message

Dmitry Torokhov Oct. 11, 2019, 11:07 p.m. UTC
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(-)

Comments

Andy Shevchenko Oct. 14, 2019, 7:37 a.m. UTC | #1
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...' ?
Dmitry Torokhov Oct. 15, 2019, 6:22 p.m. UTC | #2
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.
Andy Shevchenko Oct. 16, 2019, 7:59 a.m. UTC | #3
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.
Dmitry Torokhov Oct. 16, 2019, 4:54 p.m. UTC | #4
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.
Andy Shevchenko Oct. 17, 2019, 7:16 a.m. UTC | #5
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.
Dmitry Torokhov Oct. 17, 2019, 4 p.m. UTC | #6
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.
Dmitry Torokhov Oct. 17, 2019, 4:02 p.m. UTC | #7
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.

Patch
diff mbox series

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_ } },				\
 }