Re: [PATCH] clk: si5341: disable input clock on probe failure and remove

[Mike Looijmans]

On 6/26/26 11:33, Myeonghun Pak wrote:
> si5341_clk_select_active_input() prepares and enables the selected
> input clock, but the driver never disables it. The devm-managed clock
> lookup only releases the clock handle; it does not unwind the
> prepare/enable count.
>
> Track the input clock that was successfully prepared and disable it
> from both the post-selection probe error path and the remove path.
>
> Fixes: 3044a860fd09 ("clk: Add Si5341/Si5340 driver")
> Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> ---
>   drivers/clk/clk-si5341.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c
> index 2499b77..14f80a5 100644
> --- a/drivers/clk/clk-si5341.c
> +++ b/drivers/clk/clk-si5341.c
> @@ -74,6 +74,7 @@ struct clk_si5341 {
>   	struct clk_si5341_output clk[SI5341_MAX_NUM_OUTPUTS];
>   	struct clk *input_clk[SI5341_NUM_INPUTS];
>   	const char *input_clk_name[SI5341_NUM_INPUTS];
> +	int prepared_input;

I don't think you need this here. At the points where you want to disable the 
clock, you already know that the clock has been enabled. For example, you 
cannot reach the "remove" function without having it enabled. So in remove, 
just disable the input clock unconditionally.


>   	const u16 *reg_output_offset;
>   	const u16 *reg_rdiv_offset;
>   	u64 freq_vco; /* 13500–14256 MHz */
> @@ -1463,9 +1464,20 @@ static int si5341_clk_select_active_input(struct clk_si5341 *data)
>   	if (err < 0)
>   		return err;
>   
> +	data->prepared_input = res;
> +
>   	return res;
>   }
>   
> +static void si5341_clk_disable_active_input(struct clk_si5341 *data)
> +{
> +	if (data->prepared_input < 0)
> +		return;
> +
> +	clk_disable_unprepare(data->input_clk[data->prepared_input]);
> +	data->prepared_input = -1;
> +}
> +
>   static ssize_t input_present_show(struct device *dev,
>   				  struct device_attribute *attr,
>   				  char *buf)
> @@ -1571,6 +1583,7 @@ static int si5341_probe(struct i2c_client *client)
>   	if (!data)
>   		return -ENOMEM;
>   
> +	data->prepared_input = -1;
>   	data->i2c_client = client;
>   
>   	/* Must be done before otherwise touching hardware */
> @@ -1803,6 +1816,7 @@ static int si5341_probe(struct i2c_client *client)
>   
>   cleanup:
>   	if (err) {
> +		si5341_clk_disable_active_input(data);
>   		for (i = 0; i < SI5341_MAX_NUM_OUTPUTS; ++i) {
>   			if (data->clk[i].vddo_reg)
>   				regulator_disable(data->clk[i].vddo_reg);
> @@ -1818,6 +1832,8 @@ static void si5341_remove(struct i2c_client *client)
>   
>   	sysfs_remove_files(&client->dev.kobj, si5341_attributes);
>   
> +	si5341_clk_disable_active_input(data);
> +
>   	for (i = 0; i < SI5341_MAX_NUM_OUTPUTS; ++i) {
>   		if (data->clk[i].vddo_reg)
>   			regulator_disable(data->clk[i].vddo_reg);

--

Mike.