Re: [PATCH 1/3] software node: implement reference properties
On Fri, Sep 06, 2019 at 03:27:43PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 05, 2019 at 09:38:07PM -0700, Dmitry Torokhov wrote:
> > It is possible to store references to software nodes in the same fashion as
> > other static properties, so that users do not need to define separate
> > structures:
> >
> > const struct software_node gpio_bank_b_node = {
> > .name = "B",
> > };
>
> Why this can't be __initconst?
It may or it may not. I'll remove __inticonst from below as well to not
confuse things.
>
> > const struct property_entry simone_key_enter_props[] __initconst = {
> > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > PROPERTY_ENTRY_STRING("label", "enter"),
> > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > { }
> > };
>
> So it's basically mimics the concept of phandle, right?
Yes.
>
> > + ref_args = prop->is_array ?
> > + &prop->pointer.ref[index] : &prop->value.ref;
>
> Better to do if with explicit 'if ()' as it's done in the rest of the code.
>
> if (prop->is_array)
> ref_args = ...;
> else
> ref_args = ...;
OK, it will be gone actually.
>
> > - DEV_PROP_MAX,
> > + DEV_PROP_MAX
>
> It seems it wasn't ever used, so, can be dropped completely.
OK.
>
> > @@ -240,6 +255,7 @@ struct property_entry {
> > const u32 *u32_data;
> > const u64 *u64_data;
> > const char * const *str;
> > + const struct software_node_ref_args *ref;
> > } pointer;
> > union {
> > u8 u8_data;
> > @@ -247,6 +263,7 @@ struct property_entry {
> > u32 u32_data;
> > u64 u64_data;
> > const char *str;
> > + struct software_node_ref_args ref;
>
> Hmm... This bumps the size of union a lot for each existing property_entry.
> Is there any other way? Maybe we can keep pointer and allocate memory for it
> when copying?
Right, I think we can always store references as arrays, even when we
only need single entry, thus we do not need to increase the size of the
structure.
I just posted v2 implementing that, please take another look.
Thanks.
--
Dmitry
Re: [PATCH 1/3] software node: implement reference properties
On Fri, Sep 06, 2019 at 03:40:43PM +0300, Andy Shevchenko wrote:
> On Fri, Sep 06, 2019 at 02:17:44PM +0300, Heikki Krogerus wrote:
> > On Thu, Sep 05, 2019 at 09:38:07PM -0700, Dmitry Torokhov wrote:
> > > It is possible to store references to software nodes in the same fashion
> > > as
> > > other static properties, so that users do not need to define separate
> > > structures:
> > >
> > > const struct software_node gpio_bank_b_node = {
> > > .name = "B",
> > > };
> > >
> > > const struct property_entry simone_key_enter_props[] __initconst = {
> > > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > > PROPERTY_ENTRY_STRING("label", "enter"),
> > > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > > { }
> > > };
> > >
> > > Signed-off-by: Dmitry Torokhov
> >
> > This looks really good to me. I'll wait for Andy's comments on the
> > idea, but to me it makes sense.
>
> Idea in general is fine. Though, taking into consideration, for example,
> drivers/mfd/intel-lpss-pci.c, the size of predefined structures bumps a lot.
> I think we always should keep a pointer. In this case we may not add another
> property type.
Yeah, you have a point.
thanks,
--
heikki
Re: [PATCH 1/3] software node: implement reference properties
On Fri, Sep 06, 2019 at 02:17:44PM +0300, Heikki Krogerus wrote:
> On Thu, Sep 05, 2019 at 09:38:07PM -0700, Dmitry Torokhov wrote:
> > It is possible to store references to software nodes in the same fashion as
> > other static properties, so that users do not need to define separate
> > structures:
> >
> > const struct software_node gpio_bank_b_node = {
> > .name = "B",
> > };
> >
> > const struct property_entry simone_key_enter_props[] __initconst = {
> > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > PROPERTY_ENTRY_STRING("label", "enter"),
> > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > { }
> > };
> >
> > Signed-off-by: Dmitry Torokhov
>
> This looks really good to me. I'll wait for Andy's comments on the
> idea, but to me it makes sense.
Idea in general is fine. Though, taking into consideration, for example,
drivers/mfd/intel-lpss-pci.c, the size of predefined structures bumps a lot.
I think we always should keep a pointer. In this case we may not add another
property type.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 1/3] software node: implement reference properties
On Thu, Sep 05, 2019 at 09:38:07PM -0700, Dmitry Torokhov wrote:
> It is possible to store references to software nodes in the same fashion as
> other static properties, so that users do not need to define separate
> structures:
>
> const struct software_node gpio_bank_b_node = {
> .name = "B",
> };
Why this can't be __initconst?
> const struct property_entry simone_key_enter_props[] __initconst = {
> PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> PROPERTY_ENTRY_STRING("label", "enter"),
> PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> { }
> };
So it's basically mimics the concept of phandle, right?
> + ref_args = prop->is_array ?
> + &prop->pointer.ref[index] : &prop->value.ref;
Better to do if with explicit 'if ()' as it's done in the rest of the code.
if (prop->is_array)
ref_args = ...;
else
ref_args = ...;
> - DEV_PROP_MAX,
> + DEV_PROP_MAX
It seems it wasn't ever used, so, can be dropped completely.
> @@ -240,6 +255,7 @@ struct property_entry {
> const u32 *u32_data;
> const u64 *u64_data;
> const char * const *str;
> + const struct software_node_ref_args *ref;
> } pointer;
> union {
> u8 u8_data;
> @@ -247,6 +263,7 @@ struct property_entry {
> u32 u32_data;
> u64 u64_data;
> const char *str;
> + struct software_node_ref_args ref;
Hmm... This bumps the size of union a lot for each existing property_entry.
Is there any other way? Maybe we can keep pointer and allocate memory for it
when copying?
> } value;
> +#define PROPERTY_ENTRY_REF_ARRAY(_name_, _val_) \
> +(struct property_entry) {\
> + .name = _name_, \
> + .length = ARRAY_SIZE(_val_) * \
> + sizeof(struct software_node_ref_args), \
I would rather leave it on one line and shift right all \:s in this macro.
> + .is_array = true, \
> + .type = DEV_PROP_REF, \
> + .pointer.ref = _val_, \
> +}
> +
> +#define PROPERTY_ENTRY_REF(_name_, _ref_, ...) \
> +(struct property_entry) {\
> + .name = _name_, \
> + .length = sizeof(struct software_node_ref_args),\
> + .type = DEV_PROP_REF, \
> + .value.ref.node = _ref_,\
> + .value.ref.nargs = \
> + ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1, \
Ditto.
> + .value.ref.args = { __VA_ARGS__ }, \
> +}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 1/3] software node: implement reference properties
On Thu, Sep 05, 2019 at 09:38:07PM -0700, Dmitry Torokhov wrote:
> It is possible to store references to software nodes in the same fashion as
> other static properties, so that users do not need to define separate
> structures:
>
> const struct software_node gpio_bank_b_node = {
> .name = "B",
> };
>
> const struct property_entry simone_key_enter_props[] __initconst = {
> PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> PROPERTY_ENTRY_STRING("label", "enter"),
> PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> { }
> };
>
> Signed-off-by: Dmitry Torokhov
This looks really good to me. I'll wait for Andy's comments on the
idea, but to me it makes sense.
Thanks Dmitry!
--
heikki
