Re: [PATCH v4 1/9] platform/x86: int3472: Add skl_int3472_register_clock() helper

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3853 bytes --]

On Thu, 17 Apr 2025, Hans de Goede wrote:

> skl_int3472_register_dsm_clock() and skl_int3472_register_gpio_clock() are
> 80% the same code. Factor out the common code into a new
> skl_int3472_register_clock() helper.
> 
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../x86/intel/int3472/clk_and_regulator.c     | 57 +++++--------------
>  1 file changed, 13 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> index 16e36ac0a7b4..837990af24fe 100644
> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
> @@ -118,7 +118,7 @@ static const struct clk_ops skl_int3472_clock_ops = {
>  	.recalc_rate = skl_int3472_clk_recalc_rate,
>  };
>  
> -int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472)
> +static int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
>  {
>  	struct acpi_device *adev = int3472->adev;
>  	struct clk_init_data init = {
> @@ -127,12 +127,6 @@ int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472)
>  	};
>  	int ret;
>  
> -	if (int3472->clock.cl)
> -		return 0; /* A GPIO controlled clk has already been registered */
> -
> -	if (!acpi_check_dsm(adev->handle, &img_clk_guid, 0, BIT(1)))
> -		return 0; /* DSM clock control is not available */
> -
>  	init.name = kasprintf(GFP_KERNEL, "%s-clk", acpi_dev_name(adev));
>  	if (!init.name)
>  		return -ENOMEM;
> @@ -161,51 +155,26 @@ int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472)
>  	return ret;
>  }
>  
> +int skl_int3472_register_dsm_clock(struct int3472_discrete_device *int3472)
> +{
> +	if (int3472->clock.cl)
> +		return 0; /* A GPIO controlled clk has already been registered */
> +
> +	if (!acpi_check_dsm(int3472->adev->handle, &img_clk_guid, 0, BIT(1)))
> +		return 0; /* DSM clock control is not available */
> +
> +	return skl_int3472_register_clock(int3472);
> +}
> +
>  int skl_int3472_register_gpio_clock(struct int3472_discrete_device *int3472,
>  				    struct gpio_desc *gpio)
>  {
> -	struct clk_init_data init = {
> -		.ops = &skl_int3472_clock_ops,
> -		.flags = CLK_GET_RATE_NOCACHE,
> -	};
> -	int ret;
> -
>  	if (int3472->clock.cl)
>  		return -EBUSY;
>  
>  	int3472->clock.ena_gpio = gpio;
>  
> -	init.name = kasprintf(GFP_KERNEL, "%s-clk",
> -			      acpi_dev_name(int3472->adev));
> -	if (!init.name)
> -		return -ENOMEM;
> -
> -	int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
> -
> -	int3472->clock.clk_hw.init = &init;
> -	int3472->clock.clk = clk_register(&int3472->adev->dev,
> -					  &int3472->clock.clk_hw);
> -	if (IS_ERR(int3472->clock.clk)) {
> -		ret = PTR_ERR(int3472->clock.clk);
> -		goto out_free_init_name;
> -	}
> -
> -	int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
> -					  int3472->sensor_name);
> -	if (!int3472->clock.cl) {
> -		ret = -ENOMEM;
> -		goto err_unregister_clk;
> -	}
> -
> -	kfree(init.name);
> -	return 0;
> -
> -err_unregister_clk:
> -	clk_unregister(int3472->clock.clk);
> -out_free_init_name:
> -	kfree(init.name);
> -
> -	return ret;
> +	return skl_int3472_register_clock(int3472);
>  }
>  
>  void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

To get rid of a few kfree()s, one could consider this additional cleanup 
as a separate patch:

	char *name __free(kfree) = kasprintf(...);
	if (!name)
		return -ENOMEM;

	init.name = name;

(FYI, I won't have time to go through rest of the patch in this series 
today.)

-- 
 i.