[2/4] counter: new TI eQEP driver

Commit Message

David Lechner July 22, 2019, 3:45 p.m. UTC

This adds a new counter driver for the Texas Instruments Enhanced
Quadrature Encoder Pulse (eQEP) module.

Only very basic functionality is currently implemented - only enough to
be able to read the position. The actual device has many more features
which can be added to the driver on an as-needed basis.

Signed-off-by: David Lechner <[e-mail]>
---
 MAINTAINERS               |   6 +
 drivers/counter/Kconfig   |  12 ++
 drivers/counter/Makefile  |   1 +
 drivers/counter/ti-eqep.c | 381 ++++++++++++++++++++++++++++++++++++++
 drivers/pwm/Kconfig       |   2 +-
 5 files changed, 401 insertions(+), 1 deletion(-)
 create mode 100644 drivers/counter/ti-eqep.c

Comments

Uwe Kleine-König July 30, 2019, 12:35 p.m. UTC | #1

On Mon, Jul 22, 2019 at 10:45:36AM -0500, David Lechner wrote:
> This adds a new counter driver for the Texas Instruments Enhanced
> Quadrature Encoder Pulse (eQEP) module.
> 
> Only very basic functionality is currently implemented - only enough to
> be able to read the position. The actual device has many more features
> which can be added to the driver on an as-needed basis.
> 
> Signed-off-by: David Lechner <[e-mail]>
> ---
>  MAINTAINERS               |   6 +
>  drivers/counter/Kconfig   |  12 ++
>  drivers/counter/Makefile  |   1 +
>  drivers/counter/ti-eqep.c | 381 ++++++++++++++++++++++++++++++++++++++
>  drivers/pwm/Kconfig       |   2 +-

It's not obvious why the change to drivers/pwm/Kconfig is needed. Can
you please motivate that in the change log?

Best regards
Uwe

David Lechner July 30, 2019, 3:28 p.m. UTC | #2

On 7/30/19 7:35 AM, Uwe Kleine-König wrote:
> On Mon, Jul 22, 2019 at 10:45:36AM -0500, David Lechner wrote:
>> This adds a new counter driver for the Texas Instruments Enhanced
>> Quadrature Encoder Pulse (eQEP) module.
>>
>> Only very basic functionality is currently implemented - only enough to
>> be able to read the position. The actual device has many more features
>> which can be added to the driver on an as-needed basis.
>>
>> Signed-off-by: David Lechner <[e-mail]>
>> ---
>>   MAINTAINERS               |   6 +
>>   drivers/counter/Kconfig   |  12 ++
>>   drivers/counter/Makefile  |   1 +
>>   drivers/counter/ti-eqep.c | 381 ++++++++++++++++++++++++++++++++++++++
>>   drivers/pwm/Kconfig       |   2 +-
> 
> It's not obvious why the change to drivers/pwm/Kconfig is needed. Can
> you please motivate that in the change log?

Will do. The short version is that it is needed for power management.

> 
> Best regards
> Uwe
>

William Breathitt Gray Aug. 2, 2019, 9:27 a.m. UTC | #3

On Mon, Jul 22, 2019 at 10:45:36AM -0500, David Lechner wrote:
> This adds a new counter driver for the Texas Instruments Enhanced
> Quadrature Encoder Pulse (eQEP) module.
> 
> Only very basic functionality is currently implemented - only enough to
> be able to read the position. The actual device has many more features
> which can be added to the driver on an as-needed basis.
> 
> Signed-off-by: David Lechner <[e-mail]>

Some changes suggested below, but most of the important stuff is there
and correct so good job.

William Breathitt Gray

