Re: [PATCH v2 3/3] leds: rgb: leds-ktd202x: Skip regulator settings for Xiaomi pad2

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

On Sat, 17 Feb 2024, Kate Hsuan wrote:

> The controller is already powered by BP25890RTWR on Xiaomi Pad2 so the
> regulator settings can be ignored.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  drivers/leds/rgb/leds-ktd202x.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
> index 8eb79c342fb6..6fd0794988e9 100644
> --- a/drivers/leds/rgb/leds-ktd202x.c
> +++ b/drivers/leds/rgb/leds-ktd202x.c
> @@ -14,7 +14,9 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/regmap.h>
> +#ifndef CONFIG_ACPI
>  #include <linux/regulator/consumer.h>
> +#endif

Why you need #ifndef here?
  
>  #define KTD2026_NUM_LEDS 3
>  #define KTD2027_NUM_LEDS 4
> @@ -105,18 +107,22 @@ struct ktd202x {
>  
>  static int ktd202x_chip_disable(struct ktd202x *chip)
>  {
> +#ifndef CONFIG_ACPI
>  	int ret;
> +#endif
>  
>  	if (!chip->enabled)
>  		return 0;
>  
>  	regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_SLEEP);
>  
> +#ifndef CONFIG_ACPI
>  	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
>  	if (ret) {
>  		dev_err(chip->dev, "Failed to disable regulators: %d\n", ret);
>  		return ret;
>  	}
> +#endif
>  
>  	chip->enabled = false;
>  	return 0;
> @@ -129,11 +135,13 @@ static int ktd202x_chip_enable(struct ktd202x *chip)
>  	if (chip->enabled)
>  		return 0;
>  
> +#ifndef CONFIG_ACPI
>  	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators), chip->regulators);
>  	if (ret) {
>  		dev_err(chip->dev, "Failed to enable regulators: %d\n", ret);
>  		return ret;
>  	}
> +#endif
>  	chip->enabled = true;
>  
>  	ret = regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_ENABLE_CTRL_WAKE);
> @@ -560,6 +568,7 @@ static int ktd202x_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> +#ifndef CONFIG_ACPI
>  	chip->regulators[0].supply = "vin";
>  	chip->regulators[1].supply = "vio";
>  	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(chip->regulators), chip->regulators);
> @@ -573,10 +582,12 @@ static int ktd202x_probe(struct i2c_client *client)
>  		dev_err_probe(dev, ret, "Failed to enable regulators.\n");
>  		return ret;
>  	}
> +#endif
>  
>  	chip->num_leds = (int) (unsigned long)i2c_get_match_data(client);
>  
>  	ret = ktd202x_probe_dt(chip);
> +#ifndef CONFIG_ACPI
>  	if (ret < 0) {
>  		regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
>  		return ret;
> @@ -587,6 +598,10 @@ static int ktd202x_probe(struct i2c_client *client)
>  		dev_err_probe(dev, ret, "Failed to disable regulators.\n");
>  		return ret;
>  	}
> +#else
> +	if (ret < 0)
> +		return ret;
> +#endif
>  
>  	mutex_init(&chip->mutex);

To me this entire approach looks quite ugly. It would be much cleaner to 
have something along these lines:

#ifndef CONFIG_ACPI
static int ktd202x_regulators_disable(struct ktd202x *chip)
{
	int ret;

	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
	if (ret)
		dev_err(chip->dev, "Failed to disable regulators: %d\n", ret);

	return ret;
}
...
#else
static inline int ktd202x_regulators_disable(struct ktd202x *chip) { return 0; }
...
#endif

And call that function without any #ifdefs from the other code.

-- 
 i.