Re: [PATCH v3 03/22] mfd: adp5585: enable oscilator during probe

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2025-05-13 at 17:24 +0200, Laurent Pinchart wrote:
> Hi Nuno,
> 
> Thank you for the patch.
> 
> On Mon, May 12, 2025 at 01:38:55PM +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).
> > 
> > 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
> > 02f9e8c1c6a1d8b9516c060e0024d69886e9fb7a..d693b1ead05128e02f671ca465f9c72cab
> > 3b3395 100644
> > --- a/drivers/mfd/adp5585.c
> > +++ b/drivers/mfd/adp5585.c
> > @@ -143,6 +143,13 @@ static int adp5585_parse_fw(struct device *dev, struct
> > adp5585_dev *adp5585,
> >  	return rc;
> >  }
> >  
> > +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;
> > @@ -176,6 +183,15 @@ static int adp5585_i2c_probe(struct i2c_client *i2c)
> >  	if (n_devs < 0)
> >  		return n_devs;
> >  
> 
> Could you add a comment here to explain what's going on ? Something
> along the lines of
> 
> 	/*
> 	 * 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.
> 	 */
> 

Sure, I can add a similar remark in the regulator patch

Thanks!

> With that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +	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,
> >  				   devs, n_devs, NULL, 0, NULL);
> >  	if (ret)