diff mbox series

[v8,3/6] software node: implement reference properties

Message ID 20191108042225.45391-4-dmitry.torokhov@gmail.com (mailing list archive)
State Mainlined, archived
Headers show
Series software node: add support for reference properties | expand

Commit Message

Dmitry Torokhov Nov. 8, 2019, 4:22 a.m. UTC
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:

static const struct software_node gpio_bank_b_node = {
	.name = "B",
};

static const struct property_entry simone_key_enter_props[] = {
	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 <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c    | 49 ++++++++++++++++++++++++++++------
 include/linux/property.h | 57 +++++++++++++++++++++++++++++-----------
 2 files changed, 82 insertions(+), 24 deletions(-)

Comments

Łukasz Stelmach Nov. 9, 2020, 5:02 p.m. UTC | #1
It was <2019-11-07 czw 20:22>, when 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:
>
> static const struct software_node gpio_bank_b_node = {
> 	.name = "B",
> };
>
> static const struct property_entry simone_key_enter_props[] = {
> 	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 <dmitry.torokhov@gmail.com>
> ---

I am writing a piece that needs to provide a list of gpios to a
diriver. The above example looks like what I need.

At the moment the driver gets the list from fwnode/of_node. The list
contain references to phandles which get resolved and and the driver
ends up with a bunch of gpio descriptors. Great.

This example looks nice but does the code that reads the reference from
the gpios property and returns a gpiod actually exist? If it doesn't, I
am willing to write it.

At first glance it makes more sense to me to pass (struct gpiod_lookup
*) instead of (struct software_node *) and make gpiolib's gpiod_find()
accept lookup tables as parameter instead of searching the
gpio_lookup_list? Or do you think such temporary table should be
assembled from the above structure and then used in gpiod_find()?

Any other suggestions on how to get a bunch of gpios (the description
for gpios is available in the devicetree) for a device described with a
software nodes?

Kind regards,
Andy Shevchenko Nov. 9, 2020, 5:24 p.m. UTC | #2
On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
> It was <2019-11-07 czw 20:22>, when 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:
> >
> > static const struct software_node gpio_bank_b_node = {
> > 	.name = "B",
> > };
> >
> > static const struct property_entry simone_key_enter_props[] = {
> > 	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 <dmitry.torokhov@gmail.com>
> > ---
> 
> I am writing a piece that needs to provide a list of gpios to a
> diriver. The above example looks like what I need.

Nope.

It mustn't be used for GPIOs or PWMs or whatever that either should come via
lookup tables or corresponding firmware interface.

> At the moment the driver gets the list from fwnode/of_node. The list
> contain references to phandles which get resolved and and the driver
> ends up with a bunch of gpio descriptors. Great.
> 
> This example looks nice but does the code that reads the reference from
> the gpios property and returns a gpiod actually exist? If it doesn't, I
> am willing to write it.
> 
> At first glance it makes more sense to me to pass (struct gpiod_lookup
> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
> accept lookup tables as parameter instead of searching the
> gpio_lookup_list? Or do you think such temporary table should be
> assembled from the above structure and then used in gpiod_find()?
> 
> Any other suggestions on how to get a bunch of gpios (the description
> for gpios is available in the devicetree) for a device described with a
> software nodes?
Łukasz Stelmach Nov. 9, 2020, 6:18 p.m. UTC | #3
It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
>> It was <2019-11-07 czw 20:22>, when 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:
>> >
>> > static const struct software_node gpio_bank_b_node = {
>> > 	.name = "B",
>> > };
>> >
>> > static const struct property_entry simone_key_enter_props[] = {
>> > 	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 <dmitry.torokhov@gmail.com>
>> > ---
>> 
>> I am writing a piece that needs to provide a list of gpios to a
>> diriver. The above example looks like what I need.
>
> Nope.
>
> It mustn't be used for GPIOs or PWMs or whatever that either should come via
> lookup tables or corresponding firmware interface.
>

May I ask why? I've read commit descriptions for drivers/base/swnode.c
and the discussion on lkml and I understand software nodes as a way to
provide (synthesize) a description for a device that is missing a
description in the firmware. Another use case seems to be to replace (in
the long run) platform data. That is what I am trying to use it for.

I want my device to be configured with either DT or software_nodes
created at run time with configfs. My device is going to use GPIOs
described in the DT and it is going to be configured via configfs at run
time. I could use platform_data to pass structures from configfs but
software nodes would let me save some code in the device driver and use
the same paths for both static (DT) and dynamic (configfs)
configuration.

Probably I have missed something and I will be greatful, if you tell me
where I can find more information about software nodes. There are few
users in the kernel and it isn't obvious for me how to use software
nodes properly.

>> At the moment the driver gets the list from fwnode/of_node. The list
>> contain references to phandles which get resolved and and the driver
>> ends up with a bunch of gpio descriptors. Great.
>> 
>> This example looks nice but does the code that reads the reference from
>> the gpios property and returns a gpiod actually exist? If it doesn't, I
>> am willing to write it.
>> 
>> At first glance it makes more sense to me to pass (struct gpiod_lookup
>> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
>> accept lookup tables as parameter instead of searching the
>> gpio_lookup_list? Or do you think such temporary table should be
>> assembled from the above structure and then used in gpiod_find()?
>> 
>> Any other suggestions on how to get a bunch of gpios (the description
>> for gpios is available in the devicetree) for a device described with a
>> software nodes?

Kind regards,
Dmitry Torokhov Nov. 9, 2020, 6:53 p.m. UTC | #4
On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
> >> It was <2019-11-07 czw 20:22>, when 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:
> >> >
> >> > static const struct software_node gpio_bank_b_node = {
> >> > 	.name = "B",
> >> > };
> >> >
> >> > static const struct property_entry simone_key_enter_props[] = {
> >> > 	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 <dmitry.torokhov@gmail.com>
> >> > ---
> >> 
> >> I am writing a piece that needs to provide a list of gpios to a
> >> diriver. The above example looks like what I need.
> >
> > Nope.
> >
> > It mustn't be used for GPIOs or PWMs or whatever that either should come via
> > lookup tables or corresponding firmware interface.
> >
> 
> May I ask why? I've read commit descriptions for drivers/base/swnode.c
> and the discussion on lkml and I understand software nodes as a way to
> provide (synthesize) a description for a device that is missing a
> description in the firmware. Another use case seems to be to replace (in
> the long run) platform data. That is what I am trying to use it for.
> 
> I want my device to be configured with either DT or software_nodes
> created at run time with configfs. My device is going to use GPIOs
> described in the DT and it is going to be configured via configfs at run
> time. I could use platform_data to pass structures from configfs but
> software nodes would let me save some code in the device driver and use
> the same paths for both static (DT) and dynamic (configfs)
> configuration.
> 
> Probably I have missed something and I will be greatful, if you tell me
> where I can find more information about software nodes. There are few
> users in the kernel and it isn't obvious for me how to use software
> nodes properly.

Yeah, I disagree with Andy here. The lookup tables are a crutch that we
have until GPIO and PWM a taught to support software nodes (I need to
resurrect my patch series for GPIO, if you have time to test that would
be awesome).

Thanks.
Andy Shevchenko Nov. 9, 2020, 7:02 p.m. UTC | #5
On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
> >> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:

...

> >> I am writing a piece that needs to provide a list of gpios to a
> >> diriver. The above example looks like what I need.
> >
> > Nope.
> >
> > It mustn't be used for GPIOs or PWMs or whatever that either should come via
> > lookup tables or corresponding firmware interface.
> 
> May I ask why? I've read commit descriptions for drivers/base/swnode.c
> and the discussion on lkml and I understand software nodes as a way to
> provide (synthesize) a description for a device that is missing a
> description in the firmware. Another use case seems to be to replace (in
> the long run) platform data. That is what I am trying to use it for.

Yes. Both are correct. They are simply not applicable for everything
(it's not a silver bullet).

> I want my device to be configured with either DT or software_nodes
> created at run time with configfs.

Okay.

> My device is going to use GPIOs
> described in the DT and it is going to be configured via configfs at run
> time.

How is this related to swnodes?
Create GPIO lookup table.

> I could use platform_data to pass structures from configfs but
> software nodes would let me save some code in the device driver and use
> the same paths for both static (DT) and dynamic (configfs)
> configuration.
> 
> Probably I have missed something and I will be greatful, if you tell me
> where I can find more information about software nodes. There are few
> users in the kernel and it isn't obvious for me how to use software
> nodes properly.

gpiod_add_lookup_table().

> >> At the moment the driver gets the list from fwnode/of_node. The list
> >> contain references to phandles which get resolved and and the driver
> >> ends up with a bunch of gpio descriptors. Great.
> >> 
> >> This example looks nice but does the code that reads the reference from
> >> the gpios property and returns a gpiod actually exist? If it doesn't, I
> >> am willing to write it.
> >> 
> >> At first glance it makes more sense to me to pass (struct gpiod_lookup
> >> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
> >> accept lookup tables as parameter instead of searching the
> >> gpio_lookup_list? Or do you think such temporary table should be
> >> assembled from the above structure and then used in gpiod_find()?
> >> 
> >> Any other suggestions on how to get a bunch of gpios (the description
> >> for gpios is available in the devicetree) for a device described with a
> >> software nodes?
Andy Shevchenko Nov. 9, 2020, 7:05 p.m. UTC | #6
On Mon, Nov 09, 2020 at 10:53:05AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> > It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:

...

> > Probably I have missed something and I will be greatful, if you tell me
> > where I can find more information about software nodes. There are few
> > users in the kernel and it isn't obvious for me how to use software
> > nodes properly.
> 
> Yeah, I disagree with Andy here. The lookup tables are a crutch that we
> have until GPIO and PWM a taught to support software nodes (I need to
> resurrect my patch series for GPIO, if you have time to test that would
> be awesome).

We don't have support for list of fwnodes, this will probably break things
where swnode is already exist.

I think Heikki may send a documentation patch to clarify when swnodes can and
can't be used. Maybe I'm mistaken and above is a good use case by design.
Łukasz Stelmach Nov. 9, 2020, 7:47 p.m. UTC | #7
It was <2020-11-09 pon 21:02>, when Andy Shevchenko wrote:
> On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
>> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
>> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
>> >> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:
>
> ...
>
>> >> I am writing a piece that needs to provide a list of gpios to a
>> >> diriver. The above example looks like what I need.
>> >
>> > Nope.
>> >
>> > It mustn't be used for GPIOs or PWMs or whatever that either should come via
>> > lookup tables or corresponding firmware interface.
>> 
>> May I ask why? I've read commit descriptions for drivers/base/swnode.c
>> and the discussion on lkml and I understand software nodes as a way to
>> provide (synthesize) a description for a device that is missing a
>> description in the firmware. Another use case seems to be to replace (in
>> the long run) platform data. That is what I am trying to use it for.
>
> Yes. Both are correct. They are simply not applicable for everything
> (it's not a silver bullet).
>
>> I want my device to be configured with either DT or software_nodes
>> created at run time with configfs.
>
> Okay.
>
>> My device is going to use GPIOs
>> described in the DT and it is going to be configured via configfs at run
>> time.
>
> How is this related to swnodes?

I thought I should mention it, because as far as I can tel mixing and
merging information from different fwnode backends seems to be tricky if
supported at all.

> Create GPIO lookup table.
>
>> I could use platform_data to pass structures from configfs but
>> software nodes would let me save some code in the device driver and use
>> the same paths for both static (DT) and dynamic (configfs)
>> configuration.
>> 
>> Probably I have missed something and I will be greatful, if you tell me
>> where I can find more information about software nodes. There are few
>> users in the kernel and it isn't obvious for me how to use software
>> nodes properly.
>
> gpiod_add_lookup_table().
>

Yes, that is exactly what my POC code does now. But having a lookup
table together with the rest of the device structures has several
advantages.

1) The device may be hotpluggable and there is no
   gpiod_remove_lookup_table().

2) Having the lookup table allocated and managed together with the rest
   of the device seems like a better way to go than on gpio_lookup_list.

3) As of now I've got a minor issue with device naming. I need to set
   dev_id of the table before the device is ready and only after it is
   ready, its name is set (in the hotpluggable use case).

4) Because no other devices would use this lookup table "publishing" it
   rather than keeping together with the device seems at least slightly
   odd.

When the lookup table is attached to the devices and can be passed
around  the final lookup can be done with a function like

static struct gpio_desc *gpiod_find_from_table(struct device *dev,
                             const char *con_id, unsigned int idx,
                 unsigned long *flags, struct gpiod_lookup *table)

>>>> At the moment the driver gets the list from fwnode/of_node. The list
>>>> contain references to phandles which get resolved and and the driver
>>>> ends up with a bunch of gpio descriptors. Great.
>>>> 
>>>> This example looks nice but does the code that reads the reference from
>>>> the gpios property and returns a gpiod actually exist? If it doesn't, I
>>>> am willing to write it.
>>>> 
>>>> At first glance it makes more sense to me to pass (struct gpiod_lookup
>>>> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
>>>> accept lookup tables as parameter instead of searching the
>>>> gpio_lookup_list? Or do you think such temporary table should be
>>>> assembled from the above structure and then used in gpiod_find()?
>>>> 
>>>> Any other suggestions on how to get a bunch of gpios (the description
>>>> for gpios is available in the devicetree) for a device described with a
>>>> software nodes?
Łukasz Stelmach Nov. 9, 2020, 7:54 p.m. UTC | #8
It was <2020-11-09 pon 10:53>, when Dmitry Torokhov wrote:
> On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
>> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
>> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
>> >> It was <2019-11-07 czw 20:22>, when 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:
>> >> >
>> >> > static const struct software_node gpio_bank_b_node = {
>> >> > 	.name = "B",
>> >> > };
>> >> >
>> >> > static const struct property_entry simone_key_enter_props[] = {
>> >> > 	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 <dmitry.torokhov@gmail.com>
>> >> > ---
>> >> 
>> >> I am writing a piece that needs to provide a list of gpios to a
>> >> diriver. The above example looks like what I need.
>> >
>> > Nope.
>> >
>> > It mustn't be used for GPIOs or PWMs or whatever that either should come via
>> > lookup tables or corresponding firmware interface.
>> >
>> 
>> May I ask why? I've read commit descriptions for drivers/base/swnode.c
>> and the discussion on lkml and I understand software nodes as a way to
>> provide (synthesize) a description for a device that is missing a
>> description in the firmware. Another use case seems to be to replace (in
>> the long run) platform data. That is what I am trying to use it for.
>> 
>> I want my device to be configured with either DT or software_nodes
>> created at run time with configfs. My device is going to use GPIOs
>> described in the DT and it is going to be configured via configfs at run
>> time. I could use platform_data to pass structures from configfs but
>> software nodes would let me save some code in the device driver and use
>> the same paths for both static (DT) and dynamic (configfs)
>> configuration.
>> 
>> Probably I have missed something and I will be greatful, if you tell me
>> where I can find more information about software nodes. There are few
>> users in the kernel and it isn't obvious for me how to use software
>> nodes properly.
>
> Yeah, I disagree with Andy here. The lookup tables are a crutch that we
> have until GPIO and PWM a taught to support software nodes (I need to
> resurrect my patch series for GPIO, if you have time to test that would
> be awesome).

It will be great to help. I am not sure if I have means to test PWMs,
but I can do GPIOs.
Andy Shevchenko Nov. 9, 2020, 9:20 p.m. UTC | #9
On Mon, Nov 09, 2020 at 08:47:14PM +0100, Lukasz Stelmach wrote:
> It was <2020-11-09 pon 21:02>, when Andy Shevchenko wrote:
> > On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> >> It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> >> > On Mon, Nov 09, 2020 at 06:02:29PM +0100, Lukasz Stelmach wrote:
> >> >> It was <2019-11-07 czw 20:22>, when Dmitry Torokhov wrote:

...

> > Create GPIO lookup table.
> >
> >> I could use platform_data to pass structures from configfs but
> >> software nodes would let me save some code in the device driver and use
> >> the same paths for both static (DT) and dynamic (configfs)
> >> configuration.
> >> 
> >> Probably I have missed something and I will be greatful, if you tell me
> >> where I can find more information about software nodes. There are few
> >> users in the kernel and it isn't obvious for me how to use software
> >> nodes properly.
> >
> > gpiod_add_lookup_table().
> >
> 
> Yes, that is exactly what my POC code does now. But having a lookup
> table together with the rest of the device structures has several
> advantages.
> 
> 1) The device may be hotpluggable and there is no
>    gpiod_remove_lookup_table().

	% git grep -n -w gpiod_remove_lookup_table

Or I did get it wrong? Did you mean that the removal is not being called?

> 2) Having the lookup table allocated and managed together with the rest
>    of the device seems like a better way to go than on gpio_lookup_list.

Nice, what are you going to do with the rest of lookup tables
(PWM, regulators, etc)? If you convert, convert them all at least.

> 3) As of now I've got a minor issue with device naming. I need to set
>    dev_id of the table before the device is ready and only after it is
>    ready, its name is set (in the hotpluggable use case).

Hotpluggable devices are very much supported by ACPI assistance. DT I have
heard has overlays. What's the issue?

> 4) Because no other devices would use this lookup table "publishing" it
>    rather than keeping together with the device seems at least slightly
>    odd.
> 
> When the lookup table is attached to the devices and can be passed
> around  the final lookup can be done with a function like
> 
> static struct gpio_desc *gpiod_find_from_table(struct device *dev,
>                              const char *con_id, unsigned int idx,
>                  unsigned long *flags, struct gpiod_lookup *table)

Something sounds fishy about your case. Why do you need to have board code /
platform data in the first place? Sorry, but I didn't get why you should
reconstruct DT (or ACPI) at run-time without using proper framework / feature
(overlays)?

> >>>> At the moment the driver gets the list from fwnode/of_node. The list
> >>>> contain references to phandles which get resolved and and the driver
> >>>> ends up with a bunch of gpio descriptors. Great.
> >>>> 
> >>>> This example looks nice but does the code that reads the reference from
> >>>> the gpios property and returns a gpiod actually exist? If it doesn't, I
> >>>> am willing to write it.
> >>>> 
> >>>> At first glance it makes more sense to me to pass (struct gpiod_lookup
> >>>> *) instead of (struct software_node *) and make gpiolib's gpiod_find()
> >>>> accept lookup tables as parameter instead of searching the
> >>>> gpio_lookup_list? Or do you think such temporary table should be
> >>>> assembled from the above structure and then used in gpiod_find()?
> >>>> 
> >>>> Any other suggestions on how to get a bunch of gpios (the description
> >>>> for gpios is available in the devicetree) for a device described with a
> >>>> software nodes?
Heikki Krogerus Nov. 10, 2020, 12:39 p.m. UTC | #10
On Mon, Nov 09, 2020 at 09:05:51PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 09, 2020 at 10:53:05AM -0800, Dmitry Torokhov wrote:
> > On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> > > It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> 
> ...
> 
> > > Probably I have missed something and I will be greatful, if you tell me
> > > where I can find more information about software nodes. There are few
> > > users in the kernel and it isn't obvious for me how to use software
> > > nodes properly.
> > 
> > Yeah, I disagree with Andy here. The lookup tables are a crutch that we
> > have until GPIO and PWM a taught to support software nodes (I need to
> > resurrect my patch series for GPIO, if you have time to test that would
> > be awesome).
> 
> We don't have support for list of fwnodes, this will probably break things
> where swnode is already exist.
> 
> I think Heikki may send a documentation patch to clarify when swnodes can and
> can't be used. Maybe I'm mistaken and above is a good use case by design.

Yeah, the problem is that I'm not sure that we can limit the swnodes
for any specific purpose, or dictate very strictly how they are used
with other possible fwnodes.

Right now I'm thinking we should simply not talk about the
relationship a software node should have or can have with other
fwnodes (regardless of their type) in the swnode documentation.

Br,
Rafael J. Wysocki Nov. 10, 2020, 12:46 p.m. UTC | #11
On Tue, Nov 10, 2020 at 1:39 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Mon, Nov 09, 2020 at 09:05:51PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 09, 2020 at 10:53:05AM -0800, Dmitry Torokhov wrote:
> > > On Mon, Nov 09, 2020 at 07:18:37PM +0100, Lukasz Stelmach wrote:
> > > > It was <2020-11-09 pon 19:24>, when Andy Shevchenko wrote:
> >
> > ...
> >
> > > > Probably I have missed something and I will be greatful, if you tell me
> > > > where I can find more information about software nodes. There are few
> > > > users in the kernel and it isn't obvious for me how to use software
> > > > nodes properly.
> > >
> > > Yeah, I disagree with Andy here. The lookup tables are a crutch that we
> > > have until GPIO and PWM a taught to support software nodes (I need to
> > > resurrect my patch series for GPIO, if you have time to test that would
> > > be awesome).
> >
> > We don't have support for list of fwnodes, this will probably break things
> > where swnode is already exist.
> >
> > I think Heikki may send a documentation patch to clarify when swnodes can and
> > can't be used. Maybe I'm mistaken and above is a good use case by design.
>
> Yeah, the problem is that I'm not sure that we can limit the swnodes
> for any specific purpose, or dictate very strictly how they are used
> with other possible fwnodes.

Generally agreed, but if there are known problems, they need to be
documented at least for the time being until they are addressed.

> Right now I'm thinking we should simply not talk about the
> relationship a software node should have or can have with other
> fwnodes (regardless of their type) in the swnode documentation.

This sounds reasonable to me, with the above exception.
diff mbox series

Patch

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 3d422918a53d9..604d7327bba79 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -246,6 +246,13 @@  static int property_entry_copy_data(struct property_entry *dst,
 	if (!src->is_inline && !src->length)
 		return -ENODATA;
 
+	/*
+	 * Reference properties are never stored inline as
+	 * they are too big.
+	 */
+	if (src->type == DEV_PROP_REF && src->is_inline)
+		return -EINVAL;
+
 	if (src->length <= sizeof(dst->value)) {
 		dst_ptr = &dst->value;
 		dst->is_inline = true;
@@ -473,23 +480,49 @@  software_node_get_reference_args(const struct fwnode_handle *fwnode,
 {
 	struct swnode *swnode = to_swnode(fwnode);
 	const struct software_node_reference *ref;
+	const struct software_node_ref_args *ref_array;
+	const struct software_node_ref_args *ref_args;
 	const struct property_entry *prop;
 	struct fwnode_handle *refnode;
 	u32 nargs_prop_val;
 	int error;
 	int i;
 
-	if (!swnode || !swnode->node->references)
+	if (!swnode)
 		return -ENOENT;
 
-	for (ref = swnode->node->references; ref->name; ref++)
-		if (!strcmp(ref->name, propname))
-			break;
+	prop = property_entry_get(swnode->node->properties, propname);
+	if (prop) {
+		if (prop->type != DEV_PROP_REF)
+			return -EINVAL;
 
-	if (!ref->name || index > (ref->nrefs - 1))
-		return -ENOENT;
+		/*
+		 * We expect that references are never stored inline, even
+		 * single ones, as they are too big.
+		 */
+		if (prop->is_inline)
+			return -EINVAL;
+
+		if (index * sizeof(*ref_args) >= prop->length)
+			return -ENOENT;
+
+		ref_array = prop->pointer;
+		ref_args = &ref_array[index];
+	} else {
+		if (!swnode->node->references)
+			return -ENOENT;
+
+		for (ref = swnode->node->references; ref->name; ref++)
+			if (!strcmp(ref->name, propname))
+				break;
+
+		if (!ref->name || index > (ref->nrefs - 1))
+			return -ENOENT;
+
+		ref_args = &ref->refs[index];
+	}
 
-	refnode = software_node_fwnode(ref->refs[index].node);
+	refnode = software_node_fwnode(ref_args->node);
 	if (!refnode)
 		return -ENOENT;
 
@@ -510,7 +543,7 @@  software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	args->nargs = nargs;
 
 	for (i = 0; i < nargs; i++)
-		args->args[i] = ref->refs[index].args[i];
+		args->args[i] = ref_args->args[i];
 
 	return 0;
 }
diff --git a/include/linux/property.h b/include/linux/property.h
index c592c286e3394..19b9dcc322763 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -22,6 +22,7 @@  enum dev_prop_type {
 	DEV_PROP_U32,
 	DEV_PROP_U64,
 	DEV_PROP_STRING,
+	DEV_PROP_REF,
 };
 
 enum dev_dma_attr {
@@ -223,6 +224,20 @@  static inline int fwnode_property_count_u64(const struct fwnode_handle *fwnode,
 	return fwnode_property_read_u64_array(fwnode, propname, NULL, 0);
 }
 
+struct software_node;
+
+/**
+ * struct software_node_ref_args - Reference property with additional arguments
+ * @node: Reference to a software node
+ * @nargs: Number of elements in @args array
+ * @args: Integer arguments
+ */
+struct software_node_ref_args {
+	const struct software_node *node;
+	unsigned int nargs;
+	u64 args[NR_FWNODE_REFERENCE_ARGS];
+};
+
 /**
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
@@ -260,14 +275,20 @@  struct property_entry {
 #define __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_)				\
 	sizeof(((struct property_entry *)NULL)->value._elem_[0])
 
-#define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _elem_, _Type_, _val_, _len_)\
+#define __PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_, _elsize_, _Type_,	\
+					  _val_, _len_)			\
 (struct property_entry) {						\
 	.name = _name_,							\
-	.length = (_len_) * __PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
+	.length = (_len_) * (_elsize_),					\
 	.type = DEV_PROP_##_Type_,					\
 	{ .pointer = _val_ },						\
 }
 
+#define __PROPERTY_ENTRY_ARRAY_LEN(_name_, _elem_, _Type_, _val_, _len_)\
+	__PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_,			\
+				__PROPERTY_ENTRY_ELEMENT_SIZE(_elem_),	\
+				_Type_, _val_, _len_)
+
 #define PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, _len_)		\
 	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u8_data, U8, _val_, _len_)
 #define PROPERTY_ENTRY_U16_ARRAY_LEN(_name_, _val_, _len_)		\
@@ -278,6 +299,10 @@  struct property_entry {
 	__PROPERTY_ENTRY_ARRAY_LEN(_name_, u64_data, U64, _val_, _len_)
 #define PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, _len_)		\
 	__PROPERTY_ENTRY_ARRAY_LEN(_name_, str, STRING, _val_, _len_)
+#define PROPERTY_ENTRY_REF_ARRAY_LEN(_name_, _val_, _len_)		\
+	__PROPERTY_ENTRY_ARRAY_ELSIZE_LEN(_name_,			\
+				sizeof(struct software_node_ref_args),	\
+				REF, _val_, _len_)
 
 #define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_)				\
 	PROPERTY_ENTRY_U8_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
@@ -289,6 +314,8 @@  struct property_entry {
 	PROPERTY_ENTRY_U64_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
 #define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_)			\
 	PROPERTY_ENTRY_STRING_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
+#define PROPERTY_ENTRY_REF_ARRAY(_name_, _val_)			\
+	PROPERTY_ENTRY_REF_ARRAY_LEN(_name_, _val_, ARRAY_SIZE(_val_))
 
 #define __PROPERTY_ENTRY_ELEMENT(_name_, _elem_, _Type_, _val_)		\
 (struct property_entry) {						\
@@ -316,6 +343,18 @@  struct property_entry {
 	.is_inline = true,			\
 }
 
+#define PROPERTY_ENTRY_REF(_name_, _ref_, ...)				\
+(struct property_entry) {						\
+	.name = _name_,							\
+	.length = sizeof(struct software_node_ref_args),		\
+	.type = DEV_PROP_REF,						\
+	{ .pointer = &(const struct software_node_ref_args) {		\
+		.node = _ref_,						\
+		.nargs = ARRAY_SIZE(((u64[]){ 0, ##__VA_ARGS__ })) - 1,	\
+		.args = { __VA_ARGS__ },				\
+	} },								\
+}
+
 struct property_entry *
 property_entries_dup(const struct property_entry *properties);
 
@@ -379,20 +418,6 @@  int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
-struct software_node;
-
-/**
- * struct software_node_ref_args - Reference with additional arguments
- * @node: Reference to a software node
- * @nargs: Number of elements in @args array
- * @args: Integer arguments
- */
-struct software_node_ref_args {
-	const struct software_node *node;
-	unsigned int nargs;
-	u64 args[NR_FWNODE_REFERENCE_ARGS];
-};
-
 /**
  * struct software_node_reference - Named software node reference property
  * @name: Name of the property