> ---
>  MAINTAINERS               |   6 +
>  drivers/counter/Kconfig   |  12 ++
>  drivers/counter/Makefile  |   1 +
>  drivers/counter/ti-eqep.c | 381 ++++++++++++++++++++++++++++++++++++++
>  drivers/pwm/Kconfig       |   2 +-
>  5 files changed, 401 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/counter/ti-eqep.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 783569e3c4b4..f3b5e275732b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16014,6 +16014,12 @@ S:	Maintained
>  F:	drivers/media/platform/davinci/
>  F:	include/media/davinci/
>  
> +TI ENHANCED QUADRATURE ENCODER PULSE (eQEP) DRIVER
> +R:	David Lechner <[e-mail]>
> +L:	[e-mail]
> +F:	Documentation/devicetree/bindings/counter/ti-eqep.txt
> +F:	drivers/counter/ti-eqep.c
> +
>  TI ETHERNET SWITCH DRIVER (CPSW)
>  R:	Grygorii Strashko <[e-mail]>
>  L:	[e-mail]
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2967d0a9ff91..7eeb310f0cda 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -49,6 +49,18 @@ config STM32_LPTIMER_CNT
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stm32-lptimer-cnt.
>  
> +config TI_EQEP
> +	tristate "TI eQEP counter driver"
> +	depends on (SOC_AM33XX || COMPILE_TEST)
> +	select PWM
> +	select REGMAP_MMIO
> +	help
> +	  Select this option to enable the Texas Instruments Enhanced Quadrature
> +	  Encoder Pulse (eQEP) counter driver.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ti-eqep.
> +
>  config FTM_QUADDEC
>  	tristate "Flex Timer Module Quadrature decoder driver"
>  	depends on HAS_IOMEM && OF
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 40d35522937d..55142d1f4c43 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -8,4 +8,5 @@ obj-$(CONFIG_COUNTER) += counter.o
>  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
>  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
> +obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> new file mode 100644
> index 000000000000..7aaa4abbc9c5
> --- /dev/null
> +++ b/drivers/counter/ti-eqep.c
> @@ -0,0 +1,381 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 David Lechner <[e-mail]>
> + *
> + * Counter driver for Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP)
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/counter.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +/* 32-bit registers */
> +#define QPOSCNT		0x0
> +#define QPOSINIT	0x4
> +#define QPOSMAX		0x8
> +#define QPOSCMP		0xc
> +#define QPOSILAT	0x10
> +#define QPOSSLAT	0x14
> +#define QPOSLAT		0x18
> +#define QUTMR		0x1c
> +#define QUPRD		0x20
> +
> +/* 16-bit registers */
> +#define QWDTMR		0x0	/* 0x24 */
> +#define QWDPRD		0x2	/* 0x26 */
> +#define QDECCTL		0x4	/* 0x28 */
> +#define QEPCTL		0x6	/* 0x2a */
> +#define QCAPCTL		0x8	/* 0x2c */
> +#define QPOSCTL		0xa	/* 0x2e */
> +#define QEINT		0xc	/* 0x30 */
> +#define QFLG		0xe	/* 0x32 */
> +#define QCLR		0x10	/* 0x34 */
> +#define QFRC		0x12	/* 0x36 */
> +#define QEPSTS		0x14	/* 0x38 */
> +#define QCTMR		0x16	/* 0x3a */
> +#define QCPRD		0x18	/* 0x3c */
> +#define QCTMRLAT	0x1a	/* 0x3e */
> +#define QCPRDLAT	0x1c	/* 0x40 */
> +
> +#define QDECCTL_QSRC_SHIFT	14
> +#define QDECCTL_QSRC		GENMASK(15, 14)
> +#define QDECCTL_SOEN		BIT(13)
> +#define QDECCTL_SPSEL		BIT(12)
> +#define QDECCTL_XCR		BIT(11)
> +#define QDECCTL_SWAP		BIT(10)
> +#define QDECCTL_IGATE		BIT(9)
> +#define QDECCTL_QAP		BIT(8)
> +#define QDECCTL_QBP		BIT(7)
> +#define QDECCTL_QIP		BIT(6)
> +#define QDECCTL_QSP		BIT(5)
> +
> +#define QEPCTL_FREE_SOFT	GENMASK(15, 14)
> +#define QEPCTL_PCRM		GENMASK(13, 12)
> +#define QEPCTL_SEI		GENMASK(11, 10)
> +#define QEPCTL_IEI		GENMASK(9, 8)
> +#define QEPCTL_SWI		BIT(7)
> +#define QEPCTL_SEL		BIT(6)
> +#define QEPCTL_IEL		GENMASK(5, 4)
> +#define QEPCTL_PHEN		BIT(3)
> +#define QEPCTL_QCLM		BIT(2)
> +#define QEPCTL_UTE		BIT(1)
> +#define QEPCTL_WDE		BIT(0)
> +
> +/* EQEP Inputs */
> +enum {
> +	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
> +	TI_EQEP_SIGNAL_QEPB,	/* QEPB/XDIR */
> +	TI_EQEP_SIGNAL_QEPI,	/* Index */
> +	TI_EQEP_SIGNAL_QEPS,	/* Strobe */
> +};
> +
> +/* Position Counter Input Modes */
> +enum {
> +	TI_EQEP_COUNT_FUNC_QUAD_COUNT,
> +	TI_EQEP_COUNT_FUNC_DIR_COUNT,
> +	TI_EQEP_COUNT_FUNC_UP_COUNT,
> +	TI_EQEP_COUNT_FUNC_DOWN_COUNT,
> +};
> +
> +struct ti_eqep_cnt {
> +	struct counter_device counter;
> +	struct regmap *regmap32;
> +	struct regmap *regmap16;
> +};
> +
> +static int ti_eqep_count_read(struct counter_device *counter,
> +			      struct counter_count *count,
> +			      struct counter_count_read_value *val)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 cnt;
> +
> +	regmap_read(priv->regmap32, QPOSCNT, &cnt);
> +	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cnt);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_count_write(struct counter_device *counter,
> +			       struct counter_count *count,
> +			       struct counter_count_write_value *val)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 cnt, max;
> +	int err;
> +
> +	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
> +	if (err)
> +		return err;
> +
> +	regmap_read(priv->regmap32, QPOSMAX, &max);
> +	if (cnt > max)
> +		return -EINVAL;
> +
> +	return regmap_write(priv->regmap32, QPOSCNT, cnt);
> +}
> +
> +static int ti_eqep_function_get(struct counter_device *counter,
> +				struct counter_count *count, size_t *function)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qdecctl;
> +
> +	regmap_read(priv->regmap16, QDECCTL, &qdecctl);
> +	*function = (qdecctl & QDECCTL_QSRC) >> QDECCTL_QSRC_SHIFT;
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_function_set(struct counter_device *counter,
> +				struct counter_count *count, size_t function)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	return regmap_write_bits(priv->regmap16, QDECCTL, QDECCTL_QSRC,
> +				 function << QDECCTL_QSRC_SHIFT);
> +}
> +
> +static ssize_t ti_eqep_position_ceiling_read(struct counter_device *counter,
> +					     struct counter_count *count,
> +					     void *ext_priv, char *buf)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qposmax;
> +
> +	regmap_read(priv->regmap32, QPOSMAX, &qposmax);
> +
> +	return sprintf(buf, "%u\n", qposmax);
> +}
> +
> +static ssize_t ti_eqep_position_ceiling_write(struct counter_device *counter,
> +					      struct counter_count *count,
> +					      void *ext_priv, const char *buf,
> +					      size_t len)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	int err;
> +	u32 res;
> +
> +	err = kstrtouint(buf, 10, &res);

