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

[Greg KH]

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?

> +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?

> +
> +		/* 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?

thanks,

greg k-h