Re: [RFC PATCH 1/2] regulator: Add support for software node connections

[Mark Brown]

On Thu, Jul 08, 2021 at 11:42:25PM +0100, Daniel Scally wrote:

> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_OF) += of_regulator.o
>  obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
>  obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
>  obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
> +obj-$(CONFIG_REGULATOR_SWNODE) += swnode_regulator.o

This appears to be sorted with regulator drivers but as far as I
understand it this is not a driver?  I'd put it next to the OF file.

> @@ -1785,6 +1788,22 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,

The line numbers here look off...

> +++ b/drivers/regulator/swnode_regulator.c
> @@ -0,0 +1,111 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally <djrscally@xxxxxxxxx> */
> +

Please make the entire comment a C++ one so things look more
intentional.

> +static struct fwnode_handle *
> +regulator_swnode_get_init_node(struct fwnode_handle *fwnode,
> +			       const struct regulator_desc *desc)
> +{
> +	const struct software_node *parent, *child;
> +
> +	parent = to_software_node(fwnode->secondary);
> +
> +	if (desc->regulators_node)
> +		child = software_node_find_by_name(parent,
> +						   desc->regulators_node);
> +	else
> +		child = software_node_find_by_name(parent, desc->name);
> +
> +	return software_node_fwnode(child);
> +}

Nothing is documenting what the binding for these swnodes is supposed to
be so it's hard to tell if any of this is correct and makes sense, nor
how someone is supposed to take this stuff and integrate it into a
system.  I think this needs some more explicit documentation of what's
going on adding to the tree, this will help with review and help anyone
who needs to use this stuff figure out how to do so.

Attachment: signature.asc
Description: PGP signature