In Documentation/ABI/testing/sysfs-bus-counter, a base is not specified
for the ceiling attribute so the expectation is for the base of the
string to be automatically detected with the conventional semantics.

That means you should pass 0 here to kstrtouint instead of 10.

> +	if (err < 0)
> +		return err;
> +
> +	regmap_write(priv->regmap32, QPOSMAX, res);
> +
> +	return len;
> +}
> +
> +static ssize_t ti_eqep_position_floor_read(struct counter_device *counter,
> +					   struct counter_count *count,
> +					   void *ext_priv, char *buf)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qposinit;
> +
> +	regmap_read(priv->regmap32, QPOSINIT, &qposinit);
> +
> +	return sprintf(buf, "%u\n", qposinit);
> +}
> +
> +static ssize_t ti_eqep_position_floor_write(struct counter_device *counter,
> +					    struct counter_count *count,
> +					    void *ext_priv, const char *buf,
> +					    size_t len)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	int err;
> +	u32 res;
> +
> +	err = kstrtouint(buf, 10, &res);

For the same reason as the ceiling attribute, you should pass 0 here to
kstrtouint instead of 10.

> +	if (err < 0)
> +		return err;
> +
> +	regmap_write(priv->regmap32, QPOSINIT, res);
> +
> +	return len;
> +}
> +
> +static ssize_t ti_eqep_position_enable_read(struct counter_device *counter,
> +					    struct counter_count *count,
> +					    void *ext_priv, char *buf)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qepctl;
> +
> +	regmap_read(priv->regmap16, QEPCTL, &qepctl);
> +
> +	return sprintf(buf, "%u\n", !!(qepctl & QEPCTL_PHEN));
> +}
> +
> +static ssize_t ti_eqep_position_enable_write(struct counter_device *counter,
> +					     struct counter_count *count,
> +					     void *ext_priv, const char *buf,
> +					     size_t len)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	int err;
> +	bool res;
> +
> +	err = kstrtobool(buf, &res);
> +	if (err < 0)
> +		return err;
> +
> +	regmap_write_bits(priv->regmap16, QEPCTL, QEPCTL_PHEN, res ? -1 : 0);
> +
> +	return len;
> +}
> +
> +static const struct regmap_config ti_eqep_regmap32_config = {
> +	.name = "32-bit",
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = 0x24,
> +};
> +
> +static const struct regmap_config ti_eqep_regmap16_config = {
> +	.name = "16-bit",
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_stride = 2,
> +	.max_register = 0x1e,
> +};
> +
> +static const struct counter_ops ti_eqep_counter_ops = {
> +	.count_read	= ti_eqep_count_read,
> +	.count_write	= ti_eqep_count_write,
> +	.function_get	= ti_eqep_function_get,
> +	.function_set	= ti_eqep_function_set,
> +};

Are you able to provide a signal_read function, or are the Signals not
exposed to the user by this device? Sometimes quadrature encoder devices
provide an instanteous read of the signal lines to tell whether they are
high or low, so I figured I'd ask.

You should define an action_get function as well along with Synapses
corresponding to each Signal. This will allow users to know whether the
Synapse fires on a rising edge, falling edge, no edge, or both edges.

For example, consider the drivers/counter/104-quad-8.c file. Each count
register has three associated signal lines: Quadrature A, Quadrature B,
and Index.

