Re: [PATCH 2/6] hwmon: (ltc2992) use device_for_each_child_node_scoped() to access child nodes

Jonathan Cameron
2024-07-07T16:55:43Z

On Sat, 06 Jul 2024 17:23:34 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The iterated nodes are direct children of the device node, and the
> `device_for_each_child_node()` macro accounts for child node
> availability.
>
> `fwnode_for_each_available_child_node()` is meant to access the child
> nodes of an fwnode, and therefore not direct child nodes of the device
> node.
>
> In this case, the child nodes are not required outside the loop, and
> the scoped version of the macro can be used to remove the repetitive
> `goto put` pattern.
>
> Use `device_for_each_child_node_scoped_scoped()` to indicate device's
> direct child nodes.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
One thing that is possibly a follow up suggestion but could maybe
be done in this patch. Either way.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> drivers/hwmon/ltc2992.c | 19 +++++--------------
> 1 file changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
> index d4a93223cd3b..541fa09dc6e7 100644
> --- a/drivers/hwmon/ltc2992.c
> +++ b/drivers/hwmon/ltc2992.c
> @@ -854,33 +854,24 @@ static const struct regmap_config ltc2992_regmap_config = {
>
> static int ltc2992_parse_dt(struct ltc2992_state *st)
> {
> - struct fwnode_handle *fwnode;
> - struct fwnode_handle *child;

I'd take the opportunity for
struct device *dev = &st->client->dev;

Given there are several users and this modifies one of them.

> u32 addr;
> u32 val;
> int ret;
>
> - fwnode = dev_fwnode(&st->client->dev);
> -
> - fwnode_for_each_available_child_node(fwnode, child) {
> + device_for_each_child_node_scoped(&st->client->dev, child) {
> ret = fwnode_property_read_u32(child, "reg", &addr);
> - if (ret < 0) {
> - fwnode_handle_put(child);
> + if (ret < 0)
> return ret;
> - }
>
> - if (addr > 1) {
> - fwnode_handle_put(child);
> + if (addr > 1)
> return -EINVAL;
> - }
>
> ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", &val);
> if (!ret) {
> - if (!val) {
> - fwnode_handle_put(child);
> + if (!val)
> return dev_err_probe(&st->client->dev, -EINVAL,
> "shunt resistor value cannot be zero\n");
> - }
> +
> st->r_sense_uohm[addr] = val;
> }
> }
>