Hi Kate, Ilpo,
On 2/19/24 14:28, Ilpo Järvinen wrote:
> 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 believe that skipping the regulator stuff in the ACPI case is not
the right solution here.
There likely is some underlying issue which also happens on non ACPI
hw, but I guess no-one has ever tried to remove the module there.
I have the same tablet as on which Kate is testing this. So I plan
to make some time to reproduce this and see if I can come up with
a proper fix.
Regards,
Hans