Quadrature A and B are your typical quadrature encoder lines and
depending on the function mode selected (quadrature x4, pulse-direction,
etc.) could have a Synapse action mode of none, rising edge, falling
edge, or both edges; see the quad8_synapse_actions_list array.

In contrast, the Index signal line only has two Synapse action modes:
rising edge (in the case preset functionality is enabled) or none.

For the TI eQEP driver, there will be four Synapses corresponding to the
four Signals: QEPA, QEPB, QEPI, and QEPS. See if you are able to
implement the Synapses and action_get function by using the 104-quad-8.c
file as a reference. That file does have a lot of extra functionality
tossed in compared to yours, so if you have trouble groking it, just let
me know and I'll try to help.

> +
> +static const enum counter_count_function ti_eqep_position_functions[] = {
> +	[TI_EQEP_COUNT_FUNC_QUAD_COUNT]	= COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
> +	[TI_EQEP_COUNT_FUNC_DIR_COUNT]	= COUNTER_COUNT_FUNCTION_PULSE_DIRECTION,
> +	[TI_EQEP_COUNT_FUNC_UP_COUNT]	= COUNTER_COUNT_FUNCTION_INCREASE,
> +	[TI_EQEP_COUNT_FUNC_DOWN_COUNT]	= COUNTER_COUNT_FUNCTION_DECREASE,
> +};
> +
> +static struct counter_signal ti_eqep_signals[] = {
> +	[TI_EQEP_SIGNAL_QEPA] = {
> +		.id = TI_EQEP_SIGNAL_QEPA,
> +		.name = "QEPA"
> +	},
> +	[TI_EQEP_SIGNAL_QEPB] = {
> +		.id = TI_EQEP_SIGNAL_QEPB,
> +		.name = "QEPB"
> +	},
> +	[TI_EQEP_SIGNAL_QEPI] = {
> +		.id = TI_EQEP_SIGNAL_QEPI,
> +		.name = "QEPI"
> +	},
> +	[TI_EQEP_SIGNAL_QEPS] = {
> +		.id = TI_EQEP_SIGNAL_QEPS,
> +		.name = "QEPS"
> +	},
> +};
> +
> +static struct counter_count_ext ti_eqep_position_ext[] = {
> +	{
> +		.name	= "ceiling",
> +		.read	= ti_eqep_position_ceiling_read,
> +		.write	= ti_eqep_position_ceiling_write,
> +	},
> +	{
> +		.name	= "floor",
> +		.read	= ti_eqep_position_floor_read,
> +		.write	= ti_eqep_position_floor_write,
> +	},
> +	{
> +		.name	= "enable",
> +		.read	= ti_eqep_position_enable_read,
> +		.write	= ti_eqep_position_enable_write,
> +	},
> +};
> +
> +static struct counter_count ti_eqep_counts[] = {
> +	{
> +		.id		= 0,
> +		.name		= "QPOSCNT",
> +		.functions_list	= ti_eqep_position_functions,
> +		.num_functions	= ARRAY_SIZE(ti_eqep_position_functions),
> +		.ext		= ti_eqep_position_ext,
> +		.num_ext	= ARRAY_SIZE(ti_eqep_position_ext),

When you have your Synapses defined, pass them here to your Count via
the synapses and num_synapses members.

> +	},
> +};
> +
> +static int ti_eqep_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ti_eqep_cnt *priv;
> +	struct resource *res;
> +	void __iomem *base;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	priv->regmap32 = devm_regmap_init_mmio(dev, base,
> +					       &ti_eqep_regmap32_config);
> +	if (IS_ERR(priv->regmap32))
> +		return PTR_ERR(priv->regmap32);
> +
> +	priv->regmap16 = devm_regmap_init_mmio(dev, base + 0x24,
> +					       &ti_eqep_regmap16_config);
> +	if (IS_ERR(priv->regmap16))
> +		return PTR_ERR(priv->regmap16);
> +
> +	priv->counter.name = dev_name(dev);
> +	priv->counter.parent = dev;
> +	priv->counter.ops = &ti_eqep_counter_ops;
> +	priv->counter.counts = ti_eqep_counts;
> +	priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts);
> +	priv->counter.signals = ti_eqep_signals;
> +	priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals);
> +	priv->counter.priv = priv;
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get(dev);
> +
> +	return devm_counter_register(dev, &priv->counter);
> +}
> +
> +static int ti_eqep_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	pm_runtime_put(dev),
> +	pm_runtime_disable(dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ti_eqep_of_match[] = {
> +	{ .compatible = "ti,am3352-eqep", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ti_eqep_of_match);
> +
> +static struct platform_driver ti_eqep_driver = {
> +	.probe = ti_eqep_probe,
> +	.remove = ti_eqep_remove,
> +	.driver = {
> +		.name = "ti-eqep-cnt",
> +		.of_match_table = ti_eqep_of_match,
> +	},
> +};
> +module_platform_driver(ti_eqep_driver);
> +
> +MODULE_AUTHOR("David Lechner <[e-mail]>");
> +MODULE_DESCRIPTION("TI eQEP counter driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a7e57516959e..ddcbb8573894 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -499,7 +499,7 @@ config  PWM_TIEHRPWM
>  
>  config  PWM_TIPWMSS
>  	bool
> -	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM)
> +	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM || TI_EQEP)
>  	help
>  	  PWM Subsystem driver support for AM33xx SOC.

I was surprised to see this pwm Kconfig change in this patch. Is
PWM_TIPWMSS required for TI_EQEP to work? If not required, then this
could be a separate patch; otherwise, put in a mention about why in the
commit message so that the purpose of this change is clearer.

>  
> -- 
> 2.17.1
>

David Lechner Aug. 2, 2019, 4:09 p.m. UTC | #4

On 8/2/19 4:27 AM, William Breathitt Gray wrote:
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index a7e57516959e..ddcbb8573894 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -499,7 +499,7 @@ config  PWM_TIEHRPWM
>>   
>>   config  PWM_TIPWMSS
>>   	bool
>> -	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM)
>> +	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM || TI_EQEP)
>>   	help
>>   	  PWM Subsystem driver support for AM33xx SOC.
> I was surprised to see this pwm Kconfig change in this patch. Is
> PWM_TIPWMSS required for TI_EQEP to work? If not required, then this
> could be a separate patch; otherwise, put in a mention about why in the
> commit message so that the purpose of this change is clearer.
> 

