diff mbox series

[RFC/PATCH,2/5] device property: introduce notion of subnodes for legacy boards

Message ID 20180917181603.125492-3-dmitry.torokhov@gmail.com (mailing list archive)
State New, archived
Headers show
Series Support children for legacy device properties | expand

Commit Message

Dmitry Torokhov Sept. 17, 2018, 6:16 p.m. UTC
Several drivers rely on having notion of sub-nodes when describing
hardware, let's allow static board-defined properties also have it.

The board files will then attach properties to devices in the following
fashion:

	device_add_properties(&board_platform_device.dev,
			      main_device_props);
	device_add_child_properties(&board_platform_device.dev,
				    dev_fwnode(&board_platform_device.dev),
				    child1_device_props);
	device_add_child_properties(&board_platform_device.dev,
				    dev_fwnode(&board_platform_device.dev),
				    child2_device_props);
	...
	platform_device_register(&board_platform_device.dev);

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/pset_property.c | 110 +++++++++++++++++++++++++++++++----
 include/linux/property.h     |   4 ++
 2 files changed, 102 insertions(+), 12 deletions(-)

Comments

Heikki Krogerus Sept. 19, 2018, 3:10 p.m. UTC | #1
Hi,

On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote:
> Several drivers rely on having notion of sub-nodes when describing
> hardware, let's allow static board-defined properties also have it.
> 
> The board files will then attach properties to devices in the following
> fashion:
> 
> 	device_add_properties(&board_platform_device.dev,
> 			      main_device_props);
> 	device_add_child_properties(&board_platform_device.dev,
> 				    dev_fwnode(&board_platform_device.dev),
> 				    child1_device_props);
> 	device_add_child_properties(&board_platform_device.dev,
> 				    dev_fwnode(&board_platform_device.dev),
> 				    child2_device_props);
> 	...
> 	platform_device_register(&board_platform_device.dev);
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/pset_property.c | 110 +++++++++++++++++++++++++++++++----
>  include/linux/property.h     |   4 ++
>  2 files changed, 102 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> index 08ecc13080ae..63f2377aefe8 100644
> --- a/drivers/base/pset_property.c
> +++ b/drivers/base/pset_property.c
> @@ -18,6 +18,11 @@ struct property_set {
>  	struct device *dev;
>  	struct fwnode_handle fwnode;
>  	const struct property_entry *properties;
> +
> +	struct property_set *parent;
> +	/* Entry in parent->children list */
> +	struct list_head child_node;
> +	struct list_head children;

Add

        const char *name;

and you can implement also pset_get_named_child_node().

>  };
>  
>  static const struct fwnode_operations pset_fwnode_ops;
> @@ -283,10 +288,47 @@ pset_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
>  					   val, nval);
>  }
>  
> +struct fwnode_handle *pset_fwnode_get_parent(const struct fwnode_handle *fwnode)
> +{
> +	struct property_set *pset = to_pset_node(fwnode);
> +
> +	return pset ? &pset->parent->fwnode : NULL;
> +}
> +
> +struct fwnode_handle *
> +pset_fwnode_get_next_subnode(const struct fwnode_handle *fwnode,
> +			     struct fwnode_handle *child)
> +{
> +	const struct property_set *pset = to_pset_node(fwnode);
> +	struct property_set *first_child;
> +	struct property_set *next;
> +
> +	if (!pset)
> +		return NULL;
> +
> +	if (list_empty(&pset->children))
> +		return NULL;
> +
> +	first_child = list_first_entry(&pset->children, struct property_set,
> +				       child_node);
> +
> +	if (child) {
> +		next = list_next_entry(to_pset_node(child), child_node);
> +		if (next == first_child)
> +			return NULL;
> +	} else {
> +		next = first_child;
> +	}
> +
> +	return &next->fwnode;
> +}
> +
>  static const struct fwnode_operations pset_fwnode_ops = {
>  	.property_present = pset_fwnode_property_present,
>  	.property_read_int_array = pset_fwnode_read_int_array,
>  	.property_read_string_array = pset_fwnode_property_read_string_array,
> +	.get_parent = pset_fwnode_get_parent,
> +	.get_next_child_node = pset_fwnode_get_next_subnode,
>  };
>  
>  static void property_entry_free_data(const struct property_entry *p)
> @@ -439,24 +481,31 @@ EXPORT_SYMBOL_GPL(property_entries_free);
>   */
>  static void pset_free_set(struct property_set *pset)
>  {
> +	struct property_set *child, *next;
> +
>  	if (!pset)
>  		return;
>  
> +	list_for_each_entry_safe(child, next, &pset->children, child_node) {
> +		list_del(&child->child_node);
> +		pset_free_set(child);
> +	}
> +
>  	property_entries_free(pset->properties);
>  	kfree(pset);
>  }
>  
>  /**
> - * pset_copy_set - copies property set
> - * @pset: Property set to copy
> + * pset_create_set - creates property set.
> + * @src: property entries for the set.
>   *
> - * This function takes a deep copy of the given property set and returns
> - * pointer to the copy. Call device_free_property_set() to free resources
> - * allocated in this function.
> + * This function takes a deep copy of the given property entries and creates
> + * property set. Call pset_free_set() to free resources allocated in this
> + * function.
>   *
>   * Return: Pointer to the new property set or error pointer.
>   */
> -static struct property_set *pset_copy_set(const struct property_set *pset)
> +static struct property_set *pset_create_set(const struct property_entry *src)
>  {
>  	struct property_entry *properties;
>  	struct property_set *p;
> @@ -465,7 +514,11 @@ static struct property_set *pset_copy_set(const struct property_set *pset)
>  	if (!p)
>  		return ERR_PTR(-ENOMEM);
>  
> -	properties = property_entries_dup(pset->properties);
> +	INIT_LIST_HEAD(&p->child_node);
> +	INIT_LIST_HEAD(&p->children);
> +	p->fwnode.ops = &pset_fwnode_ops;
> +
> +	properties = property_entries_dup(src);
>  	if (IS_ERR(properties)) {
>  		kfree(p);
>  		return ERR_CAST(properties);
> @@ -521,20 +574,53 @@ EXPORT_SYMBOL_GPL(device_remove_properties);
>  int device_add_properties(struct device *dev,
>  			  const struct property_entry *properties)
>  {
> -	struct property_set *p, pset;
> +	struct property_set *p;
>  
>  	if (!properties)
>  		return -EINVAL;
>  
> -	pset.properties = properties;
> -
> -	p = pset_copy_set(&pset);
> +	p = pset_create_set(properties);
>  	if (IS_ERR(p))
>  		return PTR_ERR(p);
>  
> -	p->fwnode.ops = &pset_fwnode_ops;
>  	set_secondary_fwnode(dev, &p->fwnode);
>  	p->dev = dev;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(device_add_properties);
> +
> +/**
> + * device_add_child_properties - Add a collection of properties to a device object.
> + * @dev: Device to add properties to.

@parent: missing

> + * @properties: Collection of properties to add.
> + *
> + * Associate a collection of device properties represented by @properties as a
> + * child of given @parent firmware node.  The function takes a copy of
> + * @properties.
> + */
> +struct fwnode_handle *
> +device_add_child_properties(struct device *dev,

device_add_child_properties(struct device *dev, const char *child_name,

> +			    struct fwnode_handle *parent,
> +			    const struct property_entry *properties)
> +{
> +	struct property_set *p;
> +	struct property_set *parent_pset;
> +
> +	if (!properties)
> +		return ERR_PTR(-EINVAL);
> +
> +	parent_pset = to_pset_node(parent);
> +	if (!parent_pset)
> +		return ERR_PTR(-EINVAL);
> +
> +	p = pset_create_set(properties);
> +	if (IS_ERR(p))
> +		return ERR_CAST(p);
> +
> +	p->dev = dev;

        p->name = kstrdup_const(child_name, GFP_KERNEL);

You'll need to kfree_const(pset->name) in pset_free_set() of course.

> +	p->parent = parent_pset;
> +	list_add_tail(&p->child_node, &parent_pset->children);
> +
> +	return &p->fwnode;
> +}
> +EXPORT_SYMBOL_GPL(device_add_child_properties);


Br,
Dmitry Torokhov Sept. 19, 2018, 5:13 p.m. UTC | #2
Hi Heikki,

On Wed, Sep 19, 2018 at 06:10:26PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote:
> > Several drivers rely on having notion of sub-nodes when describing
> > hardware, let's allow static board-defined properties also have it.
> > 
> > The board files will then attach properties to devices in the following
> > fashion:
> > 
> > 	device_add_properties(&board_platform_device.dev,
> > 			      main_device_props);
> > 	device_add_child_properties(&board_platform_device.dev,
> > 				    dev_fwnode(&board_platform_device.dev),
> > 				    child1_device_props);
> > 	device_add_child_properties(&board_platform_device.dev,
> > 				    dev_fwnode(&board_platform_device.dev),
> > 				    child2_device_props);
> > 	...
> > 	platform_device_register(&board_platform_device.dev);
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/base/pset_property.c | 110 +++++++++++++++++++++++++++++++----
> >  include/linux/property.h     |   4 ++
> >  2 files changed, 102 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > index 08ecc13080ae..63f2377aefe8 100644
> > --- a/drivers/base/pset_property.c
> > +++ b/drivers/base/pset_property.c
> > @@ -18,6 +18,11 @@ struct property_set {
> >  	struct device *dev;
> >  	struct fwnode_handle fwnode;
> >  	const struct property_entry *properties;
> > +
> > +	struct property_set *parent;
> > +	/* Entry in parent->children list */
> > +	struct list_head child_node;
> > +	struct list_head children;
> 
> Add
> 
>         const char *name;
> 
> and you can implement also pset_get_named_child_node().

Or
	char name[];

to avoid separate allocation.

Alternatively, we can add it later when we need it, and add
device_add_named_child_properties().

I'll leave it up to Rafael to decide.

Thanks.
Heikki Krogerus Sept. 20, 2018, 10:16 a.m. UTC | #3
On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote:
> > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > > index 08ecc13080ae..63f2377aefe8 100644
> > > --- a/drivers/base/pset_property.c
> > > +++ b/drivers/base/pset_property.c
> > > @@ -18,6 +18,11 @@ struct property_set {
> > >  	struct device *dev;
> > >  	struct fwnode_handle fwnode;
> > >  	const struct property_entry *properties;
> > > +
> > > +	struct property_set *parent;
> > > +	/* Entry in parent->children list */
> > > +	struct list_head child_node;
> > > +	struct list_head children;
> > 
> > Add
> > 
> >         const char *name;
> > 
> > and you can implement also pset_get_named_child_node().
> 
> Or
> 	char name[];
> 
> to avoid separate allocation.

Let's not do that, especially if you are planning on exporting this
structure. If the name is coming from .rodata, there is no need to
allocate anything for the name. Check kstrdup_const().

> Alternatively, we can add it later when we need it, and add
> device_add_named_child_properties().
> 
> I'll leave it up to Rafael to decide.

Fair enough.


Thanks,
Heikki Krogerus Sept. 20, 2018, 1:53 p.m. UTC | #4
Hi Dmitry,

On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote:
> +/**
> + * device_add_child_properties - Add a collection of properties to a device object.
> + * @dev: Device to add properties to.

In case you didn't notice my comment for this, you are missing @parent
here.

But why do you need both the parent and the dev?

> + * @properties: Collection of properties to add.
> + *
> + * Associate a collection of device properties represented by @properties as a
> + * child of given @parent firmware node.  The function takes a copy of
> + * @properties.
> + */
> +struct fwnode_handle *
> +device_add_child_properties(struct device *dev,
> +			    struct fwnode_handle *parent,
> +			    const struct property_entry *properties)
> +{
> +	struct property_set *p;
> +	struct property_set *parent_pset;
> +
> +	if (!properties)
> +		return ERR_PTR(-EINVAL);
> +
> +	parent_pset = to_pset_node(parent);

For this function, the parent will in practice have to be
dev_fwnode(dev), so I don't think you need @parent at all, no?

There is something wrong here..

> +	if (!parent_pset)
> +		return ERR_PTR(-EINVAL);
> +
> +	p = pset_create_set(properties);
> +	if (IS_ERR(p))
> +		return ERR_CAST(p);
> +
> +	p->dev = dev;

That looks wrong.

I'm guessing the assumption here is that the child nodes will never be
assigned to their own devices, but you can't do that. It will limit
the use of the child nodes to a very small number of cases, possibly
only to gpios.

I think that has to be fixed. It should not be a big deal. Just expect
the child nodes to be removed separately, and add ref counting to the
struct property_set handling.

> +	p->parent = parent_pset;
> +	list_add_tail(&p->child_node, &parent_pset->children);
> +
> +	return &p->fwnode;
> +}
> +EXPORT_SYMBOL_GPL(device_add_child_properties);

The child nodes will change the purpose of the build-in property
support. Originally the goal was just to support adding of build-in
device properties to real firmware nodes, but things have changed
quite a bit from that. These child nodes are purely tied to the
build-in device property support, so we should be talking about adding
pset type child nodes to pset type parent nodes in the API:
fwnode_pset_add_child_node(), or something like that.

I'm sorry to be a bit of pain in the butt with this. The build-in
property support is a hack, it always was. A useful hack, but hack
nevertheless. That means we need to be extra careful when expanding
it, like here.


Thanks,
Linus Walleij Sept. 21, 2018, 3:36 p.m. UTC | #5
On Thu, Sep 20, 2018 at 6:53 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:

> The child nodes will change the purpose of the build-in property
> support. Originally the goal was just to support adding of build-in
> device properties to real firmware nodes, but things have changed
> quite a bit from that. These child nodes are purely tied to the
> build-in device property support, so we should be talking about adding
> pset type child nodes to pset type parent nodes in the API:
> fwnode_pset_add_child_node(), or something like that.
>
> I'm sorry to be a bit of pain in the butt with this. The build-in
> property support is a hack, it always was. A useful hack, but hack
> nevertheless. That means we need to be extra careful when expanding
> it, like here.

I dunno how we got to here, what I tried to solve and Dmitry tries
to make more general is converting old boardfiles to use fwnode, because
I initially tried to just support gpio descriptors from board files.

If boardfiles is what you mean with "build-in property support" I don't
know, it predates both device tree and ACPI and any other hardware
descriptions on the ARM architecture, from the perspective of these
systems things are fine and the hardware description languages
are the novelty...

But I guess you know because you worked with OMAP :)

So there is something I don't understand here.

Yours,
Linus Walleij
Dmitry Torokhov Sept. 21, 2018, 11:31 p.m. UTC | #6
Hi Heikki,

On Thu, Sep 20, 2018 at 04:53:48PM +0300, Heikki Krogerus wrote:
> Hi Dmitry,
> 
> On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote:
> > +/**
> > + * device_add_child_properties - Add a collection of properties to a device object.
> > + * @dev: Device to add properties to.
> 
> In case you didn't notice my comment for this, you are missing @parent
> here.
> 
> But why do you need both the parent and the dev?

I could go by parent only and fetch dev from parent.

> 
> > + * @properties: Collection of properties to add.
> > + *
> > + * Associate a collection of device properties represented by @properties as a
> > + * child of given @parent firmware node.  The function takes a copy of
> > + * @properties.
> > + */
> > +struct fwnode_handle *
> > +device_add_child_properties(struct device *dev,
> > +			    struct fwnode_handle *parent,
> > +			    const struct property_entry *properties)
> > +{
> > +	struct property_set *p;
> > +	struct property_set *parent_pset;
> > +
> > +	if (!properties)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	parent_pset = to_pset_node(parent);
> 
> For this function, the parent will in practice have to be
> dev_fwnode(dev), so I don't think you need @parent at all, no?
> 
> There is something wrong here..

Yes, I expect majority of the calls will use dev_fwnode(dev) as parent,
but nobody stops you from doing:

	device_add_properties(dev, props);
	c1 = device_add_child_properties(dev, dev_fwnode(dev), cp1);
	c2 = device_add_child_properties(dev, c1, cp2);
	c3 = device_add_child_properties(dev, c2, cp3);
	...

> 
> > +	if (!parent_pset)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	p = pset_create_set(properties);
> > +	if (IS_ERR(p))
> > +		return ERR_CAST(p);
> > +
> > +	p->dev = dev;
> 
> That looks wrong.
> 
> I'm guessing the assumption here is that the child nodes will never be
> assigned to their own devices, but you can't do that. It will limit
> the use of the child nodes to a very small number of cases, possibly
> only to gpios.

If I need to assign a node to a device I'll use device_add_properties()
API. device_add_child_properties() is for nodes living "below" the
device.

All nodes (the primary/secondary and children) would point to the owning
device, just for convenience.

> 
> I think that has to be fixed. It should not be a big deal. Just expect
> the child nodes to be removed separately, and add ref counting to the
> struct property_set handling.

Why do we need to remove them separately and what do we need refcounting
for?

> 
> > +	p->parent = parent_pset;
> > +	list_add_tail(&p->child_node, &parent_pset->children);
> > +
> > +	return &p->fwnode;
> > +}
> > +EXPORT_SYMBOL_GPL(device_add_child_properties);
> 
> The child nodes will change the purpose of the build-in property
> support. Originally the goal was just to support adding of build-in
> device properties to real firmware nodes, but things have changed
> quite a bit from that. These child nodes are purely tied to the
> build-in device property support, so we should be talking about adding
> pset type child nodes to pset type parent nodes in the API:
> fwnode_pset_add_child_node(), or something like that.

OK, I can change device_add_child_properties() to
fwnode_pset_add_child_node() if Rafael would prefer this name.

Thanks.
Dmitry Torokhov Sept. 21, 2018, 11:33 p.m. UTC | #7
On Thu, Sep 20, 2018 at 01:16:48PM +0300, Heikki Krogerus wrote:
> On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote:
> > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > > > index 08ecc13080ae..63f2377aefe8 100644
> > > > --- a/drivers/base/pset_property.c
> > > > +++ b/drivers/base/pset_property.c
> > > > @@ -18,6 +18,11 @@ struct property_set {
> > > >  	struct device *dev;
> > > >  	struct fwnode_handle fwnode;
> > > >  	const struct property_entry *properties;
> > > > +
> > > > +	struct property_set *parent;
> > > > +	/* Entry in parent->children list */
> > > > +	struct list_head child_node;
> > > > +	struct list_head children;
> > > 
> > > Add
> > > 
> > >         const char *name;
> > > 
> > > and you can implement also pset_get_named_child_node().
> > 
> > Or
> > 	char name[];
> > 
> > to avoid separate allocation.
> 
> Let's not do that, especially if you are planning on exporting this
> structure.

Can you please elaborate why? Not using pointer saves us 4/8 bytes +
however much memory we need for bookkeeping for the extra chunk. Given
that majority of pset nodes are unnamed this seems wasteful.

> If the name is coming from .rodata, there is no need to
> allocate anything for the name. Check kstrdup_const().

The data is most likely coming as __initconst so we do need to copy it.

> 
> > Alternatively, we can add it later when we need it, and add
> > device_add_named_child_properties().
> > 
> > I'll leave it up to Rafael to decide.
> 
> Fair enough.
> 
> 
> Thanks,
> 
> -- 
> heikki

Thanks.
Heikki Krogerus Sept. 24, 2018, 7:29 a.m. UTC | #8
On Fri, Sep 21, 2018 at 04:33:36PM -0700, Dmitry Torokhov wrote:
> On Thu, Sep 20, 2018 at 01:16:48PM +0300, Heikki Krogerus wrote:
> > On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote:
> > > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > > > > index 08ecc13080ae..63f2377aefe8 100644
> > > > > --- a/drivers/base/pset_property.c
> > > > > +++ b/drivers/base/pset_property.c
> > > > > @@ -18,6 +18,11 @@ struct property_set {
> > > > >  	struct device *dev;
> > > > >  	struct fwnode_handle fwnode;
> > > > >  	const struct property_entry *properties;
> > > > > +
> > > > > +	struct property_set *parent;
> > > > > +	/* Entry in parent->children list */
> > > > > +	struct list_head child_node;
> > > > > +	struct list_head children;
> > > > 
> > > > Add
> > > > 
> > > >         const char *name;
> > > > 
> > > > and you can implement also pset_get_named_child_node().
> > > 
> > > Or
> > > 	char name[];
> > > 
> > > to avoid separate allocation.
> > 
> > Let's not do that, especially if you are planning on exporting this
> > structure.
> 
> Can you please elaborate why? Not using pointer saves us 4/8 bytes +
> however much memory we need for bookkeeping for the extra chunk. Given
> that majority of pset nodes are unnamed this seems wasteful.
> 
> > If the name is coming from .rodata, there is no need to
> > allocate anything for the name. Check kstrdup_const().
> 
> The data is most likely coming as __initconst so we do need to copy it.

OK, I did not consider that. Yes, it makes sense to always copy.

Thanks,
Heikki Krogerus Sept. 24, 2018, 10:20 a.m. UTC | #9
Hi Linus,

On Fri, Sep 21, 2018 at 08:36:53AM -0700, Linus Walleij wrote:
> On Thu, Sep 20, 2018 at 6:53 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> 
> > The child nodes will change the purpose of the build-in property
> > support. Originally the goal was just to support adding of build-in
> > device properties to real firmware nodes, but things have changed
> > quite a bit from that. These child nodes are purely tied to the
> > build-in device property support, so we should be talking about adding
> > pset type child nodes to pset type parent nodes in the API:
> > fwnode_pset_add_child_node(), or something like that.
> >
> > I'm sorry to be a bit of pain in the butt with this. The build-in
> > property support is a hack, it always was. A useful hack, but hack
> > nevertheless. That means we need to be extra careful when expanding
> > it, like here.
> 
> I dunno how we got to here, what I tried to solve and Dmitry tries
> to make more general is converting old boardfiles to use fwnode, because
> I initially tried to just support gpio descriptors from board files.

I understand that, but what I'm trying to say is that the solution for
the child nodes is not generic enough. I'll try to explain below.

> If boardfiles is what you mean with "build-in property support" I don't
> know, it predates both device tree and ACPI and any other hardware
> descriptions on the ARM architecture, from the perspective of these
> systems things are fine and the hardware description languages
> are the novelty...

Rafael talked about "build-in properties" at one point, and I just
started using that expression after that, but it's probable not the
best term to describe this thing.

But please note that we are not talking about only static information
with these property sets. They can be populated dynamically as well,
so this kind of extensions must consider and support that. On top of
board files we need to consider things like glue and probing drivers
and even buses in some cases.

> But I guess you know because you worked with OMAP :)
> 
> So there is something I don't understand here.

Sorry for the poor choice of words on my behalf. I guess I'm not
explaining my point properly. Let me try again.

So I'm seeing a case that this solution does not seem to be able to
support, but ultimately it will need to do so: with USB ports we are
going to need to be able to assign a device to the child nodes that
represent those ports.

To be honest, normally I would not care about that, and I would simply
wait for this to go into mainline, and then propose a modification to
it so it can also support that other case.

However, this time I feel that we should not do that. The solution for
the child nodes should be implemented so that it can support all the
known cases from the beginning. The reason for that is because I fear
that we'll end up having a pile of ad-hoc style solutions, each
solving an individual problem, and a whole lot of duplicated stuff for
these property sets. In the end we will have spaghetti with meatballs
that nobody is able fully understand nor handle. And I really see a
strong possibility for that to happen here.


Thanks,
Heikki Krogerus Sept. 24, 2018, 1:20 p.m. UTC | #10
Hi Dmitry,

On Fri, Sep 21, 2018 at 04:31:19PM -0700, Dmitry Torokhov wrote:
> > > +	if (!parent_pset)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	p = pset_create_set(properties);
> > > +	if (IS_ERR(p))
> > > +		return ERR_CAST(p);
> > > +
> > > +	p->dev = dev;
> > 
> > That looks wrong.
> > 
> > I'm guessing the assumption here is that the child nodes will never be
> > assigned to their own devices, but you can't do that. It will limit
> > the use of the child nodes to a very small number of cases, possibly
> > only to gpios.
> 
> If I need to assign a node to a device I'll use device_add_properties()
> API. device_add_child_properties() is for nodes living "below" the
> device.

device_add_properties() is not available to us before we have the
actual struct device meant for the properties. If the child device is
populated outside of the "boardfiles" then we have to be able to link
it to the child node afterwards.

But I took a closer look at the code and realized that you are not
using that p->dev with the child nodes for anything, so I may be wrong
about this. You are not actually linking the child nodes to that
device here.

> All nodes (the primary/secondary and children) would point to the owning
> device, just for convenience.

Since you don't use that for anything, then drop that line. It's only
confusing.

And besides, since we will have separate child devices for those child
nodes, there is a small change that somebody needs to call
device_remove_properties(child_dev). At least then that operation
becomes safe, as it's a nop with the child pset nodes.

> > I think that has to be fixed. It should not be a big deal. Just expect
> > the child nodes to be removed separately, and add ref counting to the
> > struct property_set handling.
> 
> Why do we need to remove them separately and what do we need refcounting
> for?

If you could remove the nodes independently, then obviously the parent
can not be removed before the children. The ref count would be there
to prevent that.

Though my original comment is not valid, I still think that the child
nodes should be created and destroyed separately. Actually, I don't
think we should be talking about child nodes in the API at all. Check
below..

> > > +	p->parent = parent_pset;
> > > +	list_add_tail(&p->child_node, &parent_pset->children);
> > > +
> > > +	return &p->fwnode;
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_add_child_properties);
> > 
> > The child nodes will change the purpose of the build-in property
> > support. Originally the goal was just to support adding of build-in
> > device properties to real firmware nodes, but things have changed
> > quite a bit from that. These child nodes are purely tied to the
> > build-in device property support, so we should be talking about adding
> > pset type child nodes to pset type parent nodes in the API:
> > fwnode_pset_add_child_node(), or something like that.
> 
> OK, I can change device_add_child_properties() to
> fwnode_pset_add_child_node() if Rafael would prefer this name.

So not even that. We should have API like this:

struct fwnode_handle *
fwnode_pset_create(struct fwnode_handle *parent, const char *name,
                   const struct property_entry *props);

void fwnode_pset_destroy(struct fwnode_handle *fwnode);

int device_add_properties(struct device *dev,
                          const struct fwnode_handle *pset_node);

void device_remove_properties(struct device *dev);

So basically we separate creation of the nodes from linking them to the
devices completely. That would give this API much better structure. It
would scale better, for example, it would then be possible to add
child nodes from different locations if needed.


Thanks,
Dmitry Torokhov Sept. 24, 2018, 6:45 p.m. UTC | #11
On Mon, Sep 24, 2018 at 04:20:50PM +0300, Heikki Krogerus wrote:
> Hi Dmitry,
> 
> On Fri, Sep 21, 2018 at 04:31:19PM -0700, Dmitry Torokhov wrote:
> > > > +	if (!parent_pset)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	p = pset_create_set(properties);
> > > > +	if (IS_ERR(p))
> > > > +		return ERR_CAST(p);
> > > > +
> > > > +	p->dev = dev;
> > > 
> > > That looks wrong.
> > > 
> > > I'm guessing the assumption here is that the child nodes will never be
> > > assigned to their own devices, but you can't do that. It will limit
> > > the use of the child nodes to a very small number of cases, possibly
> > > only to gpios.
> > 
> > If I need to assign a node to a device I'll use device_add_properties()
> > API. device_add_child_properties() is for nodes living "below" the
> > device.
> 
> device_add_properties() is not available to us before we have the
> actual struct device meant for the properties. If the child device is
> populated outside of the "boardfiles" then we have to be able to link
> it to the child node afterwards.

I think we are talking about totally different use cases and that is why
we are having hard time coming to a mutually agreeable solution. Could
you please describe in more detail what you would like to achieve,
and preferably show how it is described now with DT and/or ACPI, so that
I have a better frame of reference.

Thanks.
Heikki Krogerus Sept. 25, 2018, 12:19 p.m. UTC | #12
On Mon, Sep 24, 2018 at 11:45:43AM -0700, Dmitry Torokhov wrote:
> I think we are talking about totally different use cases and that is why
> we are having hard time coming to a mutually agreeable solution. Could
> you please describe in more detail what you would like to achieve,
> and preferably show how it is described now with DT and/or ACPI, so that
> I have a better frame of reference.

Yes, of course. Sorry.

USB ports are devices that usually the USB controller drivers register
(or actually the USB core code). They are represented in both ACPI and
DT as child nodes of the controller device node. The USB connector OF
node is defined in file
Documentation/devicetree/bindings/connector/usb-connector.txt

In short, the controller drivers will request handle to a child node
that represents a port, and only after that register the actual port
device.

The drivers I'm looking at currently are the USB Type-C port
controller drivers and the port manager (in Greg's usb-next or
linux-next):

        drivers/usb/typec/tcpm/tcpci.c
        drivers/usb/typec/tcpm/fusb302.c
        drivers/usb/typec/tcpm/tcpm.c

The goal is simply to get rid of the platform data as usual, and
ideally so that we don't need any extra code in order to support the
"legacy" platforms.


Thanks,
Dmitry Torokhov Oct. 5, 2018, 9:47 p.m. UTC | #13
Hi Heikki,

On Tue, Sep 25, 2018 at 03:19:27PM +0300, Heikki Krogerus wrote:
> On Mon, Sep 24, 2018 at 11:45:43AM -0700, Dmitry Torokhov wrote:
> > I think we are talking about totally different use cases and that is why
> > we are having hard time coming to a mutually agreeable solution. Could
> > you please describe in more detail what you would like to achieve,
> > and preferably show how it is described now with DT and/or ACPI, so that
> > I have a better frame of reference.
> 
> Yes, of course. Sorry.
> 
> USB ports are devices that usually the USB controller drivers register
> (or actually the USB core code). They are represented in both ACPI and
> DT as child nodes of the controller device node. The USB connector OF
> node is defined in file
> Documentation/devicetree/bindings/connector/usb-connector.txt
> 
> In short, the controller drivers will request handle to a child node
> that represents a port, and only after that register the actual port
> device.
> 
> The drivers I'm looking at currently are the USB Type-C port
> controller drivers and the port manager (in Greg's usb-next or
> linux-next):
> 
>         drivers/usb/typec/tcpm/tcpci.c
>         drivers/usb/typec/tcpm/fusb302.c
>         drivers/usb/typec/tcpm/tcpm.c
> 
> The goal is simply to get rid of the platform data as usual, and
> ideally so that we don't need any extra code in order to support the
> "legacy" platforms.

Are these actually used on any of the "legacy" platforms? I fetched
linux-next today, but I do not actually see anything in
drivers/usb/typec touching platform data...

In the context of the connector, even before we descend to child nodes
details, how do you propose implementing references between fwnodes?
Especially since the other node (in case you complementing existing
topology) may be ACPI or DT instance?

I want to say that "You aren't gonna need it" for what you are asking
here and we can actually split it apart if and when we actually want to
separate creation of pset-backed device nodes and instantiating
corresponding device structures.

Thanks.
Heikki Krogerus Oct. 11, 2018, 8:18 a.m. UTC | #14
Hi Dmitry,

Sorry for the late reply. I decided to give this a try myself, and
I've now prepared something (not for the gpio stuff!). I'll do a quick
internal review round, and send it to you guys as RFC after that..

On Fri, Oct 05, 2018 at 02:47:34PM -0700, Dmitry Torokhov wrote:
> Hi Heikki,
> 
> On Tue, Sep 25, 2018 at 03:19:27PM +0300, Heikki Krogerus wrote:
> > On Mon, Sep 24, 2018 at 11:45:43AM -0700, Dmitry Torokhov wrote:
> > > I think we are talking about totally different use cases and that is why
> > > we are having hard time coming to a mutually agreeable solution. Could
> > > you please describe in more detail what you would like to achieve,
> > > and preferably show how it is described now with DT and/or ACPI, so that
> > > I have a better frame of reference.
> > 
> > Yes, of course. Sorry.
> > 
> > USB ports are devices that usually the USB controller drivers register
> > (or actually the USB core code). They are represented in both ACPI and
> > DT as child nodes of the controller device node. The USB connector OF
> > node is defined in file
> > Documentation/devicetree/bindings/connector/usb-connector.txt
> > 
> > In short, the controller drivers will request handle to a child node
> > that represents a port, and only after that register the actual port
> > device.
> > 
> > The drivers I'm looking at currently are the USB Type-C port
> > controller drivers and the port manager (in Greg's usb-next or
> > linux-next):
> > 
> >         drivers/usb/typec/tcpm/tcpci.c
> >         drivers/usb/typec/tcpm/fusb302.c
> >         drivers/usb/typec/tcpm/tcpm.c
> > 
> > The goal is simply to get rid of the platform data as usual, and
> > ideally so that we don't need any extra code in order to support the
> > "legacy" platforms.
> 
> Are these actually used on any of the "legacy" platforms? I fetched
> linux-next today, but I do not actually see anything in
> drivers/usb/typec touching platform data...

It's not touching any platform data, and I would like to keep it that
way :-). The device for fusb302 is created for example in
drivers/platform/x86/intel_cht_int33fe.c.

The fusb302.c driver already tries to find that child node named
"connector" (and so do the other port drivers). I'm trying to supply
the driver that node here somehow so I can avoid quirks.

> In the context of the connector, even before we descend to child nodes
> details, how do you propose implementing references between fwnodes?
> Especially since the other node (in case you complementing existing
> topology) may be ACPI or DT instance?

The different fwnode types (ACPI, OF, etc.) must live in parallel,
i.e. you can not have something like ACPI parent node with
property_set children if that's what you mean. The "secondary" member
will link the different types of fwnodes together, but they will live
their separate lives. I'm not planning on changing that.

I'm just looking for a smooth way of describing the devices in
software when the firmware is not supplying the hardware description,
just like you. What I really need is separation of the fwnode
registration from device registration in those cases. For that I now
have a proposal, and I believe my proposal will work for you as a
baseline as well.


Thanks,
diff mbox series

Patch

diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
index 08ecc13080ae..63f2377aefe8 100644
--- a/drivers/base/pset_property.c
+++ b/drivers/base/pset_property.c
@@ -18,6 +18,11 @@  struct property_set {
 	struct device *dev;
 	struct fwnode_handle fwnode;
 	const struct property_entry *properties;
+
+	struct property_set *parent;
+	/* Entry in parent->children list */
+	struct list_head child_node;
+	struct list_head children;
 };
 
 static const struct fwnode_operations pset_fwnode_ops;
@@ -283,10 +288,47 @@  pset_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 					   val, nval);
 }
 
+struct fwnode_handle *pset_fwnode_get_parent(const struct fwnode_handle *fwnode)
+{
+	struct property_set *pset = to_pset_node(fwnode);
+
+	return pset ? &pset->parent->fwnode : NULL;
+}
+
+struct fwnode_handle *
+pset_fwnode_get_next_subnode(const struct fwnode_handle *fwnode,
+			     struct fwnode_handle *child)
+{
+	const struct property_set *pset = to_pset_node(fwnode);
+	struct property_set *first_child;
+	struct property_set *next;
+
+	if (!pset)
+		return NULL;
+
+	if (list_empty(&pset->children))
+		return NULL;
+
+	first_child = list_first_entry(&pset->children, struct property_set,
+				       child_node);
+
+	if (child) {
+		next = list_next_entry(to_pset_node(child), child_node);
+		if (next == first_child)
+			return NULL;
+	} else {
+		next = first_child;
+	}
+
+	return &next->fwnode;
+}
+
 static const struct fwnode_operations pset_fwnode_ops = {
 	.property_present = pset_fwnode_property_present,
 	.property_read_int_array = pset_fwnode_read_int_array,
 	.property_read_string_array = pset_fwnode_property_read_string_array,
+	.get_parent = pset_fwnode_get_parent,
+	.get_next_child_node = pset_fwnode_get_next_subnode,
 };
 
 static void property_entry_free_data(const struct property_entry *p)
@@ -439,24 +481,31 @@  EXPORT_SYMBOL_GPL(property_entries_free);
  */
 static void pset_free_set(struct property_set *pset)
 {
+	struct property_set *child, *next;
+
 	if (!pset)
 		return;
 
+	list_for_each_entry_safe(child, next, &pset->children, child_node) {
+		list_del(&child->child_node);
+		pset_free_set(child);
+	}
+
 	property_entries_free(pset->properties);
 	kfree(pset);
 }
 
 /**
- * pset_copy_set - copies property set
- * @pset: Property set to copy
+ * pset_create_set - creates property set.
+ * @src: property entries for the set.
  *
- * This function takes a deep copy of the given property set and returns
- * pointer to the copy. Call device_free_property_set() to free resources
- * allocated in this function.
+ * This function takes a deep copy of the given property entries and creates
+ * property set. Call pset_free_set() to free resources allocated in this
+ * function.
  *
  * Return: Pointer to the new property set or error pointer.
  */
-static struct property_set *pset_copy_set(const struct property_set *pset)
+static struct property_set *pset_create_set(const struct property_entry *src)
 {
 	struct property_entry *properties;
 	struct property_set *p;
@@ -465,7 +514,11 @@  static struct property_set *pset_copy_set(const struct property_set *pset)
 	if (!p)
 		return ERR_PTR(-ENOMEM);
 
-	properties = property_entries_dup(pset->properties);
+	INIT_LIST_HEAD(&p->child_node);
+	INIT_LIST_HEAD(&p->children);
+	p->fwnode.ops = &pset_fwnode_ops;
+
+	properties = property_entries_dup(src);
 	if (IS_ERR(properties)) {
 		kfree(p);
 		return ERR_CAST(properties);
@@ -521,20 +574,53 @@  EXPORT_SYMBOL_GPL(device_remove_properties);
 int device_add_properties(struct device *dev,
 			  const struct property_entry *properties)
 {
-	struct property_set *p, pset;
+	struct property_set *p;
 
 	if (!properties)
 		return -EINVAL;
 
-	pset.properties = properties;
-
-	p = pset_copy_set(&pset);
+	p = pset_create_set(properties);
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
-	p->fwnode.ops = &pset_fwnode_ops;
 	set_secondary_fwnode(dev, &p->fwnode);
 	p->dev = dev;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(device_add_properties);
+
+/**
+ * device_add_child_properties - Add a collection of properties to a device object.
+ * @dev: Device to add properties to.
+ * @properties: Collection of properties to add.
+ *
+ * Associate a collection of device properties represented by @properties as a
+ * child of given @parent firmware node.  The function takes a copy of
+ * @properties.
+ */
+struct fwnode_handle *
+device_add_child_properties(struct device *dev,
+			    struct fwnode_handle *parent,
+			    const struct property_entry *properties)
+{
+	struct property_set *p;
+	struct property_set *parent_pset;
+
+	if (!properties)
+		return ERR_PTR(-EINVAL);
+
+	parent_pset = to_pset_node(parent);
+	if (!parent_pset)
+		return ERR_PTR(-EINVAL);
+
+	p = pset_create_set(properties);
+	if (IS_ERR(p))
+		return ERR_CAST(p);
+
+	p->dev = dev;
+	p->parent = parent_pset;
+	list_add_tail(&p->child_node, &parent_pset->children);
+
+	return &p->fwnode;
+}
+EXPORT_SYMBOL_GPL(device_add_child_properties);
diff --git a/include/linux/property.h b/include/linux/property.h
index ac8a1ebc4c1b..bb1cf4f30770 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -275,6 +275,10 @@  void property_entries_free(const struct property_entry *properties);
 
 int device_add_properties(struct device *dev,
 			  const struct property_entry *properties);
+struct fwnode_handle *
+device_add_child_properties(struct device *dev,
+			    struct fwnode_handle *parent,
+			    const struct property_entry *properties);
 void device_remove_properties(struct device *dev);
 
 bool device_dma_supported(struct device *dev);