Re: [PATCH v2] counter: stm32-lptimer-cnt: remove iio counter abi

[Jonathan Cameron]

On Fri, 29 Jan 2021 14:22:22 +0100
Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx> wrote:

> Currently, the STM32 LP Timer counter driver registers into both IIO and
> counter subsystems, which is redundant.
> 
> Remove the IIO counter ABI and IIO registration from the STM32 LP Timer
> counter driver since it's been superseded by the Counter subsystem
> as discussed in [1].
> 
> Keep only the counter subsystem related part.
> Move a part of the ABI documentation into a driver comment.
> 
> This also removes a duplicate ABI warning
> $ scripts/get_abi.pl validate
> ...
> /sys/bus/iio/devices/iio:deviceX/in_count0_preset is defined 2 times:
>   ./Documentation/ABI/testing/sysfs-bus-iio-timer-stm32:100
>   ./Documentation/ABI/testing/sysfs-bus-iio-lptimer-stm32:0
> 
> [1] https://lkml.org/lkml/2021/1/19/347
> 
> Acked-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxxxxxxx>

The mod_devicetable.h include confused me for a bit, but then I realised
that iio.h includes of.h (which it almost certainly shouldn't).
I guess that's one to clean up at somepoint!

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> ---
> Changes in v2:
> - remove dependency on IIO subsustem, as spotted by William
> ---
>  .../ABI/testing/sysfs-bus-iio-lptimer-stm32        |  62 -----
>  drivers/counter/Kconfig                            |   2 +-
>  drivers/counter/stm32-lptimer-cnt.c                | 297 +++------------------
>  3 files changed, 37 insertions(+), 324 deletions(-)
>  delete mode 100644 Documentation/ABI/testing/sysfs-bus-iio-lptimer-stm32
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-lptimer-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-lptimer-stm32
> deleted file mode 100644
> index 73498ff..00000000
> --- a/Documentation/ABI/testing/sysfs-bus-iio-lptimer-stm32
> +++ /dev/null
> @@ -1,62 +0,0 @@
> -What:		/sys/bus/iio/devices/iio:deviceX/in_count0_preset
> -KernelVersion:	4.13
> -Contact:	fabrice.gasnier@xxxxxx
> -Description:
> -		Reading returns the current preset value. Writing sets the
> -		preset value. Encoder counts continuously from 0 to preset
> -		value, depending on direction (up/down).
> -
> -What:		/sys/bus/iio/devices/iio:deviceX/in_count_quadrature_mode_available
> -KernelVersion:	4.13
> -Contact:	fabrice.gasnier@xxxxxx
> -Description:
> -		Reading returns the list possible quadrature modes.
> -
> -What:		/sys/bus/iio/devices/iio:deviceX/in_count0_quadrature_mode
> -KernelVersion:	4.13
> -Contact:	fabrice.gasnier@xxxxxx
> -Description:
> -		Configure the device counter quadrature modes:
> -
> -		- non-quadrature:
> -			Encoder IN1 input servers as the count input (up
> -			direction).
> -
> -		- quadrature:
> -			Encoder IN1 and IN2 inputs are mixed to get direction
> -			and count.
> -
> -What:		/sys/bus/iio/devices/iio:deviceX/in_count_polarity_available
> -KernelVersion:	4.13
> -Contact:	fabrice.gasnier@xxxxxx
> -Description:
> -		Reading returns the list possible active edges.
> -
> -What:		/sys/bus/iio/devices/iio:deviceX/in_count0_polarity
> -KernelVersion:	4.13
> -Contact:	fabrice.gasnier@xxxxxx
> -Description:
> -		Configure the device encoder/counter active edge:
> -
> -		- rising-edge
> -		- falling-edge
> -		- both-edges
> -
> -		In non-quadrature mode, device counts up on active edge.
> -
> -		In quadrature mode, encoder counting scenarios are as follows:
> -
> -		+---------+----------+--------------------+--------------------+
> -		| Active  | Level on |      IN1 signal    |     IN2 signal     |
> -		| edge    | opposite +----------+---------+----------+---------+
> -		|         | signal   |  Rising  | Falling |  Rising  | Falling |
> -		+---------+----------+----------+---------+----------+---------+
> -		| Rising  | High ->  |   Down   |    -    |   Up     |    -    |
> -		| edge    | Low  ->  |   Up     |    -    |   Down   |    -    |
> -		+---------+----------+----------+---------+----------+---------+
> -		| Falling | High ->  |    -     |   Up    |    -     |   Down  |
> -		| edge    | Low  ->  |    -     |   Down  |    -     |   Up    |
> -		+---------+----------+----------+---------+----------+---------+
> -		| Both    | High ->  |   Down   |   Up    |   Up     |   Down  |
> -		| edges   | Low  ->  |   Up     |   Down  |   Down   |   Up    |
> -		+---------+----------+----------+---------+----------+---------+
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2de53ab..cbdf842 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -41,7 +41,7 @@ config STM32_TIMER_CNT
>  
>  config STM32_LPTIMER_CNT
>  	tristate "STM32 LP Timer encoder counter driver"
> -	depends on (MFD_STM32_LPTIMER || COMPILE_TEST) && IIO
> +	depends on MFD_STM32_LPTIMER || COMPILE_TEST
>  	help
>  	  Select this option to enable STM32 Low-Power Timer quadrature encoder
>  	  and counter driver.
> diff --git a/drivers/counter/stm32-lptimer-cnt.c b/drivers/counter/stm32-lptimer-cnt.c
> index fd6828e..9374396 100644
> --- a/drivers/counter/stm32-lptimer-cnt.c
> +++ b/drivers/counter/stm32-lptimer-cnt.c
> @@ -12,8 +12,8 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/counter.h>
> -#include <linux/iio/iio.h>
>  #include <linux/mfd/stm32-lptimer.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> @@ -107,249 +107,27 @@ static int stm32_lptim_setup(struct stm32_lptim_cnt *priv, int enable)
>  	return regmap_update_bits(priv->regmap, STM32_LPTIM_CFGR, mask, val);
>  }
>  
> -static int stm32_lptim_write_raw(struct iio_dev *indio_dev,
> -				 struct iio_chan_spec const *chan,
> -				 int val, int val2, long mask)
> -{
> -	struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
> -	int ret;
> -
> -	switch (mask) {
> -	case IIO_CHAN_INFO_ENABLE:
> -		if (val < 0 || val > 1)
> -			return -EINVAL;
> -
> -		/* Check nobody uses the timer, or already disabled/enabled */
> -		ret = stm32_lptim_is_enabled(priv);
> -		if ((ret < 0) || (!ret && !val))
> -			return ret;
> -		if (val && ret)
> -			return -EBUSY;
> -
> -		ret = stm32_lptim_setup(priv, val);
> -		if (ret)
> -			return ret;
> -		return stm32_lptim_set_enable_state(priv, val);
> -
> -	default:
> -		return -EINVAL;
> -	}
> -}
> -
> -static int stm32_lptim_read_raw(struct iio_dev *indio_dev,
> -				struct iio_chan_spec const *chan,
> -				int *val, int *val2, long mask)
> -{
> -	struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
> -	u32 dat;
> -	int ret;
> -
> -	switch (mask) {
> -	case IIO_CHAN_INFO_RAW:
> -		ret = regmap_read(priv->regmap, STM32_LPTIM_CNT, &dat);
> -		if (ret)
> -			return ret;
> -		*val = dat;
> -		return IIO_VAL_INT;
> -
> -	case IIO_CHAN_INFO_ENABLE:
> -		ret = stm32_lptim_is_enabled(priv);
> -		if (ret < 0)
> -			return ret;
> -		*val = ret;
> -		return IIO_VAL_INT;
> -
> -	case IIO_CHAN_INFO_SCALE:
> -		/* Non-quadrature mode: scale = 1 */
> -		*val = 1;
> -		*val2 = 0;
> -		if (priv->quadrature_mode) {
> -			/*
> -			 * Quadrature encoder mode:
> -			 * - both edges, quarter cycle, scale is 0.25
> -			 * - either rising/falling edge scale is 0.5
> -			 */
> -			if (priv->polarity > 1)
> -				*val2 = 2;
> -			else
> -				*val2 = 1;
> -		}
> -		return IIO_VAL_FRACTIONAL_LOG2;
> -
> -	default:
> -		return -EINVAL;
> -	}
> -}
> -
> -static const struct iio_info stm32_lptim_cnt_iio_info = {
> -	.read_raw = stm32_lptim_read_raw,
> -	.write_raw = stm32_lptim_write_raw,
> -};
> -
> -static const char *const stm32_lptim_quadrature_modes[] = {
> -	"non-quadrature",
> -	"quadrature",
> -};
> -
> -static int stm32_lptim_get_quadrature_mode(struct iio_dev *indio_dev,
> -					   const struct iio_chan_spec *chan)
> -{
> -	struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
> -
> -	return priv->quadrature_mode;
> -}
> -
> -static int stm32_lptim_set_quadrature_mode(struct iio_dev *indio_dev,
> -					   const struct iio_chan_spec *chan,
> -					   unsigned int type)
> -{
> -	struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
> -
> -	if (stm32_lptim_is_enabled(priv))
> -		return -EBUSY;
> -
> -	priv->quadrature_mode = type;
> -
> -	return 0;
> -}
> -
> -static const struct iio_enum stm32_lptim_quadrature_mode_en = {
> -	.items = stm32_lptim_quadrature_modes,
> -	.num_items = ARRAY_SIZE(stm32_lptim_quadrature_modes),
> -	.get = stm32_lptim_get_quadrature_mode,
> -	.set = stm32_lptim_set_quadrature_mode,
> -};
> -
> -static const char * const stm32_lptim_cnt_polarity[] = {
> -	"rising-edge", "falling-edge", "both-edges",
> -};
> -
> -static int stm32_lptim_cnt_get_polarity(struct iio_dev *indio_dev,
> -					const struct iio_chan_spec *chan)
> -{
> -	struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
> -
> -	return priv->polarity;
> -}
> -
> -static int stm32_lptim_cnt_set_polarity(struct iio_dev *indio_dev,
> -					const struct iio_chan_spec *chan,
> -					unsigned int type)
> -{
> -	struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
> -
> -	if (stm32_lptim_is_enabled(priv))
> -		return -EBUSY;
> -
> -	priv->polarity = type;
> -
> -	return 0;
> -}
> -
> -static const struct iio_enum stm32_lptim_cnt_polarity_en = {
> -	.items = stm32_lptim_cnt_polarity,
> -	.num_items = ARRAY_SIZE(stm32_lptim_cnt_polarity),
> -	.get = stm32_lptim_cnt_get_polarity,
> -	.set = stm32_lptim_cnt_set_polarity,
> -};
> -
> -static ssize_t stm32_lptim_cnt_get_ceiling(struct stm32_lptim_cnt *priv,
> -					   char *buf)
> -{
> -	return snprintf(buf, PAGE_SIZE, "%u\n", priv->ceiling);
> -}
> -
> -static ssize_t stm32_lptim_cnt_set_ceiling(struct stm32_lptim_cnt *priv,
> -					   const char *buf, size_t len)
> -{
> -	int ret;
> -
> -	if (stm32_lptim_is_enabled(priv))
> -		return -EBUSY;
> -
> -	ret = kstrtouint(buf, 0, &priv->ceiling);
> -	if (ret)
> -		return ret;
> -
> -	if (priv->ceiling > STM32_LPTIM_MAX_ARR)
> -		return -EINVAL;
> -
> -	return len;
> -}
> -
> -static ssize_t stm32_lptim_cnt_get_preset_iio(struct iio_dev *indio_dev,
> -					      uintptr_t private,
> -					      const struct iio_chan_spec *chan,
> -					      char *buf)
> -{
> -	struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
> -
> -	return stm32_lptim_cnt_get_ceiling(priv, buf);
> -}
> -
> -static ssize_t stm32_lptim_cnt_set_preset_iio(struct iio_dev *indio_dev,
> -					      uintptr_t private,
> -					      const struct iio_chan_spec *chan,
> -					      const char *buf, size_t len)
> -{
> -	struct stm32_lptim_cnt *priv = iio_priv(indio_dev);
> -
> -	return stm32_lptim_cnt_set_ceiling(priv, buf, len);
> -}
> -
> -/* LP timer with encoder */
> -static const struct iio_chan_spec_ext_info stm32_lptim_enc_ext_info[] = {
> -	{
> -		.name = "preset",
> -		.shared = IIO_SEPARATE,
> -		.read = stm32_lptim_cnt_get_preset_iio,
> -		.write = stm32_lptim_cnt_set_preset_iio,
> -	},
> -	IIO_ENUM("polarity", IIO_SEPARATE, &stm32_lptim_cnt_polarity_en),
> -	IIO_ENUM_AVAILABLE("polarity", &stm32_lptim_cnt_polarity_en),
> -	IIO_ENUM("quadrature_mode", IIO_SEPARATE,
> -		 &stm32_lptim_quadrature_mode_en),
> -	IIO_ENUM_AVAILABLE("quadrature_mode", &stm32_lptim_quadrature_mode_en),
> -	{}
> -};
> -
> -static const struct iio_chan_spec stm32_lptim_enc_channels = {
> -	.type = IIO_COUNT,
> -	.channel = 0,
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -			      BIT(IIO_CHAN_INFO_ENABLE) |
> -			      BIT(IIO_CHAN_INFO_SCALE),
> -	.ext_info = stm32_lptim_enc_ext_info,
> -	.indexed = 1,
> -};
> -
> -/* LP timer without encoder (counter only) */
> -static const struct iio_chan_spec_ext_info stm32_lptim_cnt_ext_info[] = {
> -	{
> -		.name = "preset",
> -		.shared = IIO_SEPARATE,
> -		.read = stm32_lptim_cnt_get_preset_iio,
> -		.write = stm32_lptim_cnt_set_preset_iio,
> -	},
> -	IIO_ENUM("polarity", IIO_SEPARATE, &stm32_lptim_cnt_polarity_en),
> -	IIO_ENUM_AVAILABLE("polarity", &stm32_lptim_cnt_polarity_en),
> -	{}
> -};
> -
> -static const struct iio_chan_spec stm32_lptim_cnt_channels = {
> -	.type = IIO_COUNT,
> -	.channel = 0,
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -			      BIT(IIO_CHAN_INFO_ENABLE) |
> -			      BIT(IIO_CHAN_INFO_SCALE),
> -	.ext_info = stm32_lptim_cnt_ext_info,
> -	.indexed = 1,
> -};
> -
>  /**
>   * enum stm32_lptim_cnt_function - enumerates LPTimer counter & encoder modes
>   * @STM32_LPTIM_COUNTER_INCREASE: up count on IN1 rising, falling or both edges
>   * @STM32_LPTIM_ENCODER_BOTH_EDGE: count on both edges (IN1 & IN2 quadrature)
> + *
> + * In non-quadrature mode, device counts up on active edge.
> + * In quadrature mode, encoder counting scenarios are as follows:
> + * +---------+----------+--------------------+--------------------+
> + * | Active  | Level on |      IN1 signal    |     IN2 signal     |
> + * | edge    | opposite +----------+---------+----------+---------+
> + * |         | signal   |  Rising  | Falling |  Rising  | Falling |
> + * +---------+----------+----------+---------+----------+---------+
> + * | Rising  | High ->  |   Down   |    -    |   Up     |    -    |
> + * | edge    | Low  ->  |   Up     |    -    |   Down   |    -    |
> + * +---------+----------+----------+---------+----------+---------+
> + * | Falling | High ->  |    -     |   Up    |    -     |   Down  |
> + * | edge    | Low  ->  |    -     |   Down  |    -     |   Up    |
> + * +---------+----------+----------+---------+----------+---------+
> + * | Both    | High ->  |   Down   |   Up    |   Up     |   Down  |
> + * | edges   | Low  ->  |   Up     |   Down  |   Down   |   Up    |
> + * +---------+----------+----------+---------+----------+---------+
>   */
>  enum stm32_lptim_cnt_function {
>  	STM32_LPTIM_COUNTER_INCREASE,
> @@ -484,7 +262,7 @@ static ssize_t stm32_lptim_cnt_ceiling_read(struct counter_device *counter,
>  {
>  	struct stm32_lptim_cnt *const priv = counter->priv;
>  
> -	return stm32_lptim_cnt_get_ceiling(priv, buf);
> +	return snprintf(buf, PAGE_SIZE, "%u\n", priv->ceiling);
>  }
>  
>  static ssize_t stm32_lptim_cnt_ceiling_write(struct counter_device *counter,
> @@ -493,8 +271,22 @@ static ssize_t stm32_lptim_cnt_ceiling_write(struct counter_device *counter,
>  					     const char *buf, size_t len)
>  {
>  	struct stm32_lptim_cnt *const priv = counter->priv;
> +	unsigned int ceiling;
> +	int ret;
> +
> +	if (stm32_lptim_is_enabled(priv))
> +		return -EBUSY;
> +
> +	ret = kstrtouint(buf, 0, &ceiling);
> +	if (ret)
> +		return ret;
> +
> +	if (ceiling > STM32_LPTIM_MAX_ARR)
> +		return -EINVAL;
> +
> +	priv->ceiling = ceiling;
>  
> -	return stm32_lptim_cnt_set_ceiling(priv, buf, len);
> +	return len;
>  }
>  
>  static const struct counter_count_ext stm32_lptim_cnt_ext[] = {
> @@ -630,32 +422,19 @@ static int stm32_lptim_cnt_probe(struct platform_device *pdev)
>  {
>  	struct stm32_lptimer *ddata = dev_get_drvdata(pdev->dev.parent);
>  	struct stm32_lptim_cnt *priv;
> -	struct iio_dev *indio_dev;
> -	int ret;
>  
>  	if (IS_ERR_OR_NULL(ddata))
>  		return -EINVAL;
>  
> -	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> -	if (!indio_dev)
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
>  		return -ENOMEM;
>  
> -	priv = iio_priv(indio_dev);
>  	priv->dev = &pdev->dev;
>  	priv->regmap = ddata->regmap;
>  	priv->clk = ddata->clk;
>  	priv->ceiling = STM32_LPTIM_MAX_ARR;
>  
> -	/* Initialize IIO device */
> -	indio_dev->name = dev_name(&pdev->dev);
> -	indio_dev->dev.of_node = pdev->dev.of_node;
> -	indio_dev->info = &stm32_lptim_cnt_iio_info;
> -	if (ddata->has_encoder)
> -		indio_dev->channels = &stm32_lptim_enc_channels;
> -	else
> -		indio_dev->channels = &stm32_lptim_cnt_channels;
> -	indio_dev->num_channels = 1;
> -
>  	/* Initialize Counter device */
>  	priv->counter.name = dev_name(&pdev->dev);
>  	priv->counter.parent = &pdev->dev;
> @@ -673,10 +452,6 @@ static int stm32_lptim_cnt_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, priv);
>  
> -	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> -	if (ret)
> -		return ret;
> -
>  	return devm_counter_register(&pdev->dev, &priv->counter);
>  }
>