This enables the parent bus for power management. Since this is the second
comment about this, I wonder if it would make sense to move this out of the
PWM subsystem and into drivers/bus/ since it is no longer exclusive to PWM
devices.

David Lechner Aug. 2, 2019, 4:17 p.m. UTC | #5

On 8/2/19 4:27 AM, William Breathitt Gray wrote:
>> +static const struct counter_ops ti_eqep_counter_ops = {
>> +	.count_read	= ti_eqep_count_read,
>> +	.count_write	= ti_eqep_count_write,
>> +	.function_get	= ti_eqep_function_get,
>> +	.function_set	= ti_eqep_function_set,
>> +};
> Are you able to provide a signal_read function, or are the Signals not
> exposed to the user by this device? Sometimes quadrature encoder devices
> provide an instanteous read of the signal lines to tell whether they are
> high or low, so I figured I'd ask.

No, it does not look like these signals can be read directly.

> 
> You should define an action_get function as well along with Synapses
> corresponding to each Signal. This will allow users to know whether the
> Synapse fires on a rising edge, falling edge, no edge, or both edges.
> 
> For example, consider the drivers/counter/104-quad-8.c file. Each count
> register has three associated signal lines: Quadrature A, Quadrature B,
> and Index.
> 
> Quadrature A and B are your typical quadrature encoder lines and
> depending on the function mode selected (quadrature x4, pulse-direction,
> etc.) could have a Synapse action mode of none, rising edge, falling
> edge, or both edges; see the quad8_synapse_actions_list array.
> 
> In contrast, the Index signal line only has two Synapse action modes:
> rising edge (in the case preset functionality is enabled) or none.

The encoders I have don't use the index or strobe signals, so I was
thinking maybe I should omit those two signals from the driver for the
time being since I don't have a way of testing.

> 
> For the TI eQEP driver, there will be four Synapses corresponding to the
> four Signals: QEPA, QEPB, QEPI, and QEPS. See if you are able to
> implement the Synapses and action_get function by using the 104-quad-8.c
> file as a reference. That file does have a lot of extra functionality
> tossed in compared to yours, so if you have trouble groking it, just let
> me know and I'll try to help.
>

William Breathitt Gray Aug. 2, 2019, 4:40 p.m. UTC | #6

On Fri, Aug 02, 2019 at 11:17:11AM -0500, David Lechner wrote:
> On 8/2/19 4:27 AM, William Breathitt Gray wrote:
> >> +static const struct counter_ops ti_eqep_counter_ops = {
> >> +	.count_read	= ti_eqep_count_read,
> >> +	.count_write	= ti_eqep_count_write,
> >> +	.function_get	= ti_eqep_function_get,
> >> +	.function_set	= ti_eqep_function_set,
> >> +};
> > Are you able to provide a signal_read function, or are the Signals not
> > exposed to the user by this device? Sometimes quadrature encoder devices
> > provide an instanteous read of the signal lines to tell whether they are
> > high or low, so I figured I'd ask.
> 
> No, it does not look like these signals can be read directly.

All right, in that case you can ignore signal_read.

