Hi Hans and llpo,
On Mon, Feb 19, 2024 at 10:04 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> 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
>
Thank you for reviewing it.
This patch is used to prevent the WARN_ON() shown in the following URL.
https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L2396
I'll drop this patch in the v3 patch. And I can also try to
investigate the issue of the regulator.
--
BR,
Kate
