| Message ID | 20250521-dev-adp5589-fw-v4-3-f2c988d7a7a0@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: adp5585: support keymap events and drop legacy Input driver | expand |
On Wed, 21 May 2025, Nuno Sá via B4 Relay wrote: > From: Nuno Sá <nuno.sa@analog.com> > > Make sure to enable the oscillator in the top device. This will allow to > not control this in the child PWM device as that would not work with > future support for keyboard matrix where the oscillator needs to be > always enabled (and so cannot be disabled by disabling PWM). > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/mfd/adp5585.c | 23 +++++++++++++++++++++++ > drivers/pwm/pwm-adp5585.c | 5 ----- > 2 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > index 806867c56d6fb4ef1f461af26a424a3a05f46575..f3b74f7d6040413d066eb6dbaecfa3d5e6ee06bd 100644 > --- a/drivers/mfd/adp5585.c > +++ b/drivers/mfd/adp5585.c > @@ -147,6 +147,13 @@ static int adp5585_add_devices(struct device *dev) > return ret; > } > > +static void adp5585_osc_disable(void *data) > +{ > + const struct adp5585_dev *adp5585 = data; > + > + regmap_write(adp5585->regmap, ADP5585_GENERAL_CFG, 0); > +} > + > static int adp5585_i2c_probe(struct i2c_client *i2c) > { > const struct regmap_config *regmap_config; > @@ -175,6 +182,22 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > return dev_err_probe(&i2c->dev, -ENODEV, > "Invalid device ID 0x%02x\n", id); > > + /* > + * Enable the internal oscillator, as it's shared between multiple > + * functions. > + * > + * As a future improvement, power consumption could possibly be > + * decreased in some use cases by enabling and disabling the oscillator > + * dynamically based on the needs of the child drivers. This is normal. What's stopping us from doing this from the offset? > + */ > + ret = regmap_set_bits(adp5585->regmap, ADP5585_GENERAL_CFG, ADP5585_OSC_EN); > + if (ret) > + return ret; > + > + ret = devm_add_action_or_reset(&i2c->dev, adp5585_osc_disable, adp5585); > + if (ret) > + return ret; > + > return adp5585_add_devices(&i2c->dev); > } > > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c > index 40472ac5db6410a33e4f790fe8e6c23b517502be..c8821035b7c1412a55a642e6e8a46b66e693a5af 100644 > --- a/drivers/pwm/pwm-adp5585.c > +++ b/drivers/pwm/pwm-adp5585.c > @@ -62,7 +62,6 @@ static int pwm_adp5585_apply(struct pwm_chip *chip, > int ret; > > if (!state->enabled) { > - regmap_clear_bits(regmap, ADP5585_GENERAL_CFG, ADP5585_OSC_EN); > regmap_clear_bits(regmap, ADP5585_PWM_CFG, ADP5585_PWM_EN); > return 0; > } > @@ -100,10 +99,6 @@ static int pwm_adp5585_apply(struct pwm_chip *chip, > if (ret) > return ret; > > - ret = regmap_set_bits(regmap, ADP5585_GENERAL_CFG, ADP5585_OSC_EN); > - if (ret) > - return ret; > - > return regmap_set_bits(regmap, ADP5585_PWM_CFG, ADP5585_PWM_EN); > } > > > -- > 2.49.0 > >
On Thu, 2025-06-12 at 15:20 +0100, Lee Jones wrote: > On Wed, 21 May 2025, Nuno Sá via B4 Relay wrote: > > > From: Nuno Sá <nuno.sa@analog.com> > > > > Make sure to enable the oscillator in the top device. This will allow to > > not control this in the child PWM device as that would not work with > > future support for keyboard matrix where the oscillator needs to be > > always enabled (and so cannot be disabled by disabling PWM). > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/mfd/adp5585.c | 23 +++++++++++++++++++++++ > > drivers/pwm/pwm-adp5585.c | 5 ----- > > 2 files changed, 23 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > index > > 806867c56d6fb4ef1f461af26a424a3a05f46575..f3b74f7d6040413d066eb6dbaecfa3d5e6 > > ee06bd 100644 > > --- a/drivers/mfd/adp5585.c > > +++ b/drivers/mfd/adp5585.c > > @@ -147,6 +147,13 @@ static int adp5585_add_devices(struct device *dev) > > return ret; > > } > > > > +static void adp5585_osc_disable(void *data) > > +{ > > + const struct adp5585_dev *adp5585 = data; > > + > > + regmap_write(adp5585->regmap, ADP5585_GENERAL_CFG, 0); > > +} > > + > > static int adp5585_i2c_probe(struct i2c_client *i2c) > > { > > const struct regmap_config *regmap_config; > > @@ -175,6 +182,22 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > return dev_err_probe(&i2c->dev, -ENODEV, > > "Invalid device ID 0x%02x\n", id); > > > > + /* > > + * Enable the internal oscillator, as it's shared between multiple > > + * functions. > > + * > > + * As a future improvement, power consumption could possibly be > > + * decreased in some use cases by enabling and disabling the > > oscillator > > + * dynamically based on the needs of the child drivers. > > This is normal. What's stopping us from doing this from the offset? This is always needed when we have the input device registered. From my testing, we also need it for GPIOs configured as input. So basically the only reason this is not being done now is that it would not be trivial or really straightforward and honestly the series is already big enough :) Laurent also agreed with this not being mandatory now so hopefully it's also fine with you. - Nuno Sá > > > + */ > > + ret = regmap_set_bits(adp5585->regmap, ADP5585_GENERAL_CFG, > > ADP5585_OSC_EN); > > + if (ret) > > + return ret; > > + > > + ret = devm_add_action_or_reset(&i2c->dev, adp5585_osc_disable, > > adp5585); > > + if (ret) > > + return ret; > > + > > return adp5585_add_devices(&i2c->dev); > > } > > > > diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c > > index > > 40472ac5db6410a33e4f790fe8e6c23b517502be..c8821035b7c1412a55a642e6e8a46b66e6 > > 93a5af 100644 > > --- a/drivers/pwm/pwm-adp5585.c > > +++ b/drivers/pwm/pwm-adp5585.c > > @@ -62,7 +62,6 @@ static int pwm_adp5585_apply(struct pwm_chip *chip, > > int ret; > > > > if (!state->enabled) { > > - regmap_clear_bits(regmap, ADP5585_GENERAL_CFG, > > ADP5585_OSC_EN); > > regmap_clear_bits(regmap, ADP5585_PWM_CFG, ADP5585_PWM_EN); > > return 0; > > } > > @@ -100,10 +99,6 @@ static int pwm_adp5585_apply(struct pwm_chip *chip, > > if (ret) > > return ret; > > > > - ret = regmap_set_bits(regmap, ADP5585_GENERAL_CFG, ADP5585_OSC_EN); > > - if (ret) > > - return ret; > > - > > return regmap_set_bits(regmap, ADP5585_PWM_CFG, ADP5585_PWM_EN); > > } > > > > > > -- > > 2.49.0 > > > >
On Thu, 12 Jun 2025, Nuno Sá wrote: > On Thu, 2025-06-12 at 15:20 +0100, Lee Jones wrote: > > On Wed, 21 May 2025, Nuno Sá via B4 Relay wrote: > > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > Make sure to enable the oscillator in the top device. This will allow to > > > not control this in the child PWM device as that would not work with > > > future support for keyboard matrix where the oscillator needs to be > > > always enabled (and so cannot be disabled by disabling PWM). > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > --- > > > drivers/mfd/adp5585.c | 23 +++++++++++++++++++++++ > > > drivers/pwm/pwm-adp5585.c | 5 ----- > > > 2 files changed, 23 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > > index > > > 806867c56d6fb4ef1f461af26a424a3a05f46575..f3b74f7d6040413d066eb6dbaecfa3d5e6 > > > ee06bd 100644 > > > --- a/drivers/mfd/adp5585.c > > > +++ b/drivers/mfd/adp5585.c > > > @@ -147,6 +147,13 @@ static int adp5585_add_devices(struct device *dev) > > > return ret; > > > } > > > > > > +static void adp5585_osc_disable(void *data) > > > +{ > > > + const struct adp5585_dev *adp5585 = data; > > > + > > > + regmap_write(adp5585->regmap, ADP5585_GENERAL_CFG, 0); > > > +} > > > + > > > static int adp5585_i2c_probe(struct i2c_client *i2c) > > > { > > > const struct regmap_config *regmap_config; > > > @@ -175,6 +182,22 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > > return dev_err_probe(&i2c->dev, -ENODEV, > > > "Invalid device ID 0x%02x\n", id); > > > > > > + /* > > > + * Enable the internal oscillator, as it's shared between multiple > > > + * functions. > > > + * > > > + * As a future improvement, power consumption could possibly be > > > + * decreased in some use cases by enabling and disabling the > > > oscillator > > > + * dynamically based on the needs of the child drivers. > > > > This is normal. What's stopping us from doing this from the offset? > > This is always needed when we have the input device registered. From my testing, > we also need it for GPIOs configured as input. So basically the only reason this > is not being done now is that it would not be trivial or really straightforward > and honestly the series is already big enough :) Agreed! > Laurent also agreed with this not being mandatory now so hopefully it's also > fine with you. If there is no explicit plan to do this in the future, you may as well remove the comment. TODOs have a tendency to rot after code is accepted.
On Thu, 2025-06-12 at 16:20 +0100, Lee Jones wrote: > On Thu, 12 Jun 2025, Nuno Sá wrote: > > > On Thu, 2025-06-12 at 15:20 +0100, Lee Jones wrote: > > > On Wed, 21 May 2025, Nuno Sá via B4 Relay wrote: > > > > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > > > Make sure to enable the oscillator in the top device. This will allow to > > > > not control this in the child PWM device as that would not work with > > > > future support for keyboard matrix where the oscillator needs to be > > > > always enabled (and so cannot be disabled by disabling PWM). > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > drivers/mfd/adp5585.c | 23 +++++++++++++++++++++++ > > > > drivers/pwm/pwm-adp5585.c | 5 ----- > > > > 2 files changed, 23 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > > > index > > > > 806867c56d6fb4ef1f461af26a424a3a05f46575..f3b74f7d6040413d066eb6dbaecfa3 > > > > d5e6 > > > > ee06bd 100644 > > > > --- a/drivers/mfd/adp5585.c > > > > +++ b/drivers/mfd/adp5585.c > > > > @@ -147,6 +147,13 @@ static int adp5585_add_devices(struct device *dev) > > > > return ret; > > > > } > > > > > > > > +static void adp5585_osc_disable(void *data) > > > > +{ > > > > + const struct adp5585_dev *adp5585 = data; > > > > + > > > > + regmap_write(adp5585->regmap, ADP5585_GENERAL_CFG, 0); > > > > +} > > > > + > > > > static int adp5585_i2c_probe(struct i2c_client *i2c) > > > > { > > > > const struct regmap_config *regmap_config; > > > > @@ -175,6 +182,22 @@ static int adp5585_i2c_probe(struct i2c_client > > > > *i2c) > > > > return dev_err_probe(&i2c->dev, -ENODEV, > > > > "Invalid device ID 0x%02x\n", id); > > > > > > > > + /* > > > > + * Enable the internal oscillator, as it's shared between > > > > multiple > > > > + * functions. > > > > + * > > > > + * As a future improvement, power consumption could possibly be > > > > + * decreased in some use cases by enabling and disabling the > > > > oscillator > > > > + * dynamically based on the needs of the child drivers. > > > > > > This is normal. What's stopping us from doing this from the offset? > > > > This is always needed when we have the input device registered. From my > > testing, > > we also need it for GPIOs configured as input. So basically the only reason > > this > > is not being done now is that it would not be trivial or really > > straightforward > > and honestly the series is already big enough :) > > Agreed! > > > Laurent also agreed with this not being mandatory now so hopefully it's also > > fine with you. > > If there is no explicit plan to do this in the future, you may as well > remove the comment. TODOs have a tendency to rot after code is > accepted. Will do. At least me, I can't commit and guarantee I'll do this in the near future. So if unless someone can commit to this, I'll remove the TODO in the next version. - Nuno Sá
diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c index 806867c56d6fb4ef1f461af26a424a3a05f46575..f3b74f7d6040413d066eb6dbaecfa3d5e6ee06bd 100644 --- a/drivers/mfd/adp5585.c +++ b/drivers/mfd/adp5585.c @@ -147,6 +147,13 @@ static int adp5585_add_devices(struct device *dev) return ret; } +static void adp5585_osc_disable(void *data) +{ + const struct adp5585_dev *adp5585 = data; + + regmap_write(adp5585->regmap, ADP5585_GENERAL_CFG, 0); +} + static int adp5585_i2c_probe(struct i2c_client *i2c) { const struct regmap_config *regmap_config; @@ -175,6 +182,22 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) return dev_err_probe(&i2c->dev, -ENODEV, "Invalid device ID 0x%02x\n", id); + /* + * Enable the internal oscillator, as it's shared between multiple + * functions. + * + * As a future improvement, power consumption could possibly be + * decreased in some use cases by enabling and disabling the oscillator + * dynamically based on the needs of the child drivers. + */ + ret = regmap_set_bits(adp5585->regmap, ADP5585_GENERAL_CFG, ADP5585_OSC_EN); + if (ret) + return ret; + + ret = devm_add_action_or_reset(&i2c->dev, adp5585_osc_disable, adp5585); + if (ret) + return ret; + return adp5585_add_devices(&i2c->dev); } diff --git a/drivers/pwm/pwm-adp5585.c b/drivers/pwm/pwm-adp5585.c index 40472ac5db6410a33e4f790fe8e6c23b517502be..c8821035b7c1412a55a642e6e8a46b66e693a5af 100644 --- a/drivers/pwm/pwm-adp5585.c +++ b/drivers/pwm/pwm-adp5585.c @@ -62,7 +62,6 @@ static int pwm_adp5585_apply(struct pwm_chip *chip, int ret; if (!state->enabled) { - regmap_clear_bits(regmap, ADP5585_GENERAL_CFG, ADP5585_OSC_EN); regmap_clear_bits(regmap, ADP5585_PWM_CFG, ADP5585_PWM_EN); return 0; } @@ -100,10 +99,6 @@ static int pwm_adp5585_apply(struct pwm_chip *chip, if (ret) return ret; - ret = regmap_set_bits(regmap, ADP5585_GENERAL_CFG, ADP5585_OSC_EN); - if (ret) - return ret; - return regmap_set_bits(regmap, ADP5585_PWM_CFG, ADP5585_PWM_EN); }