Re: [PATCH v4 03/20] mfd: adp5585: enable oscilator during probe

[Lee Jones <lee@kernel.org>]

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.

-- 
Lee Jones [李琼斯]