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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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






[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux