On 17.10.2023 17:06, Dan Carpenter wrote:
> This code checks "if (parent_hw)" is non-NULL, but then it has more
> checks if parent_hw is non-NULL on the lines inside the if statement.
> It is a bit confusing.
>
> For the else statement, keep in mind that at the start of the function
> we checked:
>
> if (!(parent_name || parent_hw))
> return ERR_PTR(-EINVAL);
>
> That check ensures that if parent_hw is NULL that means that parent_name
> is non-NULL. At least one must always be non-NULL. So here again, the
> checks inside the if statement can be removed.
>
> In the original code, it was a bit confusing and you could easily get
> the impression that "init.num_parents" could be zero. When we remove
> the unnecessary checking it's more obvious that it's always set to 1.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
I had something similar in my queue to submit, but didn't manage to do it
yet. Thanks for taking care of it.
Reviewed-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxx>
> ---
> drivers/clk/at91/clk-utmi.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/at91/clk-utmi.c b/drivers/clk/at91/clk-utmi.c
> index 40c84f5af5e8..b991180beea1 100644
> --- a/drivers/clk/at91/clk-utmi.c
> +++ b/drivers/clk/at91/clk-utmi.c
> @@ -161,13 +161,11 @@ at91_clk_register_utmi_internal(struct regmap *regmap_pmc,
>
> init.name = name;
> init.ops = ops;
> - if (parent_hw) {
> - init.parent_hws = parent_hw ? (const struct clk_hw **)&parent_hw : NULL;
> - init.num_parents = parent_hw ? 1 : 0;
> - } else {
> - init.parent_names = parent_name ? &parent_name : NULL;
> - init.num_parents = parent_name ? 1 : 0;
> - }
> + if (parent_hw)
> + init.parent_hws = (const struct clk_hw **)&parent_hw;
> + else
> + init.parent_names = &parent_name;
> + init.num_parents = 1;
> init.flags = flags;
>
> utmi->hw.init = &init;
