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@xxxxxxxxxx>
> ---
> 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.
