| Message ID | 20250415-dev-adp5589-fw-v2-2-3a799c3ed812@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: adp5585: support keymap events and drop legacy Input driver | expand |
Hi Nuno, On Mon, Apr 21, 2025 at 01:14:28PM +0100, Nuno Sá wrote: > On Mon, 2025-04-21 at 11:57 +0300, Laurent Pinchart wrote: > > On Tue, Apr 15, 2025 at 03:49:18PM +0100, 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). > > > > Setting this bit unconditionally increases power consumption. It should > > only be set when needed. > > Well, not sure if the effort for that pays off... The only usecase were it would > make sense to do that would be for PWM. For the other devices (and assuming I'm > right with the GPI case) we need this always set. For the keypad, can't the device be kept powered off if the input device exposed to userspace is not open ? And for GPIOs, OSC_EN isn't needed when all requested GPIOs are configured as outputs, as far as I can tell. I'm fine addressing this issue on top of this series. > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > --- > > > drivers/mfd/adp5585.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > > index > > > 160e0b38106a6d78f7d4b7c866cb603d96ea673e..f17b5f2474cac6a403556694066f438288 > > > 264a49 100644 > > > --- a/drivers/mfd/adp5585.c > > > +++ b/drivers/mfd/adp5585.c > > > @@ -110,6 +110,13 @@ static const struct regmap_config adp5585_regmap_configs[] = { > > > }, > > > }; > > > > > > +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; > > > @@ -138,6 +145,15 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > > return dev_err_probe(&i2c->dev, -ENODEV, > > > "Invalid device ID 0x%02x\n", id); > > > > > > + 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; > > > + > > > ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > > > adp5585_devs, ARRAY_SIZE(adp5585_devs), > > > NULL, 0, NULL);
On Tue, 2025-04-22 at 01:03 +0300, Laurent Pinchart wrote: > Hi Nuno, > > On Mon, Apr 21, 2025 at 01:14:28PM +0100, Nuno Sá wrote: > > On Mon, 2025-04-21 at 11:57 +0300, Laurent Pinchart wrote: > > > On Tue, Apr 15, 2025 at 03:49:18PM +0100, 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). > > > > > > Setting this bit unconditionally increases power consumption. It should > > > only be set when needed. > > > > Well, not sure if the effort for that pays off... The only usecase were it > > would > > make sense to do that would be for PWM. For the other devices (and assuming > > I'm > > right with the GPI case) we need this always set. > > For the keypad, can't the device be kept powered off if the input device > exposed to userspace is not open ? And for GPIOs, OSC_EN isn't needed > when all requested GPIOs are configured as outputs, as far as I can > tell. Yes, I do know it's doable (well, TBH for the input case I just learned we can define .open()/.close() callbacks). My point was just this adds some complexity and I'm not sure of the added value (while saving power is always nice) > > I'm fine addressing this issue on top of this series. Agreed. I would prefer that. This series is already big enough. > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > drivers/mfd/adp5585.c | 16 ++++++++++++++++ > > > > 1 file changed, 16 insertions(+) > > > > > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > > > index > > > > 160e0b38106a6d78f7d4b7c866cb603d96ea673e..f17b5f2474cac6a403556694066f43 > > > > 8288 > > > > 264a49 100644 > > > > --- a/drivers/mfd/adp5585.c > > > > +++ b/drivers/mfd/adp5585.c > > > > @@ -110,6 +110,13 @@ static const struct regmap_config > > > > adp5585_regmap_configs[] = { > > > > }, > > > > }; > > > > > > > > +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; > > > > @@ -138,6 +145,15 @@ static int adp5585_i2c_probe(struct i2c_client > > > > *i2c) > > > > return dev_err_probe(&i2c->dev, -ENODEV, > > > > "Invalid device ID 0x%02x\n", id); > > > > > > > > + 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; > > > > + > > > > ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > > > > adp5585_devs, > > > > ARRAY_SIZE(adp5585_devs), > > > > NULL, 0, NULL);
diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c index 160e0b38106a6d78f7d4b7c866cb603d96ea673e..f17b5f2474cac6a403556694066f438288264a49 100644 --- a/drivers/mfd/adp5585.c +++ b/drivers/mfd/adp5585.c @@ -110,6 +110,13 @@ static const struct regmap_config adp5585_regmap_configs[] = { }, }; +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; @@ -138,6 +145,15 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) return dev_err_probe(&i2c->dev, -ENODEV, "Invalid device ID 0x%02x\n", id); + 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; + ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, adp5585_devs, ARRAY_SIZE(adp5585_devs), NULL, 0, NULL);