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

[Lee Jones <lee@kernel.org>]

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
> 
> 

-- 
Lee Jones [李琼斯]