Hi Hans,
Thanks for the patch.
On Fri, Jun 09, 2023 at 11:43:14AM +0200, Hans Verkuil wrote:
> The child device_node pointer was used for two different childs.
> This confused smatch, causing this warning:
>
> drivers/media/i2c/adp1653.c:444 adp1653_of_init() warn: missing unwind goto?
>
> Use two different pointers, one for each child node, and add separate
> goto labels for each node as well. This also improves error logging
> since it will now state for which node the property was missing.
It would have been better to fix smatch. :-(
I guess changing the driver isn't wrong either still.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> ---
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 98ca417b8004..04ec465aaa94 100644
> --- a/drivers/media/i2c/adp1653.c
> +++ b/drivers/media/i2c/adp1653.c
> @@ -411,43 +411,43 @@ static int adp1653_of_init(struct i2c_client *client,
> struct device_node *node)
> {
> struct adp1653_platform_data *pd;
> - struct device_node *child;
> + struct device_node *node_flash, *node_indicator;
>
> pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
> if (!pd)
> return -ENOMEM;
> flash->platform_data = pd;
>
> - child = of_get_child_by_name(node, "flash");
> - if (!child)
> + node_flash = of_get_child_by_name(node, "flash");
> + if (!node_flash)
> return -EINVAL;
>
> - if (of_property_read_u32(child, "flash-timeout-us",
> + if (of_property_read_u32(node_flash, "flash-timeout-us",
> &pd->max_flash_timeout))
> - goto err;
> + goto err_flash;
>
> - if (of_property_read_u32(child, "flash-max-microamp",
> + if (of_property_read_u32(node_flash, "flash-max-microamp",
> &pd->max_flash_intensity))
> - goto err;
> + goto err_flash;
>
> pd->max_flash_intensity /= 1000;
>
> - if (of_property_read_u32(child, "led-max-microamp",
> + if (of_property_read_u32(node_flash, "led-max-microamp",
> &pd->max_torch_intensity))
> - goto err;
> + goto err_flash;
>
> pd->max_torch_intensity /= 1000;
> - of_node_put(child);
> + of_node_put(node_flash);
How about moving this to where the other node is put, and initialise the
nodes to NULL and so continue having a single error label?
>
> - child = of_get_child_by_name(node, "indicator");
> - if (!child)
> + node_indicator = of_get_child_by_name(node, "indicator");
> + if (!node_indicator)
> return -EINVAL;
>
> - if (of_property_read_u32(child, "led-max-microamp",
> + if (of_property_read_u32(node_indicator, "led-max-microamp",
> &pd->max_indicator_intensity))
> - goto err;
> + goto err_indicator;
>
> - of_node_put(child);
> + of_node_put(node_indicator);
>
> pd->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_LOW);
> if (IS_ERR(pd->enable_gpio)) {
> @@ -456,9 +456,13 @@ static int adp1653_of_init(struct i2c_client *client,
> }
>
> return 0;
> -err:
> - dev_err(&client->dev, "Required property not found\n");
> - of_node_put(child);
> +err_flash:
> + dev_err(&client->dev, "Required flash property not found\n");
> + of_node_put(node_flash);
> + return -EINVAL;
> +err_indicator:
> + dev_err(&client->dev, "Required indicator property not found\n");
> + of_node_put(node_indicator);
> return -EINVAL;
> }
>
--
Kind regards,
Sakari Ailus