> > 
> > You should define an action_get function as well along with Synapses
> > corresponding to each Signal. This will allow users to know whether the
> > Synapse fires on a rising edge, falling edge, no edge, or both edges.
> > 
> > For example, consider the drivers/counter/104-quad-8.c file. Each count
> > register has three associated signal lines: Quadrature A, Quadrature B,
> > and Index.
> > 
> > Quadrature A and B are your typical quadrature encoder lines and
> > depending on the function mode selected (quadrature x4, pulse-direction,
> > etc.) could have a Synapse action mode of none, rising edge, falling
> > edge, or both edges; see the quad8_synapse_actions_list array.
> > 
> > In contrast, the Index signal line only has two Synapse action modes:
> > rising edge (in the case preset functionality is enabled) or none.
> 
> The encoders I have don't use the index or strobe signals, so I was
> thinking maybe I should omit those two signals from the driver for the
> time being since I don't have a way of testing.

That should be fine for now. We can add them in a later patch down the
road and keep this introduction patch simple.

William Breathitt Gray

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 783569e3c4b4..f3b5e275732b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16014,6 +16014,12 @@  S:	Maintained
 F:	drivers/media/platform/davinci/
 F:	include/media/davinci/
 
+TI ENHANCED QUADRATURE ENCODER PULSE (eQEP) DRIVER
+R:	David Lechner <[e-mail]>
+L:	[e-mail]
+F:	Documentation/devicetree/bindings/counter/ti-eqep.txt
+F:	drivers/counter/ti-eqep.c
+
 TI ETHERNET SWITCH DRIVER (CPSW)
 R:	Grygorii Strashko <[e-mail]>
 L:	[e-mail]
diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2967d0a9ff91..7eeb310f0cda 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -49,6 +49,18 @@  config STM32_LPTIMER_CNT
 	  To compile this driver as a module, choose M here: the
 	  module will be called stm32-lptimer-cnt.
 
+config TI_EQEP
+	tristate "TI eQEP counter driver"
+	depends on (SOC_AM33XX || COMPILE_TEST)
+	select PWM
+	select REGMAP_MMIO
+	help
+	  Select this option to enable the Texas Instruments Enhanced Quadrature
+	  Encoder Pulse (eQEP) counter driver.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ti-eqep.
+
 config FTM_QUADDEC
 	tristate "Flex Timer Module Quadrature decoder driver"
 	depends on HAS_IOMEM && OF
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 40d35522937d..55142d1f4c43 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -8,4 +8,5 @@  obj-$(CONFIG_COUNTER) += counter.o
 obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
+obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
 obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
