| Message ID | 20230320175150.174711-1-linmengbo0689@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: aw2013: Add vddio regulator | expand |
Hi! > Some LEDs controllers are used with external pull-up for the interrupt > line and the I2C lines, so we might need to enable a regulator to bring > the lines into usable state. Otherwise, this might cause spurious > interrupts and reading from I2C will fail. > > Implement support for "vddio-supply" that is enabled by the aw2013 driver > so that the regulator gets enabled when needed. > > Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com> > @@ -348,16 +350,20 @@ static int aw2013_probe(struct i2c_client *client) > goto error; > } > > - chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc"); > - ret = PTR_ERR_OR_ZERO(chip->vcc_regulator); > - if (ret) { > + chip->regulators[0].supply = "vcc"; > + chip->regulators[1].supply = "vddio"; > + ret = devm_regulator_bulk_get(&client->dev, > + ARRAY_SIZE(chip->regulators), > + chip->regulators); > + if (ret < 0) { > if (ret != -EPROBE_DEFER) > dev_err(&client->dev, > "Failed to request regulator: %d\n", ret); > goto error; Won't this cause failures when optional vddio is unavailable? BR, Pavel
Hi Pavel, On Thu, Mar 23, 2023 at 08:35:02PM +0100, Pavel Machek wrote: > > Some LEDs controllers are used with external pull-up for the interrupt > > line and the I2C lines, so we might need to enable a regulator to bring > > the lines into usable state. Otherwise, this might cause spurious > > interrupts and reading from I2C will fail. > > > > Implement support for "vddio-supply" that is enabled by the aw2013 driver > > so that the regulator gets enabled when needed. > > > > Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com> > > > @@ -348,16 +350,20 @@ static int aw2013_probe(struct i2c_client *client) > > goto error; > > } > > > > - chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc"); > > - ret = PTR_ERR_OR_ZERO(chip->vcc_regulator); > > - if (ret) { > > + chip->regulators[0].supply = "vcc"; > > + chip->regulators[1].supply = "vddio"; > > + ret = devm_regulator_bulk_get(&client->dev, > > + ARRAY_SIZE(chip->regulators), > > + chip->regulators); > > + if (ret < 0) { > > if (ret != -EPROBE_DEFER) > > dev_err(&client->dev, > > "Failed to request regulator: %d\n", ret); > > goto error; > > Won't this cause failures when optional vddio is unavailable? > The regulator core should print a warning "supply vddio not found, using dummy regulator" but then proceed normally. I think in almost all cases a separate I/O supply should actually exist that could be described in the device tree. It was likely just omitted because it's always-on or indirectly being enabled by other devices. So perhaps having this warning is even a good thing? Thanks, Stephan
On Freitag, 24. März 2023 09:32:52 CEST Stephan Gerhold wrote: > Hi Pavel, > > On Thu, Mar 23, 2023 at 08:35:02PM +0100, Pavel Machek wrote: > > > Some LEDs controllers are used with external pull-up for the interrupt > > > line and the I2C lines, so we might need to enable a regulator to bring > > > the lines into usable state. Otherwise, this might cause spurious > > > interrupts and reading from I2C will fail. > > > > > > Implement support for "vddio-supply" that is enabled by the aw2013 > > > driver > > > so that the regulator gets enabled when needed. > > > > > > Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com> > > > > > > @@ -348,16 +350,20 @@ static int aw2013_probe(struct i2c_client *client) > > > > > > goto error; > > > > > > } > > > > > > - chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc"); > > > - ret = PTR_ERR_OR_ZERO(chip->vcc_regulator); > > > - if (ret) { > > > + chip->regulators[0].supply = "vcc"; > > > + chip->regulators[1].supply = "vddio"; > > > + ret = devm_regulator_bulk_get(&client->dev, > > > + ARRAY_SIZE(chip->regulators), > > > + chip->regulators); > > > + if (ret < 0) { > > > > > > if (ret != -EPROBE_DEFER) > > > > > > dev_err(&client->dev, > > > > > > "Failed to request regulator: %d\n", ret); > > > > > > goto error; > > > > Won't this cause failures when optional vddio is unavailable? > > The regulator core should print a warning "supply vddio not found, using > dummy regulator" but then proceed normally. > > I think in almost all cases a separate I/O supply should actually exist > that could be described in the device tree. It was likely just omitted > because it's always-on or indirectly being enabled by other devices. > So perhaps having this warning is even a good thing? Just briefly jumping in, there was some activity adding bus_regulator to the i2c-core a while back, maybe that can be revived instead? For CCI (camera i2c) we also need pull-ups and I don't think adding vddio or whatever to all sensors is a good idea long term... https://lore.kernel.org/lkml/20210527075556.1709140-1-hsinyi@chromium.org/ Regards Luca > > Thanks, > Stephan
diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c index 0b52fc9097c6..95f2f9bf95ee 100644 --- a/drivers/leds/leds-aw2013.c +++ b/drivers/leds/leds-aw2013.c @@ -62,7 +62,7 @@ struct aw2013_led { struct aw2013 { struct mutex mutex; /* held when writing to registers */ - struct regulator *vcc_regulator; + struct regulator_bulk_data regulators[2]; struct i2c_client *client; struct aw2013_led leds[AW2013_MAX_LEDS]; struct regmap *regmap; @@ -106,7 +106,8 @@ static void aw2013_chip_disable(struct aw2013 *chip) regmap_write(chip->regmap, AW2013_GCR, 0); - ret = regulator_disable(chip->vcc_regulator); + ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), + chip->regulators); if (ret) { dev_err(&chip->client->dev, "Failed to disable regulator: %d\n", ret); @@ -123,7 +124,8 @@ static int aw2013_chip_enable(struct aw2013 *chip) if (chip->enabled) return 0; - ret = regulator_enable(chip->vcc_regulator); + ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators), + chip->regulators); if (ret) { dev_err(&chip->client->dev, "Failed to enable regulator: %d\n", ret); @@ -348,16 +350,20 @@ static int aw2013_probe(struct i2c_client *client) goto error; } - chip->vcc_regulator = devm_regulator_get(&client->dev, "vcc"); - ret = PTR_ERR_OR_ZERO(chip->vcc_regulator); - if (ret) { + chip->regulators[0].supply = "vcc"; + chip->regulators[1].supply = "vddio"; + ret = devm_regulator_bulk_get(&client->dev, + ARRAY_SIZE(chip->regulators), + chip->regulators); + if (ret < 0) { if (ret != -EPROBE_DEFER) dev_err(&client->dev, "Failed to request regulator: %d\n", ret); goto error; } - ret = regulator_enable(chip->vcc_regulator); + ret = regulator_bulk_enable(ARRAY_SIZE(chip->regulators), + chip->regulators); if (ret) { dev_err(&client->dev, "Failed to enable regulator: %d\n", ret); @@ -382,7 +388,8 @@ static int aw2013_probe(struct i2c_client *client) if (ret < 0) goto error_reg; - ret = regulator_disable(chip->vcc_regulator); + ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators), + chip->regulators); if (ret) { dev_err(&client->dev, "Failed to disable regulator: %d\n", ret); @@ -394,7 +401,8 @@ static int aw2013_probe(struct i2c_client *client) return 0; error_reg: - regulator_disable(chip->vcc_regulator); + regulator_bulk_disable(ARRAY_SIZE(chip->regulators), + chip->regulators); error: mutex_destroy(&chip->mutex);
Some LEDs controllers are used with external pull-up for the interrupt line and the I2C lines, so we might need to enable a regulator to bring the lines into usable state. Otherwise, this might cause spurious interrupts and reading from I2C will fail. Implement support for "vddio-supply" that is enabled by the aw2013 driver so that the regulator gets enabled when needed. Signed-off-by: Lin, Meng-Bo <linmengbo0689@protonmail.com> --- drivers/leds/leds-aw2013.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)