Re: [PATCH v3 14/22] mfd: adp5585: support reset and unlock events

[Lee Jones <lee@kernel.org>]

On Wed, 14 May 2025, Laurent Pinchart wrote:

> On Tue, May 13, 2025 at 05:22:46PM +0100, Lee Jones wrote:
> > On Mon, 12 May 2025, Nuno Sá via B4 Relay wrote:
> > 
> > > From: Nuno Sá <nuno.sa@analog.com>
> > > 
> > > The ADP558x family of devices can be programmed to respond to some
> > > especial events, In case of the unlock events, one can lock the keypad
> > > and use KEYS or GPIs events to unlock it. For the reset events, one can
> > > again use a combinations of GPIs/KEYs in order to generate an event that
> > > will trigger the device to generate an output reset pulse.
> > > 
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > ---
> > >  drivers/mfd/adp5585.c       | 279 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/adp5585.h |  41 +++++++
> > >  2 files changed, 320 insertions(+)
> > > 
> > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
> > > index 5851ad30e7323bbb891878167d0786bc60ef5d90..b1227a390fe2f932ba8060b0d722f53f45ec3b4b 100644
> > > --- a/drivers/mfd/adp5585.c
> > > +++ b/drivers/mfd/adp5585.c
> > > @@ -157,6 +157,9 @@ static const struct adp5585_regs adp5585_regs = {
> > >  	.int_en = ADP5585_INT_EN,
> > >  	.gen_cfg = ADP5585_GENERAL_CFG,
> > >  	.poll_ptime_cfg = ADP5585_POLL_PTIME_CFG,
> > > +	.reset_cfg = ADP5585_RESET_CFG,
> > > +	.reset1_event_a = ADP5585_RESET1_EVENT_A,
> > > +	.reset2_event_a = ADP5585_RESET2_EVENT_A,
> > >  };
> > >  
> > >  static const struct adp5585_regs adp5589_regs = {
> > > @@ -164,8 +167,52 @@ static const struct adp5585_regs adp5589_regs = {
> > >  	.int_en = ADP5589_INT_EN,
> > >  	.gen_cfg = ADP5589_GENERAL_CFG,
> > >  	.poll_ptime_cfg = ADP5589_POLL_PTIME_CFG,
> > > +	.reset_cfg = ADP5589_RESET_CFG,
> > > +	.reset1_event_a = ADP5589_RESET1_EVENT_A,
> > > +	.reset2_event_a = ADP5589_RESET2_EVENT_A,
> > >  };
> > >  
> > > +static int adp5585_validate_event(const struct adp5585_dev *adp5585,
> > > +				  unsigned int ev, bool has_pin5)
> > 
> > has_pin5 (which doesn't actually mean much to me) is passed around a lot
> > and is only used in one place, as far as I can see.  You also have 'dev'
> > available here, so why not drop it everywhere and call
> > 
> >    if (!device_property_present(dev, "gpio-reserved-ranges"))
> > 
> > ... here instead?
> 
> The information can be stored in struct adp5585_dev. I wouldn't call
> device_property_present() here, as that's costly.

Does this function get called a lot?

Storing in the device data is also good.

> > > +{
> > > +	if (has_pin5) {
> > > +		if (ev >= ADP5585_ROW5_KEY_EVENT_START && ev <= ADP5585_ROW5_KEY_EVENT_END)
> > > +			return 0;
> > > +		if (ev >= ADP5585_GPI_EVENT_START && ev <= ADP5585_GPI_EVENT_END)
> > > +			return 0;
> > > +
> > > +		return dev_err_probe(adp5585->dev, -EINVAL,
> > > +				     "Invalid unlock/reset event(%u) for this device\n", ev);
> > > +	}
> > > +
> > > +	if (ev >= ADP5585_KEY_EVENT_START && ev <= ADP5585_KEY_EVENT_END)
> > > +		return 0;
> > > +	if (ev >= ADP5585_GPI_EVENT_START && ev <= ADP5585_GPI_EVENT_END) {
> > > +		/* if it's GPI5 */
> > > +		if (ev == (ADP5585_GPI_EVENT_START + 5))
> > > +			return dev_err_probe(adp5585->dev, -EINVAL,
> > > +					     "Invalid unlock/reset event(%u). R5 not available\n",
> > > +					     ev);
> > > +		return 0;
> > > +	}
> > > +
> > > +	return dev_err_probe(adp5585->dev, -EINVAL,
> > > +			     "Invalid unlock/reset event(%u) for this device\n", ev);
> > > +}
> > > +
> > > +static int adp5589_validate_event(const struct adp5585_dev *adp5585,
> > > +				  unsigned int ev, bool has_pin5)
> > > +{
> > > +	if (ev >= ADP5589_KEY_EVENT_START && ev <= ADP5589_KEY_EVENT_END)
> > > +		return 0;
> > > +	if (ev >= ADP5589_GPI_EVENT_START && ev <= ADP5589_GPI_EVENT_END)
> > > +		return 0;
> > > +
> > > +	return dev_err_probe(adp5585->dev, -EINVAL,
> > > +			     "Invalid unlock/reset event(%u) for this device\n",
> > > +			     ev);
> > > +}
> > > +
> > >  static int adp5585_fill_chip_configs(struct adp5585_dev *adp5585,
> > >  				     struct i2c_client *i2c,
> > >  				     struct regmap_config *regmap_config)
> > > @@ -180,10 +227,13 @@ static int adp5585_fill_chip_configs(struct adp5585_dev *adp5585,
> > >  	case ADP5585_MAN_ID_VALUE:
> > >  		*regmap_config = adp5585_regmap_config_template;
> > >  		info->regs = &adp5585_regs;
> > > +		info->validate_event = adp5585_validate_event;
> > 
> > I'd take an extra if() / switch() over a driver-level pointer to a function.
> 
> Funny how we have different tastes for this kind of things, I find the
> function pointer more readable :-)

Contributors have tried to do "interesting" things with function
pointers in MFD in the past.  To the point were I now have a general
aversion to them.  I think they're great for things like subsystem-level
Ops, but beyond that, things _can_ get messy, fast.

-- 
Lee Jones [李琼斯]