new file mode 100644
index 000000000000..7aaa4abbc9c5
--- /dev/null
+++ b/drivers/counter/ti-eqep.c
@@ -0,0 +1,381 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019 David Lechner <[e-mail]>
+ *
+ * Counter driver for Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP)
+ */
+
+#include <linux/bitops.h>
+#include <linux/counter.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+/* 32-bit registers */
+#define QPOSCNT		0x0
+#define QPOSINIT	0x4
+#define QPOSMAX		0x8
+#define QPOSCMP		0xc
+#define QPOSILAT	0x10
+#define QPOSSLAT	0x14
+#define QPOSLAT		0x18
+#define QUTMR		0x1c
+#define QUPRD		0x20
+
+/* 16-bit registers */
+#define QWDTMR		0x0	/* 0x24 */
+#define QWDPRD		0x2	/* 0x26 */
+#define QDECCTL		0x4	/* 0x28 */
+#define QEPCTL		0x6	/* 0x2a */
+#define QCAPCTL		0x8	/* 0x2c */
+#define QPOSCTL		0xa	/* 0x2e */
+#define QEINT		0xc	/* 0x30 */
+#define QFLG		0xe	/* 0x32 */
+#define QCLR		0x10	/* 0x34 */
+#define QFRC		0x12	/* 0x36 */
+#define QEPSTS		0x14	/* 0x38 */
+#define QCTMR		0x16	/* 0x3a */
+#define QCPRD		0x18	/* 0x3c */
+#define QCTMRLAT	0x1a	/* 0x3e */
+#define QCPRDLAT	0x1c	/* 0x40 */
+
+#define QDECCTL_QSRC_SHIFT	14
+#define QDECCTL_QSRC		GENMASK(15, 14)
+#define QDECCTL_SOEN		BIT(13)
+#define QDECCTL_SPSEL		BIT(12)
+#define QDECCTL_XCR		BIT(11)
+#define QDECCTL_SWAP		BIT(10)
+#define QDECCTL_IGATE		BIT(9)
+#define QDECCTL_QAP		BIT(8)
+#define QDECCTL_QBP		BIT(7)
+#define QDECCTL_QIP		BIT(6)
+#define QDECCTL_QSP		BIT(5)
+
+#define QEPCTL_FREE_SOFT	GENMASK(15, 14)
+#define QEPCTL_PCRM		GENMASK(13, 12)
+#define QEPCTL_SEI		GENMASK(11, 10)
+#define QEPCTL_IEI		GENMASK(9, 8)
+#define QEPCTL_SWI		BIT(7)
+#define QEPCTL_SEL		BIT(6)
+#define QEPCTL_IEL		GENMASK(5, 4)
+#define QEPCTL_PHEN		BIT(3)
+#define QEPCTL_QCLM		BIT(2)
+#define QEPCTL_UTE		BIT(1)
+#define QEPCTL_WDE		BIT(0)
+
+/* EQEP Inputs */
+enum {
+	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
+	TI_EQEP_SIGNAL_QEPB,	/* QEPB/XDIR */
+	TI_EQEP_SIGNAL_QEPI,	/* Index */
+	TI_EQEP_SIGNAL_QEPS,	/* Strobe */
+};
+
+/* Position Counter Input Modes */
+enum {
+	TI_EQEP_COUNT_FUNC_QUAD_COUNT,
+	TI_EQEP_COUNT_FUNC_DIR_COUNT,
+	TI_EQEP_COUNT_FUNC_UP_COUNT,
+	TI_EQEP_COUNT_FUNC_DOWN_COUNT,
+};
+
+struct ti_eqep_cnt {
+	struct counter_device counter;
+	struct regmap *regmap32;
+	struct regmap *regmap16;
+};
+
+static int ti_eqep_count_read(struct counter_device *counter,
+			      struct counter_count *count,
+			      struct counter_count_read_value *val)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 cnt;
+
+	regmap_read(priv->regmap32, QPOSCNT, &cnt);
+	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cnt);
+
+	return 0;
+}
+
+static int ti_eqep_count_write(struct counter_device *counter,
+			       struct counter_count *count,
+			       struct counter_count_write_value *val)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 cnt, max;
+	int err;
+
+	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
+	if (err)
+		return err;
+
+	regmap_read(priv->regmap32, QPOSMAX, &max);
+	if (cnt > max)
+		return -EINVAL;
+
+	return regmap_write(priv->regmap32, QPOSCNT, cnt);
+}
+
+static int ti_eqep_function_get(struct counter_device *counter,
+				struct counter_count *count, size_t *function)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qdecctl;
+
+	regmap_read(priv->regmap16, QDECCTL, &qdecctl);
+	*function = (qdecctl & QDECCTL_QSRC) >> QDECCTL_QSRC_SHIFT;
+
+	return 0;
+}
+
+static int ti_eqep_function_set(struct counter_device *counter,
+				struct counter_count *count, size_t function)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+
+	return regmap_write_bits(priv->regmap16, QDECCTL, QDECCTL_QSRC,
+				 function << QDECCTL_QSRC_SHIFT);
+}
+
+static ssize_t ti_eqep_position_ceiling_read(struct counter_device *counter,
+					     struct counter_count *count,
+					     void *ext_priv, char *buf)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qposmax;
+
+	regmap_read(priv->regmap32, QPOSMAX, &qposmax);
+
+	return sprintf(buf, "%u\n", qposmax);
+}
+
+static ssize_t ti_eqep_position_ceiling_write(struct counter_device *counter,
+					      struct counter_count *count,
+					      void *ext_priv, const char *buf,
+					      size_t len)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	int err;
+	u32 res;
+
+	err = kstrtouint(buf, 10, &res);
+	if (err < 0)
+		return err;
+
+	regmap_write(priv->regmap32, QPOSMAX, res);
+
+	return len;
+}
+
+static ssize_t ti_eqep_position_floor_read(struct counter_device *counter,
+					   struct counter_count *count,
+					   void *ext_priv, char *buf)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qposinit;
+
+	regmap_read(priv->regmap32, QPOSINIT, &qposinit);
+
+	return sprintf(buf, "%u\n", qposinit);
+}
+
+static ssize_t ti_eqep_position_floor_write(struct counter_device *counter,
+					    struct counter_count *count,
+					    void *ext_priv, const char *buf,
+					    size_t len)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	int err;
+	u32 res;
+
+	err = kstrtouint(buf, 10, &res);
+	if (err < 0)
+		return err;
+
+	regmap_write(priv->regmap32, QPOSINIT, res);
+
+	return len;
+}
+
+static ssize_t ti_eqep_position_enable_read(struct counter_device *counter,
+					    struct counter_count *count,
+					    void *ext_priv, char *buf)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qepctl;
+
+	regmap_read(priv->regmap16, QEPCTL, &qepctl);
+
+	return sprintf(buf, "%u\n", !!(qepctl & QEPCTL_PHEN));
+}
+
+static ssize_t ti_eqep_position_enable_write(struct counter_device *counter,
+					     struct counter_count *count,
+					     void *ext_priv, const char *buf,
+					     size_t len)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	int err;
+	bool res;
+
+	err = kstrtobool(buf, &res);
+	if (err < 0)
+		return err;
+
+	regmap_write_bits(priv->regmap16, QEPCTL, QEPCTL_PHEN, res ? -1 : 0);
+
+	return len;
+}
+
+static const struct regmap_config ti_eqep_regmap32_config = {
+	.name = "32-bit",
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = 0x24,
+};
+
+static const struct regmap_config ti_eqep_regmap16_config = {
+	.name = "16-bit",
+	.reg_bits = 16,
+	.val_bits = 16,
+	.reg_stride = 2,
+	.max_register = 0x1e,
+};
+
+static const struct counter_ops ti_eqep_counter_ops = {
+	.count_read	= ti_eqep_count_read,
+	.count_write	= ti_eqep_count_write,
+	.function_get	= ti_eqep_function_get,
+	.function_set	= ti_eqep_function_set,
+};
+
+static const enum counter_count_function ti_eqep_position_functions[] = {
+	[TI_EQEP_COUNT_FUNC_QUAD_COUNT]	= COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
+	[TI_EQEP_COUNT_FUNC_DIR_COUNT]	= COUNTER_COUNT_FUNCTION_PULSE_DIRECTION,
+	[TI_EQEP_COUNT_FUNC_UP_COUNT]	= COUNTER_COUNT_FUNCTION_INCREASE,
+	[TI_EQEP_COUNT_FUNC_DOWN_COUNT]	= COUNTER_COUNT_FUNCTION_DECREASE,
+};
+
+static struct counter_signal ti_eqep_signals[] = {
+	[TI_EQEP_SIGNAL_QEPA] = {
+		.id = TI_EQEP_SIGNAL_QEPA,
+		.name = "QEPA"
+	},
+	[TI_EQEP_SIGNAL_QEPB] = {
+		.id = TI_EQEP_SIGNAL_QEPB,
+		.name = "QEPB"
+	},
+	[TI_EQEP_SIGNAL_QEPI] = {
+		.id = TI_EQEP_SIGNAL_QEPI,
+		.name = "QEPI"
+	},
+	[TI_EQEP_SIGNAL_QEPS] = {
+		.id = TI_EQEP_SIGNAL_QEPS,
+		.name = "QEPS"
+	},
+};
+
+static struct counter_count_ext ti_eqep_position_ext[] = {
+	{
+		.name	= "ceiling",
+		.read	= ti_eqep_position_ceiling_read,
+		.write	= ti_eqep_position_ceiling_write,
+	},
+	{
+		.name	= "floor",
+		.read	= ti_eqep_position_floor_read,
+		.write	= ti_eqep_position_floor_write,
+	},
+	{
+		.name	= "enable",
+		.read	= ti_eqep_position_enable_read,
+		.write	= ti_eqep_position_enable_write,
+	},
+};
+
+static struct counter_count ti_eqep_counts[] = {
+	{
+		.id		= 0,
+		.name		= "QPOSCNT",
+		.functions_list	= ti_eqep_position_functions,
+		.num_functions	= ARRAY_SIZE(ti_eqep_position_functions),
+		.ext		= ti_eqep_position_ext,
+		.num_ext	= ARRAY_SIZE(ti_eqep_position_ext),
+	},
+};
+
+static int ti_eqep_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ti_eqep_cnt *priv;
+	struct resource *res;
+	void __iomem *base;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->regmap32 = devm_regmap_init_mmio(dev, base,
+					       &ti_eqep_regmap32_config);
+	if (IS_ERR(priv->regmap32))
+		return PTR_ERR(priv->regmap32);
+
+	priv->regmap16 = devm_regmap_init_mmio(dev, base + 0x24,
+					       &ti_eqep_regmap16_config);
+	if (IS_ERR(priv->regmap16))
+		return PTR_ERR(priv->regmap16);
+
+	priv->counter.name = dev_name(dev);
+	priv->counter.parent = dev;
+	priv->counter.ops = &ti_eqep_counter_ops;
+	priv->counter.counts = ti_eqep_counts;
+	priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts);
+	priv->counter.signals = ti_eqep_signals;
+	priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals);
+	priv->counter.priv = priv;
+
+	pm_runtime_enable(dev);
+	pm_runtime_get(dev);
+
+	return devm_counter_register(dev, &priv->counter);
+}
+
+static int ti_eqep_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	pm_runtime_put(dev),
+	pm_runtime_disable(dev);
+
+	return 0;
+}
+
+static const struct of_device_id ti_eqep_of_match[] = {
+	{ .compatible = "ti,am3352-eqep", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ti_eqep_of_match);
+
+static struct platform_driver ti_eqep_driver = {
+	.probe = ti_eqep_probe,
+	.remove = ti_eqep_remove,
+	.driver = {
+		.name = "ti-eqep-cnt",
+		.of_match_table = ti_eqep_of_match,
+	},
+};
+module_platform_driver(ti_eqep_driver);
+
+MODULE_AUTHOR("David Lechner <[e-mail]>");
+MODULE_DESCRIPTION("TI eQEP counter driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a7e57516959e..ddcbb8573894 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -499,7 +499,7 @@  config  PWM_TIEHRPWM
 
 config  PWM_TIPWMSS
 	bool
-	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM)
+	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM || TI_EQEP)
 	help
 	  PWM Subsystem driver support for AM33xx SOC.