Message-ID: <20170710125454.GA22386@sophia>
Date:   Mon, 10 Jul 2017 08:54:54 -0400
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Fabrice Gasnier <fabrice.gasnier@...com>, lee.jones@...aro.org,
        benjamin.gaignard@...aro.org, thierry.reding@...il.com,
        robh+dt@...nel.org, mark.rutland@....com, alexandre.torgue@...com,
        mcoquelin.stm32@...il.com, benjamin.gaignard@...com,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-pwm@...r.kernel.org
Subject: Re: [PATCH v3 8/9] iio: counter: Add support for STM32 LPTimer

On Sun, Jul 09, 2017 at 09:12:43PM +0100, Jonathan Cameron wrote:
>On Fri, 7 Jul 2017 18:31:07 +0200
>Fabrice Gasnier <fabrice.gasnier@...com> wrote:
>
>> Add support for STM32 Low-Power Timer, that can be used as counter
>> or quadrature encoder.
>> 
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com>
>So the obvious question on this one is whether to to delay merging
>until we have come to some decision on William's proposed new interface.
>
>My gut feeling is no - we are supporting this interface anyway going
>forward for a couple of devices.  Even if the new interface comes
>along reasonably quickly I don't have any problem supporting this
>approach for one more driver.
>
>So I'd like to let this sit for a little while longer ideally for other
>opinions.
>
>>From me however it is good to go.
>
>Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>

For what it's worth, I don't think we should delay drivers just yet.
Although I plan on releasing the new generic counter and quadrature
counter interfaces in the coming weeks, we're certainly bound to go
through several revisions of them before they are acceptable for merge.

I wouldn't want the development of these interfaces to stall other
drivers, so maintaining the current approach with the anticipation of a
subsequent upgrade patch should be our course of action (at least until
the generic counter interface is solidified and merged).

William Breathitt Gray

