Re: [PATCH v4 13/20] mfd: adp5585: support reset and unlock events

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

On Fri, 2025-06-13 at 14:07 +0100, Lee Jones wrote:
> On Fri, 13 Jun 2025, Nuno Sá wrote:
> 
> > On Thu, 2025-06-12 at 15:55 +0100, Lee Jones wrote:
> > > On Wed, 21 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       | 270
> > > > +++++++++++++++++++++++++++++++++++++++++++-
> > > >  include/linux/mfd/adp5585.h |  39 +++++++
> > > >  2 files changed, 308 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
> > > > index
> > > > dcc09c898dd7990b39e21cb2324fa66ae171a802..6737d622a7ed9f280c439399f3709c
> > > > a816
> > > > 2dee01 100644
> > > > --- a/drivers/mfd/adp5585.c
> > > > +++ b/drivers/mfd/adp5585.c
> > > > @@ -170,6 +170,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 = {
> > > > @@ -177,8 +180,50 @@ 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)
> > > > +{
> > > > +	if (adp5585->has_pin6) {
> > > > +		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 GPI6 */
> > > 
> > > You have to tell us why this is a problem.
> > > 
> > > Nit: Comments should start with an upper case char.
> > 
> > The error message kind of states the problem :). But I'll put it in the
> > comment.
> 
> I don't think it does.  Remember, people reading this do not know the
> H/W as well as you do.  How is GPI6 even related to R5?

Yeah, you might be right. GPI6 is the same pin as R5. In a variation of the chip
we have this extra pin (though the datasheet refers to it as R5) that can either
be used as part of the keypad or a GPIO. In the other variants, it's a reset
pin.

The check is making sure we're not trying to configure an unlock/reset event on
a pin that do not exist. But I get your point, for me it's clear that R5 ==
GPI6. Me not being consistent in the comments/messages won't help anyone reading
the code.

- Nuno Sá

> 
> > - Nuno Sá
> > > 
> > > > +		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;
> > > > +	}
> 
> [...]