| Message ID | 20240216160526.235594-4-hpa@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | KTD2026 indicator LED for X86 Xiaomi Pad2 | expand |
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.
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@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 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
Hi Hans and llpo, On Mon, Feb 19, 2024 at 10:04 PM Hans de Goede <hdegoede@redhat.com> 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@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 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
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 #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);
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(+)