>> ---
>> Changes in v2:
>> - s/Low Power/Low-Power
>> - update few comments
>> ---
>>  .../ABI/testing/sysfs-bus-iio-lptimer-stm32        |  57 +++
>>  drivers/iio/counter/Kconfig                        |   9 +
>>  drivers/iio/counter/Makefile                       |   1 +
>>  drivers/iio/counter/stm32-lptimer-cnt.c            | 383 +++++++++++++++++++++
>>  4 files changed, 450 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-lptimer-stm32
>>  create mode 100644 drivers/iio/counter/stm32-lptimer-cnt.c
>> 
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-lptimer-stm32 b/Documentation/ABI/testing/sysfs-bus-iio-lptimer-stm32
>> new file mode 100644
>> index 0000000..ad2cc63
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-lptimer-stm32
>> @@ -0,0 +1,57 @@
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_count0_preset
>> +KernelVersion:	4.13
>> +Contact:	fabrice.gasnier@...com
>> +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@...com
>> +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@...com
>> +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@...com
>> +Description:
>> +		Reading returns the list possible active edges.
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_count0_polarity
>> +KernelVersion:	4.13
>> +Contact:	fabrice.gasnier@...com
>> +Description:
>> +		Configure the device encoder/counter active edge:
>> +		- rising-edge
>> +		- falling-edge
>> +		- both-edges
>So this is our one oddity.  I'm happy enough with the interface until if
>we come up with something better.  So lets go with this for now.
>> +
>> +		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/iio/counter/Kconfig b/drivers/iio/counter/Kconfig
>> index b37e5fc..474e1ac 100644
>> --- a/drivers/iio/counter/Kconfig
>> +++ b/drivers/iio/counter/Kconfig
>> @@ -21,4 +21,13 @@ config 104_QUAD_8
>>  	  The base port addresses for the devices may be configured via the base
>>  	  array module parameter.
>>  
>> +config STM32_LPTIMER_CNT
>> +	tristate "STM32 LP Timer encoder counter driver"
>> +	depends on MFD_STM32_LPTIMER || COMPILE_TEST
>> +	help
>> +	  Select this option to enable STM32 Low-Power Timer quadrature encoder
>> +	  and counter driver.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called stm32-lptimer-cnt.
>>  endmenu
>> diff --git a/drivers/iio/counter/Makefile b/drivers/iio/counter/Makefile
>> index 007e884..1b9a896 100644
>> --- a/drivers/iio/counter/Makefile
>> +++ b/drivers/iio/counter/Makefile
>> @@ -5,3 +5,4 @@
>>  # When adding new entries keep the list in alphabetical order
>>  
>>  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>> +obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
>> diff --git a/drivers/iio/counter/stm32-lptimer-cnt.c b/drivers/iio/counter/stm32-lptimer-cnt.c
>> new file mode 100644
>> index 0000000..1c5909b
>> --- /dev/null
>> +++ b/drivers/iio/counter/stm32-lptimer-cnt.c
>> @@ -0,0 +1,383 @@
>> +/*
>> + * STM32 Low-Power Timer Encoder and Counter driver
>> + *
>> + * Copyright (C) STMicroelectronics 2017
>> + *
>> + * Author: Fabrice Gasnier <fabrice.gasnier@...com>
>> + *
>> + * Inspired by 104-quad-8 and stm32-timer-trigger drivers.
>> + *
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/mfd/stm32-lptimer.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +
>> +struct stm32_lptim_cnt {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	struct clk *clk;
>> +	u32 preset;
>> +	u32 polarity;
>> +	u32 quadrature_mode;
>> +};
>> +
>> +static int stm32_lptim_is_enabled(struct stm32_lptim_cnt *priv)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = regmap_read(priv->regmap, STM32_LPTIM_CR, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return FIELD_GET(STM32_LPTIM_ENABLE, val);
>> +}
>> +
>> +static int stm32_lptim_set_enable_state(struct stm32_lptim_cnt *priv,
>> +					int enable)
>> +{
>> +	int ret;
>> +	u32 val;
>> +
>> +	val = FIELD_PREP(STM32_LPTIM_ENABLE, enable);
>> +	ret = regmap_write(priv->regmap, STM32_LPTIM_CR, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!enable) {
>> +		clk_disable(priv->clk);
>> +		return 0;
>> +	}
>> +
>> +	/* LP timer must be enabled before writing CMP & ARR */
>> +	ret = regmap_write(priv->regmap, STM32_LPTIM_ARR, priv->preset);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(priv->regmap, STM32_LPTIM_CMP, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* ensure CMP & ARR registers are properly written */
>> +	ret = regmap_read_poll_timeout(priv->regmap, STM32_LPTIM_ISR, val,
>> +				       (val & STM32_LPTIM_CMPOK_ARROK),
>> +				       100, 1000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_write(priv->regmap, STM32_LPTIM_ICR,
>> +			   STM32_LPTIM_CMPOKCF_ARROKCF);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = clk_enable(priv->clk);
>> +	if (ret) {
>> +		regmap_write(priv->regmap, STM32_LPTIM_CR, 0);
>> +		return ret;
>> +	}
>> +
>> +	/* Start LP timer in continuous mode */
>> +	return regmap_update_bits(priv->regmap, STM32_LPTIM_CR,
>> +				  STM32_LPTIM_CNTSTRT, STM32_LPTIM_CNTSTRT);
>> +}
>> +
>> +static int stm32_lptim_setup(struct stm32_lptim_cnt *priv, int enable)
>> +{
>> +	u32 mask = STM32_LPTIM_ENC | STM32_LPTIM_COUNTMODE |
>> +		   STM32_LPTIM_CKPOL | STM32_LPTIM_PRESC;
>> +	u32 val;
>> +
>> +	/* Setup LP timer encoder/counter and polarity, without prescaler */
>> +	if (priv->quadrature_mode)
>> +		val = enable ? STM32_LPTIM_ENC : 0;
>> +	else
>> +		val = enable ? STM32_LPTIM_COUNTMODE : 0;
>> +	val |= FIELD_PREP(STM32_LPTIM_CKPOL, enable ? priv->polarity : 0);
>> +
>> +	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,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +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_preset(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 snprintf(buf, PAGE_SIZE, "%u\n", priv->preset);
>> +}
>> +
>> +static ssize_t stm32_lptim_cnt_set_preset(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);
>> +	int ret;
>> +
>> +	if (stm32_lptim_is_enabled(priv))
>> +		return -EBUSY;
>> +
>> +	ret = kstrtouint(buf, 0, &priv->preset);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (priv->preset > STM32_LPTIM_MAX_ARR)
>> +		return -EINVAL;
>> +
>> +	return 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,
>> +		.write = stm32_lptim_cnt_set_preset,
>> +	},
>> +	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,
>> +		.write = stm32_lptim_cnt_set_preset,
>> +	},
>> +	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,
>> +};
>> +
>> +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;
>> +
>> +	if (IS_ERR_OR_NULL(ddata))
>> +		return -EINVAL;
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	priv = iio_priv(indio_dev);
>> +	priv->dev = &pdev->dev;
>> +	priv->regmap = ddata->regmap;
>> +	priv->clk = ddata->clk;
>> +	priv->preset = STM32_LPTIM_MAX_ARR;
>> +
>> +	indio_dev->name = dev_name(&pdev->dev);
>> +	indio_dev->dev.parent = &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;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	return devm_iio_device_register(&pdev->dev, indio_dev);
>> +}
>> +
>> +static const struct of_device_id stm32_lptim_cnt_of_match[] = {
>> +	{ .compatible = "st,stm32-lptimer-counter", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, stm32_lptim_cnt_of_match);
>> +
>> +static struct platform_driver stm32_lptim_cnt_driver = {
>> +	.probe = stm32_lptim_cnt_probe,
>> +	.driver = {
>> +		.name = "stm32-lptimer-counter",
>> +		.of_match_table = stm32_lptim_cnt_of_match,
>> +	},
>> +};
>> +module_platform_driver(stm32_lptim_cnt_driver);
>> +
>> +MODULE_AUTHOR("Fabrice Gasnier <fabrice.gasnier@...com>");
>> +MODULE_ALIAS("platform:stm32-lptimer-counter");
>> +MODULE_DESCRIPTION("STMicroelectronics STM32 LPTIM counter driver");
>> +MODULE_LICENSE("GPL v2");
>

Powered by blists - more mailing lists