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

[Lee Jones <lee@kernel.org>]

On Fri, 13 Jun 2025, Nuno Sá wrote:

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

You nailed it!  Thanks for understanding.

-- 
Lee Jones [李琼斯]