Re: [PATCH v1 1/6] clk: rockchip: Use clk_hw_register_composite instead of clk_register_composite calls

[Robin Murphy <robin.murphy@arm.com>]

On 2020-09-02 07:48, Elaine Zhang wrote:
> clk_hw_register_composite it's already exported.
> Preparation for compilation of rK common clock drivers into modules.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
>   drivers/clk/rockchip/clk-half-divider.c | 12 +++++----
>   drivers/clk/rockchip/clk.c              | 35 ++++++++++++++-----------
>   2 files changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-half-divider.c b/drivers/clk/rockchip/clk-half-divider.c
> index b333fc28c94b..35db0651ea1d 100644
> --- a/drivers/clk/rockchip/clk-half-divider.c
> +++ b/drivers/clk/rockchip/clk-half-divider.c
> @@ -166,6 +166,7 @@ struct clk *rockchip_clk_register_halfdiv(const char *name,
>   					  unsigned long flags,
>   					  spinlock_t *lock)
>   {
> +	struct clk_hw *hw;
>   	struct clk *clk;
>   	struct clk_mux *mux = NULL;
>   	struct clk_gate *gate = NULL;
> @@ -212,12 +213,13 @@ struct clk *rockchip_clk_register_halfdiv(const char *name,
>   		div_ops = &clk_half_divider_ops;
>   	}
>   
> -	clk = clk_register_composite(NULL, name, parent_names, num_parents,
> -				     mux ? &mux->hw : NULL, mux_ops,
> -				     div ? &div->hw : NULL, div_ops,
> -				     gate ? &gate->hw : NULL, gate_ops,
> -				     flags);
> +	hw = clk_hw_register_composite(NULL, name, parent_names, num_parents,
> +				       mux ? &mux->hw : NULL, mux_ops,
> +				       div ? &div->hw : NULL, div_ops,
> +				       gate ? &gate->hw : NULL, gate_ops,
> +				       flags);
>   
> +	clk = hw->clk;
>   	return clk;

Nit: there's really no point keeping the "clk" variable here, you could 
simply "return hw->clk" if registration succeeds - note that you also 
need the rest of the logic from clk_register_composite() to check that 
"hw" isn't an error value.

>   err_div:
>   	kfree(gate);
> diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
> index 546e810c3560..2cfebfb61814 100644
> --- a/drivers/clk/rockchip/clk.c
> +++ b/drivers/clk/rockchip/clk.c
> @@ -43,6 +43,7 @@ static struct clk *rockchip_clk_register_branch(const char *name,
>   		u8 gate_shift, u8 gate_flags, unsigned long flags,
>   		spinlock_t *lock)
>   {
> +	struct clk_hw *hw;
>   	struct clk *clk;
>   	struct clk_mux *mux = NULL;
>   	struct clk_gate *gate = NULL;
> @@ -100,12 +101,12 @@ static struct clk *rockchip_clk_register_branch(const char *name,
>   						: &clk_divider_ops;
>   	}
>   
> -	clk = clk_register_composite(NULL, name, parent_names, num_parents,
> -				     mux ? &mux->hw : NULL, mux_ops,
> -				     div ? &div->hw : NULL, div_ops,
> -				     gate ? &gate->hw : NULL, gate_ops,
> -				     flags);
> -
> +	hw = clk_hw_register_composite(NULL, name, parent_names, num_parents,
> +				       mux ? &mux->hw : NULL, mux_ops,
> +				       div ? &div->hw : NULL, div_ops,
> +				       gate ? &gate->hw : NULL, gate_ops,
> +				       flags);
> +	clk = hw->clk;
>   	if (IS_ERR(clk)) {

Similar to above, this is totally broken - you need to rework all the 
error handling in terms of "hw" rather than "clk" - dereferencing an 
ERR_PTR value does not yield another ERR_PTR value, it yields a crash ;)

Robin.

>   		ret = PTR_ERR(clk);
>   		goto err_composite;
> @@ -214,6 +215,7 @@ static struct clk *rockchip_clk_register_frac_branch(
>   		unsigned long flags, struct rockchip_clk_branch *child,
>   		spinlock_t *lock)
>   {
> +	struct clk_hw *hw;
>   	struct rockchip_clk_frac *frac;
>   	struct clk *clk;
>   	struct clk_gate *gate = NULL;
> @@ -255,11 +257,12 @@ static struct clk *rockchip_clk_register_frac_branch(
>   	div->approximation = rockchip_fractional_approximation;
>   	div_ops = &clk_fractional_divider_ops;
>   
> -	clk = clk_register_composite(NULL, name, parent_names, num_parents,
> -				     NULL, NULL,
> -				     &div->hw, div_ops,
> -				     gate ? &gate->hw : NULL, gate_ops,
> -				     flags | CLK_SET_RATE_UNGATE);
> +	hw = clk_hw_register_composite(NULL, name, parent_names, num_parents,
> +				       NULL, NULL,
> +				       &div->hw, div_ops,
> +				       gate ? &gate->hw : NULL, gate_ops,
> +				       flags | CLK_SET_RATE_UNGATE);
> +	clk = hw->clk;
>   	if (IS_ERR(clk)) {
>   		kfree(frac);
>   		return clk;
> @@ -320,6 +323,7 @@ static struct clk *rockchip_clk_register_factor_branch(const char *name,
>   		int gate_offset, u8 gate_shift, u8 gate_flags,
>   		unsigned long flags, spinlock_t *lock)
>   {
> +	struct clk_hw *hw;
>   	struct clk *clk;
>   	struct clk_gate *gate = NULL;
>   	struct clk_fixed_factor *fix = NULL;
> @@ -349,10 +353,11 @@ static struct clk *rockchip_clk_register_factor_branch(const char *name,
>   	fix->mult = mult;
>   	fix->div = div;
>   
> -	clk = clk_register_composite(NULL, name, parent_names, num_parents,
> -				     NULL, NULL,
> -				     &fix->hw, &clk_fixed_factor_ops,
> -				     &gate->hw, &clk_gate_ops, flags);
> +	hw = clk_hw_register_composite(NULL, name, parent_names, num_parents,
> +				       NULL, NULL,
> +				       &fix->hw, &clk_fixed_factor_ops,
> +				       &gate->hw, &clk_gate_ops, flags);
> +	clk = hw->clk;
>   	if (IS_ERR(clk)) {
>   		kfree(fix);
>   		kfree(gate);
>