Re: [PATCH v1 3/3] drivers: misc: Add support for TPS65224 pfsm driver

[Gairuboina Sirisha]

From: Gairuboina Sirisha <sirisha.gairuboina@xxxxxxxx>

> On Thu, Oct 26, 2023 at 07:02:26PM +0530, Gairuboina Sirisha wrote:
> > From: Gairuboina Sirisha <sirisha.gairuboina@xxxxxxxx>
> >
> > Added support for pmic nvm set FSM_I2C_TRIGGER functions driver,
> > state control driver, Makefile and Kconfig
> >
> > Signed-off-by: Gairuboina Sirisha <sirisha.gairuboina@xxxxxxxx>
> > ---
> >  drivers/misc/Kconfig               |  12 ++
> >  drivers/misc/Makefile              |   1 +
> >  drivers/misc/tps65224-pfsm.c       | 290 +++++++++++++++++++++++++++++
> >  include/uapi/linux/tps65224_pfsm.h |  36 ++++
> >  4 files changed, 339 insertions(+)
> >  create mode 100644 drivers/misc/tps65224-pfsm.c
> >  create mode 100644 include/uapi/linux/tps65224_pfsm.h
> >
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index cadd4a820c03..6d12404b0fa6 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -562,6 +562,18 @@ config TPS6594_PFSM
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called tps6594-pfsm.
> >
> > +config TPS65224_PFSM
> > +		tristate "TI TPS65224 Pre-configurable Finite State Machine support"
> > +		depends on MFD_TPS65224
> > +		default MFD_TPS65224
> > +		help
> > +		  Support PFSM (Pre-configurable Finite State Machine) on TPS65224 PMIC devices.
> > +		  These devices integrate a finite state machine engine, which manages the state
> > +		  of the device during operating state transition.
>
> Please wrap your lines a bit better, like the rest of the file has.
>
> > --- /dev/null
> > +++ b/drivers/misc/tps65224-pfsm.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PFSM (Pre-configurable Finite State Machine) driver for TI TPS65224 PMIC
> > + *
> > + * Copyright (C) 2015 Texas Instruments Incorporated - https://www.ti.com/
>
> Again, 2015?

Will correct style and copyright issues in the next patch version V2.

> > +static long tps65224_pfsm_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> > +{
> > +	struct tps65224_pfsm *pfsm = TPS65224_FILE_TO_PFSM(f);
> > +	struct pmic_state_opt state_opt;
> > +	void __user *argp = (void __user *)arg;
> > +	int ret = -ENOIOCTLCMD;
> > +
> > +	switch (cmd) {
> > +	case PMIC_GOTO_STANDBY:
> > +		/* Force trigger */
> > +		ret = regmap_write_bits(pfsm->regmap, TPS65224_REG_FSM_I2C_TRIGGERS,
> > +					TPS65224_BIT_TRIGGER_I2C(0), TPS65224_BIT_TRIGGER_I2C(0));
> > +		break;
> > +	case PMIC_UPDATE_PGM:
> > +		/* Force trigger */
> > +		ret = regmap_write_bits(pfsm->regmap, TPS65224_REG_FSM_I2C_TRIGGERS,
> > +					TPS65224_BIT_TRIGGER_I2C(3), TPS65224_BIT_TRIGGER_I2C(3));
> > +		break;
> > +	case PMIC_SET_ACTIVE_STATE:
> > +		/* Modify NSLEEP1-2 bits */
> > +		ret = regmap_set_bits(pfsm->regmap, TPS65224_REG_FSM_NSLEEP_TRIGGERS,
> > +					  TPS65224_BIT_NSLEEP1B | TPS65224_BIT_NSLEEP2B);
> > +		break;
> > +	case PMIC_SET_MCU_ONLY_STATE:
> > +		if (copy_from_user(&state_opt, argp, sizeof(state_opt)))
> > +			return -EFAULT;
> > +
> > +		/* Configure retention triggers */
> > +		ret = tps65224_pfsm_configure_ret_trig(pfsm->regmap, state_opt.gpio_retention,
> > +							  state_opt.ddr_retention);
>
> No error checking of userspace values at all?  That's brave.
>
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Modify NSLEEP1-2 bits */
> > +		ret = regmap_clear_bits(pfsm->regmap, TPS65224_REG_FSM_NSLEEP_TRIGGERS,
> > +					TPS65224_BIT_NSLEEP1B);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_set_bits(pfsm->regmap, TPS65224_REG_FSM_NSLEEP_TRIGGERS,
> > +					  TPS65224_BIT_NSLEEP2B);
> > +		break;
> > +	case PMIC_SET_RETENTION_STATE:
> > +		if (copy_from_user(&state_opt, argp, sizeof(state_opt)))
> > +			return -EFAULT;
> >
> > Again, no checking of valid values?

Thanks for your feedback. Will validate the user space buffer and submit in V2.

> > +
> > +		/* Configure wake-up destination */
> > +		if (state_opt.mcu_only_startup_dest)
> > +			ret = regmap_write_bits(pfsm->regmap, TPS65224_REG_CONFIG_2,
> > +						TPS65224_BIT_STARTUP_INT,
> > +						TPS65224_STARTUP_DEST_MCU_ONLY);
> > +		else
> > +			ret = regmap_write_bits(pfsm->regmap, TPS65224_REG_CONFIG_2,
> > +						TPS65224_BIT_STARTUP_INT,
> > +						TPS65224_STARTUP_DEST_ACTIVE);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Configure retention triggers */
> > +		ret = tps65224_pfsm_configure_ret_trig(pfsm->regmap, state_opt.gpio_retention,
> > +							  state_opt.ddr_retention);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Modify NSLEEP1-2 bits */
> > +		ret = regmap_clear_bits(pfsm->regmap, TPS65224_REG_FSM_NSLEEP_TRIGGERS,
> > +					TPS65224_BIT_NSLEEP2B);
> > +		break;
> > +	}
> > +
> > +	return ret;
> > +}
>
> Where is the example userspace code that uses these custom ioctls?
> Where are they documented?  How do we know what they should be doing?

We have tested the PFSM driver using sample code available in sampes/pfsm/pfsm-wakeup.c with minimal change on Number of PMICs.

Thanks & Regards,
Sirisha G.