| Message ID | 20250512-dev-adp5589-fw-v3-3-092b14b79a88@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: adp5585: support keymap events and drop legacy Input driver | expand |
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..d693b1ead05128e02f671ca465f9c72cab3b3395 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. */ 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)
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)
On Tue, 13 May 2025, Nuno Sá wrote: > 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. Yes, it like many things are subsystem / maintainer preference. It's not the 1990's anymore - we have 4k monitors - they'll come around.
diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c index 02f9e8c1c6a1d8b9516c060e0024d69886e9fb7a..d693b1ead05128e02f671ca465f9c72cab3b3395 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); + 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)