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

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

On Tue, 2025-05-13 at 15:26 +0100, Lee Jones wrote:
> On Mon, 12 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).
> > 
> > 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;
> >  
> > +	ret = regmap_set_bits(adp5585->regmap, ADP5585_GENERAL_CFG,
> > +			      ADP5585_OSC_EN);
> 
> Nit: Consider unwrapping to 100-chars to avoid these simple line breaks.
> 
> Other than that, looks okay.

This topic is always hard as some other maintainers perfect the rule "keep the
80 char and only go 100 if readability is hurt). Personally, I do prefer 100 so
happy to do it here.

> 
> > +	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)
> > 
> > -- 
> > 2.49.0
> > 
> >