* Re: [PATCH v6 4/9] counter: 104-quad-8: Add Generic Counter interface support
From: Jonathan Cameron @ 2018-05-20 15:42 UTC (permalink / raw)
To: William Breathitt Gray
Cc: benjamin.gaignard, fabrice.gasnier, linux-iio, linux-kernel,
devicetree, linux-arm-kernel
In-Reply-To: <881ede525a87ef68fad76cc757ce0ba72df03e5a.1526487615.git.vilhelm.gray@gmail.com>
On Wed, 16 May 2018 13:51:25 -0400
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> This patch adds support for the Generic Counter interface to the
> 104-QUAD-8 driver. The existing 104-QUAD-8 device interface should not
> be affected by this patch; all changes are intended as supplemental
> additions as perceived by the user.
>
> Generic Counter Counts are created for the eight quadrature channel
> counts, as well as their respective quadrature A and B Signals (which
> are associated via respective Synapse structures) and respective index
> Signals.
>
> The new Generic Counter interface sysfs attributes are intended to
> expose the same functionality and data available via the existing
> 104-QUAD-8 IIO device interface; the Generic Counter interface serves
> to provide the respective functionality and data in a standard way
> expected of counter devices.
>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
A few general comments that applied just as well to the original driver
as they do to the modified version.
I wonder if this would be easier to review as two patches.
Move the driver then add the counter interfaces?
Right now people kind of have to review the old IIO driver and
all the new stuff which is a big job..
Jonathan
> ---
> MAINTAINERS | 4 +-
> drivers/counter/104-quad-8.c | 1335 ++++++++++++++++++++++++++++++
> drivers/counter/Kconfig | 21 +
> drivers/counter/Makefile | 2 +
> drivers/iio/counter/104-quad-8.c | 596 -------------
> drivers/iio/counter/Kconfig | 17 -
> drivers/iio/counter/Makefile | 1 -
> 7 files changed, 1360 insertions(+), 616 deletions(-)
> create mode 100644 drivers/counter/104-quad-8.c
> delete mode 100644 drivers/iio/counter/104-quad-8.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a01aa63fb33..f11bf7885aeb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -266,12 +266,12 @@ L: linux-gpio@vger.kernel.org
> S: Maintained
> F: drivers/gpio/gpio-104-idio-16.c
>
> -ACCES 104-QUAD-8 IIO DRIVER
> +ACCES 104-QUAD-8 DRIVER
> M: William Breathitt Gray <vilhelm.gray@gmail.com>
> L: linux-iio@vger.kernel.org
> S: Maintained
> F: Documentation/ABI/testing/sysfs-bus-iio-counter-104-quad-8
> -F: drivers/iio/counter/104-quad-8.c
> +F: drivers/counter/104-quad-8.c
>
> ACCES PCI-IDIO-16 GPIO DRIVER
> M: William Breathitt Gray <vilhelm.gray@gmail.com>
> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
> new file mode 100644
> index 000000000000..7c72fb72d660
> --- /dev/null
> +++ b/drivers/counter/104-quad-8.c
> @@ -0,0 +1,1335 @@
> +// SPDX-License-Identifier: GPL-2.0-only
If you are happy with SPDX drop the GPL text below to keep things
short.
> +/*
> + * IIO driver for the ACCES 104-QUAD-8
> + * Copyright (C) 2016 William Breathitt Gray
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * This driver supports the ACCES 104-QUAD-8 and ACCES 104-QUAD-4.
> + */
> +#include <linux/bitops.h>
...
> +static int quad8_probe(struct device *dev, unsigned int id)
> +{
> + struct iio_dev *indio_dev;
> + struct quad8_iio *quad8iio;
> + int i, j;
> + unsigned int base_offset;
> + int err;
> +
> + if (!devm_request_region(dev, base[id], QUAD8_EXTENT, dev_name(dev))) {
> + dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
> + base[id], base[id] + QUAD8_EXTENT);
> + return -EBUSY;
> + }
> +
> + /* Allocate IIO device; this also allocates driver data structure */
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*quad8iio));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + /* Initialize IIO device */
> + indio_dev->info = &quad8_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->num_channels = ARRAY_SIZE(quad8_channels);
> + indio_dev->channels = quad8_channels;
> + indio_dev->name = dev_name(dev);
> + indio_dev->dev.parent = dev;
> +
> + /* Initialize Counter device and driver data */
> + quad8iio = iio_priv(indio_dev);
> + quad8iio->counter.name = dev_name(dev);
> + quad8iio->counter.parent = dev;
> + quad8iio->counter.ops = &quad8_ops;
> + quad8iio->counter.counts = quad8_counts;
> + quad8iio->counter.num_counts = ARRAY_SIZE(quad8_counts);
> + quad8iio->counter.signals = quad8_signals;
> + quad8iio->counter.num_signals = ARRAY_SIZE(quad8_signals);
> + quad8iio->counter.priv = quad8iio;
> + quad8iio->base = base[id];
> +
> + /* Reset all counters and disable interrupt function */
> + outb(0x01, base[id] + 0x11);
> + /* Set initial configuration for all counters */
> + for (i = 0; i < QUAD8_NUM_COUNTERS; i++) {
> + base_offset = base[id] + 2 * i;
> + /* Reset Byte Pointer */
> + outb(0x01, base_offset + 1);
I'm going to be fussy. There are lots of values
in here that look like register bits and you could exchange much of
this documentation for a some good defines...
Taking base_offset + 1 bits 5 and 6 look to select the actual register
and the rest of them do the control.
Anyhow, not critical but the readability of this code could be improved
somewhat.
> + /* Reset Preset Register */
> + for (j = 0; j < 3; j++)
> + outb(0x00, base_offset);
> + /* Reset Borrow, Carry, Compare, and Sign flags */
> + outb(0x04, base_offset + 1);
> + /* Reset Error flag */
> + outb(0x06, base_offset + 1);
> + /* Binary encoding; Normal count; non-quadrature mode */
> + outb(0x20, base_offset + 1);
> + /* Disable A and B inputs; preset on index; FLG1 as Carry */
> + outb(0x40, base_offset + 1);
> + /* Disable index function; negative index polarity */
> + outb(0x60, base_offset + 1);
> + }
> + /* Enable all counters */
> + outb(0x00, base[id] + 0x11);
> +
> + /* Register IIO device */
> + err = devm_iio_device_register(dev, indio_dev);
> + if (err)
> + return err;
> +
> + /* Register Counter device */
> + return devm_counter_register(dev, &quad8iio->counter);
> +}
> +
> +static struct isa_driver quad8_driver = {
> + .probe = quad8_probe,
> + .driver = {
> + .name = "104-quad-8"
> + }
> +};
> +
> +module_isa_driver(quad8_driver, num_quad8);
> +
> +MODULE_AUTHOR("William Breathitt Gray <vilhelm.gray@gmail.com>");
> +MODULE_DESCRIPTION("ACCES 104-QUAD-8 IIO driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 65fa92abd5a4..73f03372484f 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -16,3 +16,24 @@ menuconfig COUNTER
> consumption. The Generic Counter interface enables drivers to support
> and expose a common set of components and functionality present in
> counter devices.
> +
> +if COUNTER
> +
> +config 104_QUAD_8
> + tristate "ACCES 104-QUAD-8 driver"
> + depends on PC104 && X86 && IIO
> + select ISA_BUS_API
> + help
> + Say yes here to build support for the ACCES 104-QUAD-8 quadrature
> + encoder counter/interface device family (104-QUAD-8, 104-QUAD-4).
> +
> + Performing a write to a counter's IIO_CHAN_INFO_RAW sets the counter and
> + also clears the counter's respective error flag. Although the counters
> + have a 25-bit range, only the lower 24 bits may be set, either directly
> + or via a counter's preset attribute. Interrupts are not supported by
> + this driver.
This text probably wants to be updated to reflect the new counter subsystem support..
> +
> + The base port addresses for the devices may be configured via the base
> + array module parameter.
> +
> +endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index ad1ba7109cdc..23a4f6263e45 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -6,3 +6,5 @@
>
> obj-$(CONFIG_COUNTER) += counter.o
> counter-y := generic-counter.o
> +
> +obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
> diff --git a/drivers/iio/counter/104-quad-8.c b/drivers/iio/counter/104-quad-8.c
...
^ permalink raw reply
* Re: [PATCH v6 3/9] docs: Add Generic Counter interface documentation
From: Jonathan Cameron @ 2018-05-20 15:31 UTC (permalink / raw)
To: William Breathitt Gray
Cc: devicetree, benjamin.gaignard, linux-iio, linux-kernel,
fabrice.gasnier, linux-arm-kernel
In-Reply-To: <aa4d62315bb11ddc66d0e1a685c81b1721ffe624.1526487615.git.vilhelm.gray@gmail.com>
On Wed, 16 May 2018 13:51:06 -0400
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> This patch adds high-level documentation about the Generic Counter
> interface.
>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
Various comments inline. I've been doing a lot long reviews recently
(outside of the kernel world) and keep discovering the old rule that
everytime you read a document you'll find something else to
improve :(
Jonathan
> ---
> Documentation/driver-api/generic-counter.rst | 336 +++++++++++++++++++
> Documentation/driver-api/index.rst | 1 +
> MAINTAINERS | 1 +
> 3 files changed, 338 insertions(+)
> create mode 100644 Documentation/driver-api/generic-counter.rst
>
> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> new file mode 100644
> index 000000000000..5c6b9c008c06
> --- /dev/null
> +++ b/Documentation/driver-api/generic-counter.rst
> @@ -0,0 +1,336 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================
> +Generic Counter Interface
> +=========================
> +
> +Introduction
> +============
> +
> +Counter devices are prevalent within a diverse spectrum of industries.
> +The ubiquitous presence of these devices necessitates a common interface
> +and standard of interaction and exposure. This driver API attempts to
> +resolve the issue of duplicate code found among existing counter device
> +drivers by introducing a generic counter interface for consumption. The
> +Generic Counter interface enables drivers to support and expose a common
> +set of components and functionality present in counter devices.
> +
> +Theory
> +======
> +
> +Counter devices can vary greatly in design, but regardless of whether
> +some devices are quadrature encoder counters or tally counters, all
> +counter devices consist of a core set of components. This core set of
> +components, shared by all counter devices, is what forms the essence of
> +the Generic Counter interface.
> +
> +There are three core components to a counter:
Enumerate them here. If people are reading this as a paged document (pdf etc)
then the list of 3 as titles of next few sections may not be clear.
* Count
* Signal
* Synapse
> +
> +COUNT
> +-----
> +A Count represents the count data for a set of Signals. The Generic
> +Counter interface provides the following available count data types:
> +
> +* COUNT_POSITION_UNSIGNED:
> + Unsigned integer value representing position.
> +
> +* COUNT_POSITION_SIGNED:
> + Signed integer value representing position.
Just a thought: As the '0' position is effectively arbitrary is there any
actual difference between signed and unsigned? If we defined all counters
to be unsigned and just offset any signed ones so the range still fitted
would we end up with a simpler interface - there would be no real loss
of meaning that I can see.. I suppose it might not be what people expect
though when they see their counters start at a large positive value...
> +
> +A Count has a count function mode which represents the update behavior
> +for the count data. The Generic Counter interface provides the following
> +available count function modes:
> +
> +* Increase:
> + Accumulated count is incremented.
> +
> +* Decrease:
> + Accumulated count is decremented.
> +
> +* Pulse-Direction:
> + Rising edges on quadrature pair signal A updates the respective count.
> + The input level of quadrature pair signal B determines direction.
> +
Perhaps group the quadrature modes for the point of view of this document?
Might be slightly easier to read?
* Quadrature Modes
- x1 A: etc.
> +* Quadrature x1 A:
> + If direction is forward, rising edges on quadrature pair signal A
> + updates the respective count; if the direction is backward, falling
> + edges on quadrature pair signal A updates the respective count.
> + Quadrature encoding determines the direction.
> +
> +* Quadrature x1 B:
> + If direction is forward, rising edges on quadrature pair signal B
> + updates the respective count; if the direction is backward, falling
> + edges on quadrature pair signal B updates the respective count.
> + Quadrature encoding determines the direction.
> +
> +* Quadrature x2 A:
> + Any state transition on quadrature pair signal A updates the
> + respective count. Quadrature encoding determines the direction.
> +
> +* Quadrature x2 B:
> + Any state transition on quadrature pair signal B updates the
> + respective count. Quadrature encoding determines the direction.
> +
> +* Quadrature x2 Rising:
> + Rising edges on either quadrature pair signals updates the respective
> + count. Quadrature encoding determines the direction.
This one I've never met. Really? There are devices who do this form
of crazy? It gives really uneven counting and I'm failing to see when
it would ever make sense... References for these odd corner cases
would be good.
__|---|____|-----|____
____|----|____|-----|____
001122222223334444444
> +
> +* Quadrature x2 Falling:
> + Falling edges on either quadrature pair signals updates the respective
> + count. Quadrature encoding determines the direction.
> +
> +* Quadrature x4:
> + Any state transition on either quadrature pair signals updates the
> + respective count. Quadrature encoding determines the direction.
> +
> +A Count has a set of one or more associated Signals.
> +
> +SIGNAL
> +------
> +A Signal represents a counter input data; this is the input data that is
> +analyzed by the counter to determine the count data; e.g. a quadrature
> +signal output line of a rotary encoder. Not all counter devices provide
> +user access to the Signal data.
> +
> +The Generic Counter interface provides the following available signal
> +data types for when the Signal data is available for user access:
> +
> +* SIGNAL_LEVEL_LOW:
> + Signal line is in a low state.
> +
> +* SIGNAL_LEVEL_HIGH:
> + Signal line is in a high state.
> +
> +A Signal may be associated with one or more Counts.
> +
> +SYNAPSE
> +-------
> +A Synapse represents the association of a Signal with a respective
> +Count. Signal data affects respective Count data, and the Synapse
> +represents this relationship.
> +
> +The Synapse action mode specifies the Signal data condition which
> +triggers the respective Count's count function evaluation to update the
> +count data. The Generic Counter interface provides the following
> +available action modes:
> +
> +* None:
> + Signal does not trigger the count function. In Pulse-Direction count
> + function mode, this Signal is evaluated as Direction.
> +
> +* Rising Edge:
> + Low state transitions to high state.
> +
> +* Falling Edge:
> + High state transitions to low state.
> +
> +* Both Edges:
> + Any state transition.
> +
> +A counter is defined as a set of input signals associated with count
> +data that are generated by the evaluation of the state of the associated
> +input signals as defined by the respective count functions. Within the
> +context of the Generic Counter interface, a counter consists of Counts
> +each associated with a set of Signals, whose respective Synapse
> +instances represent the count function update conditions for the
> +associated Counts.
> +
> +Paradigm
> +========
> +
> +The most basic counter device may be expressed as a single Count
> +associated with a single Signal via a single Synapse. Take for example
> +a counter device which simply accumulates a count of rising edges on a
> +source input line::
> +
> + Count Synapse Signal
> + ----- ------- ------
> + +---------------------+
> + | Data: Count | Rising Edge ________
> + | Function: Increase | <------------- / Source \
> + | | ____________
> + +---------------------+
> +
> +In this example, the Signal is a source input line with a pulsing
> +voltage, while the Count is a persistent count value which is repeatedly
> +incremented. The Signal is associated with the respective Count via a
> +Synapse. The increase function is triggered by the Signal data condition
> +specified by the Synapse -- in this case a rising edge condition on the
> +voltage input line. In summary, the counter device existence and
> +behavior is aptly represented by respective Count, Signal, and Synapse
> +components: a rising edge condition triggers an increase function on an
> +accumulating count datum.
> +
> +A counter device is not limited to a single Signal; in fact, in theory
> +many Signals may be associated with even a single Count. For example, a
> +quadrature encoder counter device can keep track of position based on
> +the states of two input lines::
> +
> + Count Synapse Signal
> + ----- ------- ------
> + +-------------------------+
> + | Data: Position | Both Edges ___
> + | Function: Quadrature x4 | <------------ / A \
> + | | _______
> + | |
> + | | Both Edges ___
> + | | <------------ / B \
> + | | _______
> + +-------------------------+
> +
> +In this example, two Signals (quadrature encoder lines A and B) are
> +associated with a single Count: a rising or falling edge on either A or
> +B triggers the "Quadrature x4" function which determines the direction
> +of movement and updates the respective position data. The "Quadrature
> +x4" function is likely implemented in the hardware of the quadrature
> +encoder counter device; the Count, Signals, and Synapses simply
> +represent this hardware behavior and functionality.
> +
> +Signals associated with the same Count can have differing Synapse action
> +mode conditions. For example, a quadrature encoder counter device
> +operating in a non-quadrature Pulse-Direction mode could have one input
> +line dedicated for movement and a second input line dedicated for
> +direction::
> +
> + Count Synapse Signal
> + ----- ------- ------
> + +---------------------------+
> + | Data: Position | Rising Edge ___
> + | Function: Pulse-Direction | <------------- / A \ (Movement)
> + | | _______
> + | |
> + | | None ___
> + | | <------------- / B \ (Direction)
> + | | _______
> + +---------------------------+
> +
> +Only Signal A triggers the "Pulse-Direction" update function, but the
> +instantaneous state of Signal B is still required in order to know the
> +direction so that the position data may be properly updated. Ultimately,
> +both Signals are associated with the same Count via two respective
> +Synapses, but only one Synapse has an active action mode condition which
> +triggers the respective count function while the other is left with a
> +"None" condition action mode to indicate its respective Signal's
> +availability for state evaluation despite its non-triggering mode.
> +
> +Keep in mind that the Signal, Synapse, and Count are abstract
> +representations which do not need to be closely married to their
> +respective physical sources. This allows the user of a counter to
> +divorce themselves from the nuances of physical components (such as
> +whether an input line is differential or single-ended) and instead focus
> +on the core idea of what the data and process represent (e.g. position
> +as interpreted from quadrature encoding data).
> +
> +Userspace Interface
> +===================
> +
> +Several sysfs attributes are generated by the Generic Counter interface,
> +and reside under the /sys/bus/counter/devices/counterX directory, where
> +counterX refers to the respective counter device. Please see
> +Documentation/ABI/testing/sys-bus-counter-generic-sysfs for detailed
> +information on each Generic Counter interface sysfs attribute.
> +
> +Through these sysfs attributes, programs and scripts may interact with
> +the Generic Counter paradigm Counts, Signals, and Synapses of respective
> +counter devices.
> +
> +Driver API
> +==========
> +
> +Driver authors may utilize the Generic Counter interface in their code
> +by including the include/linux/iio/counter.h header file. This header
Didn't this move?
> +file provides several core data structures, function prototypes, and
> +macros for defining a counter device.
> +
> +.. kernel-doc:: include/linux/counter.h
> + :internal:
> +
> +.. kernel-doc:: drivers/counter/generic-counter.c
> + :export:
> +
> +Implementation
> +==============
> +
> +To support a counter device, a driver must first allocate the available
> +Counter Signals via counter_signal structures. These Signals should
> +be stored as an array and set to the signals array member of an
> +allocated counter_device structure before the Counter is registered to
> +the system.
> +
> +Counter Counts may be allocated via counter_count structures, and
> +respective Counter Signal associations (Synapses) made via
> +counter_synapse structures. Associated counter_synapse structures are
> +stored as an array and set to the the synapses array member of the
> +respective counter_count structure. These counter_count structures are
> +set to the counts array member of an allocated counter_device structure
> +before the Counter is registered to the system.
> +
> +Driver callbacks should be provided to the counter_device structure via
> +a constant counter_ops structure in order to communicate with the
> +device: to read and write various Signals and Counts, and to set and get
> +the "action mode" and "function mode" for various Synapses and Counts
> +respectively.
> +
> +A defined counter_device structure may be registered to the system by
> +passing it to the counter_register function, and unregistered by passing
> +it to the counter_unregister function. Similarly, the
> +devm_counter_register and devm_counter_unregister functions may be used
> +if device memory-managed registration is desired.
> +
> +Extension sysfs attributes can be created for auxiliary functionality
> +and data by passing in defined counter_device_ext, counter_count_ext,
> +and counter_signal_ext structures. In these cases, the
> +counter_device_ext structure is used for global configuration of the
> +respective Counter device, while the counter_count_ext and
> +counter_signal_ext structures allow for auxiliary exposure and
> +configuration of a specific Count or Signal respectively.
> +
> +Architecture
> +============
> +
> +When the Generic Counter interface counter module is loaded, the
> +counter_init function is called which registers a bus_type named
> +"counter" to the system. Subsequently, when the module is unloaded, the
> +counter_exit function is called which unregisters the bus_type named
> +"counter" from the system.
> +
> +Counter devices are registered to the system via the counter_register
> +function, and later removed via the counter_unregister function. The
> +counter_register function establishes a unique ID for the Counter
> +device and creates a respective sysfs directory, where X is the
> +mentioned unique ID:
> +
> + /sys/bus/counter/devices/counterX
> +
> +Sysfs attributes are created within the counterX directory to expose
> +functionality, configurations, and data relating to the Counts, Signals,
> +and Synapses of the Counter device, as well as options and information
> +for the Counter device itself.
> +
> +Each Signal has a directory created to house its relevant sysfs
> +attributes, where Y is the unique ID of the respective Signal:
> +
> + /sys/bus/counter/devices/counterX/signalY
> +
> +Similarly, each Count has a directory created to house its relevant
> +sysfs attributes, where Y is the unique ID of the respective Count:
> +
> + /sys/bus/counter/devices/counterX/countY
> +
> +For a more detailed breakdown of the available Generic Counter interface
> +sysfs attributes, please refer to the
> +Documentation/ABI/testing/sys-bus-counter file.
> +
> +The Signals and Counts associated with the Counter device are registered
> +to the system as well by the counter_register function. The
> +signal_read/signal_write driver callbacks are associated with their
> +respective Signal attributes, while the count_read/count_write and
> +function_get/function_set driver callbacks are associated with their
> +respective Count attributes; similarly, the same is true for the
> +action_get/action_set driver callbacks and their respective Synapse
> +attributes. If a driver callback is left undefined, then the respective
> +read/write permission is left disabled for the relevant attributes.
> +
> +Similarly, extension sysfs attributes are created for the defined
> +counter_device_ext, counter_count_ext, and counter_signal_ext
> +structures that are passed in.
> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
> index 6d8352c0f354..5fd747c4f2ce 100644
> --- a/Documentation/driver-api/index.rst
> +++ b/Documentation/driver-api/index.rst
> @@ -25,6 +25,7 @@ available subsections can be seen below.
> frame-buffer
> regulator
> iio/index
> + generic-counter
> input
> usb/index
> pci
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1413e3eb49e5..7a01aa63fb33 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3674,6 +3674,7 @@ M: William Breathitt Gray <vilhelm.gray@gmail.com>
> L: linux-iio@vger.kernel.org
> S: Maintained
> F: Documentation/ABI/testing/sysfs-bus-counter*
> +F: Documentation/driver-api/generic-counter.rst
> F: drivers/counter/
> F: include/linux/counter.h
>
^ permalink raw reply
* [PATCH] ASoC: codec: realtek: Make the node name generic
From: Fabio Estevam @ 2018-05-20 15:22 UTC (permalink / raw)
To: broonie; +Cc: Fabio Estevam, devicetree, alsa-devel, robh+dt
From: Fabio Estevam <fabio.estevam@nxp.com>
"The name of a node should be somewhat generic, reflecting the function
of the device and not its precise programming model."
Do as suggested in the bindings examples.
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
Documentation/devicetree/bindings/sound/rt274.txt | 2 +-
Documentation/devicetree/bindings/sound/rt5514.txt | 2 +-
Documentation/devicetree/bindings/sound/rt5616.txt | 2 +-
Documentation/devicetree/bindings/sound/rt5645.txt | 2 +-
Documentation/devicetree/bindings/sound/rt5651.txt | 2 +-
Documentation/devicetree/bindings/sound/rt5663.txt | 2 +-
6 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/rt274.txt b/Documentation/devicetree/bindings/sound/rt274.txt
index e9a6178..791a1bd 100644
--- a/Documentation/devicetree/bindings/sound/rt274.txt
+++ b/Documentation/devicetree/bindings/sound/rt274.txt
@@ -26,7 +26,7 @@ Pins on the device (for linking into audio routes) for RT274:
Example:
-codec: rt274@1c {
+rt274: codec@1c {
compatible = "realtek,rt274";
reg = <0x1c>;
interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
diff --git a/Documentation/devicetree/bindings/sound/rt5514.txt b/Documentation/devicetree/bindings/sound/rt5514.txt
index 4f33b0d..b25ed08 100644
--- a/Documentation/devicetree/bindings/sound/rt5514.txt
+++ b/Documentation/devicetree/bindings/sound/rt5514.txt
@@ -32,7 +32,7 @@ Pins on the device (for linking into audio routes) for I2C:
Example:
-codec: rt5514@57 {
+rt5514: codec@57 {
compatible = "realtek,rt5514";
reg = <0x57>;
};
diff --git a/Documentation/devicetree/bindings/sound/rt5616.txt b/Documentation/devicetree/bindings/sound/rt5616.txt
index e410858..540a4bf 100644
--- a/Documentation/devicetree/bindings/sound/rt5616.txt
+++ b/Documentation/devicetree/bindings/sound/rt5616.txt
@@ -26,7 +26,7 @@ Pins on the device (for linking into audio routes) for RT5616:
Example:
-codec: rt5616@1b {
+rt5616: codec@1b {
compatible = "realtek,rt5616";
reg = <0x1b>;
};
diff --git a/Documentation/devicetree/bindings/sound/rt5645.txt b/Documentation/devicetree/bindings/sound/rt5645.txt
index 7cee1f51..a03f9a8 100644
--- a/Documentation/devicetree/bindings/sound/rt5645.txt
+++ b/Documentation/devicetree/bindings/sound/rt5645.txt
@@ -69,4 +69,4 @@ codec: rt5650@1a {
realtek,dmic-en = "true";
realtek,en-jd-func = "true";
realtek,jd-mode = <3>;
-};
\ No newline at end of file
+};
diff --git a/Documentation/devicetree/bindings/sound/rt5651.txt b/Documentation/devicetree/bindings/sound/rt5651.txt
index b852218..a41199a 100644
--- a/Documentation/devicetree/bindings/sound/rt5651.txt
+++ b/Documentation/devicetree/bindings/sound/rt5651.txt
@@ -50,7 +50,7 @@ Pins on the device (for linking into audio routes) for RT5651:
Example:
-codec: rt5651@1a {
+rt5651: codec@1a {
compatible = "realtek,rt5651";
reg = <0x1a>;
realtek,dmic-en = "true";
diff --git a/Documentation/devicetree/bindings/sound/rt5663.txt b/Documentation/devicetree/bindings/sound/rt5663.txt
index 497bcfc..2338644 100644
--- a/Documentation/devicetree/bindings/sound/rt5663.txt
+++ b/Documentation/devicetree/bindings/sound/rt5663.txt
@@ -47,7 +47,7 @@ Pins on the device (for linking into audio routes) for RT5663:
Example:
-codec: rt5663@12 {
+rt5663: codec@12 {
compatible = "realtek,rt5663";
reg = <0x12>;
interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v6 2/9] counter: Documentation: Add Generic Counter sysfs documentation
From: Jonathan Cameron @ 2018-05-20 15:12 UTC (permalink / raw)
To: William Breathitt Gray
Cc: benjamin.gaignard, fabrice.gasnier, linux-iio, linux-kernel,
devicetree, linux-arm-kernel
In-Reply-To: <1bf56af37a1d6dab8181ce71f827914acdf5c4f7.1526487615.git.vilhelm.gray@gmail.com>
On Wed, 16 May 2018 13:50:55 -0400
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> This patch adds standard documentation for the userspace sysfs
> attributes of the Generic Counter interface.
>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
Some really minor stuff inline. No functional changes.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> Documentation/ABI/testing/sysfs-bus-counter | 241 ++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 242 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-counter
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> new file mode 100644
> index 000000000000..e4a45d231b4f
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -0,0 +1,241 @@
> +What: /sys/bus/counter/devices/counterX/countY/count
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Count data of Count Y represented as a string.
> +
> +What: /sys/bus/counter/devices/counterX/countY/ceiling
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Count value ceiling for Count Y. This is the upper limit for the
> + respective counter.
> +
> +What: /sys/bus/counter/devices/counterX/countY/floor
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Count value floor for Count Y. This is the lower limit for the
> + respective counter.
> +
> +What: /sys/bus/counter/devices/counterX/countY/count_mode
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Count mode for channel Y. The ceiling and floor values for
> + Count Y are used by the count mode where required. The following
> + count modes are available:
> +
> + Normal:
> + Counting is continuous in either direction.
> +
> + Range Limit:
> + An upper or lower limit is set, mimicking limit switches
> + in the mechanical counterpart. The upper limit is set to
> + the Count Y ceiling value, while the lower limit is set
> + to the Count Y floor value. The counter freezes at
> + count = ceiling when counting up, and at count = floor
> + when counting down. At either of these limits, the
> + counting is resumed only when the count direction is
> + reversed.
> +
> + Non-Recycle:
> + The counter is disabled whenever a counter overflow or
> + underflow takes place. The counter is re-enabled when a
> + new count value is loaded to the counter via a preset
> + operation or direct write.
> +
> + Modulo-N:
> + A count value boundary is set between the Count Y floor
> + value and the Count Y ceiling value. The counter is
> + reset to the Cunt Y floor value at count = ceiling when
> + counting up, while the counter is set to the Count Y
> + ceiling value at count = floor when counting down; the
> + counter does not freeze at the boundary points, but
> + counts continuously throughout.
> +
> +What: /sys/bus/counter/devices/counterX/countY/count_mode_available
> +What: /sys/bus/counter/devices/counterX/countY/error_noise_available
> +What: /sys/bus/counter/devices/counterX/countY/function_available
> +What: /sys/bus/counter/devices/counterX/countY/signalZ_action_available
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Discrete set of available values for the respective Count Y
> + configuration are listed in this file. Values are delineated by
> + newline characters.
> +
> +What: /sys/bus/counter/devices/counterX/countY/direction
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Read-only attribute that indicates the count direction of Count
> + Y. Two count directions are available: forward and backward.
> +
> + Some counter devices are able to determine the direction of
> + their counting. For example, quadrature encoding counters can
> + determine the direction of movement by evaluating the leading
> + phase of the respective A and B quadrature encoding signals.
> + This attribute exposes such count directions.
> +
> +What: /sys/bus/counter/devices/counterX/countY/enable
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Whether channel Y counter is enabled. Valid attribute values are
> + boolean.
> +
> + This attribute is intended to serve as a pause/unpause mechanism
> + for Count Y. Suppose a counter device is used to count the total
> + movement of a conveyor belt: this attribute allows an operator
> + to temporarily pause the counter, service the conveyor belt,
> + and then finally unpause the counter to continue where it had
> + left off.
> +
> +What: /sys/bus/counter/devices/counterX/countY/error_noise
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Read-only attribute that indicates whether excessive noise is
> + present at the channel Y counter inputs.
> +
> +What: /sys/bus/counter/devices/counterX/countY/function
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Count function mode of Count Y; count function evaluation is
> + triggered by conditions specified by the Count Y signalZ_action
> + attributes. The following count functions are available:
> +
> + Increase:
> + Accumulated count is incremented.
> +
> + Decrease:
> + Accumulated count is decremented.
> +
> + Pulse-Direction:
> + Rising edges on quadrature pair signal A updates the
> + respective count. The input level of quadrature pair
> + signal B determines direction.
> +
> + Quadrature x1 A:
> + If direction is forward, rising edges on quadrature pair
> + signal A updates the respective count; if the direction
> + is backward, falling edges on quadrature pair signal A
> + updates the respective count. Quadrature encoding
> + determines the direction.
> +
> + Quadrature x1 B:
> + If direction is forward, rising edges on quadrature pair
> + signal B updates the respective count; if the direction
> + is backward, falling edges on quadrature pair signal B
> + updates the respective count. Quadrature encoding
> + determines the direction.
> +
> + Quadrature x2 A:
> + Any state transition on quadrature pair signal A updates
> + the respective count. Quadrature encoding determines the
> + direction.
> +
> + Quadrature x2 B:
> + Any state transition on quadrature pair signal B updates
> + the respective count. Quadrature encoding determines the
> + direction.
> +
> + Quadrature x2 Rising:
> + Rising edges on either quadrature pair signals updates
> + the respective count. Quadrature encoding determines the
> + direction.
> +
> + Quadrature x2 Falling:
> + Falling edges on either quadrature pair signals updates
> + the respective count. Quadrature encoding determines the
> + direction.
> +
> + Quadrature x4:
> + Any state transition on either quadrature pair signals
> + updates the respective count. Quadrature encoding
> + determines the direction.
> +
> +What: /sys/bus/counter/devices/counterX/countY/name
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Read-only attribute that indicates the device-specific name of
> + Count Y. If possible, this should match the name of the
> + respective channel as it appears in the device datasheet
> + documentation text.
> +
> +What: /sys/bus/counter/devices/counterX/countY/preset
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + If the counter device supports preset registers, the preset
> + count for channel Y is provided by this attribute.
I would add a small block of text here saying what a "preset" typically is.
It is a term heavily used in encoders etc, but perhaps not some other types
of counter.
> +
> +What: /sys/bus/counter/devices/counterX/countY/preset_enable
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Whether channel Y counter preset operation is enabled. Valid
> + attribute values are boolean.
> +
> +What: /sys/bus/counter/devices/counterX/countY/signalZ_action
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Action mode of Count Y for Signal Z. This attribute indicates
> + the condition of Signal Z that triggers the count function
> + evaluation for Count Y. The following action modes are
> + available:
> +
> + None:
> + Signal does not trigger the count function. In
> + Pulse-Direction count function mode, this Signal is
> + evaluated as Direction.
> +
> + Rising Edge:
> + Low state transitions to high state.
> +
> + Falling Edge:
> + High state transitions to low state.
> +
> + Both Edges:
> + Any state transition.
> +
> +What: /sys/bus/counter/devices/counterX/name
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Read-only attribute that indicates the device-specific name of
> + the Counter. This should match the name of the device as it
> + appears in its respective datasheet documentation text.
As below, I'm not sure if "documentation text" makes it clearer or less clear..
> +
> +What: /sys/bus/counter/devices/counterX/num_counts
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Read-only attribute that indicates the total number of Counts
> + belonging to the Counter.
> +
> +What: /sys/bus/counter/devices/counterX/num_signals
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Read-only attribute that indicates the total number of Signals
> + belonging to the Counter.
> +
> +What: /sys/bus/counter/devices/counterX/signalY/signal
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Signal data of Signal Y represented as a string.
> +
> +What: /sys/bus/counter/devices/counterX/signalY/name
> +KernelVersion: 4.18
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Read-only attribute that indicates the device-specific name of
> + Signal Y. If possible, this should match the name of the
> + respective signal as it appears in the device datasheet
> + documentation text.
Not sure "documentation text" adds any clarity over the simply "device datasheet"
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2a016d73ab72..1413e3eb49e5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3673,6 +3673,7 @@ COUNTER SUBSYSTEM
> M: William Breathitt Gray <vilhelm.gray@gmail.com>
> L: linux-iio@vger.kernel.org
> S: Maintained
> +F: Documentation/ABI/testing/sysfs-bus-counter*
> F: drivers/counter/
> F: include/linux/counter.h
>
^ permalink raw reply
* Re: [PATCH v6 1/9] counter: Introduce the Generic Counter interface
From: Jonathan Cameron @ 2018-05-20 15:06 UTC (permalink / raw)
To: William Breathitt Gray
Cc: benjamin.gaignard, fabrice.gasnier, linux-iio, linux-kernel,
devicetree, linux-arm-kernel
In-Reply-To: <6e0c93ccc771033f0a8bc08bd92b453a1dacbd1b.1526487615.git.vilhelm.gray@gmail.com>
On Wed, 16 May 2018 13:50:43 -0400
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> This patch introduces the Generic Counter interface for supporting
> counter devices.
>
> In the context of the Generic Counter interface, a counter is defined as
> a device that reports one or more "counts" based on the state changes of
> one or more "signals" as evaluated by a defined "count function."
>
> Driver callbacks should be provided to communicate with the device: to
> read and write various Signals and Counts, and to set and get the
> "action mode" and "count function" for various Synapses and Counts
> respectively.
>
> To support a counter device, a driver must first allocate the available
> Counter Signals via counter_signal structures. These Signals should
> be stored as an array and set to the signals array member of an
> allocated counter_device structure before the Counter is registered to
> the system.
>
> Counter Counts may be allocated via counter_count structures, and
> respective Counter Signal associations (Synapses) made via
> counter_synapse structures. Associated counter_synapse structures are
> stored as an array and set to the the synapses array member of the
> respective counter_count structure. These counter_count structures are
> set to the counts array member of an allocated counter_device structure
> before the Counter is registered to the system.
>
> A counter device is registered to the system by passing the respective
> initialized counter_device structure to the counter_register function;
> similarly, the counter_unregister function unregisters the respective
> Counter. The devm_counter_register and devm_counter_unregister functions
> serve as device memory-managed versions of the counter_register and
> counter_unregister functions respectively.
>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
A few minor comments inline. I do somewhat wonder if we can cut
back on the huge amount of 'similar' code in here, but there tend to
be just enough small differences to make that really tricky...
Nothing major enough in here that I really plan on reading it again
(though you never know if you change lots ;)
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> MAINTAINERS | 7 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/counter/Kconfig | 18 +
> drivers/counter/Makefile | 8 +
> drivers/counter/generic-counter.c | 1541 +++++++++++++++++++++++++++++
> include/linux/counter.h | 554 +++++++++++
> 7 files changed, 2131 insertions(+)
> create mode 100644 drivers/counter/Kconfig
> create mode 100644 drivers/counter/Makefile
> create mode 100644 drivers/counter/generic-counter.c
> create mode 100644 include/linux/counter.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b65225d443a..2a016d73ab72 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3669,6 +3669,13 @@ W: http://www.fi.muni.cz/~kas/cosa/
> S: Maintained
> F: drivers/net/wan/cosa*
>
> +COUNTER SUBSYSTEM
> +M: William Breathitt Gray <vilhelm.gray@gmail.com>
> +L: linux-iio@vger.kernel.org
> +S: Maintained
> +F: drivers/counter/
> +F: include/linux/counter.h
> +
> CPMAC ETHERNET DRIVER
> M: Florian Fainelli <f.fainelli@gmail.com>
> L: netdev@vger.kernel.org
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 95b9ccc08165..70b3cc88dc0b 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -165,6 +165,8 @@ source "drivers/memory/Kconfig"
>
> source "drivers/iio/Kconfig"
>
> +source "drivers/counter/Kconfig"
> +
Same comment as below.
> source "drivers/ntb/Kconfig"
>
> source "drivers/vme/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 24cd47014657..5914c78688c3 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -165,6 +165,7 @@ obj-$(CONFIG_PM_DEVFREQ) += devfreq/
> obj-$(CONFIG_EXTCON) += extcon/
> obj-$(CONFIG_MEMORY) += memory/
> obj-$(CONFIG_IIO) += iio/
> +obj-$(CONFIG_COUNTER) += counter/
I can see your logic in putting this here, but I think the convention
is to go at the end rather than grouping.
> obj-$(CONFIG_VME_BUS) += vme/
> obj-$(CONFIG_IPACK_BUS) += ipack/
> obj-$(CONFIG_NTB) += ntb/
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> new file mode 100644
> index 000000000000..65fa92abd5a4
> --- /dev/null
> +++ b/drivers/counter/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# Counter devices
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menuconfig COUNTER
> + tristate "Counter support"
> + help
> + Provides Generic Counter interface support for counter devices.
> +
> + Counter devices are prevalent within a diverse spectrum of industries.
> + The ubiquitous presence of these devices necessitates a common
> + interface and standard of interaction and exposure. This driver API
> + attempts to resolve the issue of duplicate code found among existing
> + counter device drivers by providing a generic counter interface for
> + consumption. The Generic Counter interface enables drivers to support
> + and expose a common set of components and functionality present in
> + counter devices.
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> new file mode 100644
> index 000000000000..ad1ba7109cdc
> --- /dev/null
> +++ b/drivers/counter/Makefile
> @@ -0,0 +1,8 @@
> +#
> +# Makefile for Counter devices
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +
> +obj-$(CONFIG_COUNTER) += counter.o
> +counter-y := generic-counter.o
> diff --git a/drivers/counter/generic-counter.c b/drivers/counter/generic-counter.c
> new file mode 100644
> index 000000000000..0d83b862453f
> --- /dev/null
> +++ b/drivers/counter/generic-counter.c
> @@ -0,0 +1,1541 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Generic Counter interface
> + * Copyright (C) 2017 William Breathitt Gray
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
As below, SPDX and license seems silly. Unless you are feeling paranoid
just drop the license text.
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/fs.h>
> +#include <linux/gfp.h>
> +#include <linux/idr.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#include <linux/counter.h>
> +
> +const char *const count_direction_str[2] = {
> + [COUNT_DIRECTION_FORWARD] = "forward",
> + [COUNT_DIRECTION_BACKWARD] = "backward"
> +};
> +EXPORT_SYMBOL(count_direction_str);
> +
> +const char *const count_mode_str[4] = {
> + [COUNT_MODE_NORMAL] = "normal",
> + [COUNT_MODE_RANGE_LIMIT] = "range limit",
> + [COUNT_MODE_NON_RECYCLE] = "non-recycle",
> + [COUNT_MODE_MODULO_N] = "modulo-n"
> +};
> +EXPORT_SYMBOL(count_mode_str);
> +
> +ssize_t counter_signal_enum_read(struct counter_device *counter,
> + struct counter_signal *signal, void *priv,
> + char *buf)
> +{
> + const struct counter_signal_enum_ext *const e = priv;
> + int err;
> + size_t index;
> +
> + if (!e->get)
> + return -EINVAL;
> +
> + err = e->get(counter, signal, &index);
> + if (err)
> + return err;
> +
> + if (index >= e->num_items)
> + return -EINVAL;
> +
> + return scnprintf(buf, PAGE_SIZE, "%s\n", e->items[index]);
> +}
> +EXPORT_SYMBOL(counter_signal_enum_read);
> +
> +ssize_t counter_signal_enum_write(struct counter_device *counter,
> + struct counter_signal *signal, void *priv,
> + const char *buf, size_t len)
> +{
> + const struct counter_signal_enum_ext *const e = priv;
> + ssize_t index;
> + int err;
> +
> + if (!e->set)
> + return -EINVAL;
> +
> + index = __sysfs_match_string(e->items, e->num_items, buf);
> + if (index < 0)
> + return index;
> +
> + err = e->set(counter, signal, index);
> + if (err)
> + return err;
> +
> + return len;
> +}
> +EXPORT_SYMBOL(counter_signal_enum_write);
> +
> +ssize_t counter_signal_enum_available_read(struct counter_device *counter,
> + struct counter_signal *signal,
> + void *priv, char *buf)
> +{
> + const struct counter_signal_enum_ext *const e = priv;
> + size_t i;
> + size_t len = 0;
> +
> + if (!e->num_items)
> + return 0;
> +
> + for (i = 0; i < e->num_items; i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
> + e->items[i]);
> +
> + return len;
> +}
> +EXPORT_SYMBOL(counter_signal_enum_available_read);
> +
> +ssize_t counter_count_enum_read(struct counter_device *counter,
> + struct counter_count *count, void *priv,
> + char *buf)
> +{
> + const struct counter_count_enum_ext *const e = priv;
> + int err;
> + size_t index;
> +
> + if (!e->get)
> + return -EINVAL;
> +
> + err = e->get(counter, count, &index);
> + if (err)
> + return err;
> +
> + if (index >= e->num_items)
> + return -EINVAL;
> +
> + return scnprintf(buf, PAGE_SIZE, "%s\n", e->items[index]);
> +}
> +EXPORT_SYMBOL(counter_count_enum_read);
> +
> +ssize_t counter_count_enum_write(struct counter_device *counter,
> + struct counter_count *count, void *priv,
> + const char *buf, size_t len)
> +{
> + const struct counter_count_enum_ext *const e = priv;
> + ssize_t index;
> + int err;
> +
> + if (!e->set)
> + return -EINVAL;
> +
> + index = __sysfs_match_string(e->items, e->num_items, buf);
> + if (index < 0)
> + return index;
> +
> + err = e->set(counter, count, index);
> + if (err)
> + return err;
> +
> + return len;
> +}
> +EXPORT_SYMBOL(counter_count_enum_write);
> +
> +ssize_t counter_count_enum_available_read(struct counter_device *counter,
> + struct counter_count *count,
> + void *priv, char *buf)
> +{
> + const struct counter_count_enum_ext *const e = priv;
> + size_t i;
> + size_t len = 0;
> +
> + if (!e->num_items)
> + return 0;
> +
> + for (i = 0; i < e->num_items; i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
> + e->items[i]);
> +
> + return len;
> +}
> +EXPORT_SYMBOL(counter_count_enum_available_read);
> +
> +ssize_t counter_device_enum_read(struct counter_device *counter, void *priv,
> + char *buf)
> +{
> + const struct counter_device_enum_ext *const e = priv;
> + int err;
> + size_t index;
> +
> + if (!e->get)
> + return -EINVAL;
> +
> + err = e->get(counter, &index);
> + if (err)
> + return err;
> +
> + if (index >= e->num_items)
> + return -EINVAL;
> +
> + return scnprintf(buf, PAGE_SIZE, "%s\n", e->items[index]);
> +}
> +EXPORT_SYMBOL(counter_device_enum_read);
> +
> +ssize_t counter_device_enum_write(struct counter_device *counter, void *priv,
> + const char *buf, size_t len)
> +{
> + const struct counter_device_enum_ext *const e = priv;
> + ssize_t index;
> + int err;
> +
> + if (!e->set)
> + return -EINVAL;
> +
> + index = __sysfs_match_string(e->items, e->num_items, buf);
> + if (index < 0)
> + return index;
> +
> + err = e->set(counter, index);
> + if (err)
> + return err;
> +
> + return len;
> +}
> +EXPORT_SYMBOL(counter_device_enum_write);
> +
> +ssize_t counter_device_enum_available_read(struct counter_device *counter,
> + void *priv, char *buf)
> +{
> + const struct counter_device_enum_ext *const e = priv;
> + size_t i;
> + size_t len = 0;
> +
> + if (!e->num_items)
> + return 0;
> +
> + for (i = 0; i < e->num_items; i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
> + e->items[i]);
> +
> + return len;
> +}
> +EXPORT_SYMBOL(counter_device_enum_available_read);
> +
> +static const char *const signal_level_str[] = {
> + [SIGNAL_LEVEL_LOW] = "low",
> + [SIGNAL_LEVEL_HIGH] = "high"
> +};
> +
> +/**
> + * set_signal_read_value - set signal_read_value data
> + * @val: signal_read_value structure to set
> + * @type: property Signal data represents
> + * @data: Signal data
> + *
> + * This function sets an opaque signal_read_value structure with the provided
> + * Signal data.
> + */
> +void set_signal_read_value(struct signal_read_value *const val,
> + const enum signal_value_type type, void *const data)
> +{
> + if (type == SIGNAL_LEVEL)
> + val->len = scnprintf(val->buf, PAGE_SIZE, "%s\n",
> + signal_level_str[*(enum signal_level *)data]);
> + else
> + val->len = 0;
> +}
> +EXPORT_SYMBOL(set_signal_read_value);
> +
> +/**
> + * set_count_read_value - set count_read_value data
> + * @val: count_read_value structure to set
> + * @type: property Count data represents
> + * @data: Count data
> + *
> + * This function sets an opaque count_read_value structure with the provided
> + * Count data.
> + */
> +void set_count_read_value(struct count_read_value *const val,
> + const enum count_value_type type, void *const data)
> +{
> + switch (type) {
> + case COUNT_POSITION_UNSIGNED:
> + val->len = scnprintf(val->buf, PAGE_SIZE, "%lu\n",
> + *(unsigned long *)data);
> + break;
> + case COUNT_POSITION_SIGNED:
> + val->len = scnprintf(val->buf, PAGE_SIZE, "%ld\n",
> + *(long *)data);
> + break;
> + default:
> + val->len = 0;
> + }
> +}
> +EXPORT_SYMBOL(set_count_read_value);
> +
> +/**
> + * get_count_write_value - get count_write_value data
> + * @data: Count data
> + * @type: property Count data represents
> + * @val: count_write_value structure containing data
> + *
> + * This function extracts Count data from the provided opaque count_write_value
> + * structure and stores it at the address provided by @data.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int get_count_write_value(void *const data, const enum count_value_type type,
> + const struct count_write_value *const val)
> +{
> + int err;
> +
> + switch (type) {
> + case COUNT_POSITION_UNSIGNED:
> + err = kstrtoul(val->buf, 0, data);
> + if (err)
> + return err;
> + break;
> + case COUNT_POSITION_SIGNED:
> + err = kstrtol(val->buf, 0, data);
> + if (err)
> + return err;
> + break;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(get_count_write_value);
> +
> +struct counter_device_attr {
> + struct device_attribute dev_attr;
> + struct list_head l;
> + void *component;
> +};
> +
> +static int counter_attribute_create(
> + struct counter_device_attr_group *const group,
> + const char *const prefix,
> + const char *const name,
> + ssize_t (*show)(struct device *dev, struct device_attribute *attr,
> + char *buf),
> + ssize_t (*store)(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t len),
> + void *const component)
> +{
> + struct counter_device_attr *counter_attr;
> + struct device_attribute *dev_attr;
> + int err;
> + struct list_head *const attr_list = &group->attr_list;
> +
> + /* Allocate a Counter device attribute */
> + counter_attr = kzalloc(sizeof(*counter_attr), GFP_KERNEL);
> + if (!counter_attr)
> + return -ENOMEM;
> + dev_attr = &counter_attr->dev_attr;
> +
> + sysfs_attr_init(&dev_attr->attr);
> +
> + /* Configure device attribute */
> + dev_attr->attr.name = kasprintf(GFP_KERNEL, "%s%s", prefix, name);
> + if (!dev_attr->attr.name) {
> + err = -ENOMEM;
> + goto err_free_counter_attr;
> + }
> + if (show) {
> + dev_attr->attr.mode |= 0444;
> + dev_attr->show = show;
> + }
> + if (store) {
> + dev_attr->attr.mode |= 0200;
> + dev_attr->store = store;
> + }
> +
> + /* Store associated Counter component with attribute */
> + counter_attr->component = component;
> +
> + /* Keep track of the attribute for later cleanup */
> + list_add(&counter_attr->l, attr_list);
> + group->num_attr++;
> +
> + return 0;
> +
> +err_free_counter_attr:
> + kfree(counter_attr);
> + return err;
> +}
> +
> +#define to_counter_attr(_dev_attr) \
> + container_of(_dev_attr, struct counter_device_attr, dev_attr)
> +
> +struct signal_comp_t {
> + struct counter_signal *signal;
> +};
> +
> +static ssize_t counter_signal_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct counter_device *const counter = dev_get_drvdata(dev);
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct signal_comp_t *const component = devattr->component;
> + struct counter_signal *const signal = component->signal;
> + int err;
> + struct signal_read_value val = { .buf = buf };
> +
> + err = counter->ops->signal_read(counter, signal, &val);
> + if (err)
> + return err;
> +
> + return val.len;
> +}
> +
> +struct name_comp_t {
> + const char *name;
> +};
> +
> +static ssize_t counter_device_attr_name_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + const struct name_comp_t *const comp = to_counter_attr(attr)->component;
> +
> + return scnprintf(buf, PAGE_SIZE, "%s\n", comp->name);
> +}
> +
> +static int counter_name_attribute_create(
> + struct counter_device_attr_group *const group,
> + const char *const name)
> +{
> + struct name_comp_t *name_comp;
> + int err;
> +
> + /* Skip if no name */
> + if (!name)
> + return 0;
> +
> + /* Allocate name attribute component */
> + name_comp = kmalloc(sizeof(*name_comp), GFP_KERNEL);
> + if (!name_comp)
> + return -ENOMEM;
> + name_comp->name = name;
> +
> + /* Allocate Signal name attribute */
> + err = counter_attribute_create(group, "", "name",
> + counter_device_attr_name_show, NULL,
> + name_comp);
> + if (err)
> + goto err_free_name_comp;
> +
> + return 0;
> +
> +err_free_name_comp:
> + kfree(name_comp);
> + return err;
> +}
> +
> +struct signal_ext_comp_t {
> + struct counter_signal *signal;
> + const struct counter_signal_ext *ext;
> +};
> +
> +static ssize_t counter_signal_ext_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct signal_ext_comp_t *const comp = devattr->component;
> + const struct counter_signal_ext *const ext = comp->ext;
> +
> + return ext->read(dev_get_drvdata(dev), comp->signal, ext->priv, buf);
> +}
> +
> +static ssize_t counter_signal_ext_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct signal_ext_comp_t *const comp = devattr->component;
> + const struct counter_signal_ext *const ext = comp->ext;
> +
> + return ext->write(dev_get_drvdata(dev), comp->signal, ext->priv, buf,
> + len);
> +}
> +
> +static void free_counter_device_attr_list(struct list_head *attr_list)
> +{
> + struct counter_device_attr *p, *n;
> +
> + list_for_each_entry_safe(p, n, attr_list, l) {
> + kfree(p->dev_attr.attr.name);
> + kfree(p->component);
> + list_del(&p->l);
> + kfree(p);
> + }
> +}
> +
> +static int counter_signal_ext_register(
> + struct counter_device_attr_group *const group,
> + struct counter_signal *const signal)
> +{
> + const size_t num_ext = signal->num_ext;
> + size_t i;
> + const struct counter_signal_ext *ext;
> + struct signal_ext_comp_t *signal_ext_comp;
> + int err;
> +
> + /* Create an attribute for each extension */
> + for (i = 0 ; i < num_ext; i++) {
> + ext = signal->ext + i;
> +
> + /* Allocate signal_ext attribute component */
> + signal_ext_comp = kmalloc(sizeof(*signal_ext_comp), GFP_KERNEL);
> + if (!signal_ext_comp) {
> + err = -ENOMEM;
> + goto err_free_attr_list;
> + }
> + signal_ext_comp->signal = signal;
> + signal_ext_comp->ext = ext;
> +
> + /* Allocate a Counter device attribute */
> + err = counter_attribute_create(group, "", ext->name,
> + (ext->read) ? counter_signal_ext_show : NULL,
> + (ext->write) ? counter_signal_ext_store : NULL,
> + signal_ext_comp);
> + if (err) {
> + kfree(signal_ext_comp);
> + goto err_free_attr_list;
> + }
> + }
> +
> + return 0;
> +
> +err_free_attr_list:
> + free_counter_device_attr_list(&group->attr_list);
> + return err;
> +}
> +
> +static int counter_signal_attributes_create(
> + struct counter_device_attr_group *const group,
> + const struct counter_device *const counter,
> + struct counter_signal *const signal)
> +{
> + struct signal_comp_t *signal_comp;
> + int err;
> +
> + /* Allocate Signal attribute component */
> + signal_comp = kmalloc(sizeof(*signal_comp), GFP_KERNEL);
> + if (!signal_comp)
> + return -ENOMEM;
> + signal_comp->signal = signal;
> +
> + /* Create main Signal attribute */
> + err = counter_attribute_create(group, "", "signal",
> + (counter->ops->signal_read) ? counter_signal_show : NULL, NULL,
> + signal_comp);
> + if (err) {
> + kfree(signal_comp);
> + return err;
> + }
> +
> + /* Create Signal name attribute */
> + err = counter_name_attribute_create(group, signal->name);
> + if (err)
> + goto err_free_attr_list;
> +
> + /* Register Signal extension attributes */
> + err = counter_signal_ext_register(group, signal);
> + if (err)
> + goto err_free_attr_list;
> +
> + return 0;
> +
> +err_free_attr_list:
> + free_counter_device_attr_list(&group->attr_list);
> + return err;
> +}
> +
> +static int counter_signals_register(
> + struct counter_device_attr_group *const groups_list,
> + const struct counter_device *const counter)
> +{
> + const size_t num_signals = counter->num_signals;
> + size_t i;
> + struct counter_signal *signal;
> + const char *name;
> + int err;
> +
> + /* Register each Signal */
> + for (i = 0; i < num_signals; i++) {
> + signal = counter->signals + i;
> +
> + /* Generate Signal attribute directory name */
> + name = kasprintf(GFP_KERNEL, "signal%d", signal->id);
> + if (!name) {
> + err = -ENOMEM;
> + goto err_free_attr_groups;
> + }
> + groups_list[i].attr_group.name = name;
> +
> + /* Create all attributes associated with Signal */
> + err = counter_signal_attributes_create(groups_list + i, counter,
> + signal);
> + if (err)
> + goto err_free_attr_groups;
> + }
> +
> + return 0;
> +
> +err_free_attr_groups:
> + do {
> + kfree(groups_list[i].attr_group.name);
> + free_counter_device_attr_list(&groups_list[i].attr_list);
> + } while (i--);
> + return err;
> +}
> +
> +static const char *const synapse_action_str[] = {
> + [SYNAPSE_ACTION_NONE] = "none",
> + [SYNAPSE_ACTION_RISING_EDGE] = "rising edge",
> + [SYNAPSE_ACTION_FALLING_EDGE] = "falling edge",
> + [SYNAPSE_ACTION_BOTH_EDGES] = "both edges"
> +};
> +
> +struct action_comp_t {
> + struct counter_synapse *synapse;
> + struct counter_count *count;
> +};
> +
> +static ssize_t counter_action_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + int err;
> + struct counter_device *const counter = dev_get_drvdata(dev);
> + const struct action_comp_t *const component = devattr->component;
> + struct counter_count *const count = component->count;
> + struct counter_synapse *const synapse = component->synapse;
> + size_t action_index;
> + enum synapse_action action;
> +
> + err = counter->ops->action_get(counter, count, synapse, &action_index);
> + if (err)
> + return err;
> +
> + synapse->action = action_index;
> +
> + action = synapse->actions_list[action_index];
> + return scnprintf(buf, PAGE_SIZE, "%s\n", synapse_action_str[action]);
> +}
> +
> +static ssize_t counter_action_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct action_comp_t *const component = devattr->component;
> + struct counter_synapse *const synapse = component->synapse;
> + size_t action_index;
> + const size_t num_actions = synapse->num_actions;
> + enum synapse_action action;
> + int err;
> + struct counter_device *const counter = dev_get_drvdata(dev);
> + struct counter_count *const count = component->count;
> +
> + /* Find requested action mode */
> + for (action_index = 0; action_index < num_actions; action_index++) {
> + action = synapse->actions_list[action_index];
> + if (sysfs_streq(buf, synapse_action_str[action]))
> + break;
> + }
> + /* If requested action mode not found */
> + if (action_index >= num_actions)
> + return -EINVAL;
> +
> + err = counter->ops->action_set(counter, count, synapse, action_index);
> + if (err)
> + return err;
> +
> + synapse->action = action_index;
> +
> + return len;
> +}
> +
> +struct action_avail_comp_t {
> + const enum synapse_action *actions_list;
> + size_t num_actions;
> +};
> +
> +static ssize_t counter_synapse_action_available_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct action_avail_comp_t *const component = devattr->component;
> + const enum synapse_action *const actions_list = component->actions_list;
I'm not sure this local variable helps much either...
> + const size_t num_actions = component->num_actions;
Local variable adds nothing as far as I can see.. Just use it inline.
> + size_t i;
> + enum synapse_action action;
> + ssize_t len = 0;
> +
> + for (i = 0; i < num_actions; i++) {
> + action = actions_list[i];
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
> + synapse_action_str[action]);
> + }
> +
> + return len;
> +}
> +
> +static int counter_synapses_register(
> + struct counter_device_attr_group *const group,
> + const struct counter_device *const counter,
> + struct counter_count *const count, const char *const count_attr_name)
> +{
> + const size_t num_synapses = count->num_synapses;
Local variable doesn't add anything. Only used once.
> + size_t i;
> + struct counter_synapse *synapse;
> + const char *prefix;
> + struct action_comp_t *action_comp;
> + int err;
> + struct action_avail_comp_t *avail_comp;
> +
> + /* Register each Synapse */
> + for (i = 0; i < num_synapses; i++) {
> + synapse = count->synapses + i;
> +
> + /* Generate attribute prefix */
> + prefix = kasprintf(GFP_KERNEL, "signal%d_",
> + synapse->signal->id);
> + if (!prefix) {
> + err = -ENOMEM;
> + goto err_free_attr_list;
> + }
> +
> + /* Allocate action attribute component */
> + action_comp = kmalloc(sizeof(*action_comp), GFP_KERNEL);
> + if (!action_comp) {
> + err = -ENOMEM;
> + goto err_free_prefix;
> + }
> + action_comp->synapse = synapse;
> + action_comp->count = count;
> +
> + /* Create action attribute */
> + err = counter_attribute_create(group, prefix, "action",
> + (counter->ops->action_get) ? counter_action_show : NULL,
> + (counter->ops->action_set) ? counter_action_store : NULL,
> + action_comp);
> + if (err) {
> + kfree(action_comp);
> + goto err_free_prefix;
> + }
> +
> + /* Allocate action available attribute component */
> + avail_comp = kmalloc(sizeof(*avail_comp), GFP_KERNEL);
> + if (!avail_comp) {
> + err = -ENOMEM;
> + goto err_free_prefix;
> + }
> + avail_comp->actions_list = synapse->actions_list;
> + avail_comp->num_actions = synapse->num_actions;
> +
> + /* Create action_available attribute */
> + err = counter_attribute_create(group, prefix,
> + "action_available",
> + counter_synapse_action_available_show, NULL,
> + avail_comp);
> + if (err) {
> + kfree(avail_comp);
> + goto err_free_prefix;
> + }
> +
> + kfree(prefix);
> + }
> +
> + return 0;
> +
> +err_free_prefix:
> + kfree(prefix);
> +err_free_attr_list:
> + free_counter_device_attr_list(&group->attr_list);
> + return err;
> +}
> +
> +struct count_comp_t {
> + struct counter_count *count;
> +};
> +
> +static ssize_t counter_count_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct counter_device *const counter = dev_get_drvdata(dev);
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct count_comp_t *const component = devattr->component;
> + struct counter_count *const count = component->count;
> + int err;
> + struct count_read_value val = { .buf = buf };
> +
> + err = counter->ops->count_read(counter, count, &val);
> + if (err)
> + return err;
> +
> + return val.len;
> +}
> +
> +static ssize_t counter_count_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct counter_device *const counter = dev_get_drvdata(dev);
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct count_comp_t *const component = devattr->component;
> + struct counter_count *const count = component->count;
> + int err;
> + struct count_write_value val = { .buf = buf };
> +
> + err = counter->ops->count_write(counter, count, &val);
> + if (err)
> + return err;
> +
> + return len;
> +}
> +
> +static const char *const count_function_str[] = {
> + [COUNT_FUNCTION_INCREASE] = "increase",
> + [COUNT_FUNCTION_DECREASE] = "decrease",
> + [COUNT_FUNCTION_PULSE_DIRECTION] = "pulse-direction",
> + [COUNT_FUNCTION_QUADRATURE_X1_A] = "quadrature x1 a",
> + [COUNT_FUNCTION_QUADRATURE_X1_B] = "quadrature x1 b",
> + [COUNT_FUNCTION_QUADRATURE_X2_A] = "quadrature x2 a",
> + [COUNT_FUNCTION_QUADRATURE_X2_B] = "quadrature x2 b",
> + [COUNT_FUNCTION_QUADRATURE_X2_RISING] = "quadrature x2 rising",
> + [COUNT_FUNCTION_QUADRATURE_X2_FALLING] = "quadrature x2 falling",
> + [COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4"
> +};
> +
> +static ssize_t counter_function_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int err;
> + struct counter_device *const counter = dev_get_drvdata(dev);
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct count_comp_t *const component = devattr->component;
> + struct counter_count *const count = component->count;
> + size_t func_index;
> + enum count_function function;
> +
> + err = counter->ops->function_get(counter, count, &func_index);
> + if (err)
> + return err;
> +
> + count->function = func_index;
> +
> + function = count->functions_list[func_index];
> + return scnprintf(buf, PAGE_SIZE, "%s\n", count_function_str[function]);
> +}
> +
> +static ssize_t counter_function_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct count_comp_t *const component = devattr->component;
> + struct counter_count *const count = component->count;
> + const size_t num_functions = count->num_functions;
> + size_t func_index;
> + enum count_function function;
> + int err;
> + struct counter_device *const counter = dev_get_drvdata(dev);
> +
> + /* Find requested Count function mode */
> + for (func_index = 0; func_index < num_functions; func_index++) {
> + function = count->functions_list[func_index];
> + if (sysfs_streq(buf, count_function_str[function]))
> + break;
> + }
> + /* Return error if requested Count function mode not found */
> + if (func_index >= num_functions)
> + return -EINVAL;
> +
> + err = counter->ops->function_set(counter, count, func_index);
> + if (err)
> + return err;
> +
> + count->function = func_index;
> +
> + return len;
> +}
> +
> +struct count_ext_comp_t {
> + struct counter_count *count;
> + const struct counter_count_ext *ext;
> +};
> +
> +static ssize_t counter_count_ext_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct count_ext_comp_t *const comp = devattr->component;
> + const struct counter_count_ext *const ext = comp->ext;
> +
> + return ext->read(dev_get_drvdata(dev), comp->count, ext->priv, buf);
> +}
> +
> +static ssize_t counter_count_ext_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct count_ext_comp_t *const comp = devattr->component;
> + const struct counter_count_ext *const ext = comp->ext;
> +
> + return ext->write(dev_get_drvdata(dev), comp->count, ext->priv, buf,
> + len);
> +}
> +
> +static int counter_count_ext_register(
> + struct counter_device_attr_group *const group,
> + struct counter_count *const count)
> +{
> + const size_t num_ext = count->num_ext;
Used in one place, just put it inline?
> + size_t i;
> + const struct counter_count_ext *ext;
> + struct count_ext_comp_t *count_ext_comp;
> + int err;
> +
> + /* Create an attribute for each extension */
> + for (i = 0 ; i < num_ext; i++) {
> + ext = count->ext + i;
> +
> + /* Allocate count_ext attribute component */
> + count_ext_comp = kmalloc(sizeof(*count_ext_comp), GFP_KERNEL);
> + if (!count_ext_comp) {
> + err = -ENOMEM;
> + goto err_free_attr_list;
> + }
> + count_ext_comp->count = count;
> + count_ext_comp->ext = ext;
> +
> + /* Allocate count_ext attribute */
> + err = counter_attribute_create(group, "", ext->name,
> + (ext->read) ? counter_count_ext_show : NULL,
> + (ext->write) ? counter_count_ext_store : NULL,
> + count_ext_comp);
> + if (err) {
> + kfree(count_ext_comp);
> + goto err_free_attr_list;
> + }
> + }
> +
> + return 0;
> +
> +err_free_attr_list:
> + free_counter_device_attr_list(&group->attr_list);
> + return err;
> +}
> +
> +struct func_avail_comp_t {
> + const enum count_function *functions_list;
> + size_t num_functions;
> +};
> +
> +static ssize_t counter_count_function_available_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct func_avail_comp_t *const component = devattr->component;
> + const enum count_function *const func_list = component->functions_list;
> + const size_t num_functions = component->num_functions;
> + size_t i;
> + enum count_function function;
> + ssize_t len = 0;
> +
> + for (i = 0; i < num_functions; i++) {
> + function = func_list[i];
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s\n",
> + count_function_str[function]);
> + }
> +
> + return len;
> +}
> +
> +static int counter_count_attributes_create(
> + struct counter_device_attr_group *const group,
> + const struct counter_device *const counter,
> + struct counter_count *const count)
> +{
> + struct count_comp_t *count_comp;
> + int err;
> + struct count_comp_t *func_comp;
> + struct func_avail_comp_t *avail_comp;
> +
> + /* Allocate count attribute component */
> + count_comp = kmalloc(sizeof(*count_comp), GFP_KERNEL);
> + if (!count_comp)
> + return -ENOMEM;
> + count_comp->count = count;
> +
> + /* Create main Count attribute */
> + err = counter_attribute_create(group, "", "count",
> + (counter->ops->count_read) ? counter_count_show : NULL,
> + (counter->ops->count_write) ? counter_count_store : NULL,
> + count_comp);
> + if (err) {
> + kfree(count_comp);
> + return err;
> + }
> +
> + /* Allocate function attribute component */
> + func_comp = kmalloc(sizeof(*func_comp), GFP_KERNEL);
> + if (!func_comp) {
> + err = -ENOMEM;
> + goto err_free_attr_list;
> + }
> + func_comp->count = count;
> +
> + /* Create Count function attribute */
> + err = counter_attribute_create(group, "", "function",
> + (counter->ops->function_get) ? counter_function_show : NULL,
> + (counter->ops->function_set) ? counter_function_store : NULL,
> + func_comp);
> + if (err) {
> + kfree(func_comp);
> + goto err_free_attr_list;
> + }
> +
> + /* Allocate function available attribute component */
> + avail_comp = kmalloc(sizeof(*avail_comp), GFP_KERNEL);
> + if (!avail_comp) {
> + err = -ENOMEM;
> + goto err_free_attr_list;
> + }
> + avail_comp->functions_list = count->functions_list;
> + avail_comp->num_functions = count->num_functions;
> +
> + /* Create Count function_available attribute */
> + err = counter_attribute_create(group, "", "function_available",
> + counter_count_function_available_show,
> + NULL, avail_comp);
> + if (err) {
> + kfree(avail_comp);
> + goto err_free_attr_list;
> + }
> +
> + /* Create Count name attribute */
> + err = counter_name_attribute_create(group, count->name);
> + if (err)
> + goto err_free_attr_list;
> +
> + /* Register Count extension attributes */
> + err = counter_count_ext_register(group, count);
> + if (err)
> + goto err_free_attr_list;
> +
> + return 0;
> +
> +err_free_attr_list:
> + free_counter_device_attr_list(&group->attr_list);
> + return err;
> +}
> +
> +static int counter_counts_register(
> + struct counter_device_attr_group *const groups_list,
> + const struct counter_device *const counter)
> +{
> + const size_t num_counts = counter->num_counts;
I think this is only used on one place. Not sure having
a local variable is worthwhile.
> + size_t i;
> + struct counter_count *count;
> + const char *name;
> + int err;
> +
> + /* Register each Count */
> + for (i = 0; i < num_counts; i++) {
> + count = counter->counts + i;
> +
> + /* Generate Count attribute directory name */
> + name = kasprintf(GFP_KERNEL, "count%d", count->id);
> + if (!name) {
> + err = -ENOMEM;
> + goto err_free_attr_groups;
> + }
> + groups_list[i].attr_group.name = name;
> +
> + /* Register the Synapses associated with each Count */
> + err = counter_synapses_register(groups_list + i, counter, count,
> + name);
> + if (err)
> + goto err_free_attr_groups;
> +
> + /* Create all attributes associated with Count */
> + err = counter_count_attributes_create(groups_list + i, counter,
> + count);
> + if (err)
> + goto err_free_attr_groups;
> + }
> +
> + return 0;
> +
> +err_free_attr_groups:
> + do {
> + kfree(groups_list[i].attr_group.name);
> + free_counter_device_attr_list(&groups_list[i].attr_list);
> + } while (i--);
> + return err;
> +}
> +
> +struct size_comp_t {
> + size_t size;
> +};
> +
> +static ssize_t counter_device_attr_size_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + const struct size_comp_t *const comp = to_counter_attr(attr)->component;
> +
> + return scnprintf(buf, PAGE_SIZE, "%zu\n", comp->size);
> +}
> +
> +static int counter_size_attribute_create(
> + struct counter_device_attr_group *const group,
> + const size_t size, const char *const name)
> +{
> + struct size_comp_t *size_comp;
> + int err;
> +
> + /* Allocate size attribute component */
> + size_comp = kmalloc(sizeof(*size_comp), GFP_KERNEL);
> + if (!size_comp)
> + return -ENOMEM;
> + size_comp->size = size;
> +
> + err = counter_attribute_create(group, "", name,
> + counter_device_attr_size_show, NULL,
> + size_comp);
> + if (err)
> + goto err_free_size_comp;
> +
> + return 0;
> +
> +err_free_size_comp:
> + kfree(size_comp);
> + return err;
> +}
> +
> +struct ext_comp_t {
> + const struct counter_device_ext *ext;
> +};
> +
> +static ssize_t counter_device_ext_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct ext_comp_t *const component = devattr->component;
> + const struct counter_device_ext *const ext = component->ext;
> +
> + return ext->read(dev_get_drvdata(dev), ext->priv, buf);
> +}
> +
> +static ssize_t counter_device_ext_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + const struct counter_device_attr *const devattr = to_counter_attr(attr);
> + const struct ext_comp_t *const component = devattr->component;
> + const struct counter_device_ext *const ext = component->ext;
> +
> + return ext->write(dev_get_drvdata(dev), ext->priv, buf, len);
> +}
> +
> +static int counter_device_ext_register(
> + struct counter_device_attr_group *const group,
> + struct counter_device *const counter)
> +{
> + const size_t num_ext = counter->num_ext;
num_ext only used in one place so if anything the local variable
is hurting readability.
> + struct ext_comp_t *ext_comp;
> + size_t i;
> + const struct counter_device_ext *ext;
> + int err;
> +
> + /* Create an attribute for each extension */
> + for (i = 0 ; i < num_ext; i++) {
> + ext = counter->ext + i;
This local variable isn't gaining us much that I can see. Just
use the value directly.
> +
> + /* Allocate extension attribute component */
> + ext_comp = kmalloc(sizeof(*ext_comp), GFP_KERNEL);
> + if (!ext_comp) {
> + err = -ENOMEM;
> + goto err_free_attr_list;
> + }
> +
> + ext_comp->ext = ext;
> +
> + /* Allocate extension attribute */
> + err = counter_attribute_create(group, "", ext->name,
> + (ext->read) ? counter_device_ext_show : NULL,
> + (ext->write) ? counter_device_ext_store : NULL,
> + ext_comp);
> + if (err) {
> + kfree(ext_comp);
> + goto err_free_attr_list;
> + }
> + }
> +
> + return 0;
> +
> +err_free_attr_list:
> + free_counter_device_attr_list(&group->attr_list);
> + return err;
> +}
> +
> +static int counter_global_attr_register(
> + struct counter_device_attr_group *const group,
> + struct counter_device *const counter)
> +{
> + int err;
> +
> + /* Create name attribute */
> + err = counter_name_attribute_create(group, counter->name);
> + if (err)
> + return err;
> +
> + /* Create num_counts attribute */
> + err = counter_size_attribute_create(group, counter->num_counts,
> + "num_counts");
> + if (err)
> + goto err_free_attr_list;
> +
> + /* Create num_signals attribute */
> + err = counter_size_attribute_create(group, counter->num_signals,
> + "num_signals");
> + if (err)
> + goto err_free_attr_list;
> +
> + /* Register Counter device extension attributes */
> + err = counter_device_ext_register(group, counter);
> + if (err)
> + goto err_free_attr_list;
> +
> + return 0;
> +
> +err_free_attr_list:
> + free_counter_device_attr_list(&group->attr_list);
> + return err;
> +}
> +
> +static void free_counter_device_groups_list(
> + struct counter_device_attr_group *const groups_list,
> + const size_t num_groups)
> +{
> + struct counter_device_attr_group *group;
> + size_t i;
> +
> + for (i = 0; i < num_groups; i++) {
> + group = groups_list + i;
> +
I'd like to see a comment somewhere here on the fact this cleans up both
the registered per counter stuff and the global attributes (that took
a bit of chasing to check it did...)
> + kfree(group->attr_group.name);
> + kfree(group->attr_group.attrs);
> + free_counter_device_attr_list(&group->attr_list);
> + }
> +
> + kfree(groups_list);
> +}
> +
> +static int prepare_counter_device_groups_list(
> + struct counter_device *const counter)
> +{
> + const size_t total_num_groups =
> + counter->num_signals + counter->num_counts + 1;
> + struct counter_device_attr_group *groups_list;
> + size_t i;
> + int err;
> + size_t num_groups = 0;
> +
> + /* Allocate space for attribute groups (signals. counts, and ext) */
> + groups_list = kcalloc(total_num_groups, sizeof(*groups_list),
> + GFP_KERNEL);
> + if (!groups_list)
> + return -ENOMEM;
> +
> + /* Initialize attribute lists */
> + for (i = 0; i < total_num_groups; i++)
> + INIT_LIST_HEAD(&groups_list[i].attr_list);
> +
> + /* Register Signals */
> + err = counter_signals_register(groups_list, counter);
> + if (err)
> + goto err_free_groups_list;
> + num_groups += counter->num_signals;
> +
> + /* Register Counts and respective Synapses */
> + err = counter_counts_register(groups_list + num_groups, counter);
> + if (err)
> + goto err_free_groups_list;
> + num_groups += counter->num_counts;
> +
> + /* Register Counter global attributes */
> + err = counter_global_attr_register(groups_list + num_groups, counter);
> + if (err)
> + goto err_free_groups_list;
> + num_groups++;
> +
> + /* Store groups_list in device_state */
> + counter->device_state->groups_list = groups_list;
> + counter->device_state->num_groups = num_groups;
> +
> + return 0;
> +
> +err_free_groups_list:
> + free_counter_device_groups_list(groups_list, num_groups);
Consistent naming would be good.
counter_device_groups_list_free.
I would tidy this up throughout. I know from experience that
you'll probably end up doing so eventually and it is easier whilst
there isn't too much code.
> + return err;
> +}
> +
> +static int prepare_counter_device_groups(
> + struct counter_device_state *const device_state)
> +{
> + size_t i;
> + struct counter_device_attr_group *group;
> + int err;
> + size_t j;
Tidy this up a little,
size_t i, j;
> + struct counter_device_attr *p;
> +
> + /* Allocate attribute groups for association with device */
> + device_state->groups = kcalloc(device_state->num_groups + 1,
> + sizeof(*device_state->groups),
> + GFP_KERNEL);
> + if (!device_state->groups)
> + return -ENOMEM;
> +
> + /* Prepare each group of attributes for association */
> + for (i = 0; i < device_state->num_groups; i++) {
> + group = device_state->groups_list + i;
> +
> + /* Allocate space for attribute pointers in attribute group */
> + group->attr_group.attrs = kcalloc(group->num_attr + 1,
> + sizeof(*group->attr_group.attrs), GFP_KERNEL);
> + if (!group->attr_group.attrs) {
> + err = -ENOMEM;
> + goto err_free_groups;
> + }
> +
> + /* Add attribute pointers to attribute group */
> + j = 0;
> + list_for_each_entry(p, &group->attr_list, l)
> + group->attr_group.attrs[j++] = &p->dev_attr.attr;
> +
> + /* Group attributes in attribute group */
> + device_state->groups[i] = &group->attr_group;
> + }
> + /* Associate attributes with device */
> + device_state->dev.groups = device_state->groups;
> +
> + return 0;
> +
> +err_free_groups:
I'm not convinced this is cleaning up properly. What about
the kcalloc of group->attr_group.attrs for previous loops?
> + kfree(device_state->groups);
> + return err;
> +}
> +
> +/* Provides a unique ID for each counter device */
> +static DEFINE_IDA(counter_ida);
> +
> +static void counter_device_release(struct device *dev)
> +{
> + struct counter_device *const counter = dev_get_drvdata(dev);
> + struct counter_device_state *const device_state = counter->device_state;
> +
> + kfree(device_state->groups);
> + free_counter_device_groups_list(device_state->groups_list,
> + device_state->num_groups);
> + ida_simple_remove(&counter_ida, device_state->id);
> + kfree(device_state);
> +}
> +
> +static struct device_type counter_device_type = {
> + .name = "counter_device",
> + .release = counter_device_release
> +};
> +
> +static struct bus_type counter_bus_type = {
> + .name = "counter"
> +};
> +
> +/**
> + * counter_register - register Counter to the system
> + * @counter: pointer to Counter to register
> + *
> + * This function registers a Counter to the system. A sysfs "counter" directory
> + * will be created and populated with sysfs attributes correlating with the
> + * Counter Signals, Synapses, and Counts respectively.
> + */
> +int counter_register(struct counter_device *const counter)
> +{
> + struct counter_device_state *device_state;
> + int err;
> +
> + /* Allocate internal state container for Counter device */
> + device_state = kzalloc(sizeof(*device_state), GFP_KERNEL);
> + if (!device_state)
> + return -ENOMEM;
> + counter->device_state = device_state;
> +
> + /* Acquire unique ID */
> + device_state->id = ida_simple_get(&counter_ida, 0, 0, GFP_KERNEL);
> + if (device_state->id < 0) {
> + err = device_state->id;
> + goto err_free_device_state;
> + }
> +
> + /* Configure device structure for Counter */
> + device_state->dev.type = &counter_device_type;
> + device_state->dev.bus = &counter_bus_type;
> + if (counter->parent) {
> + device_state->dev.parent = counter->parent;
> + device_state->dev.of_node = counter->parent->of_node;
> + }
> + dev_set_name(&device_state->dev, "counter%d", device_state->id);
> + device_initialize(&device_state->dev);
> + dev_set_drvdata(&device_state->dev, counter);
> +
> + /* Prepare device attributes */
> + err = prepare_counter_device_groups_list(counter);
> + if (err)
> + goto err_free_id;
> +
> + /* Organize device attributes to groups and match to device */
> + err = prepare_counter_device_groups(device_state);
> + if (err)
> + goto err_free_groups_list;
> +
> + /* Add device to system */
> + err = device_add(&device_state->dev);
> + if (err)
> + goto err_free_groups;
> +
> + return 0;
> +
> +err_free_groups:
> + kfree(device_state->groups);
> +err_free_groups_list:
> + free_counter_device_groups_list(device_state->groups_list,
> + device_state->num_groups);
> +err_free_id:
> + ida_simple_remove(&counter_ida, device_state->id);
> +err_free_device_state:
> + kfree(device_state);
> + return err;
> +}
> +EXPORT_SYMBOL(counter_register);
> +
> +/**
> + * counter_unregister - unregister Counter from the system
> + * @counter: pointer to Counter to unregister
> + *
> + * The Counter is unregistered from the system; all allocated memory is freed.
> + */
> +void counter_unregister(struct counter_device *const counter)
> +{
> + if (counter)
> + device_del(&counter->device_state->dev);
> +}
> +EXPORT_SYMBOL(counter_unregister);
> +
> +static void devm_counter_unreg(struct device *dev, void *res)
> +{
> + counter_unregister(*(struct counter_device **)res);
> +}
> +
> +/**
> + * devm_counter_register - Resource-managed counter_register
> + * @dev: device to allocate counter_device for
> + * @counter: pointer to Counter to register
> + *
> + * Managed counter_register. The Counter registered with this function is
> + * automatically unregistered on driver detach. This function calls
> + * counter_register internally. Refer to that function for more information.
> + *
> + * If an Counter registered with this function needs to be unregistered
> + * separately, devm_counter_unregister must be used.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int devm_counter_register(struct device *dev,
> + struct counter_device *const counter)
> +{
> + struct counter_device **ptr;
> + int ret;
> +
> + ptr = devres_alloc(devm_counter_unreg, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + ret = counter_register(counter);
> + if (!ret) {
> + *ptr = counter;
> + devres_add(dev, ptr);
> + } else {
> + devres_free(ptr);
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(devm_counter_register);
> +
> +static int devm_counter_match(struct device *dev, void *res, void *data)
> +{
> + struct counter_device **r = res;
> +
> + if (!r || !*r) {
> + WARN_ON(!r || !*r);
> + return 0;
> + }
> +
> + return *r == data;
> +}
> +
> +/**
> + * devm_counter_unregister - Resource-managed counter_unregister
> + * @dev: device this counter_device belongs to
> + * @counter: pointer to Counter associated with the device
> + *
> + * Unregister Counter registered with devm_counter_register.
> + */
> +void devm_counter_unregister(struct device *dev,
> + struct counter_device *const counter)
> +{
> + int rc;
> +
> + rc = devres_release(dev, devm_counter_unreg, devm_counter_match,
> + counter);
> + WARN_ON(rc);
> +}
> +EXPORT_SYMBOL(devm_counter_unregister);
> +
> +static int __init counter_init(void)
> +{
> + return bus_register(&counter_bus_type);
> +}
> +
> +static void __exit counter_exit(void)
> +{
> + bus_unregister(&counter_bus_type);
> +}
> +
> +subsys_initcall(counter_init);
> +module_exit(counter_exit);
> +
> +MODULE_AUTHOR("William Breathitt Gray <vilhelm.gray@gmail.com>");
> +MODULE_DESCRIPTION("Generic Counter interface");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> new file mode 100644
> index 000000000000..a0b0349d098a
> --- /dev/null
> +++ b/include/linux/counter.h
> @@ -0,0 +1,554 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Counter interface
> + * Copyright (C) 2017 William Breathitt Gray
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
It's a bit controversial, but most people consider SPDX headers equivalent
of the license statement. As such you normally don't have both and just
go for the SPDK header.
I thought we were also standardising on
// SPDX-...
> + */
> +#ifndef _COUNTER_H_
> +#define _COUNTER_H_
> +
> +#include <linux/device.h>
> +#include <linux/types.h>
> +
> +enum count_direction {
> + COUNT_DIRECTION_FORWARD = 0,
> + COUNT_DIRECTION_BACKWARD
> +};
> +extern const char *const count_direction_str[2];
> +
> +enum count_mode {
> + COUNT_MODE_NORMAL = 0,
> + COUNT_MODE_RANGE_LIMIT,
> + COUNT_MODE_NON_RECYCLE,
> + COUNT_MODE_MODULO_N
> +};
> +extern const char *const count_mode_str[4];
> +
> +struct counter_device;
> +struct counter_signal;
> +
> +/**
> + * struct counter_signal_ext - Counter Signal extensions
> + * @name: attribute name
> + * @read: read callback for this attribute; may be NULL
> + * @write: write callback for this attribute; may be NULL
> + * @priv: data private to the driver
> + */
> +struct counter_signal_ext {
> + const char *name;
> + ssize_t (*read)(struct counter_device *counter,
> + struct counter_signal *signal, void *priv,
> + char *buf);
> + ssize_t (*write)(struct counter_device *counter,
> + struct counter_signal *signal, void *priv,
> + const char *buf, size_t len);
> + void *priv;
> +};
> +
> +/**
> + * struct counter_signal - Counter Signal node
> + * @id: unique ID used to identify signal
> + * @name: device-specific Signal name; ideally, this should match the name
> + * as it appears in the datasheet documentation
> + * @ext: optional array of Counter Signal extensions
> + * @num_ext: number of Counter Signal extensions specified in @ext
> + * @priv: optional private data supplied by driver
> + */
> +struct counter_signal {
> + int id;
> + const char *name;
> +
> + const struct counter_signal_ext *ext;
> + size_t num_ext;
> +
> + void *priv;
> +};
> +
> +/**
> + * struct counter_signal_enum_ext - Signal enum extension attribute
> + * @items: Array of strings
> + * @num_items: Number of items specified in @items
> + * @set: Set callback function; may be NULL
> + * @get: Get callback function; may be NULL
> + *
> + * The counter_signal_enum_ext structure can be used to implement enum style
> + * Signal extension attributes. Enum style attributes are those which have a set
> + * of strings that map to unsigned integer values. The Generic Counter Signal
> + * enum extension helper code takes care of mapping between value and string, as
> + * well as generating a "_available" file which contains a list of all available
> + * items. The get callback is used to query the currently active item; the index
> + * of the item within the respective items array is returned via the 'item'
> + * parameter. The set callback is called when the attribute is updated; the
> + * 'item' parameter contains the index of the newly activated item within the
> + * respective items array.
> + */
> +struct counter_signal_enum_ext {
> + const char * const *items;
> + size_t num_items;
> + int (*get)(struct counter_device *counter,
> + struct counter_signal *signal,
> + size_t *item);
> + int (*set)(struct counter_device *counter,
> + struct counter_signal *signal,
> + size_t item);
> +};
> +
> +ssize_t counter_signal_enum_read(struct counter_device *counter,
> + struct counter_signal *signal, void *priv,
> + char *buf);
> +ssize_t counter_signal_enum_write(struct counter_device *counter,
> + struct counter_signal *signal, void *priv,
> + const char *buf, size_t len);
> +
> +/**
> + * COUNTER_SIGNAL_ENUM() - Initialize Signal enum extension
> + * @_name: Attribute name
> + * @_e: Pointer to a counter_count_enum structure
> + *
> + * This should usually be used together with COUNTER_SIGNAL_ENUM_AVAILABLE()
> + */
> +#define COUNTER_SIGNAL_ENUM(_name, _e) \
> +{ \
> + .name = (_name), \
> + .read = counter_signal_enum_read, \
> + .write = counter_signal_enum_write, \
> + .priv = (_e) \
> +}
> +
> +ssize_t counter_signal_enum_available_read(struct counter_device *counter,
> + struct counter_signal *signal,
> + void *priv, char *buf);
> +
> +/**
> + * COUNTER_SIGNAL_ENUM_AVAILABLE() - Initialize Signal enum available extension
> + * @_name: Attribute name ("_available" will be appended to the name)
> + * @_e: Pointer to a counter_signal_enum structure
> + *
> + * Creates a read only attribute that lists all the available enum items in a
> + * newline separated list. This should usually be used together with
> + * COUNTER_SIGNAL_ENUM()
> + */
> +#define COUNTER_SIGNAL_ENUM_AVAILABLE(_name, _e) \
> +{ \
> + .name = (_name "_available"), \
> + .read = counter_signal_enum_available_read, \
> + .priv = (_e) \
> +}
> +
> +enum synapse_action {
> + SYNAPSE_ACTION_NONE = 0,
> + SYNAPSE_ACTION_RISING_EDGE,
> + SYNAPSE_ACTION_FALLING_EDGE,
> + SYNAPSE_ACTION_BOTH_EDGES
> +};
> +
> +/**
> + * struct counter_synapse - Counter Synapse node
> + * @action: index of current action mode
> + * @actions_list: array of available action modes
> + * @num_actions: number of action modes specified in @actions_list
> + * @signal: pointer to associated signal
> + */
> +struct counter_synapse {
> + size_t action;
> + const enum synapse_action *actions_list;
> + size_t num_actions;
> +
> + struct counter_signal *signal;
> +};
> +
> +struct counter_count;
> +
> +/**
> + * struct counter_count_ext - Counter Count extension
> + * @name: attribute name
> + * @read: read callback for this attribute; may be NULL
> + * @write: write callback for this attribute; may be NULL
> + * @priv: data private to the driver
> + */
> +struct counter_count_ext {
> + const char *name;
> + ssize_t (*read)(struct counter_device *counter,
> + struct counter_count *count, void *priv,
> + char *buf);
> + ssize_t (*write)(struct counter_device *counter,
> + struct counter_count *count, void *priv,
> + const char *buf, size_t len);
> + void *priv;
> +};
> +
> +enum count_function {
> + COUNT_FUNCTION_INCREASE = 0,
> + COUNT_FUNCTION_DECREASE,
> + COUNT_FUNCTION_PULSE_DIRECTION,
> + COUNT_FUNCTION_QUADRATURE_X1_A,
> + COUNT_FUNCTION_QUADRATURE_X1_B,
> + COUNT_FUNCTION_QUADRATURE_X2_A,
> + COUNT_FUNCTION_QUADRATURE_X2_B,
> + COUNT_FUNCTION_QUADRATURE_X2_RISING,
> + COUNT_FUNCTION_QUADRATURE_X2_FALLING,
> + COUNT_FUNCTION_QUADRATURE_X4
> +};
> +
> +/**
> + * struct counter_count - Counter Count node
> + * @id: unique ID used to identify Count
> + * @name: device-specific Count name; ideally, this should match
> + * the name as it appears in the datasheet documentation
> + * @function: index of current function mode
> + * @functions_list: array available function modes
> + * @num_functions: number of function modes specified in @functions_list
> + * @synapses: array of synapses for initialization
> + * @num_synapses: number of synapses specified in @synapses
> + * @ext: optional array of Counter Count extensions
> + * @num_ext: number of Counter Count extensions specified in @ext
> + * @priv: optional private data supplied by driver
> + */
> +struct counter_count {
> + int id;
> + const char *name;
> +
> + size_t function;
> + const enum count_function *functions_list;
> + size_t num_functions;
There is a very good illustration here of the issues with using
tabs to pretty print structure elements. I would drop them entirely as
personally I'm not sure they help readability and you will forever be having
more noise in patches because of the need to change the number of tabs
due to name changes etc.
> +
> + struct counter_synapse *synapses;
> + size_t num_synapses;
> +
> + const struct counter_count_ext *ext;
> + size_t num_ext;
> +
> + void *priv;
> +};
> +
> +/**
> + * struct counter_count_enum_ext - Count enum extension attribute
> + * @items: Array of strings
> + * @num_items: Number of items specified in @items
> + * @set: Set callback function; may be NULL
> + * @get: Get callback function; may be NULL
> + *
> + * The counter_count_enum_ext structure can be used to implement enum style
> + * Count extension attributes. Enum style attributes are those which have a set
> + * of strings that map to unsigned integer values. The Generic Counter Count
> + * enum extension helper code takes care of mapping between value and string, as
> + * well as generating a "_available" file which contains a list of all available
> + * items. The get callback is used to query the currently active item; the index
> + * of the item within the respective items array is returned via the 'item'
> + * parameter. The set callback is called when the attribute is updated; the
> + * 'item' parameter contains the index of the newly activated item within the
> + * respective items array.
> + */
> +struct counter_count_enum_ext {
> + const char * const *items;
> + size_t num_items;
> + int (*get)(struct counter_device *counter,
> + struct counter_count *count,
> + size_t *item);
> + int (*set)(struct counter_device *counter,
> + struct counter_count *count,
> + size_t item);
> +};
> +
> +ssize_t counter_count_enum_read(struct counter_device *counter,
> + struct counter_count *count, void *priv,
> + char *buf);
> +ssize_t counter_count_enum_write(struct counter_device *counter,
> + struct counter_count *count, void *priv,
> + const char *buf, size_t len);
> +
> +/**
> + * COUNTER_COUNT_ENUM() - Initialize Count enum extension
> + * @_name: Attribute name
> + * @_e: Pointer to a counter_count_enum structure
> + *
> + * This should usually be used together with COUNTER_COUNT_ENUM_AVAILABLE()
> + */
> +#define COUNTER_COUNT_ENUM(_name, _e) \
> +{ \
> + .name = (_name), \
> + .read = counter_count_enum_read, \
> + .write = counter_count_enum_write, \
> + .priv = (_e) \
> +}
> +
> +ssize_t counter_count_enum_available_read(struct counter_device *counter,
> + struct counter_count *count,
> + void *priv, char *buf);
> +
> +/**
> + * COUNTER_COUNT_ENUM_AVAILABLE() - Initialize Count enum available extension
> + * @_name: Attribute name ("_available" will be appended to the name)
> + * @_e: Pointer to a counter_count_enum structure
> + *
> + * Creates a read only attribute that lists all the available enum items in a
> + * newline separated list. This should usually be used together with
> + * COUNTER_COUNT_ENUM()
> + */
> +#define COUNTER_COUNT_ENUM_AVAILABLE(_name, _e) \
> +{ \
> + .name = (_name "_available"), \
> + .read = counter_count_enum_available_read, \
> + .priv = (_e) \
> +}
> +
> +/**
> + * struct counter_device_attr_group - internal container for attribute group
> + * @attr_group: Counter sysfs attributes group
> + * @attr_list: list to keep track of created Counter sysfs attributes
> + * @num_attr: number of Counter sysfs attributes
> + */
> +struct counter_device_attr_group {
> + struct attribute_group attr_group;
> + struct list_head attr_list;
> + size_t num_attr;
> +};
> +
> +/**
> + * struct counter_device_state - internal state container for a Counter device
> + * @id: unique ID used to identify the Counter
> + * @dev: internal device structure
> + * @groups_list attribute groups list (groups for Signals, Counts, and ext)
Run kernel-doc script over these files. You are missing some :s and it would
have told you that.
> + * @num_groups number of attribute groups containers
> + * @groups: Counter sysfs attribute groups (used to populate @dev.groups)
> + */
> +struct counter_device_state {
> + int id;
> + struct device dev;
> + struct counter_device_attr_group *groups_list;
> + size_t num_groups;
> + const struct attribute_group **groups;
> +};
> +
> +/**
> + * struct signal_read_value - Opaque Signal read value
> + * @buf: string representation of Signal read value
> + * @len: length of string in @buf
> + */
> +struct signal_read_value {
> + char *buf;
> + size_t len;
> +};
> +
> +/**
> + * struct count_read_value - Opaque Count read value
> + * @buf: string representation of Count read value
> + * @len: length of string in @buf
> + */
> +struct count_read_value {
> + char *buf;
> + size_t len;
> +};
> +
> +/**
> + * struct count_write_value - Opaque Count write value
> + * @buf: string representation of Count write value
> + */
> +struct count_write_value {
> + const char *buf;
> +};
> +
> +/**
> + * struct counter_ops - Callbacks from driver
> + * @signal_read: optional read callback for Signal attribute. The read
> + * value of the respective Signal should be passed back via
> + * the val parameter. val points to an opaque type which
> + * should be set only via the set_signal_read_value
> + * function.
This last part had me a little confused. I would make it clear that this
set_signal_read_value function should be called to set the value within this
signal_read callback rather than elsewhere...
> + * @count_read: optional read callback for Count attribute. The read
> + * value of the respective Count should be passed back via
> + * the val parameter. val points to an opaque type which
> + * should be set only via the set_count_read_value
> + * function.
> + * @count_write: optional write callback for Count attribute. The write
> + * value for the respective Count is passed in via the val
> + * parameter. val points to an opaque type which should be
> + * access only via the get_count_write_value function.
> + * @function_get: function to get the current count function mode. Returns
> + * 0 on success and negative error code on error. The index
> + * of the respective Count's returned function mode should
> + * be passed back via the function parameter.
> + * @function_set: function to set the count function mode. function is the
> + * index of the requested function mode from the respective
> + * Count's functions_list array.
> + * @action_get: function to get the current action mode. Returns 0 on
> + * success and negative error code on error. The index of
> + * the respective Signal's returned action mode should be
> + * passed back via the action parameter.
> + * @action_set: function to set the action mode. action is the index of
> + * the requested action mode from the respective Synapse's
> + * actions_list array.
> + */
> +struct counter_ops {
> + int (*signal_read)(struct counter_device *counter,
> + struct counter_signal *signal,
> + struct signal_read_value *val);
> + int (*count_read)(struct counter_device *counter,
> + struct counter_count *count,
> + struct count_read_value *val);
> + int (*count_write)(struct counter_device *counter,
> + struct counter_count *count,
> + struct count_write_value *val);
> + int (*function_get)(struct counter_device *counter,
> + struct counter_count *count, size_t *function);
> + int (*function_set)(struct counter_device *counter,
> + struct counter_count *count, size_t function);
> + int (*action_get)(struct counter_device *counter,
> + struct counter_count *count,
> + struct counter_synapse *synapse, size_t *action);
> + int (*action_set)(struct counter_device *counter,
> + struct counter_count *count,
> + struct counter_synapse *synapse, size_t action);
> +};
> +
> +/**
> + * struct counter_device_ext - Counter device extension
> + * @name: attribute name
> + * @read: read callback for this attribute; may be NULL
> + * @write: write callback for this attribute; may be NULL
> + * @priv: data private to the driver
> + */
> +struct counter_device_ext {
> + const char *name;
> + ssize_t (*read)(struct counter_device *counter, void *priv,
> + char *buf);
> + ssize_t (*write)(struct counter_device *counter, void *priv,
> + const char *buf, size_t len);
> + void *priv;
> +};
> +
> +/**
> + * struct counter_device_enum_ext - Counter enum extension attribute
> + * @items: Array of strings
> + * @num_items: Number of items specified in @items
> + * @set: Set callback function; may be NULL
> + * @get: Get callback function; may be NULL
> + *
> + * The counter_device_enum_ext structure can be used to implement enum style
> + * Counter extension attributes. Enum style attributes are those which have a
> + * set of strings that map to unsigned integer values. The Generic Counter enum
> + * extension helper code takes care of mapping between value and string, as well
> + * as generating a "_available" file which contains a list of all available
> + * items. The get callback is used to query the currently active item; the index
> + * of the item within the respective items array is returned via the 'item'
> + * parameter. The set callback is called when the attribute is updated; the
> + * 'item' parameter contains the index of the newly activated item within the
> + * respective items array.
> + */
> +struct counter_device_enum_ext {
> + const char * const *items;
> + size_t num_items;
> + int (*get)(struct counter_device *counter,
> + size_t *item);
> + int (*set)(struct counter_device *counter,
> + size_t item);
> +};
> +
> +ssize_t counter_device_enum_read(struct counter_device *counter, void *priv,
> + char *buf);
> +ssize_t counter_device_enum_write(struct counter_device *counter, void *priv,
> + const char *buf, size_t len);
> +
> +/**
> + * COUNTER_DEVICE_ENUM() - Initialize Counter enum extension
> + * @_name: Attribute name
> + * @_e: Pointer to a counter_device_enum structure
> + *
> + * This should usually be used together with COUNTER_DEVICE_ENUM_AVAILABLE()
> + */
> +#define COUNTER_DEVICE_ENUM(_name, _e) \
> +{ \
> + .name = (_name), \
> + .read = counter_device_enum_read, \
> + .write = counter_device_enum_write, \
> + .priv = (_e) \
> +}
> +
> +ssize_t counter_device_enum_available_read(struct counter_device *counter,
> + void *priv, char *buf);
> +
> +/**
> + * COUNTER_DEVICE_ENUM_AVAILABLE() - Initialize Counter enum available extension
> + * @_name: Attribute name ("_available" will be appended to the name)
> + * @_e: Pointer to a counter_device_enum structure
> + *
> + * Creates a read only attribute that lists all the available enum items in a
> + * newline separated list. This should usually be used together with
> + * COUNTER_DEVICE_ENUM()
> + */
> +#define COUNTER_DEVICE_ENUM_AVAILABLE(_name, _e) \
> +{ \
> + .name = (_name "_available"), \
> + .read = counter_device_enum_available_read, \
> + .priv = (_e) \
> +}
> +
> +/**
> + * struct counter_device - Counter data structure
> + * @name: name of the device as it appears in the datasheet
> + * @parent: optional parent device providing the counters
> + * @device_state: internal device state container
> + * @ops: callbacks from driver
> + * @signals: array of Signals
> + * @num_signals: number of Signals specified in @signals
> + * @counts: array of Counts
> + * @num_counts: number of Counts specified in @counts
> + * @ext: optional array of Counter device extensions
> + * @num_ext: number of Counter device extensions specified in @ext
> + * @priv: optional private data supplied by driver
> + */
> +struct counter_device {
> + const char *name;
> + struct device *parent;
> + struct counter_device_state *device_state;
> +
> + const struct counter_ops *ops;
> +
> + struct counter_signal *signals;
> + size_t num_signals;
> + struct counter_count *counts;
> + size_t num_counts;
> +
> + const struct counter_device_ext *ext;
> + size_t num_ext;
> +
> + void *priv;
> +};
> +
> +enum signal_level {
> + SIGNAL_LEVEL_LOW = 0,
> + SIGNAL_LEVEL_HIGH
> +};
> +
> +enum signal_value_type {
> + SIGNAL_LEVEL = 0
> +};
> +
> +enum count_value_type {
> + COUNT_POSITION_UNSIGNED = 0,
> + COUNT_POSITION_SIGNED
> +};
> +
> +void set_signal_read_value(struct signal_read_value *const val,
> + const enum signal_value_type type, void *const data);
> +void set_count_read_value(struct count_read_value *const val,
> + const enum count_value_type type, void *const data);
> +int get_count_write_value(void *const data, const enum count_value_type type,
> + const struct count_write_value *const val);
I wonder if naming wise, we would be better sticking to the
noun_verb naming format.
signal_read_value_set
count_read_value_set
count_write_value_get
for example?
> +
> +int counter_register(struct counter_device *const counter);
> +void counter_unregister(struct counter_device *const counter);
> +int devm_counter_register(struct device *dev,
> + struct counter_device *const counter);
> +void devm_counter_unregister(struct device *dev,
> + struct counter_device *const counter);
> +
> +#endif /* _COUNTER_H_ */
^ permalink raw reply
* [PATCH 7/7 v5] arm64: dts: ls208xa: comply with the iommu map binding for fsl_mc
From: Nipun Gupta @ 2018-05-20 13:49 UTC (permalink / raw)
To: robin.murphy, will.deacon, robh+dt, robh, mark.rutland,
catalin.marinas, gregkh, laurentiu.tudor, bhelgaas
Cc: hch, joro, m.szyprowski, shawnguo, frowand.list, iommu,
linux-kernel, devicetree, linux-arm-kernel, linuxppc-dev,
linux-pci, bharat.bhushan, stuyoder, leoyang.li, Nipun Gupta
In-Reply-To: <1526824191-7000-1-git-send-email-nipun.gupta@nxp.com>
fsl-mc bus support the new iommu-map property. Comply to this binding
for fsl_mc bus.
Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Reviewed-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
---
arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
index 137ef4d..6010505 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi
@@ -184,6 +184,7 @@
#address-cells = <2>;
#size-cells = <2>;
ranges;
+ dma-ranges = <0x0 0x0 0x0 0x0 0x10000 0x00000000>;
clockgen: clocking@1300000 {
compatible = "fsl,ls2080a-clockgen";
@@ -357,6 +358,8 @@
reg = <0x00000008 0x0c000000 0 0x40>, /* MC portal base */
<0x00000000 0x08340000 0 0x40000>; /* MC control reg */
msi-parent = <&its>;
+ iommu-map = <0 &smmu 0 0>; /* This is fixed-up by u-boot */
+ dma-coherent;
#address-cells = <3>;
#size-cells = <1>;
@@ -460,6 +463,8 @@
compatible = "arm,mmu-500";
reg = <0 0x5000000 0 0x800000>;
#global-interrupts = <12>;
+ #iommu-cells = <1>;
+ stream-match-mask = <0x7C00>;
interrupts = <0 13 4>, /* global secure fault */
<0 14 4>, /* combined secure interrupt */
<0 15 4>, /* global non-secure fault */
@@ -502,7 +507,6 @@
<0 204 4>, <0 205 4>,
<0 206 4>, <0 207 4>,
<0 208 4>, <0 209 4>;
- mmu-masters = <&fsl_mc 0x300 0>;
};
dspi: dspi@2100000 {
--
1.9.1
^ permalink raw reply related
* [PATCH 6/7 v5] bus: fsl-mc: set coherent dma mask for devices on fsl-mc bus
From: Nipun Gupta @ 2018-05-20 13:49 UTC (permalink / raw)
To: robin.murphy, will.deacon, robh+dt, robh, mark.rutland,
catalin.marinas, gregkh, laurentiu.tudor, bhelgaas
Cc: hch, joro, m.szyprowski, shawnguo, frowand.list, iommu,
linux-kernel, devicetree, linux-arm-kernel, linuxppc-dev,
linux-pci, bharat.bhushan, stuyoder, leoyang.li, Nipun Gupta
In-Reply-To: <1526824191-7000-1-git-send-email-nipun.gupta@nxp.com>
of_dma_configure() API expects coherent_dma_mask to be correctly
set in the devices. This patch does the needful.
Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
drivers/bus/fsl-mc/fsl-mc-bus.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index fa43c7d..624828b 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -627,6 +627,7 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
mc_dev->icid = parent_mc_dev->icid;
mc_dev->dma_mask = FSL_MC_DEFAULT_DMA_MASK;
mc_dev->dev.dma_mask = &mc_dev->dma_mask;
+ mc_dev->dev.coherent_dma_mask = mc_dev->dma_mask;
dev_set_msi_domain(&mc_dev->dev,
dev_get_msi_domain(&parent_mc_dev->dev));
}
--
1.9.1
^ permalink raw reply related
* [PATCH 5/7 v5] bus: fsl-mc: support dma configure for devices on fsl-mc bus
From: Nipun Gupta @ 2018-05-20 13:49 UTC (permalink / raw)
To: robin.murphy, will.deacon, robh+dt, robh, mark.rutland,
catalin.marinas, gregkh, laurentiu.tudor, bhelgaas
Cc: hch, joro, m.szyprowski, shawnguo, frowand.list, iommu,
linux-kernel, devicetree, linux-arm-kernel, linuxppc-dev,
linux-pci, bharat.bhushan, stuyoder, leoyang.li, Nipun Gupta
In-Reply-To: <1526824191-7000-1-git-send-email-nipun.gupta@nxp.com>
This patch adds support of dma configuration for devices on fsl-mc
bus using 'dma_configure' callback for busses. Also, directly calling
arch_setup_dma_ops is removed from the fsl-mc bus.
Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Reviewed-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
---
drivers/bus/fsl-mc/fsl-mc-bus.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 5d8266c..fa43c7d 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -127,6 +127,16 @@ static int fsl_mc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
return 0;
}
+static int fsl_mc_dma_configure(struct device *dev)
+{
+ struct device *dma_dev = dev;
+
+ while (dev_is_fsl_mc(dma_dev))
+ dma_dev = dma_dev->parent;
+
+ return of_dma_configure(dev, dma_dev->of_node, 0);
+}
+
static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -148,6 +158,7 @@ struct bus_type fsl_mc_bus_type = {
.name = "fsl-mc",
.match = fsl_mc_bus_match,
.uevent = fsl_mc_bus_uevent,
+ .dma_configure = fsl_mc_dma_configure,
.dev_groups = fsl_mc_dev_groups,
};
EXPORT_SYMBOL_GPL(fsl_mc_bus_type);
@@ -633,10 +644,6 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
goto error_cleanup_dev;
}
- /* Objects are coherent, unless 'no shareability' flag set. */
- if (!(obj_desc->flags & FSL_MC_OBJ_FLAG_NO_MEM_SHAREABILITY))
- arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
-
/*
* The device-specific probe callback will get invoked by device_add()
*/
--
1.9.1
^ permalink raw reply related
* [PATCH 4/7 v5] iommu: arm-smmu: Add support for the fsl-mc bus
From: Nipun Gupta @ 2018-05-20 13:49 UTC (permalink / raw)
To: robin.murphy, will.deacon, robh+dt, robh, mark.rutland,
catalin.marinas, gregkh, laurentiu.tudor, bhelgaas
Cc: hch, joro, m.szyprowski, shawnguo, frowand.list, iommu,
linux-kernel, devicetree, linux-arm-kernel, linuxppc-dev,
linux-pci, bharat.bhushan, stuyoder, leoyang.li, Nipun Gupta
In-Reply-To: <1526824191-7000-1-git-send-email-nipun.gupta@nxp.com>
Implement bus specific support for the fsl-mc bus including
registering arm_smmu_ops and bus specific device add operations.
Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
drivers/iommu/arm-smmu.c | 7 +++++++
drivers/iommu/iommu.c | 21 +++++++++++++++++++++
include/linux/fsl/mc.h | 8 ++++++++
include/linux/iommu.h | 2 ++
4 files changed, 38 insertions(+)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 69e7c60..e1d5090 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -52,6 +52,7 @@
#include <linux/spinlock.h>
#include <linux/amba/bus.h>
+#include <linux/fsl/mc.h>
#include "io-pgtable.h"
#include "arm-smmu-regs.h"
@@ -1459,6 +1460,8 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
if (dev_is_pci(dev))
group = pci_device_group(dev);
+ else if (dev_is_fsl_mc(dev))
+ group = fsl_mc_device_group(dev);
else
group = generic_device_group(dev);
@@ -2037,6 +2040,10 @@ static void arm_smmu_bus_init(void)
bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
}
#endif
+#ifdef CONFIG_FSL_MC_BUS
+ if (!iommu_present(&fsl_mc_bus_type))
+ bus_set_iommu(&fsl_mc_bus_type, &arm_smmu_ops);
+#endif
}
static int arm_smmu_device_probe(struct platform_device *pdev)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d2aa2320..6d4ce35 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -32,6 +32,7 @@
#include <linux/pci.h>
#include <linux/bitops.h>
#include <linux/property.h>
+#include <linux/fsl/mc.h>
#include <trace/events/iommu.h>
static struct kset *iommu_group_kset;
@@ -987,6 +988,26 @@ struct iommu_group *pci_device_group(struct device *dev)
return iommu_group_alloc();
}
+/* Get the IOMMU group for device on fsl-mc bus */
+struct iommu_group *fsl_mc_device_group(struct device *dev)
+{
+ struct device *cont_dev = fsl_mc_cont_dev(dev);
+ struct iommu_group *group;
+
+ /* Container device is responsible for creating the iommu group */
+ if (fsl_mc_is_cont_dev(dev)) {
+ group = iommu_group_alloc();
+ if (IS_ERR(group))
+ return NULL;
+ } else {
+ get_device(cont_dev);
+ group = iommu_group_get(cont_dev);
+ put_device(cont_dev);
+ }
+
+ return group;
+}
+
/**
* iommu_group_get_for_dev - Find or create the IOMMU group for a device
* @dev: target device
diff --git a/include/linux/fsl/mc.h b/include/linux/fsl/mc.h
index f27cb14..dddaca1 100644
--- a/include/linux/fsl/mc.h
+++ b/include/linux/fsl/mc.h
@@ -351,6 +351,14 @@ struct fsl_mc_io {
#define dev_is_fsl_mc(_dev) (0)
#endif
+/* Macro to check if a device is a container device */
+#define fsl_mc_is_cont_dev(_dev) (to_fsl_mc_device(_dev)->flags & \
+ FSL_MC_IS_DPRC)
+
+/* Macro to get the container device of a MC device */
+#define fsl_mc_cont_dev(_dev) (fsl_mc_is_cont_dev(_dev) ? \
+ (_dev) : (_dev)->parent)
+
/*
* module_fsl_mc_driver() - Helper macro for drivers that don't do
* anything special in module init/exit. This eliminates a lot of
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 19938ee..2981200 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -389,6 +389,8 @@ static inline size_t iommu_map_sg(struct iommu_domain *domain,
extern struct iommu_group *pci_device_group(struct device *dev);
/* Generic device grouping function */
extern struct iommu_group *generic_device_group(struct device *dev);
+/* FSL-MC device grouping function */
+struct iommu_group *fsl_mc_device_group(struct device *dev);
/**
* struct iommu_fwspec - per-device IOMMU instance data
--
1.9.1
^ permalink raw reply related
* [PATCH 3/7 v5] iommu: support iommu configuration for fsl-mc devices
From: Nipun Gupta @ 2018-05-20 13:49 UTC (permalink / raw)
To: robin.murphy, will.deacon, robh+dt, robh, mark.rutland,
catalin.marinas, gregkh, laurentiu.tudor, bhelgaas
Cc: devicetree, stuyoder, bharat.bhushan, shawnguo, joro,
linuxppc-dev, linux-kernel, leoyang.li, iommu, Nipun Gupta,
linux-pci, frowand.list, hch, linux-arm-kernel, m.szyprowski
In-Reply-To: <1526824191-7000-1-git-send-email-nipun.gupta@nxp.com>
With of_pci_map_rid available for all the busses, use the function
for configuration of devices on fsl-mc bus
Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
drivers/iommu/of_iommu.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 811e160..284474d 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -24,6 +24,7 @@
#include <linux/of_iommu.h>
#include <linux/of_pci.h>
#include <linux/slab.h>
+#include <linux/fsl/mc.h>
#define NO_IOMMU 1
@@ -159,6 +160,23 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
return err;
}
+static int of_fsl_mc_iommu_init(struct fsl_mc_device *mc_dev,
+ struct device_node *master_np)
+{
+ struct of_phandle_args iommu_spec = { .args_count = 1 };
+ int err;
+
+ err = of_map_rid(master_np, mc_dev->icid, "iommu-map",
+ "iommu-map-mask", &iommu_spec.np,
+ iommu_spec.args);
+ if (err)
+ return err == -ENODEV ? NO_IOMMU : err;
+
+ err = of_iommu_xlate(&mc_dev->dev, &iommu_spec);
+ of_node_put(iommu_spec.np);
+ return err;
+}
+
const struct iommu_ops *of_iommu_configure(struct device *dev,
struct device_node *master_np)
{
@@ -190,6 +208,8 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
err = pci_for_each_dma_alias(to_pci_dev(dev),
of_pci_iommu_init, &info);
+ } else if (dev_is_fsl_mc(dev)) {
+ err = of_fsl_mc_iommu_init(to_fsl_mc_device(dev), master_np);
} else {
struct of_phandle_args iommu_spec;
int idx = 0;
--
1.9.1
^ permalink raw reply related
* [PATCH 2/7 v5] iommu: of: make of_pci_map_rid() available for other devices too
From: Nipun Gupta @ 2018-05-20 13:49 UTC (permalink / raw)
To: robin.murphy, will.deacon, robh+dt, robh, mark.rutland,
catalin.marinas, gregkh, laurentiu.tudor, bhelgaas
Cc: hch, joro, m.szyprowski, shawnguo, frowand.list, iommu,
linux-kernel, devicetree, linux-arm-kernel, linuxppc-dev,
linux-pci, bharat.bhushan, stuyoder, leoyang.li, Nipun Gupta
In-Reply-To: <1526824191-7000-1-git-send-email-nipun.gupta@nxp.com>
iommu-map property is also used by devices with fsl-mc. This
patch moves the of_pci_map_rid to generic location, so that it
can be used by other busses too.
'of_pci_map_rid' is renamed here to 'of_map_rid' and there is no
functional change done in the API.
Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/iommu/of_iommu.c | 5 +--
drivers/of/base.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
drivers/of/irq.c | 5 +--
drivers/pci/of.c | 101 ----------------------------------------------
include/linux/of.h | 11 +++++
include/linux/of_pci.h | 10 -----
6 files changed, 117 insertions(+), 117 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5c36a8b..811e160 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -149,9 +149,8 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
struct of_phandle_args iommu_spec = { .args_count = 1 };
int err;
- err = of_pci_map_rid(info->np, alias, "iommu-map",
- "iommu-map-mask", &iommu_spec.np,
- iommu_spec.args);
+ err = of_map_rid(info->np, alias, "iommu-map", "iommu-map-mask",
+ &iommu_spec.np, iommu_spec.args);
if (err)
return err == -ENODEV ? NO_IOMMU : err;
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 848f549..c7aac81 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1995,3 +1995,105 @@ int of_find_last_cache_level(unsigned int cpu)
return cache_level;
}
+
+/**
+ * of_map_rid - Translate a requester ID through a downstream mapping.
+ * @np: root complex device node.
+ * @rid: device requester ID to map.
+ * @map_name: property name of the map to use.
+ * @map_mask_name: optional property name of the mask to use.
+ * @target: optional pointer to a target device node.
+ * @id_out: optional pointer to receive the translated ID.
+ *
+ * Given a device requester ID, look up the appropriate implementation-defined
+ * platform ID and/or the target device which receives transactions on that
+ * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
+ * @id_out may be NULL if only the other is required. If @target points to
+ * a non-NULL device node pointer, only entries targeting that node will be
+ * matched; if it points to a NULL value, it will receive the device node of
+ * the first matching target phandle, with a reference held.
+ *
+ * Return: 0 on success or a standard error code on failure.
+ */
+int of_map_rid(struct device_node *np, u32 rid,
+ const char *map_name, const char *map_mask_name,
+ struct device_node **target, u32 *id_out)
+{
+ u32 map_mask, masked_rid;
+ int map_len;
+ const __be32 *map = NULL;
+
+ if (!np || !map_name || (!target && !id_out))
+ return -EINVAL;
+
+ map = of_get_property(np, map_name, &map_len);
+ if (!map) {
+ if (target)
+ return -ENODEV;
+ /* Otherwise, no map implies no translation */
+ *id_out = rid;
+ return 0;
+ }
+
+ if (!map_len || map_len % (4 * sizeof(*map))) {
+ pr_err("%pOF: Error: Bad %s length: %d\n", np,
+ map_name, map_len);
+ return -EINVAL;
+ }
+
+ /* The default is to select all bits. */
+ map_mask = 0xffffffff;
+
+ /*
+ * Can be overridden by "{iommu,msi}-map-mask" property.
+ * If of_property_read_u32() fails, the default is used.
+ */
+ if (map_mask_name)
+ of_property_read_u32(np, map_mask_name, &map_mask);
+
+ masked_rid = map_mask & rid;
+ for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4) {
+ struct device_node *phandle_node;
+ u32 rid_base = be32_to_cpup(map + 0);
+ u32 phandle = be32_to_cpup(map + 1);
+ u32 out_base = be32_to_cpup(map + 2);
+ u32 rid_len = be32_to_cpup(map + 3);
+
+ if (rid_base & ~map_mask) {
+ pr_err("%pOF: Invalid %s translation - %s-mask (0x%x) ignores rid-base (0x%x)\n",
+ np, map_name, map_name,
+ map_mask, rid_base);
+ return -EFAULT;
+ }
+
+ if (masked_rid < rid_base || masked_rid >= rid_base + rid_len)
+ continue;
+
+ phandle_node = of_find_node_by_phandle(phandle);
+ if (!phandle_node)
+ return -ENODEV;
+
+ if (target) {
+ if (*target)
+ of_node_put(phandle_node);
+ else
+ *target = phandle_node;
+
+ if (*target != phandle_node)
+ continue;
+ }
+
+ if (id_out)
+ *id_out = masked_rid - rid_base + out_base;
+
+ pr_debug("%pOF: %s, using mask %08x, rid-base: %08x, out-base: %08x, length: %08x, rid: %08x -> %08x\n",
+ np, map_name, map_mask, rid_base, out_base,
+ rid_len, rid, masked_rid - rid_base + out_base);
+ return 0;
+ }
+
+ pr_err("%pOF: Invalid %s translation - no match for rid 0x%x on %pOF\n",
+ np, map_name, rid, target && *target ? *target : NULL);
+ return -EFAULT;
+}
+EXPORT_SYMBOL_GPL(of_map_rid);
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 02ad93a..e1f6f39 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -22,7 +22,6 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_irq.h>
-#include <linux/of_pci.h>
#include <linux/string.h>
#include <linux/slab.h>
@@ -588,8 +587,8 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
* "msi-map" property.
*/
for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent)
- if (!of_pci_map_rid(parent_dev->of_node, rid_in, "msi-map",
- "msi-map-mask", np, &rid_out))
+ if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
+ "msi-map-mask", np, &rid_out))
break;
return rid_out;
}
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index a28355c..d2cebbe 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -362,107 +362,6 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
#endif /* CONFIG_OF_ADDRESS */
-/**
- * of_pci_map_rid - Translate a requester ID through a downstream mapping.
- * @np: root complex device node.
- * @rid: PCI requester ID to map.
- * @map_name: property name of the map to use.
- * @map_mask_name: optional property name of the mask to use.
- * @target: optional pointer to a target device node.
- * @id_out: optional pointer to receive the translated ID.
- *
- * Given a PCI requester ID, look up the appropriate implementation-defined
- * platform ID and/or the target device which receives transactions on that
- * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
- * @id_out may be NULL if only the other is required. If @target points to
- * a non-NULL device node pointer, only entries targeting that node will be
- * matched; if it points to a NULL value, it will receive the device node of
- * the first matching target phandle, with a reference held.
- *
- * Return: 0 on success or a standard error code on failure.
- */
-int of_pci_map_rid(struct device_node *np, u32 rid,
- const char *map_name, const char *map_mask_name,
- struct device_node **target, u32 *id_out)
-{
- u32 map_mask, masked_rid;
- int map_len;
- const __be32 *map = NULL;
-
- if (!np || !map_name || (!target && !id_out))
- return -EINVAL;
-
- map = of_get_property(np, map_name, &map_len);
- if (!map) {
- if (target)
- return -ENODEV;
- /* Otherwise, no map implies no translation */
- *id_out = rid;
- return 0;
- }
-
- if (!map_len || map_len % (4 * sizeof(*map))) {
- pr_err("%pOF: Error: Bad %s length: %d\n", np,
- map_name, map_len);
- return -EINVAL;
- }
-
- /* The default is to select all bits. */
- map_mask = 0xffffffff;
-
- /*
- * Can be overridden by "{iommu,msi}-map-mask" property.
- * If of_property_read_u32() fails, the default is used.
- */
- if (map_mask_name)
- of_property_read_u32(np, map_mask_name, &map_mask);
-
- masked_rid = map_mask & rid;
- for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4) {
- struct device_node *phandle_node;
- u32 rid_base = be32_to_cpup(map + 0);
- u32 phandle = be32_to_cpup(map + 1);
- u32 out_base = be32_to_cpup(map + 2);
- u32 rid_len = be32_to_cpup(map + 3);
-
- if (rid_base & ~map_mask) {
- pr_err("%pOF: Invalid %s translation - %s-mask (0x%x) ignores rid-base (0x%x)\n",
- np, map_name, map_name,
- map_mask, rid_base);
- return -EFAULT;
- }
-
- if (masked_rid < rid_base || masked_rid >= rid_base + rid_len)
- continue;
-
- phandle_node = of_find_node_by_phandle(phandle);
- if (!phandle_node)
- return -ENODEV;
-
- if (target) {
- if (*target)
- of_node_put(phandle_node);
- else
- *target = phandle_node;
-
- if (*target != phandle_node)
- continue;
- }
-
- if (id_out)
- *id_out = masked_rid - rid_base + out_base;
-
- pr_debug("%pOF: %s, using mask %08x, rid-base: %08x, out-base: %08x, length: %08x, rid: %08x -> %08x\n",
- np, map_name, map_mask, rid_base, out_base,
- rid_len, rid, masked_rid - rid_base + out_base);
- return 0;
- }
-
- pr_err("%pOF: Invalid %s translation - no match for rid 0x%x on %pOF\n",
- np, map_name, rid, target && *target ? *target : NULL);
- return -EFAULT;
-}
-
#if IS_ENABLED(CONFIG_OF_IRQ)
/**
* of_irq_parse_pci - Resolve the interrupt for a PCI device
diff --git a/include/linux/of.h b/include/linux/of.h
index 4d25e4f..f4251c3 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -545,6 +545,10 @@ const __be32 *of_prop_next_u32(struct property *prop, const __be32 *cur,
extern int of_cpu_node_to_id(struct device_node *np);
+int of_map_rid(struct device_node *np, u32 rid,
+ const char *map_name, const char *map_mask_name,
+ struct device_node **target, u32 *id_out);
+
#else /* CONFIG_OF */
static inline void of_core_init(void)
@@ -931,6 +935,13 @@ static inline int of_cpu_node_to_id(struct device_node *np)
return -ENODEV;
}
+static inline int of_map_rid(struct device_node *np, u32 rid,
+ const char *map_name, const char *map_mask_name,
+ struct device_node **target, u32 *id_out)
+{
+ return -EINVAL;
+}
+
#define of_match_ptr(_ptr) NULL
#define of_match_node(_matches, _node) NULL
#endif /* CONFIG_OF */
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index 091033a..a23b44a 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -17,9 +17,6 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
int of_get_pci_domain_nr(struct device_node *node);
int of_pci_get_max_link_speed(struct device_node *node);
void of_pci_check_probe_only(void);
-int of_pci_map_rid(struct device_node *np, u32 rid,
- const char *map_name, const char *map_mask_name,
- struct device_node **target, u32 *id_out);
#else
static inline struct device_node *of_pci_find_child_device(struct device_node *parent,
unsigned int devfn)
@@ -44,13 +41,6 @@ static inline int of_pci_get_devfn(struct device_node *np)
return -1;
}
-static inline int of_pci_map_rid(struct device_node *np, u32 rid,
- const char *map_name, const char *map_mask_name,
- struct device_node **target, u32 *id_out)
-{
- return -EINVAL;
-}
-
static inline int
of_pci_get_max_link_speed(struct device_node *node)
{
--
1.9.1
^ permalink raw reply related
* [PATCH 1/7 v5] Docs: dt: add fsl-mc iommu-map device-tree binding
From: Nipun Gupta @ 2018-05-20 13:49 UTC (permalink / raw)
To: robin.murphy, will.deacon, robh+dt, robh, mark.rutland,
catalin.marinas, gregkh, laurentiu.tudor, bhelgaas
Cc: hch, joro, m.szyprowski, shawnguo, frowand.list, iommu,
linux-kernel, devicetree, linux-arm-kernel, linuxppc-dev,
linux-pci, bharat.bhushan, stuyoder, leoyang.li, Nipun Gupta
In-Reply-To: <1526824191-7000-1-git-send-email-nipun.gupta@nxp.com>
The existing IOMMU bindings cannot be used to specify the relationship
between fsl-mc devices and IOMMUs. This patch adds a generic binding for
mapping fsl-mc devices to IOMMUs, using iommu-map property.
Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../devicetree/bindings/misc/fsl,qoriq-mc.txt | 39 ++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
index 6611a7c..8cbed4f 100644
--- a/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
+++ b/Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
@@ -9,6 +9,25 @@ blocks that can be used to create functional hardware objects/devices
such as network interfaces, crypto accelerator instances, L2 switches,
etc.
+For an overview of the DPAA2 architecture and fsl-mc bus see:
+drivers/staging/fsl-mc/README.txt
+
+As described in the above overview, all DPAA2 objects in a DPRC share the
+same hardware "isolation context" and a 10-bit value called an ICID
+(isolation context id) is expressed by the hardware to identify
+the requester.
+
+The generic 'iommus' property is insufficient to describe the relationship
+between ICIDs and IOMMUs, so an iommu-map property is used to define
+the set of possible ICIDs under a root DPRC and how they map to
+an IOMMU.
+
+For generic IOMMU bindings, see
+Documentation/devicetree/bindings/iommu/iommu.txt.
+
+For arm-smmu binding, see:
+Documentation/devicetree/bindings/iommu/arm,smmu.txt.
+
Required properties:
- compatible
@@ -88,14 +107,34 @@ Sub-nodes:
Value type: <phandle>
Definition: Specifies the phandle to the PHY device node associated
with the this dpmac.
+Optional properties:
+
+- iommu-map: Maps an ICID to an IOMMU and associated iommu-specifier
+ data.
+
+ The property is an arbitrary number of tuples of
+ (icid-base,iommu,iommu-base,length).
+
+ Any ICID i in the interval [icid-base, icid-base + length) is
+ associated with the listed IOMMU, with the iommu-specifier
+ (i - icid-base + iommu-base).
Example:
+ smmu: iommu@5000000 {
+ compatible = "arm,mmu-500";
+ #iommu-cells = <2>;
+ stream-match-mask = <0x7C00>;
+ ...
+ };
+
fsl_mc: fsl-mc@80c000000 {
compatible = "fsl,qoriq-mc";
reg = <0x00000008 0x0c000000 0 0x40>, /* MC portal base */
<0x00000000 0x08340000 0 0x40000>; /* MC control reg */
msi-parent = <&its>;
+ /* define map for ICIDs 23-64 */
+ iommu-map = <23 &smmu 23 41>;
#address-cells = <3>;
#size-cells = <1>;
--
1.9.1
^ permalink raw reply related
* [PATCH 0/7 v5] Support for fsl-mc bus and its devices in SMMU
From: Nipun Gupta @ 2018-05-20 13:49 UTC (permalink / raw)
To: robin.murphy-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, robh-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, catalin.marinas-5wv7dgnIgG8,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
laurentiu.tudor-3arQi8VN3Tc, bhelgaas-hpIqsD4AKlfQT0dZR+AlfA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
stuyoder-Re5JQEeQqe8AvxtiuMwx3w, shawnguo-DgEjT+Ai2ygdnm+yROfE0A,
linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, leoyang.li-3arQi8VN3Tc,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-pci-u79uwXL29TY76Z2rM5mHXA,
frowand.list-Re5JQEeQqe8AvxtiuMwx3w, hch-jcswGhMUV9g,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
This patchset defines IOMMU DT binding for fsl-mc bus and adds
support in SMMU for fsl-mc bus.
The patch series is based on top of dma-mapping tree (for-next branch):
http://git.infradead.org/users/hch/dma-mapping.git
These patches
- Define property 'iommu-map' for fsl-mc bus (patch 1)
- Integrates the fsl-mc bus with the SMMU using this
IOMMU binding (patch 2,3,4)
- Adds the dma configuration support for fsl-mc bus (patch 5, 6)
- Updates the fsl-mc device node with iommu/dma related changes (patch 7)
Changes in v2:
- use iommu-map property for fsl-mc bus
- rebase over patchset https://patchwork.kernel.org/patch/10317337/
and make corresponding changes for dma configuration of devices on
fsl-mc bus
Changes in v3:
- move of_map_rid in drivers/of/address.c
Changes in v4:
- move of_map_rid in drivers/of/base.c
Changes in v5:
- break patch 5 in two separate patches (now patch 5/7 and patch 6/7)
- add changelog text in patch 3/7 and patch 5/7
- typo fix
Nipun Gupta (7):
Docs: dt: add fsl-mc iommu-map device-tree binding
iommu: of: make of_pci_map_rid() available for other devices too
iommu: support iommu configuration for fsl-mc devices
iommu: arm-smmu: Add support for the fsl-mc bus
bus: fsl-mc: support dma configure for devices on fsl-mc bus
bus: fsl-mc: set coherent dma mask for devices on fsl-mc bus
arm64: dts: ls208xa: comply with the iommu map binding for fsl_mc
.../devicetree/bindings/misc/fsl,qoriq-mc.txt | 39 ++++++++
arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 6 +-
drivers/bus/fsl-mc/fsl-mc-bus.c | 16 +++-
drivers/iommu/arm-smmu.c | 7 ++
drivers/iommu/iommu.c | 21 +++++
drivers/iommu/of_iommu.c | 25 ++++-
drivers/of/base.c | 102 +++++++++++++++++++++
drivers/of/irq.c | 5 +-
drivers/pci/of.c | 101 --------------------
include/linux/fsl/mc.h | 8 ++
include/linux/iommu.h | 2 +
include/linux/of.h | 11 +++
include/linux/of_pci.h | 10 --
13 files changed, 231 insertions(+), 122 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH v2 3/3] ARM: dts: imx28/imx53: enable edt-ft5x06 wakeup source
From: Shawn Guo @ 2018-05-20 13:05 UTC (permalink / raw)
To: Daniel Mack
Cc: mark.rutland, devicetree, dmitry.torokhov, robh+dt, kernel,
linux-input, fabio.estevam, linux-arm-kernel
In-Reply-To: <20180517090552.5704-4-daniel@zonque.org>
On Thu, May 17, 2018 at 11:05:52AM +0200, Daniel Mack wrote:
> The touchscreen driver no longer configures the device as wakeup source by
> default. A "wakeup-source" property is needed.
>
> To avoid regressions, this patch changes the DTS files for the only two
> users of this driver that didn't have this property yet.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Rob Herring <robh+dt@kernel.org>
Applied this one, thanks.
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx51-zii-rdu1: cleanup eMMC node
From: Shawn Guo @ 2018-05-20 12:59 UTC (permalink / raw)
To: Nikita Yushchenko
Cc: Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Rob Herring,
Mark Rutland, linux-arm-kernel, devicetree, linux-kernel,
Chris Healy, Andrey Smirnov
In-Reply-To: <20180516065349.11160-1-nikita.yoush@cogentembedded.com>
On Wed, May 16, 2018 at 09:53:49AM +0300, Nikita Yushchenko wrote:
> On RDU1, sdhc1 is used for eMMC, and that is 3.3V only.
>
> Thus configure device node not to probe it as SD/SDIO and not try 1.8V.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] ARM: dts: vf610-zii-dev: enable vf610 builtin temp sensor
From: Shawn Guo @ 2018-05-20 12:56 UTC (permalink / raw)
To: Nikita Yushchenko
Cc: Sascha Hauer, Pengutronix Kernel Team, Stefan Agner, Rob Herring,
Mark Rutland, linux-arm-kernel, devicetree, linux-kernel,
Chris Healy
In-Reply-To: <20180516063921.10406-1-nikita.yoush@cogentembedded.com>
On Wed, May 16, 2018 at 09:39:21AM +0300, Nikita Yushchenko wrote:
> Vybrid has single internal temperature sensor connected to both internal
> ADC modules.
>
> vf610-zii-dev already has ADC0 enabled. Now, to get temperature sensor
> captured by iio_hwmon driver, need to configure iio_hwmon node to use
> that ADC.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Applied, thanks.
^ permalink raw reply
* [PATCH] ASoC: codec: wolfson: Make the node name generic
From: Fabio Estevam @ 2018-05-20 12:53 UTC (permalink / raw)
To: broonie; +Cc: Fabio Estevam, devicetree, alsa-devel, robh+dt
From: Fabio Estevam <fabio.estevam@nxp.com>
According to Devicetree Specification v0.2 document:
"The name of a node should be somewhat generic, reflecting the function
of the device and not its precise programming model."
Do as suggested in the bindings examples.
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
Documentation/devicetree/bindings/sound/wm8510.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8523.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8524.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8580.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8711.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8728.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8731.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8737.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8741.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8750.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8753.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8770.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8776.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8804.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8903.txt | 2 +-
Documentation/devicetree/bindings/sound/wm8994.txt | 2 +-
16 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/wm8510.txt b/Documentation/devicetree/bindings/sound/wm8510.txt
index fa1a32b..e6b6cc0 100644
--- a/Documentation/devicetree/bindings/sound/wm8510.txt
+++ b/Documentation/devicetree/bindings/sound/wm8510.txt
@@ -12,7 +12,7 @@ Required properties:
Example:
-codec: wm8510@1a {
+wm8510: codec@1a {
compatible = "wlf,wm8510";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8523.txt b/Documentation/devicetree/bindings/sound/wm8523.txt
index 0474618..f3a6485 100644
--- a/Documentation/devicetree/bindings/sound/wm8523.txt
+++ b/Documentation/devicetree/bindings/sound/wm8523.txt
@@ -10,7 +10,7 @@ Required properties:
Example:
-codec: wm8523@1a {
+wm8523: codec@1a {
compatible = "wlf,wm8523";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8524.txt b/Documentation/devicetree/bindings/sound/wm8524.txt
index 0f05535..f6c0c26 100644
--- a/Documentation/devicetree/bindings/sound/wm8524.txt
+++ b/Documentation/devicetree/bindings/sound/wm8524.txt
@@ -10,7 +10,7 @@ Required properties:
Example:
-codec: wm8524 {
+wm8524: codec {
compatible = "wlf,wm8524";
wlf,mute-gpios = <&gpio1 8 GPIO_ACTIVE_LOW>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8580.txt b/Documentation/devicetree/bindings/sound/wm8580.txt
index 78fce9b..ff3f9f5 100644
--- a/Documentation/devicetree/bindings/sound/wm8580.txt
+++ b/Documentation/devicetree/bindings/sound/wm8580.txt
@@ -10,7 +10,7 @@ Required properties:
Example:
-codec: wm8580@1a {
+wm8580: codec@1a {
compatible = "wlf,wm8580";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8711.txt b/Documentation/devicetree/bindings/sound/wm8711.txt
index 8ed9998..c30a138 100644
--- a/Documentation/devicetree/bindings/sound/wm8711.txt
+++ b/Documentation/devicetree/bindings/sound/wm8711.txt
@@ -12,7 +12,7 @@ Required properties:
Example:
-codec: wm8711@1a {
+wm8711: codec@1a {
compatible = "wlf,wm8711";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8728.txt b/Documentation/devicetree/bindings/sound/wm8728.txt
index a8b5c36..a3608b4 100644
--- a/Documentation/devicetree/bindings/sound/wm8728.txt
+++ b/Documentation/devicetree/bindings/sound/wm8728.txt
@@ -12,7 +12,7 @@ Required properties:
Example:
-codec: wm8728@1a {
+wm8728: codec@1a {
compatible = "wlf,wm8728";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8731.txt b/Documentation/devicetree/bindings/sound/wm8731.txt
index 236690e..f660d9b 100644
--- a/Documentation/devicetree/bindings/sound/wm8731.txt
+++ b/Documentation/devicetree/bindings/sound/wm8731.txt
@@ -12,7 +12,7 @@ Required properties:
Example:
-codec: wm8731@1a {
+wm8731: codec@1a {
compatible = "wlf,wm8731";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8737.txt b/Documentation/devicetree/bindings/sound/wm8737.txt
index 4bc2cea..eda1ec6 100644
--- a/Documentation/devicetree/bindings/sound/wm8737.txt
+++ b/Documentation/devicetree/bindings/sound/wm8737.txt
@@ -12,7 +12,7 @@ Required properties:
Example:
-codec: wm8737@1a {
+wm8737: codec@1a {
compatible = "wlf,wm8737";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8741.txt b/Documentation/devicetree/bindings/sound/wm8741.txt
index a133154..b69e196 100644
--- a/Documentation/devicetree/bindings/sound/wm8741.txt
+++ b/Documentation/devicetree/bindings/sound/wm8741.txt
@@ -21,7 +21,7 @@ Optional properties:
Example:
-codec: wm8741@1a {
+wm8741: codec@1a {
compatible = "wlf,wm8741";
reg = <0x1a>;
diff --git a/Documentation/devicetree/bindings/sound/wm8750.txt b/Documentation/devicetree/bindings/sound/wm8750.txt
index 8db239f..682f221 100644
--- a/Documentation/devicetree/bindings/sound/wm8750.txt
+++ b/Documentation/devicetree/bindings/sound/wm8750.txt
@@ -12,7 +12,7 @@ Required properties:
Example:
-codec: wm8750@1a {
+wm8750: codec@1a {
compatible = "wlf,wm8750";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8753.txt b/Documentation/devicetree/bindings/sound/wm8753.txt
index 8eee612..eca9e5a 100644
--- a/Documentation/devicetree/bindings/sound/wm8753.txt
+++ b/Documentation/devicetree/bindings/sound/wm8753.txt
@@ -34,7 +34,7 @@ Pins on the device (for linking into audio routes):
Example:
-codec: wm8753@1a {
+wm8753: codec@1a {
compatible = "wlf,wm8753";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8770.txt b/Documentation/devicetree/bindings/sound/wm8770.txt
index 866e00c..cac762a 100644
--- a/Documentation/devicetree/bindings/sound/wm8770.txt
+++ b/Documentation/devicetree/bindings/sound/wm8770.txt
@@ -10,7 +10,7 @@ Required properties:
Example:
-codec: wm8770@1 {
+wm8770: codec@1 {
compatible = "wlf,wm8770";
reg = <1>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8776.txt b/Documentation/devicetree/bindings/sound/wm8776.txt
index 3b9ca49..0117336 100644
--- a/Documentation/devicetree/bindings/sound/wm8776.txt
+++ b/Documentation/devicetree/bindings/sound/wm8776.txt
@@ -12,7 +12,7 @@ Required properties:
Example:
-codec: wm8776@1a {
+wm8776: codec@1a {
compatible = "wlf,wm8776";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8804.txt b/Documentation/devicetree/bindings/sound/wm8804.txt
index 6fd124b..2c1641c 100644
--- a/Documentation/devicetree/bindings/sound/wm8804.txt
+++ b/Documentation/devicetree/bindings/sound/wm8804.txt
@@ -19,7 +19,7 @@ Optional properties:
Example:
-codec: wm8804@1a {
+wm8804: codec@1a {
compatible = "wlf,wm8804";
reg = <0x1a>;
};
diff --git a/Documentation/devicetree/bindings/sound/wm8903.txt b/Documentation/devicetree/bindings/sound/wm8903.txt
index afc51ca..6371c24 100644
--- a/Documentation/devicetree/bindings/sound/wm8903.txt
+++ b/Documentation/devicetree/bindings/sound/wm8903.txt
@@ -57,7 +57,7 @@ Pins on the device (for linking into audio routes):
Example:
-codec: wm8903@1a {
+wm8903: codec@1a {
compatible = "wlf,wm8903";
reg = <0x1a>;
interrupts = < 347 >;
diff --git a/Documentation/devicetree/bindings/sound/wm8994.txt b/Documentation/devicetree/bindings/sound/wm8994.txt
index 68c4e8d..4a9dead 100644
--- a/Documentation/devicetree/bindings/sound/wm8994.txt
+++ b/Documentation/devicetree/bindings/sound/wm8994.txt
@@ -59,7 +59,7 @@ Optional properties:
Example:
-codec: wm8994@1a {
+wm8994: codec@1a {
compatible = "wlf,wm8994";
reg = <0x1a>;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] ARM: dts: imx7d: use operating-points-v2 for cpu
From: Shawn Guo @ 2018-05-20 12:50 UTC (permalink / raw)
To: Anson Huang
Cc: kernel, fabio.estevam, robh+dt, mark.rutland, Linux-imx,
linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <1526446097-7111-1-git-send-email-Anson.Huang@nxp.com>
On Wed, May 16, 2018 at 12:48:17PM +0800, Anson Huang wrote:
> This patch uses "operating-points-v2" instead of
> "operating-points" to be more fit with cpufreq-dt
> driver.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> arch/arm/boot/dts/imx7d.dtsi | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
> index 4c9877e..28980c8 100644
> --- a/arch/arm/boot/dts/imx7d.dtsi
> +++ b/arch/arm/boot/dts/imx7d.dtsi
> @@ -9,12 +9,8 @@
> / {
> cpus {
> cpu0: cpu@0 {
> - operating-points = <
> - /* KHz uV */
> - 996000 1075000
> - 792000 975000
> - >;
> clock-frequency = <996000000>;
> + operating-points-v2 = <&cpu0_opp_table>;
> };
>
> cpu1: cpu@1 {
> @@ -22,6 +18,24 @@
> device_type = "cpu";
> reg = <1>;
> clock-frequency = <996000000>;
> + operating-points-v2 = <&cpu0_opp_table>;
> + };
> + };
> +
> + cpu0_opp_table: opp_table0 {
Hyphen is recommended in node name. Also the suffix 0 doesn't mean too
much here. That said, a better node name would be 'opp-table'.
> + compatible = "operating-points-v2";
> + opp-shared;
> +
> + opp-792000000 {
> + opp-hz = /bits/ 64 <792000000>;
> + opp-microvolt = <975000>;
> + clock-latency-ns = <150000>;
> + };
We recommend to have a newline between nodes.
I fixed them all and applied the patch.
Shawn
> + opp-996000000 {
> + opp-hz = /bits/ 64 <996000000>;
> + opp-microvolt = <1075000>;
> + clock-latency-ns = <150000>;
> + opp-suspend;
> };
> };
>
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx7s-warp: remove unnecessary cpu regulator supply
From: Shawn Guo @ 2018-05-20 12:43 UTC (permalink / raw)
To: Anson Huang
Cc: kernel, fabio.estevam, robh+dt, mark.rutland, Linux-imx,
linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <1526375895-23633-1-git-send-email-Anson.Huang@nxp.com>
On Tue, May 15, 2018 at 05:18:15PM +0800, Anson Huang wrote:
> i.MX7S does NOT support CPU frequency scaling, so no
> need to specify the CPU regulator supply.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH V2] ARM: dts: imx7d: correct cpu supply name for voltage scaling
From: Shawn Guo @ 2018-05-20 12:39 UTC (permalink / raw)
To: Anson Huang
Cc: kernel, fabio.estevam, robh+dt, mark.rutland, devicetree,
Linux-imx, linux-arm-kernel, linux-kernel
In-Reply-To: <1526437548-32372-1-git-send-email-Anson.Huang@nxp.com>
On Wed, May 16, 2018 at 10:25:48AM +0800, Anson Huang wrote:
> Correct CPU supply name to meet cpufreq-dt driver's
> requirement for voltage scaling.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx51-zii-rdu1: limit usbh1 to full-speed
From: Shawn Guo @ 2018-05-20 12:36 UTC (permalink / raw)
To: Nikita Yushchenko
Cc: Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, Rob Herring,
Mark Rutland, Lucas Stach, Chris Healy, Andrey Smirnov,
Marco Franchi, linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <20180515084502.17148-1-nikita.yoush@cogentembedded.com>
On Tue, May 15, 2018 at 11:45:02AM +0300, Nikita Yushchenko wrote:
> On RDU1, imx51 usbh1 interface is either not used, or used via external
> block that breaks USB2 signalling.
>
> To keep things working if high-speed device gets connected to that
> block, use ChipIdea feature to limit port to full speed.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v2] ARM: dts: imx6/7: Remove unit-address from anatop regulators
From: Shawn Guo @ 2018-05-20 12:23 UTC (permalink / raw)
To: Fabio Estevam; +Cc: Fabio Estevam, devicetree, robh+dt, linux-arm-kernel
In-Reply-To: <1526304714-23821-1-git-send-email-festevam@gmail.com>
On Mon, May 14, 2018 at 10:31:54AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
>
> Remove unit-address and reg property from anatop regulators to fix
> the following DTC warnings with W=1:
>
> arch/arm/boot/dts/imx6dl-apf6dev.dtb: Warning (unique_unit_address): /soc/aips-bus@2000000/anatop@20c8000/regulator-vddcore@20c8140: duplicate unit-address (also used in node /soc/aips-bus@2000000/anatop@20c8000/regulator-vddpu@20c8140)
> arch/arm/boot/dts/imx6dl-apf6dev.dtb: Warning (unique_unit_address): /soc/aips-bus@2000000/anatop@20c8000/regulator-vddcore@20c8140: duplicate unit-address (also used in node /soc/aips-bus@2000000/anatop@20c8000/regulator-vddsoc@20c8140)
> arch/arm/boot/dts/imx6dl-apf6dev.dtb: Warning (unique_unit_address): /soc/aips-bus@2000000/anatop@20c8000/regulator-vddpu@20c8140: duplicate unit-address (also used in node /soc/aips-bus@2000000/anatop@20c8000/regulator-vddsoc@20c8140)
>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v4 12/12] staging: iio: ad2s1200: Move driver out of staging
From: Jonathan Cameron @ 2018-05-20 11:17 UTC (permalink / raw)
To: David Veenstra
Cc: devel, devicetree, lars, Michael.Hennerich, linux-iio, robh+dt,
pmeerw, knaack.h
In-Reply-To: <fb7eafef53d6766a49c978be98d71ecfecd8d392.1526667118.git.davidjulianveenstra@gmail.com>
On Fri, 18 May 2018 20:23:40 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:
> Move the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital
> converter out of staging, into mainline iio subsystems.
>
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Some totally trivial rubbish inline that I noticed.
Doesn't need fixing really, I just might as well point it out whilst
looking at the rest.
Anyhow, the device tree bindings are outstanding, but only for reasons
of the formatting of the binding doc rather than real issues. Please
follow through on that with a new version and I'd like to see an Ack
from Rob Herring for that.
Otherwise, looks good to me. Thanks for you hard work on this.
Ideally we'd get it tested. I do have a possibly working resolver now
so I might get to this in a few months. If we find stuff then we can
fix it up outside staging just fine. Seems fairly unlikely given
how simple the code ended up after you had cleaned it up but who knows.
Anyhow, applied to the togreg branch of iio.git and pushed out as
testing for the autobuilders to play with it.
Thanks,
Jonathan
> ---
> drivers/iio/Kconfig | 1 +
> drivers/iio/Makefile | 1 +
> drivers/iio/resolver/Kconfig | 17 ++
> drivers/iio/resolver/Makefile | 5 +
> drivers/iio/resolver/ad2s1200.c | 210 ++++++++++++++++++++++++
> drivers/staging/iio/resolver/Kconfig | 12 --
> drivers/staging/iio/resolver/Makefile | 1 -
> drivers/staging/iio/resolver/ad2s1200.c | 210 ------------------------
> 8 files changed, 234 insertions(+), 223 deletions(-)
> create mode 100644 drivers/iio/resolver/Kconfig
> create mode 100644 drivers/iio/resolver/Makefile
> create mode 100644 drivers/iio/resolver/ad2s1200.c
> delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c
>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index d69e85a8bdc3..d08aeb41cd07 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -93,6 +93,7 @@ source "drivers/iio/potentiometer/Kconfig"
> source "drivers/iio/potentiostat/Kconfig"
> source "drivers/iio/pressure/Kconfig"
> source "drivers/iio/proximity/Kconfig"
> +source "drivers/iio/resolver/Kconfig"
> source "drivers/iio/temperature/Kconfig"
>
> endif # IIO
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index d8cba9c229c0..cb5993251381 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -36,5 +36,6 @@ obj-y += potentiometer/
> obj-y += potentiostat/
> obj-y += pressure/
> obj-y += proximity/
> +obj-y += resolver/
> obj-y += temperature/
> obj-y += trigger/
> diff --git a/drivers/iio/resolver/Kconfig b/drivers/iio/resolver/Kconfig
> new file mode 100644
> index 000000000000..2ced9f22aa70
> --- /dev/null
> +++ b/drivers/iio/resolver/Kconfig
> @@ -0,0 +1,17 @@
> +#
> +# Resolver/Synchro drivers
> +#
> +menu "Resolver to digital converters"
> +
> +config AD2S1200
> + tristate "Analog Devices ad2s1200/ad2s1205 driver"
> + depends on SPI
> + depends on GPIOLIB || COMPILE_TEST
> + help
> + Say yes here to build support for Analog Devices spi resolver
> + to digital converters, ad2s1200 and ad2s1205, provides direct access
> + via sysfs.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad2s1200.
> +endmenu
> diff --git a/drivers/iio/resolver/Makefile b/drivers/iio/resolver/Makefile
> new file mode 100644
> index 000000000000..4e1dccae07e7
> --- /dev/null
> +++ b/drivers/iio/resolver/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for Resolver/Synchro drivers
> +#
> +
> +obj-$(CONFIG_AD2S1200) += ad2s1200.o
> diff --git a/drivers/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
> new file mode 100644
> index 000000000000..28e618af9939
> --- /dev/null
> +++ b/drivers/iio/resolver/ad2s1200.c
> @@ -0,0 +1,210 @@
> +/*
> + * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
> + * AD2S1200/1205
> + *
> + * Copyright (c) 2018-2018 David Veenstra <davidjulianveenstra@gmail.com>
> + * Copyright (c) 2010-2010 Analog Devices Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define DRV_NAME "ad2s1200"
This feels a little pointless given the fact it is only used in
one place and is hardly a magic value (requiring a define to give it
a useful name.).
Meh, doesn't matter much though
> +
> +/* input clock on serial interface */
> +#define AD2S1200_HZ 8192000
> +/* clock period in nano second */
> +#define AD2S1200_TSCLK (1000000000 / AD2S1200_HZ)
> +
> +/**
> + * struct ad2s1200_state - driver instance specific data.
> + * @lock: protects both the GPIO pins and the rx buffer.
> + * @sdev: spi device.
> + * @sample: GPIO pin SAMPLE.
> + * @rdvel: GPIO pin RDVEL.
> + * @rx: buffer for spi transfers.
> + */
> +struct ad2s1200_state {
> + struct mutex lock;
> + struct spi_device *sdev;
> + struct gpio_desc *sample;
> + struct gpio_desc *rdvel;
> + __be16 rx ____cacheline_aligned;
> +};
> +
> +static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long m)
> +{
> + struct ad2s1200_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_ANGL:
> + /* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> + *val = 0;
> + *val2 = 1534355;
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_ANGL_VEL:
> + /* 2 * Pi ~= 6.283185 */
> + *val = 6;
> + *val2 = 283185;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> + break;
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> + gpiod_set_value(st->sample, 0);
> +
> + /* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
> + udelay(1);
> + gpiod_set_value(st->sample, 1);
> + gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
> +
> + ret = spi_read(st->sdev, &st->rx, 2);
> + if (ret < 0) {
> + mutex_unlock(&st->lock);
> + return ret;
> + }
> +
> + switch (chan->type) {
> + case IIO_ANGL:
> + *val = be16_to_cpup(&st->rx) >> 4;
> + break;
> + case IIO_ANGL_VEL:
> + *val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> + break;
> + default:
> + mutex_unlock(&st->lock);
> + return -EINVAL;
> + }
> +
> + /* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> + udelay(1);
> + mutex_unlock(&st->lock);
> +
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
Totally trivial, but could move into the above switch
and save a couple of lines of code.. Only get here from the default
path after all.
> +}
> +
> +static const struct iio_chan_spec ad2s1200_channels[] = {
> + {
> + .type = IIO_ANGL,
> + .indexed = 1,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> + }, {
> + .type = IIO_ANGL_VEL,
> + .indexed = 1,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> + }
> +};
> +
> +static const struct iio_info ad2s1200_info = {
> + .read_raw = ad2s1200_read_raw,
> +};
> +
> +static int ad2s1200_probe(struct spi_device *spi)
> +{
> + struct ad2s1200_state *st;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + spi_set_drvdata(spi, indio_dev);
> + st = iio_priv(indio_dev);
> + mutex_init(&st->lock);
> + st->sdev = spi;
> +
> + st->sample = devm_gpiod_get(&spi->dev, "adi,sample", GPIOD_OUT_LOW);
> + if (IS_ERR(st->sample)) {
> + dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
> + PTR_ERR(st->sample));
> + return PTR_ERR(st->sample);
> + }
> +
> + st->rdvel = devm_gpiod_get(&spi->dev, "adi,rdvel", GPIOD_OUT_LOW);
> + if (IS_ERR(st->rdvel)) {
> + dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
> + PTR_ERR(st->rdvel));
> + return PTR_ERR(st->rdvel);
> + }
> +
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->info = &ad2s1200_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = ad2s1200_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ad2s1200_channels);
> + indio_dev->name = spi_get_device_id(spi)->name;
> +
> + spi->max_speed_hz = AD2S1200_HZ;
> + spi->mode = SPI_MODE_3;
> + ret = spi_setup(spi);
> +
> + if (ret < 0) {
> + dev_err(&spi->dev, "spi_setup failed!\n");
> + return ret;
> + }
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct of_device_id ad2s1200_of_match[] = {
> + { .compatible = "adi,ad2s1200", },
> + { .compatible = "adi,ad2s1205", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad2s1200_of_match);
> +
> +static const struct spi_device_id ad2s1200_id[] = {
> + { "ad2s1200" },
> + { "ad2s1205" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, ad2s1200_id);
> +
> +static struct spi_driver ad2s1200_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .of_match_table = of_match_ptr(ad2s1200_of_match),
> + },
> + .probe = ad2s1200_probe,
> + .id_table = ad2s1200_id,
> +};
> +module_spi_driver(ad2s1200_driver);
> +
> +MODULE_AUTHOR("David Veenstra <davidjulianveenstra@gmail.com>");
> +MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
> +MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
> +MODULE_LICENSE("GPL v2");
...
^ permalink raw reply
* Re: [PATCH v4 11/12] staging: iio: ad2s1200: Add copyright
From: Jonathan Cameron @ 2018-05-20 11:11 UTC (permalink / raw)
To: David Veenstra
Cc: devel, devicetree, lars, Michael.Hennerich, linux-iio, robh+dt,
pmeerw, knaack.h
In-Reply-To: <8e02a6b4147ebceee1412ee521b4f4be6886cdaa.1526667118.git.davidjulianveenstra@gmail.com>
On Fri, 18 May 2018 20:23:25 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:
> Add David Veenstra as a copyright holders and as an author,
> for all of the staging clean ups of the ad2s1200 driver.
>
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.
Thanks,
Jonathan
> ---
> Changes in v4:
> - Introduced in this version.
>
> drivers/staging/iio/resolver/ad2s1200.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 10d6d79dce79..28e618af9939 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -2,6 +2,7 @@
> * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
> * AD2S1200/1205
> *
> + * Copyright (c) 2018-2018 David Veenstra <davidjulianveenstra@gmail.com>
> * Copyright (c) 2010-2010 Analog Devices Inc.
> *
> * This program is free software; you can redistribute it and/or modify
> @@ -203,6 +204,7 @@ static struct spi_driver ad2s1200_driver = {
> };
> module_spi_driver(ad2s1200_driver);
>
> +MODULE_AUTHOR("David Veenstra <davidjulianveenstra@gmail.com>");
> MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
> MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
> MODULE_LICENSE("GPL v2");
^ permalink raw reply
* Re: [PATCH v4 10/12] staging: iio: ad2s1200: Add scaling factor for angle channel
From: Jonathan Cameron @ 2018-05-20 11:10 UTC (permalink / raw)
To: David Veenstra
Cc: devel, devicetree, lars, Michael.Hennerich, linux-iio, robh+dt,
pmeerw, knaack.h
In-Reply-To: <559356a5b2467bd871e12460faa4edb3c80980ec.1526667118.git.davidjulianveenstra@gmail.com>
On Fri, 18 May 2018 20:23:14 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:
> A scaling factor of approximately 2 * Pi / (2^12 -1) is added,
> to scale the 12-bits angular position to radians.
>
> A return type of IIO_VAL_INT_PLUS_NANO is used, so that the scale of
> both the angle channel and angular velocity channel has 7 significant
> digits.
>
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Applied
Thanks,
Jonathan
> ---
> drivers/staging/iio/resolver/ad2s1200.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 7b8af558e921..10d6d79dce79 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -58,6 +58,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> switch (m) {
> case IIO_CHAN_INFO_SCALE:
> switch (chan->type) {
> + case IIO_ANGL:
> + /* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> + *val = 0;
> + *val2 = 1534355;
> + return IIO_VAL_INT_PLUS_NANO;
> case IIO_ANGL_VEL:
> /* 2 * Pi ~= 6.283185 */
> *val = 6;
> @@ -112,6 +117,7 @@ static const struct iio_chan_spec ad2s1200_channels[] = {
> .indexed = 1,
> .channel = 0,
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> }, {
> .type = IIO_ANGL_VEL,
> .indexed = 1,
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox