* [PATCH 0/3] Atmel TCB capture driver
@ 2020-04-06 15:53 Kamel Bouhara
2020-04-06 15:53 ` [PATCH 1/3] ARM: at91: add atmel tcb capabilities Kamel Bouhara
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Kamel Bouhara @ 2020-04-06 15:53 UTC (permalink / raw)
To: William Breathitt Gray, Rob Herring, Mark Rutland, Nicolas Ferre,
Alexandre Belloni, Ludovic Desroches, linux-arm-kernel
Cc: Thomas Petazzoni, linux-input, devicetree, linux-iio, Kamel Bouhara
Hello,
Here is a new counter driver to support Atmel TCB capture devices.
Each SoC has two TCB blocks, each one including three independent
channels.The following series adds support for two counter modes:
increase and quadrature decoder.
As for the atmel clocksource and pwm, the counter driver needs to fill
some tcb capabilities in order to operate with the right configuration.
This is achieved in first patch of this series.
Please feel free to comment.
Cheers,
Kamel Bouhara (3):
ARM: at91: add atmel tcb capabilities
dt-bindings: counter: atmel-tcb-capture counter
counter: Add atmel TCB capture counter
.../bindings/counter/atmel-tcb-capture.yaml | 35 ++
drivers/counter/Kconfig | 11 +
drivers/counter/Makefile | 1 +
drivers/counter/atmel-tcb-capture.c | 388 ++++++++++++++++++
include/soc/at91/atmel_tcb.h | 3 +-
5 files changed, 437 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/counter/atmel-tcb-capture.yaml
create mode 100644 drivers/counter/atmel-tcb-capture.c
--
2.25.0
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] ARM: at91: add atmel tcb capabilities 2020-04-06 15:53 [PATCH 0/3] Atmel TCB capture driver Kamel Bouhara @ 2020-04-06 15:53 ` Kamel Bouhara 2020-04-07 16:53 ` William Breathitt Gray 2020-04-06 15:53 ` [PATCH 2/3] dt-bindings: counter: atmel-tcb-capture counter Kamel Bouhara 2020-04-06 15:53 ` [PATCH 3/3] counter: Add atmel TCB capture counter Kamel Bouhara 2 siblings, 1 reply; 10+ messages in thread From: Kamel Bouhara @ 2020-04-06 15:53 UTC (permalink / raw) To: William Breathitt Gray, Rob Herring, Mark Rutland, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel Cc: Thomas Petazzoni, linux-input, devicetree, linux-iio, Kamel Bouhara Some atmel socs have extra tcb capabilities that allow using a generic clock source or enabling a quadrature decoder. Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> --- include/soc/at91/atmel_tcb.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/soc/at91/atmel_tcb.h b/include/soc/at91/atmel_tcb.h index fbf5474f4484..6b9c9bec982e 100644 --- a/include/soc/at91/atmel_tcb.h +++ b/include/soc/at91/atmel_tcb.h @@ -39,7 +39,8 @@ struct clk; */ struct atmel_tcb_config { size_t counter_width; - unsigned int has_gclk:1; + bool has_gclk; + bool has_qdec; }; /** -- 2.25.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ARM: at91: add atmel tcb capabilities 2020-04-06 15:53 ` [PATCH 1/3] ARM: at91: add atmel tcb capabilities Kamel Bouhara @ 2020-04-07 16:53 ` William Breathitt Gray 2020-04-07 20:21 ` Kamel Bouhara 0 siblings, 1 reply; 10+ messages in thread From: William Breathitt Gray @ 2020-04-07 16:53 UTC (permalink / raw) To: Kamel Bouhara Cc: Rob Herring, Mark Rutland, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel, Thomas Petazzoni, linux-input, devicetree, linux-iio [-- Attachment #1: Type: text/plain, Size: 1077 bytes --] On Mon, Apr 06, 2020 at 05:53:18PM +0200, Kamel Bouhara wrote: > Some atmel socs have extra tcb capabilities that allow using a generic > clock source or enabling a quadrature decoder. > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > --- > include/soc/at91/atmel_tcb.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/soc/at91/atmel_tcb.h b/include/soc/at91/atmel_tcb.h > index fbf5474f4484..6b9c9bec982e 100644 > --- a/include/soc/at91/atmel_tcb.h > +++ b/include/soc/at91/atmel_tcb.h > @@ -39,7 +39,8 @@ struct clk; > */ > struct atmel_tcb_config { > size_t counter_width; > - unsigned int has_gclk:1; Hi Kamel, I'm having trouble applying this patch, so I think we have different bases. Specifically, I'm not sure where this has_gclk member was introduced. It doesn't look like it's in the testing branch of the IIO tree yet. What branch did you base your patches on? Thanks, William Breathitt Gray > + bool has_gclk; > + bool has_qdec; > }; > > /** > -- > 2.25.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] ARM: at91: add atmel tcb capabilities 2020-04-07 16:53 ` William Breathitt Gray @ 2020-04-07 20:21 ` Kamel Bouhara 0 siblings, 0 replies; 10+ messages in thread From: Kamel Bouhara @ 2020-04-07 20:21 UTC (permalink / raw) To: William Breathitt Gray Cc: Rob Herring, Mark Rutland, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel, Thomas Petazzoni, linux-input, devicetree, linux-iio On Tue, Apr 07, 2020 at 12:53:35PM -0400, William Breathitt Gray wrote: > On Mon, Apr 06, 2020 at 05:53:18PM +0200, Kamel Bouhara wrote: > > Some atmel socs have extra tcb capabilities that allow using a generic > > clock source or enabling a quadrature decoder. > > > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > > --- > > include/soc/at91/atmel_tcb.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/include/soc/at91/atmel_tcb.h b/include/soc/at91/atmel_tcb.h > > index fbf5474f4484..6b9c9bec982e 100644 > > --- a/include/soc/at91/atmel_tcb.h > > +++ b/include/soc/at91/atmel_tcb.h > > @@ -39,7 +39,8 @@ struct clk; > > */ > > struct atmel_tcb_config { > > size_t counter_width; > > - unsigned int has_gclk:1; > > Hi Kamel, > Hi William, > I'm having trouble applying this patch, so I think we have different > bases. Specifically, I'm not sure where this has_gclk member was > introduced. It doesn't look like it's in the testing branch of the IIO > tree yet. > > What branch did you base your patches on? You're right, this line should not be here. I'll fix it for v2. Thanks, Kamel > > Thanks, > > William Breathitt Gray > > > + bool has_gclk; > > + bool has_qdec; > > }; > > > > /** > > -- > > 2.25.0 > > -- Kamel Bouhara, Bootlin Embedded Linux and kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] dt-bindings: counter: atmel-tcb-capture counter 2020-04-06 15:53 [PATCH 0/3] Atmel TCB capture driver Kamel Bouhara 2020-04-06 15:53 ` [PATCH 1/3] ARM: at91: add atmel tcb capabilities Kamel Bouhara @ 2020-04-06 15:53 ` Kamel Bouhara 2020-04-06 15:53 ` [PATCH 3/3] counter: Add atmel TCB capture counter Kamel Bouhara 2 siblings, 0 replies; 10+ messages in thread From: Kamel Bouhara @ 2020-04-06 15:53 UTC (permalink / raw) To: William Breathitt Gray, Rob Herring, Mark Rutland, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel Cc: Thomas Petazzoni, linux-input, devicetree, linux-iio, Kamel Bouhara Describe the devicetree binding for the ATMEL TCB counter. Each counter blocks exposes three independent counters. However, when configured in quadrature decoder, both channel <0> and <1> are required for speed/position and rotation capture (yet only the position is captured). Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> --- .../bindings/counter/atmel-tcb-capture.yaml | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 Documentation/devicetree/bindings/counter/atmel-tcb-capture.yaml diff --git a/Documentation/devicetree/bindings/counter/atmel-tcb-capture.yaml b/Documentation/devicetree/bindings/counter/atmel-tcb-capture.yaml new file mode 100644 index 000000000000..484b2942ad5c --- /dev/null +++ b/Documentation/devicetree/bindings/counter/atmel-tcb-capture.yaml @@ -0,0 +1,35 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/counter/atmel-tcb-capture.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Atmel TCB Counter + +maintainers: + - Kamel Bouhara <kamel.bouhara@bootlin.com> + +properties: + compatible: + const: "atmel,tcb-capture" + + reg: + description: TCB capture channel to register as counter device. + Each channel is independent therefore only one channel is + registered by default execpt for the QDEC mode where both TCB0's + channels <0> and <1> are required. + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + tcb0: timer@f800c000 { + qdec: timer@0 { + compatible = "atmel,tcb-capture"; + reg = <0>, <1>; + }; + }; -- 2.25.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] counter: Add atmel TCB capture counter 2020-04-06 15:53 [PATCH 0/3] Atmel TCB capture driver Kamel Bouhara 2020-04-06 15:53 ` [PATCH 1/3] ARM: at91: add atmel tcb capabilities Kamel Bouhara 2020-04-06 15:53 ` [PATCH 2/3] dt-bindings: counter: atmel-tcb-capture counter Kamel Bouhara @ 2020-04-06 15:53 ` Kamel Bouhara 2020-04-08 21:31 ` William Breathitt Gray 2 siblings, 1 reply; 10+ messages in thread From: Kamel Bouhara @ 2020-04-06 15:53 UTC (permalink / raw) To: William Breathitt Gray, Rob Herring, Mark Rutland, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel Cc: Thomas Petazzoni, linux-input, devicetree, linux-iio, Kamel Bouhara This drivers allows to use the capture mode of the Timer Counter Block hardware block available in Atmel SoCs through the counter subsystem. Two functions of the counter are supported for the moment: period capture and quadrature decoder. The latter is only supported by the SAMA5 series of SoCs. For the period capture mode a basic setup has been chosen that will reset the counter each time the period is actually reached. Of course the device offers much more possibilities. For quadrature mode, both channel 0 and 1 must be configured even if we only capture the position (no revolution/rotation). Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> --- drivers/counter/Kconfig | 11 + drivers/counter/Makefile | 1 + drivers/counter/atmel-tcb-capture.c | 388 ++++++++++++++++++++++++++++ 3 files changed, 400 insertions(+) create mode 100644 drivers/counter/atmel-tcb-capture.c diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig index c80fa76bb531..c50d7453ec33 100644 --- a/drivers/counter/Kconfig +++ b/drivers/counter/Kconfig @@ -70,4 +70,15 @@ config FTM_QUADDEC To compile this driver as a module, choose M here: the module will be called ftm-quaddec. +config ATMEL_TCB_CAPTURE + tristate "Atmel Timer Counter Capture driver" + depends on HAS_IOMEM && OF + select REGMAP_MMIO + help + Select this option to enable the Atmel Timer Counter Block + capture driver. + + To compile this driver as a module, choose M here: the + module will be called atmel-tcb-capture. + endif # COUNTER diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile index 55142d1f4c43..70c5b8924588 100644 --- a/drivers/counter/Makefile +++ b/drivers/counter/Makefile @@ -10,3 +10,4 @@ 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 +obj-$(CONFIG_ATMEL_TCB_CAPTURE) += atmel-tcb-capture.o diff --git a/drivers/counter/atmel-tcb-capture.c b/drivers/counter/atmel-tcb-capture.c new file mode 100644 index 000000000000..6ac809503f90 --- /dev/null +++ b/drivers/counter/atmel-tcb-capture.c @@ -0,0 +1,388 @@ +// SPDX-License-Identifier: GPL-2.0-only +/** + * Copyright (C) 2020 Atmel + * + * Author: Kamel Bouhara <kamel.bouhara@bootlin.com> + * + */ +#include <linux/clk.h> +#include <linux/counter.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <soc/at91/atmel_tcb.h> + +#define ATMEL_TC_CMR_MASK (ATMEL_TC_LDRA_RISING | ATMEL_TC_LDRB_FALLING | \ + ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_LDBDIS | \ + ATMEL_TC_LDBSTOP) + +#define ATMEL_TC_QDEN BIT(8) +#define ATMEL_TC_POSEN BIT(9) + +struct atmel_tc_data { + const struct atmel_tcb_config *tc_cfg; + struct counter_device counter; + struct regmap *regmap; + int qdec_mode; + int num_channels; + int channel[2]; + bool trig_inverted; +}; + +enum atmel_tc_count_function { + ATMEL_TC_FUNCTION_INCREASE, + ATMEL_TC_FUNCTION_QUADRATURE, +}; + +static enum counter_count_function atmel_tc_count_functions[] = { + [ATMEL_TC_FUNCTION_INCREASE] = COUNTER_COUNT_FUNCTION_INCREASE, + [ATMEL_TC_FUNCTION_QUADRATURE] = COUNTER_COUNT_FUNCTION_QUADRATURE_X4, +}; + +enum atmel_tc_synapse_action { + ATMEL_TC_SYNAPSE_ACTION_NONE = 0, + ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE, + ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE, + ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE +}; + +static enum counter_synapse_action atmel_tc_synapse_actions[] = { + [ATMEL_TC_SYNAPSE_ACTION_NONE] = COUNTER_SYNAPSE_ACTION_NONE, + [ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE] = COUNTER_SYNAPSE_ACTION_RISING_EDGE, + [ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE] = COUNTER_SYNAPSE_ACTION_FALLING_EDGE, + [ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE] = COUNTER_SYNAPSE_ACTION_BOTH_EDGES, +}; + +static struct counter_signal atmel_tc_count_signals[] = { + { + .id = 0, + .name = "Channel A", + }, + { + .id = 1, + .name = "Channel B", + } +}; + +static struct counter_synapse atmel_tc_count_synapses[] = { + { + .actions_list = atmel_tc_synapse_actions, + .num_actions = ARRAY_SIZE(atmel_tc_synapse_actions), + .signal = &atmel_tc_count_signals[0] + }, + { + .actions_list = atmel_tc_synapse_actions, + .num_actions = ARRAY_SIZE(atmel_tc_synapse_actions), + .signal = &atmel_tc_count_signals[1] + } +}; + +static int atmel_tc_count_function_get(struct counter_device *counter, + struct counter_count *count, + size_t *function) +{ + struct atmel_tc_data *const priv = counter->priv; + + if (priv->qdec_mode) + *function = ATMEL_TC_FUNCTION_QUADRATURE; + else + *function = ATMEL_TC_FUNCTION_INCREASE; + + return 0; +} + +static int atmel_tc_count_function_set(struct counter_device *counter, + struct counter_count *count, + size_t function) +{ + struct atmel_tc_data *const priv = counter->priv; + u32 bmr, cmr; + + regmap_read(priv->regmap, ATMEL_TC_BMR, &bmr); + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr); + + /* Set capture mode */ + cmr &= ~ATMEL_TC_WAVE; + + switch (function) { + case ATMEL_TC_FUNCTION_INCREASE: + priv->qdec_mode = 0; + /* Set highest rate based on whether soc has gclk or not */ + bmr &= ~(ATMEL_TC_QDEN | ATMEL_TC_POSEN); + if (priv->tc_cfg->has_gclk) + cmr |= ATMEL_TC_TIMER_CLOCK2; + else + cmr |= ATMEL_TC_TIMER_CLOCK1; + /* Setup the period capture mode */ + cmr |= ATMEL_TC_CMR_MASK; + cmr &= ~(ATMEL_TC_ABETRG | ATMEL_TC_XC0); + break; + case ATMEL_TC_FUNCTION_QUADRATURE: + if (!priv->tc_cfg->has_qdec) + return -ENODEV; + /* In QDEC mode settings both channels 0 and 1 are required */ + if (priv->num_channels < 2 || priv->channel[0] != 0 || + priv->channel[1] != 1) { + pr_err("Invalid channels number or id for quadrature mode\n"); + return -EINVAL; + } + priv->qdec_mode = 1; + bmr |= ATMEL_TC_QDEN | ATMEL_TC_POSEN; + cmr |= ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_ABETRG | ATMEL_TC_XC0; + break; + } + + regmap_write(priv->regmap, ATMEL_TC_BMR, bmr); + regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), cmr); + + /* Enable clock and trigger counter */ + regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], CCR), + ATMEL_TC_CLKEN | ATMEL_TC_SWTRG); + + if (priv->qdec_mode) { + regmap_write(priv->regmap, + ATMEL_TC_REG(priv->channel[1], CMR), cmr); + regmap_write(priv->regmap, + ATMEL_TC_REG(priv->channel[1], CCR), + ATMEL_TC_CLKEN | ATMEL_TC_SWTRG); + } + + return 0; +} + +static int atmel_tc_count_signal_read(struct counter_device *counter, + struct counter_signal *signal, + enum counter_signal_value *val) +{ + struct atmel_tc_data *const priv = counter->priv; + bool sigstatus; + u32 sr; + + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr); + + if (priv->trig_inverted) + sigstatus = (sr & ATMEL_TC_MTIOB); + else + sigstatus = (sr & ATMEL_TC_MTIOA); + + *val = sigstatus ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW; + + return 0; +} + +static int atmel_tc_count_action_get(struct counter_device *counter, + struct counter_count *count, + struct counter_synapse *synapse, + size_t *action) +{ + struct atmel_tc_data *const priv = counter->priv; + u32 cmr; + + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr); + + *action = ATMEL_TC_SYNAPSE_ACTION_NONE; + + if (cmr & ATMEL_TC_ETRGEDG_NONE) + *action = ATMEL_TC_SYNAPSE_ACTION_NONE; + else if (cmr & ATMEL_TC_ETRGEDG_RISING) + *action = ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE; + else if (cmr & ATMEL_TC_ETRGEDG_FALLING) + *action = ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE; + else if (cmr & ATMEL_TC_ETRGEDG_BOTH) + *action = ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE; + + return 0; +} + +static int atmel_tc_count_action_set(struct counter_device *counter, + struct counter_count *count, + struct counter_synapse *synapse, + size_t action) +{ + struct atmel_tc_data *const priv = counter->priv; + u32 edge = ATMEL_TC_ETRGEDG_NONE; + + switch (action) { + case ATMEL_TC_SYNAPSE_ACTION_NONE: + edge = ATMEL_TC_ETRGEDG_NONE; + break; + case ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE: + edge = ATMEL_TC_ETRGEDG_RISING; + break; + case ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE: + edge = ATMEL_TC_ETRGEDG_FALLING; + break; + case ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE: + edge = ATMEL_TC_ETRGEDG_BOTH; + break; + } + + return regmap_write_bits(priv->regmap, + ATMEL_TC_REG(priv->channel[0], CMR), + ATMEL_TC_ETRGEDG, edge); +} + +static int atmel_tc_count_read(struct counter_device *counter, + struct counter_count *count, + unsigned long *val) +{ + struct atmel_tc_data *const priv = counter->priv; + u32 cnt; + + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CV), &cnt); + *val = cnt; + + return 0; +} + +static struct counter_count atmel_tc_count = { + .id = 0, + .name = "Timer Counter", + .functions_list = atmel_tc_count_functions, + .num_functions = ARRAY_SIZE(atmel_tc_count_functions), + .synapses = atmel_tc_count_synapses, + .num_synapses = ARRAY_SIZE(atmel_tc_count_synapses), +}; + +static struct counter_ops atmel_tc_ops = { + .signal_read = atmel_tc_count_signal_read, + .count_read = atmel_tc_count_read, + .count_write = NULL, + .function_get = atmel_tc_count_function_get, + .function_set = atmel_tc_count_function_set, + .action_get = atmel_tc_count_action_get, + .action_set = atmel_tc_count_action_set +}; + +static const struct atmel_tcb_config tcb_rm9200_config = { + .counter_width = 16, +}; + +static const struct atmel_tcb_config tcb_sam9x5_config = { + .counter_width = 32, +}; + +static const struct atmel_tcb_config tcb_sama5d2_config = { + .counter_width = 32, + .has_gclk = true, + .has_qdec = true, +}; + +static const struct atmel_tcb_config tcb_sama5d3_config = { + .counter_width = 32, + .has_qdec = true, +}; + +static const struct of_device_id atmel_tc_of_match[] = { + { .compatible = "atmel,at91rm9200-tcb", .data = &tcb_rm9200_config, }, + { .compatible = "atmel,at91sam9x5-tcb", .data = &tcb_sam9x5_config, }, + { .compatible = "atmel,sama5d2-tcb", .data = &tcb_sama5d2_config, }, + { .compatible = "atmel,sama5d3-tcb", .data = &tcb_sama5d3_config, }, + { /* sentinel */ } +}; + +static int atmel_tc_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + const struct atmel_tcb_config *tcb_config; + const struct of_device_id *match; + struct atmel_tc_data *priv; + char clk_name[] = "t0_clk"; + struct regmap *regmap; + struct clk *clk[3]; + int channel; + int ret, i; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + platform_set_drvdata(pdev, priv); + + match = of_match_node(atmel_tc_of_match, np->parent); + tcb_config = match->data; + if (!tcb_config) { + dev_err(&pdev->dev, "No matching parent node found\n"); + return -ENODEV; + } + + regmap = syscon_node_to_regmap(np->parent); + if (IS_ERR(priv->regmap)) + return PTR_ERR(priv->regmap); + + /* max. channels number is 2 when in QDEC mode */ + priv->num_channels = of_property_count_u32_elems(np, "reg"); + if (priv->num_channels < 0) { + dev_err(&pdev->dev, "Invalid or missing channel\n"); + return -EINVAL; + } + + /* Register channels and initialize clocks */ + for (i = 0; i < priv->num_channels; i++) { + ret = of_property_read_u32_index(np, "reg", i, &channel); + if (ret < 0 || channel > 2) + return -ENODEV; + + priv->channel[i] = channel; + + clk_name[1] += channel; + clk[i] = of_clk_get_by_name(np->parent, clk_name); + if (IS_ERR(clk[i])) { + /* Fallback to t0_clk */ + clk[i] = of_clk_get_by_name(np->parent, "t0_clk"); + if (IS_ERR(clk[i])) + return PTR_ERR(clk[i]); + } + + ret = clk_prepare_enable(clk[i]); + if (ret) + return ret; + + dev_info(&pdev->dev, + "Initialized capture mode on channel %d\n", + channel); + } + + priv->tc_cfg = tcb_config; + priv->regmap = regmap; + priv->counter.name = dev_name(&pdev->dev); + priv->counter.parent = &pdev->dev; + priv->counter.ops = &atmel_tc_ops; + priv->counter.num_counts = 1; + priv->counter.counts = &atmel_tc_count; + priv->counter.num_signals = ARRAY_SIZE(atmel_tc_count_signals); + priv->counter.signals = atmel_tc_count_signals; + priv->counter.priv = priv; + + ret = devm_counter_register(&pdev->dev, &priv->counter); + if (ret < 0) { + for (i = 0; i < priv->num_channels; i++) + clk_disable_unprepare(clk[i]); + return ret; + } + + return 0; +} + +static const struct of_device_id atmel_tc_dt_ids[] = { + { .compatible = "atmel,tcb-capture", }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, atmel_tc_dt_ids); + +static struct platform_driver atmel_tc_driver = { + .probe = atmel_tc_probe, + .driver = { + .name = "atmel-tcb-capture", + .of_match_table = atmel_tc_dt_ids, + }, +}; +module_platform_driver(atmel_tc_driver); + +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>"); +MODULE_DESCRIPTION("Atmel TCB Capture driver"); +MODULE_LICENSE("GPL v2"); -- 2.25.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] counter: Add atmel TCB capture counter 2020-04-06 15:53 ` [PATCH 3/3] counter: Add atmel TCB capture counter Kamel Bouhara @ 2020-04-08 21:31 ` William Breathitt Gray 2020-04-08 21:56 ` Alexandre Belloni 2020-04-09 8:25 ` Kamel Bouhara 0 siblings, 2 replies; 10+ messages in thread From: William Breathitt Gray @ 2020-04-08 21:31 UTC (permalink / raw) To: Kamel Bouhara Cc: Rob Herring, Mark Rutland, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel, Thomas Petazzoni, linux-input, devicetree, linux-iio [-- Attachment #1: Type: text/plain, Size: 15907 bytes --] On Mon, Apr 06, 2020 at 05:53:20PM +0200, Kamel Bouhara wrote: > This drivers allows to use the capture mode of the Timer Counter Block > hardware block available in Atmel SoCs through the counter subsystem. > > Two functions of the counter are supported for the moment: period > capture and quadrature decoder. The latter is only supported by the > SAMA5 series of SoCs. > > For the period capture mode a basic setup has been chosen that will > reset the counter each time the period is actually reached. Of course > the device offers much more possibilities. > > For quadrature mode, both channel 0 and 1 must be configured even if we > only capture the position (no revolution/rotation). > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> Hi Kamel, Thank you for submitting support for this driver. Since this is a new counter driver, make sure to create an entry for it in the top-level MAINTAINERS file so users know who to contact to report bugs and other issues. I've written some more comments inline below. > --- > drivers/counter/Kconfig | 11 + > drivers/counter/Makefile | 1 + > drivers/counter/atmel-tcb-capture.c | 388 ++++++++++++++++++++++++++++ > 3 files changed, 400 insertions(+) > create mode 100644 drivers/counter/atmel-tcb-capture.c > > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig > index c80fa76bb531..c50d7453ec33 100644 > --- a/drivers/counter/Kconfig > +++ b/drivers/counter/Kconfig > @@ -70,4 +70,15 @@ config FTM_QUADDEC > To compile this driver as a module, choose M here: the > module will be called ftm-quaddec. > > +config ATMEL_TCB_CAPTURE > + tristate "Atmel Timer Counter Capture driver" > + depends on HAS_IOMEM && OF > + select REGMAP_MMIO > + help > + Select this option to enable the Atmel Timer Counter Block > + capture driver. > + > + To compile this driver as a module, choose M here: the > + module will be called atmel-tcb-capture. > + > endif # COUNTER > diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile > index 55142d1f4c43..70c5b8924588 100644 > --- a/drivers/counter/Makefile > +++ b/drivers/counter/Makefile > @@ -10,3 +10,4 @@ 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 > +obj-$(CONFIG_ATMEL_TCB_CAPTURE) += atmel-tcb-capture.o > diff --git a/drivers/counter/atmel-tcb-capture.c b/drivers/counter/atmel-tcb-capture.c > new file mode 100644 > index 000000000000..6ac809503f90 > --- /dev/null > +++ b/drivers/counter/atmel-tcb-capture.c > @@ -0,0 +1,388 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/** > + * Copyright (C) 2020 Atmel > + * > + * Author: Kamel Bouhara <kamel.bouhara@bootlin.com> > + * > + */ > +#include <linux/clk.h> > +#include <linux/counter.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <soc/at91/atmel_tcb.h> > + > +#define ATMEL_TC_CMR_MASK (ATMEL_TC_LDRA_RISING | ATMEL_TC_LDRB_FALLING | \ > + ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_LDBDIS | \ > + ATMEL_TC_LDBSTOP) > + > +#define ATMEL_TC_QDEN BIT(8) > +#define ATMEL_TC_POSEN BIT(9) > + > +struct atmel_tc_data { > + const struct atmel_tcb_config *tc_cfg; > + struct counter_device counter; > + struct regmap *regmap; > + int qdec_mode; > + int num_channels; > + int channel[2]; > + bool trig_inverted; > +}; > + > +enum atmel_tc_count_function { > + ATMEL_TC_FUNCTION_INCREASE, > + ATMEL_TC_FUNCTION_QUADRATURE, > +}; > + > +static enum counter_count_function atmel_tc_count_functions[] = { > + [ATMEL_TC_FUNCTION_INCREASE] = COUNTER_COUNT_FUNCTION_INCREASE, > + [ATMEL_TC_FUNCTION_QUADRATURE] = COUNTER_COUNT_FUNCTION_QUADRATURE_X4, > +}; > + > +enum atmel_tc_synapse_action { > + ATMEL_TC_SYNAPSE_ACTION_NONE = 0, > + ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE, > + ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE, > + ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE > +}; > + > +static enum counter_synapse_action atmel_tc_synapse_actions[] = { > + [ATMEL_TC_SYNAPSE_ACTION_NONE] = COUNTER_SYNAPSE_ACTION_NONE, > + [ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE] = COUNTER_SYNAPSE_ACTION_RISING_EDGE, > + [ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE] = COUNTER_SYNAPSE_ACTION_FALLING_EDGE, > + [ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE] = COUNTER_SYNAPSE_ACTION_BOTH_EDGES, > +}; > + > +static struct counter_signal atmel_tc_count_signals[] = { > + { > + .id = 0, > + .name = "Channel A", > + }, > + { > + .id = 1, > + .name = "Channel B", > + } > +}; > + > +static struct counter_synapse atmel_tc_count_synapses[] = { > + { > + .actions_list = atmel_tc_synapse_actions, > + .num_actions = ARRAY_SIZE(atmel_tc_synapse_actions), > + .signal = &atmel_tc_count_signals[0] > + }, > + { > + .actions_list = atmel_tc_synapse_actions, > + .num_actions = ARRAY_SIZE(atmel_tc_synapse_actions), > + .signal = &atmel_tc_count_signals[1] > + } > +}; > + > +static int atmel_tc_count_function_get(struct counter_device *counter, > + struct counter_count *count, > + size_t *function) > +{ > + struct atmel_tc_data *const priv = counter->priv; > + > + if (priv->qdec_mode) > + *function = ATMEL_TC_FUNCTION_QUADRATURE; > + else > + *function = ATMEL_TC_FUNCTION_INCREASE; > + > + return 0; > +} > + > +static int atmel_tc_count_function_set(struct counter_device *counter, > + struct counter_count *count, > + size_t function) > +{ > + struct atmel_tc_data *const priv = counter->priv; > + u32 bmr, cmr; > + > + regmap_read(priv->regmap, ATMEL_TC_BMR, &bmr); > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr); > + > + /* Set capture mode */ > + cmr &= ~ATMEL_TC_WAVE; > + > + switch (function) { > + case ATMEL_TC_FUNCTION_INCREASE: > + priv->qdec_mode = 0; > + /* Set highest rate based on whether soc has gclk or not */ > + bmr &= ~(ATMEL_TC_QDEN | ATMEL_TC_POSEN); > + if (priv->tc_cfg->has_gclk) > + cmr |= ATMEL_TC_TIMER_CLOCK2; > + else > + cmr |= ATMEL_TC_TIMER_CLOCK1; > + /* Setup the period capture mode */ > + cmr |= ATMEL_TC_CMR_MASK; > + cmr &= ~(ATMEL_TC_ABETRG | ATMEL_TC_XC0); > + break; > + case ATMEL_TC_FUNCTION_QUADRATURE: > + if (!priv->tc_cfg->has_qdec) > + return -ENODEV; I think returning an -EINVAL here might be better. I can understand that the system does not have a quadrature encoder device in this case, but from the user perspective they are requesting to set the function for the existing counter device. Therefore, if the existing counter device does not support the requested function, it makes sense to report the issue as simply an invalid argument (return -EINVAL) for this particular device. > + /* In QDEC mode settings both channels 0 and 1 are required */ > + if (priv->num_channels < 2 || priv->channel[0] != 0 || > + priv->channel[1] != 1) { > + pr_err("Invalid channels number or id for quadrature mode\n"); > + return -EINVAL; > + } > + priv->qdec_mode = 1; > + bmr |= ATMEL_TC_QDEN | ATMEL_TC_POSEN; > + cmr |= ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_ABETRG | ATMEL_TC_XC0; > + break; > + } > + > + regmap_write(priv->regmap, ATMEL_TC_BMR, bmr); > + regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), cmr); > + > + /* Enable clock and trigger counter */ > + regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], CCR), > + ATMEL_TC_CLKEN | ATMEL_TC_SWTRG); > + > + if (priv->qdec_mode) { > + regmap_write(priv->regmap, > + ATMEL_TC_REG(priv->channel[1], CMR), cmr); > + regmap_write(priv->regmap, > + ATMEL_TC_REG(priv->channel[1], CCR), > + ATMEL_TC_CLKEN | ATMEL_TC_SWTRG); > + } > + > + return 0; > +} > + > +static int atmel_tc_count_signal_read(struct counter_device *counter, > + struct counter_signal *signal, > + enum counter_signal_value *val) > +{ > + struct atmel_tc_data *const priv = counter->priv; > + bool sigstatus; > + u32 sr; > + > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr); > + > + if (priv->trig_inverted) > + sigstatus = (sr & ATMEL_TC_MTIOB); > + else > + sigstatus = (sr & ATMEL_TC_MTIOA); > + > + *val = sigstatus ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW; > + > + return 0; > +} > + > +static int atmel_tc_count_action_get(struct counter_device *counter, > + struct counter_count *count, > + struct counter_synapse *synapse, > + size_t *action) > +{ > + struct atmel_tc_data *const priv = counter->priv; > + u32 cmr; > + > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr); > + > + *action = ATMEL_TC_SYNAPSE_ACTION_NONE; > + > + if (cmr & ATMEL_TC_ETRGEDG_NONE) > + *action = ATMEL_TC_SYNAPSE_ACTION_NONE; > + else if (cmr & ATMEL_TC_ETRGEDG_RISING) > + *action = ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE; > + else if (cmr & ATMEL_TC_ETRGEDG_FALLING) > + *action = ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE; > + else if (cmr & ATMEL_TC_ETRGEDG_BOTH) > + *action = ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE; > + > + return 0; > +} > + > +static int atmel_tc_count_action_set(struct counter_device *counter, > + struct counter_count *count, > + struct counter_synapse *synapse, > + size_t action) > +{ > + struct atmel_tc_data *const priv = counter->priv; > + u32 edge = ATMEL_TC_ETRGEDG_NONE; > + > + switch (action) { > + case ATMEL_TC_SYNAPSE_ACTION_NONE: > + edge = ATMEL_TC_ETRGEDG_NONE; > + break; > + case ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE: > + edge = ATMEL_TC_ETRGEDG_RISING; > + break; > + case ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE: > + edge = ATMEL_TC_ETRGEDG_FALLING; > + break; > + case ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE: > + edge = ATMEL_TC_ETRGEDG_BOTH; > + break; > + } > + > + return regmap_write_bits(priv->regmap, > + ATMEL_TC_REG(priv->channel[0], CMR), > + ATMEL_TC_ETRGEDG, edge); Are users able to adjust the edges like this even if the device is configured for QDEC mode? If not, you should return -EINVAL if the device is in QDEC mode. > +} > + > +static int atmel_tc_count_read(struct counter_device *counter, > + struct counter_count *count, > + unsigned long *val) > +{ > + struct atmel_tc_data *const priv = counter->priv; > + u32 cnt; > + > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CV), &cnt); > + *val = cnt; > + > + return 0; > +} > + > +static struct counter_count atmel_tc_count = { > + .id = 0, > + .name = "Timer Counter", > + .functions_list = atmel_tc_count_functions, > + .num_functions = ARRAY_SIZE(atmel_tc_count_functions), > + .synapses = atmel_tc_count_synapses, > + .num_synapses = ARRAY_SIZE(atmel_tc_count_synapses), > +}; > + > +static struct counter_ops atmel_tc_ops = { > + .signal_read = atmel_tc_count_signal_read, > + .count_read = atmel_tc_count_read, > + .count_write = NULL, No need to explicitly set count_write to NULL since it will be NULL by default in this static structure. > + .function_get = atmel_tc_count_function_get, > + .function_set = atmel_tc_count_function_set, > + .action_get = atmel_tc_count_action_get, > + .action_set = atmel_tc_count_action_set > +}; > + > +static const struct atmel_tcb_config tcb_rm9200_config = { > + .counter_width = 16, > +}; > + > +static const struct atmel_tcb_config tcb_sam9x5_config = { > + .counter_width = 32, > +}; > + > +static const struct atmel_tcb_config tcb_sama5d2_config = { > + .counter_width = 32, > + .has_gclk = true, > + .has_qdec = true, > +}; > + > +static const struct atmel_tcb_config tcb_sama5d3_config = { > + .counter_width = 32, > + .has_qdec = true, > +}; > + > +static const struct of_device_id atmel_tc_of_match[] = { > + { .compatible = "atmel,at91rm9200-tcb", .data = &tcb_rm9200_config, }, > + { .compatible = "atmel,at91sam9x5-tcb", .data = &tcb_sam9x5_config, }, > + { .compatible = "atmel,sama5d2-tcb", .data = &tcb_sama5d2_config, }, > + { .compatible = "atmel,sama5d3-tcb", .data = &tcb_sama5d3_config, }, > + { /* sentinel */ } > +}; > + > +static int atmel_tc_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + const struct atmel_tcb_config *tcb_config; > + const struct of_device_id *match; > + struct atmel_tc_data *priv; > + char clk_name[] = "t0_clk"; > + struct regmap *regmap; > + struct clk *clk[3]; > + int channel; > + int ret, i; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, priv); > + > + match = of_match_node(atmel_tc_of_match, np->parent); > + tcb_config = match->data; > + if (!tcb_config) { > + dev_err(&pdev->dev, "No matching parent node found\n"); > + return -ENODEV; > + } > + > + regmap = syscon_node_to_regmap(np->parent); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + /* max. channels number is 2 when in QDEC mode */ > + priv->num_channels = of_property_count_u32_elems(np, "reg"); > + if (priv->num_channels < 0) { > + dev_err(&pdev->dev, "Invalid or missing channel\n"); > + return -EINVAL; > + } > + > + /* Register channels and initialize clocks */ > + for (i = 0; i < priv->num_channels; i++) { > + ret = of_property_read_u32_index(np, "reg", i, &channel); > + if (ret < 0 || channel > 2) > + return -ENODEV; > + > + priv->channel[i] = channel; > + > + clk_name[1] += channel; You're being clever here (not a bad thing) but this looks too easy to get wrong and confused. I'd much rather see a snprintf here for clarity and know that nothing funny is going to happen. > + clk[i] = of_clk_get_by_name(np->parent, clk_name); > + if (IS_ERR(clk[i])) { > + /* Fallback to t0_clk */ > + clk[i] = of_clk_get_by_name(np->parent, "t0_clk"); > + if (IS_ERR(clk[i])) > + return PTR_ERR(clk[i]); > + } > + > + ret = clk_prepare_enable(clk[i]); > + if (ret) > + return ret; > + > + dev_info(&pdev->dev, > + "Initialized capture mode on channel %d\n", > + channel); > + } > + > + priv->tc_cfg = tcb_config; > + priv->regmap = regmap; > + priv->counter.name = dev_name(&pdev->dev); > + priv->counter.parent = &pdev->dev; > + priv->counter.ops = &atmel_tc_ops; > + priv->counter.num_counts = 1; Use ARRAY_SIZE here so that future reviewers will know that num_counts matches what's in the atmel_tc_count array without having to check so themselves. William Breathitt Gray > + priv->counter.counts = &atmel_tc_count; > + priv->counter.num_signals = ARRAY_SIZE(atmel_tc_count_signals); > + priv->counter.signals = atmel_tc_count_signals; > + priv->counter.priv = priv; > + > + ret = devm_counter_register(&pdev->dev, &priv->counter); > + if (ret < 0) { > + for (i = 0; i < priv->num_channels; i++) > + clk_disable_unprepare(clk[i]); > + return ret; > + } > + > + return 0; > +} > + > +static const struct of_device_id atmel_tc_dt_ids[] = { > + { .compatible = "atmel,tcb-capture", }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, atmel_tc_dt_ids); > + > +static struct platform_driver atmel_tc_driver = { > + .probe = atmel_tc_probe, > + .driver = { > + .name = "atmel-tcb-capture", > + .of_match_table = atmel_tc_dt_ids, > + }, > +}; > +module_platform_driver(atmel_tc_driver); > + > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>"); > +MODULE_DESCRIPTION("Atmel TCB Capture driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.25.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] counter: Add atmel TCB capture counter 2020-04-08 21:31 ` William Breathitt Gray @ 2020-04-08 21:56 ` Alexandre Belloni 2020-04-09 8:25 ` Kamel Bouhara 1 sibling, 0 replies; 10+ messages in thread From: Alexandre Belloni @ 2020-04-08 21:56 UTC (permalink / raw) To: William Breathitt Gray Cc: Kamel Bouhara, Rob Herring, Mark Rutland, Nicolas Ferre, Ludovic Desroches, linux-arm-kernel, Thomas Petazzoni, linux-input, devicetree, linux-iio Hi William, On 08/04/2020 17:31:03-0400, William Breathitt Gray wrote: > On Mon, Apr 06, 2020 at 05:53:20PM +0200, Kamel Bouhara wrote: > > This drivers allows to use the capture mode of the Timer Counter Block > > hardware block available in Atmel SoCs through the counter subsystem. > > > > Two functions of the counter are supported for the moment: period > > capture and quadrature decoder. The latter is only supported by the > > SAMA5 series of SoCs. > > > > For the period capture mode a basic setup has been chosen that will > > reset the counter each time the period is actually reached. Of course > > the device offers much more possibilities. > > > > For quadrature mode, both channel 0 and 1 must be configured even if we > > only capture the position (no revolution/rotation). > > > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Hi Kamel, > > Thank you for submitting support for this driver. Since this is a new > counter driver, make sure to create an entry for it in the top-level > MAINTAINERS file so users know who to contact to report bugs and other > issues. > The file name already matches for the at91 maintainers so I think we are good on this front. -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] counter: Add atmel TCB capture counter 2020-04-08 21:31 ` William Breathitt Gray 2020-04-08 21:56 ` Alexandre Belloni @ 2020-04-09 8:25 ` Kamel Bouhara 2020-04-09 13:16 ` William Breathitt Gray 1 sibling, 1 reply; 10+ messages in thread From: Kamel Bouhara @ 2020-04-09 8:25 UTC (permalink / raw) To: William Breathitt Gray Cc: Rob Herring, Mark Rutland, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel, Thomas Petazzoni, linux-input, devicetree, linux-iio On Wed, Apr 08, 2020 at 05:31:03PM -0400, William Breathitt Gray wrote: > On Mon, Apr 06, 2020 at 05:53:20PM +0200, Kamel Bouhara wrote: > > This drivers allows to use the capture mode of the Timer Counter Block > > hardware block available in Atmel SoCs through the counter subsystem. > > > > Two functions of the counter are supported for the moment: period > > capture and quadrature decoder. The latter is only supported by the > > SAMA5 series of SoCs. > > > > For the period capture mode a basic setup has been chosen that will > > reset the counter each time the period is actually reached. Of course > > the device offers much more possibilities. > > > > For quadrature mode, both channel 0 and 1 must be configured even if we > > only capture the position (no revolution/rotation). > > > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Hi Kamel, > Hi William, > Thank you for submitting support for this driver. Since this is a new > counter driver, make sure to create an entry for it in the top-level > MAINTAINERS file so users know who to contact to report bugs and other > issues. > I'll skip this if you're ok with the fact that the file name already match the at91 maintainers ? > I've written some more comments inline below. > > > --- > > drivers/counter/Kconfig | 11 + > > drivers/counter/Makefile | 1 + > > drivers/counter/atmel-tcb-capture.c | 388 ++++++++++++++++++++++++++++ > > 3 files changed, 400 insertions(+) > > create mode 100644 drivers/counter/atmel-tcb-capture.c > > > > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig > > index c80fa76bb531..c50d7453ec33 100644 > > --- a/drivers/counter/Kconfig > > +++ b/drivers/counter/Kconfig > > @@ -70,4 +70,15 @@ config FTM_QUADDEC > > To compile this driver as a module, choose M here: the > > module will be called ftm-quaddec. > > > > +config ATMEL_TCB_CAPTURE > > + tristate "Atmel Timer Counter Capture driver" > > + depends on HAS_IOMEM && OF > > + select REGMAP_MMIO > > + help > > + Select this option to enable the Atmel Timer Counter Block > > + capture driver. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called atmel-tcb-capture. > > + > > endif # COUNTER > > diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile > > index 55142d1f4c43..70c5b8924588 100644 > > --- a/drivers/counter/Makefile > > +++ b/drivers/counter/Makefile > > @@ -10,3 +10,4 @@ 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 > > +obj-$(CONFIG_ATMEL_TCB_CAPTURE) += atmel-tcb-capture.o > > diff --git a/drivers/counter/atmel-tcb-capture.c b/drivers/counter/atmel-tcb-capture.c > > new file mode 100644 > > index 000000000000..6ac809503f90 > > --- /dev/null > > +++ b/drivers/counter/atmel-tcb-capture.c > > @@ -0,0 +1,388 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/** > > + * Copyright (C) 2020 Atmel > > + * > > + * Author: Kamel Bouhara <kamel.bouhara@bootlin.com> > > + * > > + */ > > +#include <linux/clk.h> > > +#include <linux/counter.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <soc/at91/atmel_tcb.h> > > + > > +#define ATMEL_TC_CMR_MASK (ATMEL_TC_LDRA_RISING | ATMEL_TC_LDRB_FALLING | \ > > + ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_LDBDIS | \ > > + ATMEL_TC_LDBSTOP) > > + > > +#define ATMEL_TC_QDEN BIT(8) > > +#define ATMEL_TC_POSEN BIT(9) > > + > > +struct atmel_tc_data { > > + const struct atmel_tcb_config *tc_cfg; > > + struct counter_device counter; > > + struct regmap *regmap; > > + int qdec_mode; > > + int num_channels; > > + int channel[2]; > > + bool trig_inverted; > > +}; > > + > > +enum atmel_tc_count_function { > > + ATMEL_TC_FUNCTION_INCREASE, > > + ATMEL_TC_FUNCTION_QUADRATURE, > > +}; > > + > > +static enum counter_count_function atmel_tc_count_functions[] = { > > + [ATMEL_TC_FUNCTION_INCREASE] = COUNTER_COUNT_FUNCTION_INCREASE, > > + [ATMEL_TC_FUNCTION_QUADRATURE] = COUNTER_COUNT_FUNCTION_QUADRATURE_X4, > > +}; > > + > > +enum atmel_tc_synapse_action { > > + ATMEL_TC_SYNAPSE_ACTION_NONE = 0, > > + ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE, > > + ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE, > > + ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE > > +}; > > + > > +static enum counter_synapse_action atmel_tc_synapse_actions[] = { > > + [ATMEL_TC_SYNAPSE_ACTION_NONE] = COUNTER_SYNAPSE_ACTION_NONE, > > + [ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE] = COUNTER_SYNAPSE_ACTION_RISING_EDGE, > > + [ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE] = COUNTER_SYNAPSE_ACTION_FALLING_EDGE, > > + [ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE] = COUNTER_SYNAPSE_ACTION_BOTH_EDGES, > > +}; > > + > > +static struct counter_signal atmel_tc_count_signals[] = { > > + { > > + .id = 0, > > + .name = "Channel A", > > + }, > > + { > > + .id = 1, > > + .name = "Channel B", > > + } > > +}; > > + > > +static struct counter_synapse atmel_tc_count_synapses[] = { > > + { > > + .actions_list = atmel_tc_synapse_actions, > > + .num_actions = ARRAY_SIZE(atmel_tc_synapse_actions), > > + .signal = &atmel_tc_count_signals[0] > > + }, > > + { > > + .actions_list = atmel_tc_synapse_actions, > > + .num_actions = ARRAY_SIZE(atmel_tc_synapse_actions), > > + .signal = &atmel_tc_count_signals[1] > > + } > > +}; > > + > > +static int atmel_tc_count_function_get(struct counter_device *counter, > > + struct counter_count *count, > > + size_t *function) > > +{ > > + struct atmel_tc_data *const priv = counter->priv; > > + > > + if (priv->qdec_mode) > > + *function = ATMEL_TC_FUNCTION_QUADRATURE; > > + else > > + *function = ATMEL_TC_FUNCTION_INCREASE; > > + > > + return 0; > > +} > > + > > +static int atmel_tc_count_function_set(struct counter_device *counter, > > + struct counter_count *count, > > + size_t function) > > +{ > > + struct atmel_tc_data *const priv = counter->priv; > > + u32 bmr, cmr; > > + > > + regmap_read(priv->regmap, ATMEL_TC_BMR, &bmr); > > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr); > > + > > + /* Set capture mode */ > > + cmr &= ~ATMEL_TC_WAVE; > > + > > + switch (function) { > > + case ATMEL_TC_FUNCTION_INCREASE: > > + priv->qdec_mode = 0; > > + /* Set highest rate based on whether soc has gclk or not */ > > + bmr &= ~(ATMEL_TC_QDEN | ATMEL_TC_POSEN); > > + if (priv->tc_cfg->has_gclk) > > + cmr |= ATMEL_TC_TIMER_CLOCK2; > > + else > > + cmr |= ATMEL_TC_TIMER_CLOCK1; > > + /* Setup the period capture mode */ > > + cmr |= ATMEL_TC_CMR_MASK; > > + cmr &= ~(ATMEL_TC_ABETRG | ATMEL_TC_XC0); > > + break; > > + case ATMEL_TC_FUNCTION_QUADRATURE: > > + if (!priv->tc_cfg->has_qdec) > > + return -ENODEV; > > I think returning an -EINVAL here might be better. I can understand that > the system does not have a quadrature encoder device in this case, but > from the user perspective they are requesting to set the function for > the existing counter device. Therefore, if the existing counter device > does not support the requested function, it makes sense to report the > issue as simply an invalid argument (return -EINVAL) for this particular > device. > Good point, fixed for v2. > > + /* In QDEC mode settings both channels 0 and 1 are required */ > > + if (priv->num_channels < 2 || priv->channel[0] != 0 || > > + priv->channel[1] != 1) { > > + pr_err("Invalid channels number or id for quadrature mode\n"); > > + return -EINVAL; > > + } > > + priv->qdec_mode = 1; > > + bmr |= ATMEL_TC_QDEN | ATMEL_TC_POSEN; > > + cmr |= ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_ABETRG | ATMEL_TC_XC0; > > + break; > > + } > > + > > + regmap_write(priv->regmap, ATMEL_TC_BMR, bmr); > > + regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), cmr); > > + > > + /* Enable clock and trigger counter */ > > + regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], CCR), > > + ATMEL_TC_CLKEN | ATMEL_TC_SWTRG); > > + > > + if (priv->qdec_mode) { > > + regmap_write(priv->regmap, > > + ATMEL_TC_REG(priv->channel[1], CMR), cmr); > > + regmap_write(priv->regmap, > > + ATMEL_TC_REG(priv->channel[1], CCR), > > + ATMEL_TC_CLKEN | ATMEL_TC_SWTRG); > > + } > > + > > + return 0; > > +} > > + > > +static int atmel_tc_count_signal_read(struct counter_device *counter, > > + struct counter_signal *signal, > > + enum counter_signal_value *val) > > +{ > > + struct atmel_tc_data *const priv = counter->priv; > > + bool sigstatus; > > + u32 sr; > > + > > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr); > > + > > + if (priv->trig_inverted) > > + sigstatus = (sr & ATMEL_TC_MTIOB); > > + else > > + sigstatus = (sr & ATMEL_TC_MTIOA); > > + > > + *val = sigstatus ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW; > > + > > + return 0; > > +} > > + > > +static int atmel_tc_count_action_get(struct counter_device *counter, > > + struct counter_count *count, > > + struct counter_synapse *synapse, > > + size_t *action) > > +{ > > + struct atmel_tc_data *const priv = counter->priv; > > + u32 cmr; > > + > > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr); > > + > > + *action = ATMEL_TC_SYNAPSE_ACTION_NONE; > > + > > + if (cmr & ATMEL_TC_ETRGEDG_NONE) > > + *action = ATMEL_TC_SYNAPSE_ACTION_NONE; > > + else if (cmr & ATMEL_TC_ETRGEDG_RISING) > > + *action = ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE; > > + else if (cmr & ATMEL_TC_ETRGEDG_FALLING) > > + *action = ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE; > > + else if (cmr & ATMEL_TC_ETRGEDG_BOTH) > > + *action = ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE; > > + > > + return 0; > > +} > > + > > +static int atmel_tc_count_action_set(struct counter_device *counter, > > + struct counter_count *count, > > + struct counter_synapse *synapse, > > + size_t action) > > +{ > > + struct atmel_tc_data *const priv = counter->priv; > > + u32 edge = ATMEL_TC_ETRGEDG_NONE; > > + > > + switch (action) { > > + case ATMEL_TC_SYNAPSE_ACTION_NONE: > > + edge = ATMEL_TC_ETRGEDG_NONE; > > + break; > > + case ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE: > > + edge = ATMEL_TC_ETRGEDG_RISING; > > + break; > > + case ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE: > > + edge = ATMEL_TC_ETRGEDG_FALLING; > > + break; > > + case ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE: > > + edge = ATMEL_TC_ETRGEDG_BOTH; > > + break; > > + } > > + > > + return regmap_write_bits(priv->regmap, > > + ATMEL_TC_REG(priv->channel[0], CMR), > > + ATMEL_TC_ETRGEDG, edge); > > Are users able to adjust the edges like this even if the device is > configured for QDEC mode? If not, you should return -EINVAL if the > device is in QDEC mode. > You're right, in QDEC mode, rising edge must be selected as the external trigger edge. Fixed. > > +} > > + > > +static int atmel_tc_count_read(struct counter_device *counter, > > + struct counter_count *count, > > + unsigned long *val) > > +{ > > + struct atmel_tc_data *const priv = counter->priv; > > + u32 cnt; > > + > > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CV), &cnt); > > + *val = cnt; > > + > > + return 0; > > +} > > + > > +static struct counter_count atmel_tc_count = { > > + .id = 0, > > + .name = "Timer Counter", > > + .functions_list = atmel_tc_count_functions, > > + .num_functions = ARRAY_SIZE(atmel_tc_count_functions), > > + .synapses = atmel_tc_count_synapses, > > + .num_synapses = ARRAY_SIZE(atmel_tc_count_synapses), > > +}; > > + > > +static struct counter_ops atmel_tc_ops = { > > + .signal_read = atmel_tc_count_signal_read, > > + .count_read = atmel_tc_count_read, > > + .count_write = NULL, > > No need to explicitly set count_write to NULL since it will be NULL by > default in this static structure. > Fixed, thanks. > > + .function_get = atmel_tc_count_function_get, > > + .function_set = atmel_tc_count_function_set, > > + .action_get = atmel_tc_count_action_get, > > + .action_set = atmel_tc_count_action_set > > +}; > > + > > +static const struct atmel_tcb_config tcb_rm9200_config = { > > + .counter_width = 16, > > +}; > > + > > +static const struct atmel_tcb_config tcb_sam9x5_config = { > > + .counter_width = 32, > > +}; > > + > > +static const struct atmel_tcb_config tcb_sama5d2_config = { > > + .counter_width = 32, > > + .has_gclk = true, > > + .has_qdec = true, > > +}; > > + > > +static const struct atmel_tcb_config tcb_sama5d3_config = { > > + .counter_width = 32, > > + .has_qdec = true, > > +}; > > + > > +static const struct of_device_id atmel_tc_of_match[] = { > > + { .compatible = "atmel,at91rm9200-tcb", .data = &tcb_rm9200_config, }, > > + { .compatible = "atmel,at91sam9x5-tcb", .data = &tcb_sam9x5_config, }, > > + { .compatible = "atmel,sama5d2-tcb", .data = &tcb_sama5d2_config, }, > > + { .compatible = "atmel,sama5d3-tcb", .data = &tcb_sama5d3_config, }, > > + { /* sentinel */ } > > +}; > > + > > +static int atmel_tc_probe(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + const struct atmel_tcb_config *tcb_config; > > + const struct of_device_id *match; > > + struct atmel_tc_data *priv; > > + char clk_name[] = "t0_clk"; > > + struct regmap *regmap; > > + struct clk *clk[3]; > > + int channel; > > + int ret, i; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, priv); > > + > > + match = of_match_node(atmel_tc_of_match, np->parent); > > + tcb_config = match->data; > > + if (!tcb_config) { > > + dev_err(&pdev->dev, "No matching parent node found\n"); > > + return -ENODEV; > > + } > > + > > + regmap = syscon_node_to_regmap(np->parent); > > + if (IS_ERR(priv->regmap)) > > + return PTR_ERR(priv->regmap); > > + > > + /* max. channels number is 2 when in QDEC mode */ > > + priv->num_channels = of_property_count_u32_elems(np, "reg"); > > + if (priv->num_channels < 0) { > > + dev_err(&pdev->dev, "Invalid or missing channel\n"); > > + return -EINVAL; > > + } > > + > > + /* Register channels and initialize clocks */ > > + for (i = 0; i < priv->num_channels; i++) { > > + ret = of_property_read_u32_index(np, "reg", i, &channel); > > + if (ret < 0 || channel > 2) > > + return -ENODEV; > > + > > + priv->channel[i] = channel; > > + > > + clk_name[1] += channel; > > You're being clever here (not a bad thing) but this looks too easy to > get wrong and confused. I'd much rather see a snprintf here for clarity > and know that nothing funny is going to happen. > Well ok let's be less clever for the sake of clarity :). > > + clk[i] = of_clk_get_by_name(np->parent, clk_name); > > + if (IS_ERR(clk[i])) { > > + /* Fallback to t0_clk */ > > + clk[i] = of_clk_get_by_name(np->parent, "t0_clk"); > > + if (IS_ERR(clk[i])) > > + return PTR_ERR(clk[i]); > > + } > > + > > + ret = clk_prepare_enable(clk[i]); > > + if (ret) > > + return ret; > > + > > + dev_info(&pdev->dev, > > + "Initialized capture mode on channel %d\n", > > + channel); > > + } > > + > > + priv->tc_cfg = tcb_config; > > + priv->regmap = regmap; > > + priv->counter.name = dev_name(&pdev->dev); > > + priv->counter.parent = &pdev->dev; > > + priv->counter.ops = &atmel_tc_ops; > > + priv->counter.num_counts = 1; > > Use ARRAY_SIZE here so that future reviewers will know that num_counts > matches what's in the atmel_tc_count array without having to check so > themselves. > OK, yet the atmel_tc_count isn't an array I guess that it shall be. Thanks, Kamel > William Breathitt Gray > > > + priv->counter.counts = &atmel_tc_count; > > + priv->counter.num_signals = ARRAY_SIZE(atmel_tc_count_signals); > > + priv->counter.signals = atmel_tc_count_signals; > > + priv->counter.priv = priv; > > + > > + ret = devm_counter_register(&pdev->dev, &priv->counter); > > + if (ret < 0) { > > + for (i = 0; i < priv->num_channels; i++) > > + clk_disable_unprepare(clk[i]); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static const struct of_device_id atmel_tc_dt_ids[] = { > > + { .compatible = "atmel,tcb-capture", }, > > + { /* sentinel */ }, > > +}; > > +MODULE_DEVICE_TABLE(of, atmel_tc_dt_ids); > > + > > +static struct platform_driver atmel_tc_driver = { > > + .probe = atmel_tc_probe, > > + .driver = { > > + .name = "atmel-tcb-capture", > > + .of_match_table = atmel_tc_dt_ids, > > + }, > > +}; > > +module_platform_driver(atmel_tc_driver); > > + > > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>"); > > +MODULE_DESCRIPTION("Atmel TCB Capture driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.25.0 > > -- Kamel Bouhara, Bootlin Embedded Linux and kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] counter: Add atmel TCB capture counter 2020-04-09 8:25 ` Kamel Bouhara @ 2020-04-09 13:16 ` William Breathitt Gray 0 siblings, 0 replies; 10+ messages in thread From: William Breathitt Gray @ 2020-04-09 13:16 UTC (permalink / raw) To: Kamel Bouhara Cc: Rob Herring, Mark Rutland, Nicolas Ferre, Alexandre Belloni, Ludovic Desroches, linux-arm-kernel, Thomas Petazzoni, linux-input, devicetree, linux-iio [-- Attachment #1: Type: text/plain, Size: 18678 bytes --] On Thu, Apr 09, 2020 at 10:25:31AM +0200, Kamel Bouhara wrote: > On Wed, Apr 08, 2020 at 05:31:03PM -0400, William Breathitt Gray wrote: > > On Mon, Apr 06, 2020 at 05:53:20PM +0200, Kamel Bouhara wrote: > > > This drivers allows to use the capture mode of the Timer Counter Block > > > hardware block available in Atmel SoCs through the counter subsystem. > > > > > > Two functions of the counter are supported for the moment: period > > > capture and quadrature decoder. The latter is only supported by the > > > SAMA5 series of SoCs. > > > > > > For the period capture mode a basic setup has been chosen that will > > > reset the counter each time the period is actually reached. Of course > > > the device offers much more possibilities. > > > > > > For quadrature mode, both channel 0 and 1 must be configured even if we > > > only capture the position (no revolution/rotation). > > > > > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > > > > Hi Kamel, > > > > Hi William, > > > Thank you for submitting support for this driver. Since this is a new > > counter driver, make sure to create an entry for it in the top-level > > MAINTAINERS file so users know who to contact to report bugs and other > > issues. > > > > I'll skip this if you're ok with the fact that the file name already > match the at91 maintainers ? No problem, you can skip this, I didn't realize it was already matching. William Breathitt Gray > > > I've written some more comments inline below. > > > > > --- > > > drivers/counter/Kconfig | 11 + > > > drivers/counter/Makefile | 1 + > > > drivers/counter/atmel-tcb-capture.c | 388 ++++++++++++++++++++++++++++ > > > 3 files changed, 400 insertions(+) > > > create mode 100644 drivers/counter/atmel-tcb-capture.c > > > > > > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig > > > index c80fa76bb531..c50d7453ec33 100644 > > > --- a/drivers/counter/Kconfig > > > +++ b/drivers/counter/Kconfig > > > @@ -70,4 +70,15 @@ config FTM_QUADDEC > > > To compile this driver as a module, choose M here: the > > > module will be called ftm-quaddec. > > > > > > +config ATMEL_TCB_CAPTURE > > > + tristate "Atmel Timer Counter Capture driver" > > > + depends on HAS_IOMEM && OF > > > + select REGMAP_MMIO > > > + help > > > + Select this option to enable the Atmel Timer Counter Block > > > + capture driver. > > > + > > > + To compile this driver as a module, choose M here: the > > > + module will be called atmel-tcb-capture. > > > + > > > endif # COUNTER > > > diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile > > > index 55142d1f4c43..70c5b8924588 100644 > > > --- a/drivers/counter/Makefile > > > +++ b/drivers/counter/Makefile > > > @@ -10,3 +10,4 @@ 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 > > > +obj-$(CONFIG_ATMEL_TCB_CAPTURE) += atmel-tcb-capture.o > > > diff --git a/drivers/counter/atmel-tcb-capture.c b/drivers/counter/atmel-tcb-capture.c > > > new file mode 100644 > > > index 000000000000..6ac809503f90 > > > --- /dev/null > > > +++ b/drivers/counter/atmel-tcb-capture.c > > > @@ -0,0 +1,388 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/** > > > + * Copyright (C) 2020 Atmel > > > + * > > > + * Author: Kamel Bouhara <kamel.bouhara@bootlin.com> > > > + * > > > + */ > > > +#include <linux/clk.h> > > > +#include <linux/counter.h> > > > +#include <linux/mfd/syscon.h> > > > +#include <linux/module.h> > > > +#include <linux/mutex.h> > > > +#include <linux/of.h> > > > +#include <linux/of_device.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/regmap.h> > > > +#include <soc/at91/atmel_tcb.h> > > > + > > > +#define ATMEL_TC_CMR_MASK (ATMEL_TC_LDRA_RISING | ATMEL_TC_LDRB_FALLING | \ > > > + ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_LDBDIS | \ > > > + ATMEL_TC_LDBSTOP) > > > + > > > +#define ATMEL_TC_QDEN BIT(8) > > > +#define ATMEL_TC_POSEN BIT(9) > > > + > > > +struct atmel_tc_data { > > > + const struct atmel_tcb_config *tc_cfg; > > > + struct counter_device counter; > > > + struct regmap *regmap; > > > + int qdec_mode; > > > + int num_channels; > > > + int channel[2]; > > > + bool trig_inverted; > > > +}; > > > + > > > +enum atmel_tc_count_function { > > > + ATMEL_TC_FUNCTION_INCREASE, > > > + ATMEL_TC_FUNCTION_QUADRATURE, > > > +}; > > > + > > > +static enum counter_count_function atmel_tc_count_functions[] = { > > > + [ATMEL_TC_FUNCTION_INCREASE] = COUNTER_COUNT_FUNCTION_INCREASE, > > > + [ATMEL_TC_FUNCTION_QUADRATURE] = COUNTER_COUNT_FUNCTION_QUADRATURE_X4, > > > +}; > > > + > > > +enum atmel_tc_synapse_action { > > > + ATMEL_TC_SYNAPSE_ACTION_NONE = 0, > > > + ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE, > > > + ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE, > > > + ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE > > > +}; > > > + > > > +static enum counter_synapse_action atmel_tc_synapse_actions[] = { > > > + [ATMEL_TC_SYNAPSE_ACTION_NONE] = COUNTER_SYNAPSE_ACTION_NONE, > > > + [ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE] = COUNTER_SYNAPSE_ACTION_RISING_EDGE, > > > + [ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE] = COUNTER_SYNAPSE_ACTION_FALLING_EDGE, > > > + [ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE] = COUNTER_SYNAPSE_ACTION_BOTH_EDGES, > > > +}; > > > + > > > +static struct counter_signal atmel_tc_count_signals[] = { > > > + { > > > + .id = 0, > > > + .name = "Channel A", > > > + }, > > > + { > > > + .id = 1, > > > + .name = "Channel B", > > > + } > > > +}; > > > + > > > +static struct counter_synapse atmel_tc_count_synapses[] = { > > > + { > > > + .actions_list = atmel_tc_synapse_actions, > > > + .num_actions = ARRAY_SIZE(atmel_tc_synapse_actions), > > > + .signal = &atmel_tc_count_signals[0] > > > + }, > > > + { > > > + .actions_list = atmel_tc_synapse_actions, > > > + .num_actions = ARRAY_SIZE(atmel_tc_synapse_actions), > > > + .signal = &atmel_tc_count_signals[1] > > > + } > > > +}; > > > + > > > +static int atmel_tc_count_function_get(struct counter_device *counter, > > > + struct counter_count *count, > > > + size_t *function) > > > +{ > > > + struct atmel_tc_data *const priv = counter->priv; > > > + > > > + if (priv->qdec_mode) > > > + *function = ATMEL_TC_FUNCTION_QUADRATURE; > > > + else > > > + *function = ATMEL_TC_FUNCTION_INCREASE; > > > + > > > + return 0; > > > +} > > > + > > > +static int atmel_tc_count_function_set(struct counter_device *counter, > > > + struct counter_count *count, > > > + size_t function) > > > +{ > > > + struct atmel_tc_data *const priv = counter->priv; > > > + u32 bmr, cmr; > > > + > > > + regmap_read(priv->regmap, ATMEL_TC_BMR, &bmr); > > > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr); > > > + > > > + /* Set capture mode */ > > > + cmr &= ~ATMEL_TC_WAVE; > > > + > > > + switch (function) { > > > + case ATMEL_TC_FUNCTION_INCREASE: > > > + priv->qdec_mode = 0; > > > + /* Set highest rate based on whether soc has gclk or not */ > > > + bmr &= ~(ATMEL_TC_QDEN | ATMEL_TC_POSEN); > > > + if (priv->tc_cfg->has_gclk) > > > + cmr |= ATMEL_TC_TIMER_CLOCK2; > > > + else > > > + cmr |= ATMEL_TC_TIMER_CLOCK1; > > > + /* Setup the period capture mode */ > > > + cmr |= ATMEL_TC_CMR_MASK; > > > + cmr &= ~(ATMEL_TC_ABETRG | ATMEL_TC_XC0); > > > + break; > > > + case ATMEL_TC_FUNCTION_QUADRATURE: > > > + if (!priv->tc_cfg->has_qdec) > > > + return -ENODEV; > > > > I think returning an -EINVAL here might be better. I can understand that > > the system does not have a quadrature encoder device in this case, but > > from the user perspective they are requesting to set the function for > > the existing counter device. Therefore, if the existing counter device > > does not support the requested function, it makes sense to report the > > issue as simply an invalid argument (return -EINVAL) for this particular > > device. > > > > Good point, fixed for v2. > > > > + /* In QDEC mode settings both channels 0 and 1 are required */ > > > + if (priv->num_channels < 2 || priv->channel[0] != 0 || > > > + priv->channel[1] != 1) { > > > + pr_err("Invalid channels number or id for quadrature mode\n"); > > > + return -EINVAL; > > > + } > > > + priv->qdec_mode = 1; > > > + bmr |= ATMEL_TC_QDEN | ATMEL_TC_POSEN; > > > + cmr |= ATMEL_TC_ETRGEDG_RISING | ATMEL_TC_ABETRG | ATMEL_TC_XC0; > > > + break; > > > + } > > > + > > > + regmap_write(priv->regmap, ATMEL_TC_BMR, bmr); > > > + regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), cmr); > > > + > > > + /* Enable clock and trigger counter */ > > > + regmap_write(priv->regmap, ATMEL_TC_REG(priv->channel[0], CCR), > > > + ATMEL_TC_CLKEN | ATMEL_TC_SWTRG); > > > + > > > + if (priv->qdec_mode) { > > > + regmap_write(priv->regmap, > > > + ATMEL_TC_REG(priv->channel[1], CMR), cmr); > > > + regmap_write(priv->regmap, > > > + ATMEL_TC_REG(priv->channel[1], CCR), > > > + ATMEL_TC_CLKEN | ATMEL_TC_SWTRG); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int atmel_tc_count_signal_read(struct counter_device *counter, > > > + struct counter_signal *signal, > > > + enum counter_signal_value *val) > > > +{ > > > + struct atmel_tc_data *const priv = counter->priv; > > > + bool sigstatus; > > > + u32 sr; > > > + > > > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], SR), &sr); > > > + > > > + if (priv->trig_inverted) > > > + sigstatus = (sr & ATMEL_TC_MTIOB); > > > + else > > > + sigstatus = (sr & ATMEL_TC_MTIOA); > > > + > > > + *val = sigstatus ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW; > > > + > > > + return 0; > > > +} > > > + > > > +static int atmel_tc_count_action_get(struct counter_device *counter, > > > + struct counter_count *count, > > > + struct counter_synapse *synapse, > > > + size_t *action) > > > +{ > > > + struct atmel_tc_data *const priv = counter->priv; > > > + u32 cmr; > > > + > > > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CMR), &cmr); > > > + > > > + *action = ATMEL_TC_SYNAPSE_ACTION_NONE; > > > + > > > + if (cmr & ATMEL_TC_ETRGEDG_NONE) > > > + *action = ATMEL_TC_SYNAPSE_ACTION_NONE; > > > + else if (cmr & ATMEL_TC_ETRGEDG_RISING) > > > + *action = ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE; > > > + else if (cmr & ATMEL_TC_ETRGEDG_FALLING) > > > + *action = ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE; > > > + else if (cmr & ATMEL_TC_ETRGEDG_BOTH) > > > + *action = ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE; > > > + > > > + return 0; > > > +} > > > + > > > +static int atmel_tc_count_action_set(struct counter_device *counter, > > > + struct counter_count *count, > > > + struct counter_synapse *synapse, > > > + size_t action) > > > +{ > > > + struct atmel_tc_data *const priv = counter->priv; > > > + u32 edge = ATMEL_TC_ETRGEDG_NONE; > > > + > > > + switch (action) { > > > + case ATMEL_TC_SYNAPSE_ACTION_NONE: > > > + edge = ATMEL_TC_ETRGEDG_NONE; > > > + break; > > > + case ATMEL_TC_SYNAPSE_ACTION_RISING_EDGE: > > > + edge = ATMEL_TC_ETRGEDG_RISING; > > > + break; > > > + case ATMEL_TC_SYNAPSE_ACTION_FALLING_EDGE: > > > + edge = ATMEL_TC_ETRGEDG_FALLING; > > > + break; > > > + case ATMEL_TC_SYNAPSE_ACTION_BOTH_EDGE: > > > + edge = ATMEL_TC_ETRGEDG_BOTH; > > > + break; > > > + } > > > + > > > + return regmap_write_bits(priv->regmap, > > > + ATMEL_TC_REG(priv->channel[0], CMR), > > > + ATMEL_TC_ETRGEDG, edge); > > > > Are users able to adjust the edges like this even if the device is > > configured for QDEC mode? If not, you should return -EINVAL if the > > device is in QDEC mode. > > > > You're right, in QDEC mode, rising edge must be selected as the external > trigger edge. > > Fixed. > > > > +} > > > + > > > +static int atmel_tc_count_read(struct counter_device *counter, > > > + struct counter_count *count, > > > + unsigned long *val) > > > +{ > > > + struct atmel_tc_data *const priv = counter->priv; > > > + u32 cnt; > > > + > > > + regmap_read(priv->regmap, ATMEL_TC_REG(priv->channel[0], CV), &cnt); > > > + *val = cnt; > > > + > > > + return 0; > > > +} > > > + > > > +static struct counter_count atmel_tc_count = { > > > + .id = 0, > > > + .name = "Timer Counter", > > > + .functions_list = atmel_tc_count_functions, > > > + .num_functions = ARRAY_SIZE(atmel_tc_count_functions), > > > + .synapses = atmel_tc_count_synapses, > > > + .num_synapses = ARRAY_SIZE(atmel_tc_count_synapses), > > > +}; > > > + > > > +static struct counter_ops atmel_tc_ops = { > > > + .signal_read = atmel_tc_count_signal_read, > > > + .count_read = atmel_tc_count_read, > > > + .count_write = NULL, > > > > No need to explicitly set count_write to NULL since it will be NULL by > > default in this static structure. > > > > Fixed, thanks. > > > > + .function_get = atmel_tc_count_function_get, > > > + .function_set = atmel_tc_count_function_set, > > > + .action_get = atmel_tc_count_action_get, > > > + .action_set = atmel_tc_count_action_set > > > +}; > > > + > > > +static const struct atmel_tcb_config tcb_rm9200_config = { > > > + .counter_width = 16, > > > +}; > > > + > > > +static const struct atmel_tcb_config tcb_sam9x5_config = { > > > + .counter_width = 32, > > > +}; > > > + > > > +static const struct atmel_tcb_config tcb_sama5d2_config = { > > > + .counter_width = 32, > > > + .has_gclk = true, > > > + .has_qdec = true, > > > +}; > > > + > > > +static const struct atmel_tcb_config tcb_sama5d3_config = { > > > + .counter_width = 32, > > > + .has_qdec = true, > > > +}; > > > + > > > +static const struct of_device_id atmel_tc_of_match[] = { > > > + { .compatible = "atmel,at91rm9200-tcb", .data = &tcb_rm9200_config, }, > > > + { .compatible = "atmel,at91sam9x5-tcb", .data = &tcb_sam9x5_config, }, > > > + { .compatible = "atmel,sama5d2-tcb", .data = &tcb_sama5d2_config, }, > > > + { .compatible = "atmel,sama5d3-tcb", .data = &tcb_sama5d3_config, }, > > > + { /* sentinel */ } > > > +}; > > > + > > > +static int atmel_tc_probe(struct platform_device *pdev) > > > +{ > > > + struct device_node *np = pdev->dev.of_node; > > > + const struct atmel_tcb_config *tcb_config; > > > + const struct of_device_id *match; > > > + struct atmel_tc_data *priv; > > > + char clk_name[] = "t0_clk"; > > > + struct regmap *regmap; > > > + struct clk *clk[3]; > > > + int channel; > > > + int ret, i; > > > + > > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > > + if (!priv) > > > + return -ENOMEM; > > > + > > > + platform_set_drvdata(pdev, priv); > > > + > > > + match = of_match_node(atmel_tc_of_match, np->parent); > > > + tcb_config = match->data; > > > + if (!tcb_config) { > > > + dev_err(&pdev->dev, "No matching parent node found\n"); > > > + return -ENODEV; > > > + } > > > + > > > + regmap = syscon_node_to_regmap(np->parent); > > > + if (IS_ERR(priv->regmap)) > > > + return PTR_ERR(priv->regmap); > > > + > > > + /* max. channels number is 2 when in QDEC mode */ > > > + priv->num_channels = of_property_count_u32_elems(np, "reg"); > > > + if (priv->num_channels < 0) { > > > + dev_err(&pdev->dev, "Invalid or missing channel\n"); > > > + return -EINVAL; > > > + } > > > + > > > + /* Register channels and initialize clocks */ > > > + for (i = 0; i < priv->num_channels; i++) { > > > + ret = of_property_read_u32_index(np, "reg", i, &channel); > > > + if (ret < 0 || channel > 2) > > > + return -ENODEV; > > > + > > > + priv->channel[i] = channel; > > > + > > > + clk_name[1] += channel; > > > > You're being clever here (not a bad thing) but this looks too easy to > > get wrong and confused. I'd much rather see a snprintf here for clarity > > and know that nothing funny is going to happen. > > > > Well ok let's be less clever for the sake of clarity :). > > > > + clk[i] = of_clk_get_by_name(np->parent, clk_name); > > > + if (IS_ERR(clk[i])) { > > > + /* Fallback to t0_clk */ > > > + clk[i] = of_clk_get_by_name(np->parent, "t0_clk"); > > > + if (IS_ERR(clk[i])) > > > + return PTR_ERR(clk[i]); > > > + } > > > + > > > + ret = clk_prepare_enable(clk[i]); > > > + if (ret) > > > + return ret; > > > + > > > + dev_info(&pdev->dev, > > > + "Initialized capture mode on channel %d\n", > > > + channel); > > > + } > > > + > > > + priv->tc_cfg = tcb_config; > > > + priv->regmap = regmap; > > > + priv->counter.name = dev_name(&pdev->dev); > > > + priv->counter.parent = &pdev->dev; > > > + priv->counter.ops = &atmel_tc_ops; > > > + priv->counter.num_counts = 1; > > > > Use ARRAY_SIZE here so that future reviewers will know that num_counts > > matches what's in the atmel_tc_count array without having to check so > > themselves. > > > > OK, yet the atmel_tc_count isn't an array I guess that it shall be. > > Thanks, > > Kamel > > > William Breathitt Gray > > > > > + priv->counter.counts = &atmel_tc_count; > > > + priv->counter.num_signals = ARRAY_SIZE(atmel_tc_count_signals); > > > + priv->counter.signals = atmel_tc_count_signals; > > > + priv->counter.priv = priv; > > > + > > > + ret = devm_counter_register(&pdev->dev, &priv->counter); > > > + if (ret < 0) { > > > + for (i = 0; i < priv->num_channels; i++) > > > + clk_disable_unprepare(clk[i]); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static const struct of_device_id atmel_tc_dt_ids[] = { > > > + { .compatible = "atmel,tcb-capture", }, > > > + { /* sentinel */ }, > > > +}; > > > +MODULE_DEVICE_TABLE(of, atmel_tc_dt_ids); > > > + > > > +static struct platform_driver atmel_tc_driver = { > > > + .probe = atmel_tc_probe, > > > + .driver = { > > > + .name = "atmel-tcb-capture", > > > + .of_match_table = atmel_tc_dt_ids, > > > + }, > > > +}; > > > +module_platform_driver(atmel_tc_driver); > > > + > > > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>"); > > > +MODULE_DESCRIPTION("Atmel TCB Capture driver"); > > > +MODULE_LICENSE("GPL v2"); > > > -- > > > 2.25.0 > > > > > > > -- > Kamel Bouhara, Bootlin > Embedded Linux and kernel engineering > https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-09 13:16 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-06 15:53 [PATCH 0/3] Atmel TCB capture driver Kamel Bouhara 2020-04-06 15:53 ` [PATCH 1/3] ARM: at91: add atmel tcb capabilities Kamel Bouhara 2020-04-07 16:53 ` William Breathitt Gray 2020-04-07 20:21 ` Kamel Bouhara 2020-04-06 15:53 ` [PATCH 2/3] dt-bindings: counter: atmel-tcb-capture counter Kamel Bouhara 2020-04-06 15:53 ` [PATCH 3/3] counter: Add atmel TCB capture counter Kamel Bouhara 2020-04-08 21:31 ` William Breathitt Gray 2020-04-08 21:56 ` Alexandre Belloni 2020-04-09 8:25 ` Kamel Bouhara 2020-04-09 13:16 ` William Breathitt Gray
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).