[PATCH v3 0/2] Add support for ADI TDD Engine

[PATCH v3 0/2] Add support for ADI TDD Engine

[Eliza Balas]

Add support for Analog Devices TDD Engine.
This driver is created for a specific FPGA Core named
TDD Controller (Time-Division Duplex).
We choose the sysfs interface so that the users can access the device registers
directly, in an easy way, without using a complex interface. If there will be
other future revisions of the TDD FPGA Core, the register space will remain
compatible, so we don't break the current functionality of the driver.

Even though the device attributes might resamble a bit with the ones from
the iio subsystem, the device also contains a lot of attributes which are not
part of the iio subsystem.
We do not want to confuse this device with an IIO device, so we concluded that
the driver should reside in the misc subsystem.

V2 -> V3:
- change from dual-license to single license driver
- remove version number from the compatible string
- the driver should reside in the misc subsystem

Eliza Balas (2):
  dt-bindings: misc: adi,axi-tdd: Add device-tree binding for TDD engine
  drivers: misc: adi-axi-tdd: Add TDD engine

 .../sysfs-bus-platform-drivers-adi-axi-tdd    | 156 ++++
 .../devicetree/bindings/misc/adi,axi-tdd.yaml |  65 ++
 MAINTAINERS                                   |   9 +
 drivers/misc/Kconfig                          |  10 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/adi-axi-tdd.c                    | 780 ++++++++++++++++++
 6 files changed, 1021 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd
 create mode 100644 Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
 create mode 100644 drivers/misc/adi-axi-tdd.c

-- 
2.25.1

[PATCH v3 1/2] dt-bindings: misc: adi,axi-tdd: Add device-tree binding for TDD engine

[Eliza Balas]

Add device tree documentation for the AXI TDD Core.
The generic TDD controller is in essence a waveform generator
capable of addressing RF applications which require Time Division
Duplexing, as well as controlling other modules of general
applications through its dedicated 32 channel outputs.

Signed-off-by: Eliza Balas <eliza.balas@analog.com>
---
 .../devicetree/bindings/misc/adi,axi-tdd.yaml | 65 +++++++++++++++++++
 MAINTAINERS                                   |  7 ++
 2 files changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml

diff --git a/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml b/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
new file mode 100644
index 000000000000..4449d9abf46e
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2023 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/adi,axi-tdd.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AXI TDD Core
+
+maintainers:
+  - Eliza Balas <eliza.balas@analog.com>
+
+description: |
+  The TDD controller is a waveform generator capable of addressing RF
+  applications which require Time Division Duplexing, as well as controlling
+  other modules of general applications through its dedicated 32 channel
+  outputs. It solves the synchronization issue when transmitting and receiving
+  multiple frames of data through multiple buffers.
+  The TDD IP core is part of the Analog Devices hdl reference designs and has
+  the following features:
+    * Up to 32 independent output channels
+    * Start/stop time values per channel
+    * Enable and polarity bit values per channel
+    * 32 bit-max internal reference counter
+    * Initial startup delay before waveform generation
+    * Configurable frame length and number of frames per burst
+    * 3 sources of synchronization: external, internal and software generated
+  For more information see the wiki:
+  https://wiki.analog.com/resources/fpga/docs/axi_tdd
+
+properties:
+  compatible:
+    enum:
+      - adi,axi-tdd
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: System clock
+      - description: TDD Core clock
+
+  clock-names:
+    items:
+      - const: s_axi_aclk
+      - const: intf_clk
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    tdd@84a00000 {
+        compatible = "adi,axi-tdd";
+        reg = <0x84a00000 0x10000>;
+        clocks = <&zynqmp_clk_PL0_REF>, <&zynqmp_clk_PL1_REF>;
+        clock-names = "s_axi_aclk", "intf_clk";
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 59831c69a275..13a1e1990c19 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1344,6 +1344,13 @@ S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	drivers/dma/dma-axi-dmac.c
 
+ANALOG DEVICES INC GENERIC TDD ENGINE DRIVER
+M:	Eliza Balas <eliza.balas@analog.com>
+S:	Supported
+W:	http://wiki.analog.com/resources/fpga/docs/axi_tdd
+W:	http://ez.analog.com/linux-software-drivers/
+F:	Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
+
 ANALOG DEVICES INC IIO DRIVERS
 M:	Lars-Peter Clausen <lars@metafoo.de>
 M:	Michael Hennerich <Michael.Hennerich@analog.com>
-- 
2.25.1

Re: [PATCH v3 1/2] dt-bindings: misc: adi,axi-tdd: Add device-tree binding for TDD engine

[Rob Herring]

On Thu, Oct 19, 2023 at 03:56:45PM +0300, Eliza Balas wrote:
> Add device tree documentation for the AXI TDD Core.
> The generic TDD controller is in essence a waveform generator
> capable of addressing RF applications which require Time Division
> Duplexing, as well as controlling other modules of general
> applications through its dedicated 32 channel outputs.
> 
> Signed-off-by: Eliza Balas <eliza.balas@analog.com>
> ---
>  .../devicetree/bindings/misc/adi,axi-tdd.yaml | 65 +++++++++++++++++++
>  MAINTAINERS                                   |  7 ++
>  2 files changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml b/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
> new file mode 100644
> index 000000000000..4449d9abf46e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2023 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/adi,axi-tdd.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AXI TDD Core
> +
> +maintainers:
> +  - Eliza Balas <eliza.balas@analog.com>
> +
> +description: |
> +  The TDD controller is a waveform generator capable of addressing RF
> +  applications which require Time Division Duplexing, as well as controlling
> +  other modules of general applications through its dedicated 32 channel
> +  outputs. It solves the synchronization issue when transmitting and receiving
> +  multiple frames of data through multiple buffers.
> +  The TDD IP core is part of the Analog Devices hdl reference designs and has
> +  the following features:
> +    * Up to 32 independent output channels
> +    * Start/stop time values per channel
> +    * Enable and polarity bit values per channel
> +    * 32 bit-max internal reference counter
> +    * Initial startup delay before waveform generation
> +    * Configurable frame length and number of frames per burst
> +    * 3 sources of synchronization: external, internal and software generated
> +  For more information see the wiki:
> +  https://wiki.analog.com/resources/fpga/docs/axi_tdd
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,axi-tdd

I don't understand where the version number went. Now my question is is 
there only one version of this (ever). It's not specific enough unless 
there's a version register that can be relied on.

This patch needs to answer any questions raised in previous versions. 
Assume the reviewers don't remember your patch and will ask the same 
questions again otherwise.

Rob

RE: [PATCH v3 1/2] dt-bindings: misc: adi,axi-tdd: Add device-tree binding for TDD engine

[Balas, Eliza]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, October 24, 2023 22:10
> To: Balas, Eliza <Eliza.Balas@analog.com>
> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor
> Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan Cvetic <dragan.cvetic@amd.com>; Arnd
> Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Subject: Re: [PATCH v3 1/2] dt-bindings: misc: adi,axi-tdd: Add device-tree binding for TDD engine
> 
> [External]
> 
> On Thu, Oct 19, 2023 at 03:56:45PM +0300, Eliza Balas wrote:
> > Add device tree documentation for the AXI TDD Core.
> > The generic TDD controller is in essence a waveform generator
> > capable of addressing RF applications which require Time Division
> > Duplexing, as well as controlling other modules of general
> > applications through its dedicated 32 channel outputs.
> >
> > Signed-off-by: Eliza Balas <eliza.balas@analog.com>
> > ---
> >  .../devicetree/bindings/misc/adi,axi-tdd.yaml | 65 +++++++++++++++++++
> >  MAINTAINERS                                   |  7 ++
> >  2 files changed, 72 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml b/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
> > new file mode 100644
> > index 000000000000..4449d9abf46e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
> > @@ -0,0 +1,65 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2023 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: https://urldefense.com/v3/__http://devicetree.org/schemas/misc/adi,axi-
> tdd.yaml*__;Iw!!A3Ni8CS0y2Y!4FubpNBZ9iITDA4tWD49qCUp7AqEs72W2OQmpSkLyK2lCiWNWSbXGS6MGhJSdKMVza7CV5IRsVxNlA$
> > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!4FubpNBZ9iITDA4tWD49qCUp7AqEs72W2OQmpSkLyK2lCiWNWSbXGS6MGhJSdKMVza7CV5
> J20y6r-A$
> > +
> > +title: Analog Devices AXI TDD Core
> > +
> > +maintainers:
> > +  - Eliza Balas <eliza.balas@analog.com>
> > +
> > +description: |
> > +  The TDD controller is a waveform generator capable of addressing RF
> > +  applications which require Time Division Duplexing, as well as controlling
> > +  other modules of general applications through its dedicated 32 channel
> > +  outputs. It solves the synchronization issue when transmitting and receiving
> > +  multiple frames of data through multiple buffers.
> > +  The TDD IP core is part of the Analog Devices hdl reference designs and has
> > +  the following features:
> > +    * Up to 32 independent output channels
> > +    * Start/stop time values per channel
> > +    * Enable and polarity bit values per channel
> > +    * 32 bit-max internal reference counter
> > +    * Initial startup delay before waveform generation
> > +    * Configurable frame length and number of frames per burst
> > +    * 3 sources of synchronization: external, internal and software generated
> > +  For more information see the wiki:
> > +  https://wiki.analog.com/resources/fpga/docs/axi_tdd
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,axi-tdd
> 
> I don't understand where the version number went. Now my question is is
> there only one version of this (ever). It's not specific enough unless
> there's a version register that can be relied on.
> 
> This patch needs to answer any questions raised in previous versions.
> Assume the reviewers don't remember your patch and will ask the same
> questions again otherwise.
> 
> Rob

This is the only TDD driver and I use the information from
the version register to determine the version.
That is why I had to remove the version number from 
the compatible string.

Thank you,
Eliza

[PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Eliza Balas]

This patch introduces the driver for the new ADI TDD engine HDL.
The generic TDD controller is in essence a waveform generator
capable of addressing RF applications which require Time Division
Duplexing, as well as controlling other modules of general
applications through its dedicated 32 channel outputs.

Signed-off-by: Eliza Balas <eliza.balas@analog.com>
---
 .../sysfs-bus-platform-drivers-adi-axi-tdd    | 156 ++++
 MAINTAINERS                                   |   2 +
 drivers/misc/Kconfig                          |  10 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/adi-axi-tdd.c                    | 780 ++++++++++++++++++
 5 files changed, 949 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd
 create mode 100644 drivers/misc/adi-axi-tdd.c

diff --git a/Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd b/Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd
new file mode 100644
index 000000000000..e7f58ec41452
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd
@@ -0,0 +1,156 @@
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/burst_count
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the number of TDD frames per burst.
+                If set to 0x0 and the TDD module is enabled, then the controller
+                operates in TDD mode as long as the ENABLE property is set.
+                If set to a non-zero value, the controller
+                operates for the set number of frames and then stops.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/core_id
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Displays the value of the ID configuration parameter.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/enable
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to enable or disable the TDD module.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/frame_length_ms
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the length of the transmission frame,
+                defined in milliseconds.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/frame_length_raw
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the length of the transmission frame,
+                defined in clock cycles of the input clock.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/internal_sync_period_ms
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the period of the internal sync generator,
+                defined in milliseconds.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/internal_sync_period_raw
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the period of the internal sync generator,
+                defined in clock cycles of the input clock.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/magic
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Displays the code name of the TDD module for identification.
+                The value is 0x5444444E ('T', 'D', 'D', 'N').
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/out_channel<n>_enable
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to enable or disable the output channel CH<n>.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/out_channel<n>_off_ms
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the offset from the beginning of a new
+                frame when channel CH<n> is reset, defined in milliseconds.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/out_channel<n>_off_raw
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the offset from the beginning of a new
+                frame when channel CH<n> is reset, defined in clock cycles
+                of the input clock.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/out_channel<n>_on_ms
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the offset from the beginning of a new
+                frame when channel CH<n> is set, defined in milliseconds.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/out_channel<n>_on_raw
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the offset from the beginning of a new
+                frame when channel CH<n> is set, defined in clock cycles
+                of the input clock.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/out_channel<n>_polarity
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the polarity of the output channel CH<n>.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/scratch
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to write and read the scratch register.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/startup_delay_ms
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the initial delay before the first frame,
+                defined in milliseconds.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/startup_delay_raw
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to set the initial delay before the first frame,
+                defined in clock cycles of the input clock.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/state
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Displays the current state of the peripheral FSM.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/sync_external
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to enable or disable the external sync trigger.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/sync_internal
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to enable or disable the internal sync trigger.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/sync_reset
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to reset the internal counter when receiving a
+                sync event.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/sync_soft
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Allows the user to trigger one software sync pulse.
+
+What:           /sys/bus/platform/drivers/adi-axi-tdd/*/version
+Date:           November 2023
+KernelVersion:  6.7
+Contact:        Eliza Balas <eliza.balas@analog.com>
+Description:    Displays the version of the peripheral. Follows semantic
+                versioning. Current version is 2.00.61.
diff --git a/MAINTAINERS b/MAINTAINERS
index 13a1e1990c19..51b87d90d3bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1349,7 +1349,9 @@ M:	Eliza Balas <eliza.balas@analog.com>
 S:	Supported
 W:	http://wiki.analog.com/resources/fpga/docs/axi_tdd
 W:	http://ez.analog.com/linux-software-drivers/
+F:	Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd
 F:	Documentation/devicetree/bindings/misc/adi,axi-tdd.yaml
+F:	drivers/misc/adi-axi-tdd.c
 
 ANALOG DEVICES INC IIO DRIVERS
 M:	Lars-Peter Clausen <lars@metafoo.de>
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index cadd4a820c03..45074a93fa55 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -50,6 +50,16 @@ config AD525X_DPOT_SPI
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad525x_dpot-spi.
 
+config ADI_AXI_TDD
+	tristate "Analog Devices TDD Engine support"
+	depends on HAS_IOMEM
+	select REGMAP_MMIO
+	help
+	  The ADI AXI TDD core is the reworked and generic TDD engine which
+	  was developed for general use in Analog Devices HDL projects. Unlike
+	  the previous TDD engine, this core can only be used standalone mode,
+	  and is not embedded into other devices.
+
 config DUMMY_IRQ
 	tristate "Dummy IRQ handler"
 	help
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index f2a4d1ff65d4..d97afe1eeba5 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_IBMVMC)		+= ibmvmc.o
 obj-$(CONFIG_AD525X_DPOT)	+= ad525x_dpot.o
 obj-$(CONFIG_AD525X_DPOT_I2C)	+= ad525x_dpot-i2c.o
 obj-$(CONFIG_AD525X_DPOT_SPI)	+= ad525x_dpot-spi.o
+obj-$(CONFIG_ADI_AXI_TDD)	+= adi-axi-tdd.o
 obj-$(CONFIG_ATMEL_SSC)		+= atmel-ssc.o
 obj-$(CONFIG_DUMMY_IRQ)		+= dummy-irq.o
 obj-$(CONFIG_ICS932S401)	+= ics932s401.o
diff --git a/drivers/misc/adi-axi-tdd.c b/drivers/misc/adi-axi-tdd.c
new file mode 100644
index 000000000000..94ad0f805c55
--- /dev/null
+++ b/drivers/misc/adi-axi-tdd.c
@@ -0,0 +1,780 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TDD HDL CORE driver
+ *
+ * Copyright 2023 Analog Devices Inc.
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/fpga/adi-axi-common.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+
+/* Register Map */
+#define ADI_REG_TDD_VERSION                 0x0000
+#define ADI_REG_TDD_PERIPHERAL_ID           0x0004
+#define ADI_REG_TDD_SCRATCH                 0x0008
+#define ADI_REG_TDD_IDENTIFICATION          0x000c
+#define ADI_REG_TDD_INTERFACE_DESCRIPTION   0x0010
+#define ADI_REG_TDD_DEFAULT_POLARITY        0x0014
+#define ADI_REG_TDD_CONTROL                 0x0040
+#define ADI_REG_TDD_CHANNEL_ENABLE          0x0044
+#define ADI_REG_TDD_CHANNEL_POLARITY        0x0048
+#define ADI_REG_TDD_BURST_COUNT             0x004c
+#define ADI_REG_TDD_STARTUP_DELAY           0x0050
+#define ADI_REG_TDD_FRAME_LENGTH            0x0054
+#define ADI_REG_TDD_SYNC_COUNTER_LOW        0x0058
+#define ADI_REG_TDD_SYNC_COUNTER_HIGH       0x005c
+#define ADI_REG_TDD_STATUS                  0x0060
+#define ADI_REG_TDD_CHANNEL_BASE            0x0080
+
+/* Identification Register */
+#define ADI_TDD_MAGIC                       0x5444444E
+
+/* Interface Description Register */
+#define ADI_TDD_SYNC_COUNT_WIDTH            GENMASK(30, 24)
+#define ADI_TDD_BURST_COUNT_WIDTH           GENMASK(21, 16)
+#define ADI_TDD_REG_WIDTH                   GENMASK(13, 8)
+#define ADI_TDD_SYNC_EXTERNAL_CDC           BIT(7)
+#define ADI_TDD_SYNC_EXTERNAL               BIT(6)
+#define ADI_TDD_SYNC_INTERNAL               BIT(5)
+#define ADI_TDD_CHANNEL_COUNT               GENMASK(4, 0)
+
+/* Control Register */
+#define ADI_TDD_SYNC_SOFT                   BIT(4)
+#define ADI_TDD_SYNC_EXT                    BIT(3)
+#define ADI_TDD_SYNC_INT                    BIT(2)
+#define ADI_TDD_SYNC_RST                    BIT(1)
+#define ADI_TDD_ENABLE                      BIT(0)
+
+/* Channel Definitions */
+#define ADI_TDD_CHANNEL_OFFSET              0x0008
+#define ADI_TDD_CHANNEL_ON                  0x0000
+#define ADI_TDD_CHANNEL_OFF                 0x0004
+
+#define ADI_REG_TDD_CHANNEL(c, o)           \
+	(ADI_REG_TDD_CHANNEL_BASE + ADI_TDD_CHANNEL_OFFSET * (c) + (o))
+
+enum adi_axi_tdd_attribute_id {
+	ADI_TDD_ATTR_VERSION,
+	ADI_TDD_ATTR_CORE_ID,
+	ADI_TDD_ATTR_SCRATCH,
+	ADI_TDD_ATTR_MAGIC,
+
+	ADI_TDD_ATTR_SYNC_SOFT,
+	ADI_TDD_ATTR_SYNC_EXT,
+	ADI_TDD_ATTR_SYNC_INT,
+	ADI_TDD_ATTR_SYNC_RST,
+	ADI_TDD_ATTR_ENABLE,
+
+	ADI_TDD_ATTR_BURST_COUNT,
+	ADI_TDD_ATTR_STARTUP_DELAY_RAW,
+	ADI_TDD_ATTR_STARTUP_DELAY_MS,
+	ADI_TDD_ATTR_FRAME_LENGTH_RAW,
+	ADI_TDD_ATTR_FRAME_LENGTH_MS,
+	ADI_TDD_ATTR_INTERNAL_SYNC_PERIOD_RAW,
+	ADI_TDD_ATTR_INTERNAL_SYNC_PERIOD_MS,
+
+	ADI_TDD_ATTR_STATE,
+
+	ADI_TDD_ATTR_CHANNEL_ENABLE,
+	ADI_TDD_ATTR_CHANNEL_POLARITY,
+	ADI_TDD_ATTR_CHANNEL_ON_RAW,
+	ADI_TDD_ATTR_CHANNEL_OFF_RAW,
+	ADI_TDD_ATTR_CHANNEL_ON_MS,
+	ADI_TDD_ATTR_CHANNEL_OFF_MS,
+};
+
+struct adi_axi_tdd_clk {
+	struct notifier_block nb;
+	unsigned long rate;
+	struct clk *clk;
+
+};
+
+struct adi_axi_tdd_attribute {
+	enum adi_axi_tdd_attribute_id id;
+	struct device_attribute attr;
+	u32 channel;
+	u8 name[32];
+};
+
+#define to_tdd_attribute(x) container_of(x, struct adi_axi_tdd_attribute, attr)
+
+/**
+ * struct adi_axi_tdd_state - Driver state information for the TDD CORE
+ * @clk: Interface clock definition. Used to translate ms into cycle counts
+ * @base: Device register base address in memory
+ * @regs: Device memory-mapped region regmap
+ * @sync_count_width: Bit width of the internal synchronization counter, <= 64
+ * @sync_count_mask: Bit mask of sync counter
+ * @burst_count_width: Bit width of the burst counter, <= 32
+ * @burst_count_mask: Bit mask of the burst counter
+ * @reg_width: Timing register bit width, <= 32
+ * @reg_mask: Timing register bit mask
+ * @sync_external_cdc: Whether the external sync input is synchronized into the main clock domain
+ * @sync_external: Whether external synchronization support was enabled at synthesis time
+ * @sync_internal: Whether internal synchronization support was enabled at synthesis time
+ * @channel_count: Available channel count
+ * @enabled: Whether the TDD engine is currently enabled.
+ *	     Note: Most configuration registers cannot be changed while the TDD core is enabled.
+ */
+struct adi_axi_tdd_state {
+	struct adi_axi_tdd_clk clk;
+	void __iomem *base;
+	struct regmap *regs;
+	u32 sync_count_width;
+	u64 sync_count_mask;
+	u32 burst_count_width;
+	u32 burst_count_mask;
+	u32 reg_width;
+	u32 reg_mask;
+	bool sync_external_cdc;
+	bool sync_external;
+	bool sync_internal;
+	u32 channel_count;
+	bool enabled;
+};
+
+static const struct regmap_config adi_axi_tdd_regmap_cfg = {
+	.name = "adi-axi-tdd",
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static int adi_axi_tdd_format_ms(struct adi_axi_tdd_state *st, u64 x, char *buf)
+{
+	u32 vals[2];
+	u64 t_ns;
+
+	t_ns = div_u64(x * 1000000000ULL, READ_ONCE(st->clk.rate));
+	vals[0] = div_u64_rem(t_ns, 1000000, &vals[1]);
+
+	return sysfs_emit(buf, "%d.%06u\n", vals[0], vals[1]);
+}
+
+static ssize_t adi_axi_tdd_show(struct device *dev,
+				struct device_attribute *dev_attr, char *buf)
+{
+	const struct adi_axi_tdd_attribute *attr = to_tdd_attribute(dev_attr);
+	struct adi_axi_tdd_state *st = dev_get_drvdata(dev);
+	u32 channel = attr->channel;
+	bool ms = false;
+	u64 data64;
+	u32 data;
+	int ret;
+
+	switch (attr->id) {
+	case ADI_TDD_ATTR_VERSION:
+		ret = regmap_read(st->regs, ADI_AXI_REG_VERSION, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%d.%.2d.%c\n",
+				 ADI_AXI_PCORE_VER_MAJOR(data),
+				 ADI_AXI_PCORE_VER_MINOR(data),
+				 ADI_AXI_PCORE_VER_PATCH(data));
+	case ADI_TDD_ATTR_CORE_ID:
+		ret = regmap_read(st->regs, ADI_REG_TDD_PERIPHERAL_ID, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%u\n", data);
+	case ADI_TDD_ATTR_SCRATCH:
+		ret = regmap_read(st->regs, ADI_REG_TDD_SCRATCH, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "0x%08x\n", data);
+	case ADI_TDD_ATTR_MAGIC:
+		ret = regmap_read(st->regs, ADI_REG_TDD_IDENTIFICATION, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "0x%08x\n", data);
+	case ADI_TDD_ATTR_SYNC_EXT:
+		ret = regmap_read(st->regs, ADI_REG_TDD_CONTROL, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%d\n", !!(data & ADI_TDD_SYNC_EXT));
+	case ADI_TDD_ATTR_SYNC_INT:
+		ret = regmap_read(st->regs, ADI_REG_TDD_CONTROL, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%d\n", !!(data & ADI_TDD_SYNC_INT));
+	case ADI_TDD_ATTR_SYNC_RST:
+		ret = regmap_read(st->regs, ADI_REG_TDD_CONTROL, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%d\n", !!(data & ADI_TDD_SYNC_RST));
+	case ADI_TDD_ATTR_ENABLE:
+		ret = regmap_read(st->regs, ADI_REG_TDD_CONTROL, &data);
+		if (ret)
+			return ret;
+		st->enabled = !!(data & ADI_TDD_ENABLE);
+		return sysfs_emit(buf, "%d\n", st->enabled);
+	case ADI_TDD_ATTR_BURST_COUNT:
+		ret = regmap_read(st->regs, ADI_REG_TDD_BURST_COUNT, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%u\n", data);
+	case ADI_TDD_ATTR_STARTUP_DELAY_RAW:
+		ret = regmap_read(st->regs, ADI_REG_TDD_STARTUP_DELAY, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%u\n", data);
+	case ADI_TDD_ATTR_STARTUP_DELAY_MS:
+		ret = regmap_read(st->regs, ADI_REG_TDD_STARTUP_DELAY, &data);
+		if (ret)
+			return ret;
+		return adi_axi_tdd_format_ms(st, data, buf);
+	case ADI_TDD_ATTR_FRAME_LENGTH_RAW:
+		ret = regmap_read(st->regs, ADI_REG_TDD_FRAME_LENGTH, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%u\n", data);
+	case ADI_TDD_ATTR_FRAME_LENGTH_MS:
+		ret = regmap_read(st->regs, ADI_REG_TDD_FRAME_LENGTH, &data);
+		if (ret)
+			return ret;
+		return adi_axi_tdd_format_ms(st, data, buf);
+	case ADI_TDD_ATTR_INTERNAL_SYNC_PERIOD_RAW:
+		ret = regmap_bulk_read(st->regs, ADI_REG_TDD_SYNC_COUNTER_LOW,
+				       &data64, 2);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%llu\n", data64);
+	case ADI_TDD_ATTR_INTERNAL_SYNC_PERIOD_MS:
+		ret = regmap_bulk_read(st->regs, ADI_REG_TDD_SYNC_COUNTER_LOW,
+				       &data64, 2);
+		if (ret)
+			return ret;
+		return adi_axi_tdd_format_ms(st, data64, buf);
+	case ADI_TDD_ATTR_STATE:
+		ret = regmap_read(st->regs, ADI_REG_TDD_STATUS, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%u\n", data);
+	case ADI_TDD_ATTR_CHANNEL_ENABLE:
+		ret = regmap_read(st->regs, ADI_REG_TDD_CHANNEL_ENABLE, &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%d\n", !!(BIT(channel) & data));
+	case ADI_TDD_ATTR_CHANNEL_POLARITY:
+		ret = regmap_read(st->regs, ADI_REG_TDD_CHANNEL_POLARITY,
+				  &data);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%d\n", !!(BIT(channel) & data));
+	case ADI_TDD_ATTR_CHANNEL_ON_MS:
+		ms = true;
+		fallthrough;
+	case ADI_TDD_ATTR_CHANNEL_ON_RAW:
+		ret = regmap_read(st->regs,
+				  ADI_REG_TDD_CHANNEL(channel,
+						      ADI_TDD_CHANNEL_ON),
+				  &data);
+		if (ret)
+			return ret;
+		if (ms)
+			return adi_axi_tdd_format_ms(st, data, buf);
+		return sysfs_emit(buf, "%u\n", data);
+	case ADI_TDD_ATTR_CHANNEL_OFF_MS:
+		ms = true;
+		fallthrough;
+	case ADI_TDD_ATTR_CHANNEL_OFF_RAW:
+		ret = regmap_read(st->regs,
+				  ADI_REG_TDD_CHANNEL(channel,
+						      ADI_TDD_CHANNEL_OFF),
+				  &data);
+		if (ret)
+			return ret;
+		if (ms)
+			return adi_axi_tdd_format_ms(st, data, buf);
+		return sysfs_emit(buf, "%u\n", data);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
+				const char *buf,
+				u64 *res)
+{
+	u64 clk_rate = READ_ONCE(st->clk.rate);
+	char *orig_str, *modf_str, *int_part, frac_part[7];
+	long ival, frac;
+	int ret;
+
+	orig_str = kstrdup(buf, GFP_KERNEL);
+	int_part = strsep(&orig_str, ".");
+	ret = kstrtol(int_part, 10, &ival);
+	if (ret || ival < 0)
+		return -EINVAL;
+	modf_str = strsep(&orig_str, ".");
+	if (modf_str) {
+		snprintf(frac_part, 7, "%s00000", modf_str);
+		ret = kstrtol(frac_part, 10, &frac);
+		if (ret)
+			return -EINVAL;
+	} else {
+		frac = 0;
+	}
+
+	*res = DIV_ROUND_CLOSEST_ULL((u64)ival * clk_rate, 1000)
+		+ DIV_ROUND_CLOSEST_ULL((u64)frac * clk_rate, 1000000000);
+
+	kfree(orig_str);
+
+	return ret;
+}
+
+static int adi_axi_tdd_write_regs(const struct adi_axi_tdd_attribute *attr,
+				  struct adi_axi_tdd_state *st,
+				  const char *buf)
+{
+	u32 channel = attr->channel;
+	u64 data64;
+	u32 data;
+	int ret;
+
+	switch (attr->id) {
+	case ADI_TDD_ATTR_SCRATCH:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_write(st->regs, ADI_REG_TDD_SCRATCH, data);
+	case ADI_TDD_ATTR_SYNC_SOFT:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_update_bits_base(st->regs, ADI_REG_TDD_CONTROL,
+					       ADI_TDD_SYNC_SOFT,
+					       ADI_TDD_SYNC_SOFT * !!data,
+					       NULL, false, false);
+	case ADI_TDD_ATTR_SYNC_EXT:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_update_bits_base(st->regs, ADI_REG_TDD_CONTROL,
+					       ADI_TDD_SYNC_EXT,
+					       ADI_TDD_SYNC_EXT * !!data,
+					       NULL, false, false);
+	case ADI_TDD_ATTR_SYNC_INT:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_update_bits_base(st->regs, ADI_REG_TDD_CONTROL,
+					       ADI_TDD_SYNC_INT,
+					       ADI_TDD_SYNC_INT * !!data,
+					       NULL, false, false);
+	case ADI_TDD_ATTR_SYNC_RST:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_update_bits_base(st->regs, ADI_REG_TDD_CONTROL,
+					       ADI_TDD_SYNC_RST,
+					       ADI_TDD_SYNC_RST * !!data,
+					       NULL, false, false);
+	case ADI_TDD_ATTR_ENABLE:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_update_bits_base(st->regs, ADI_REG_TDD_CONTROL,
+					       ADI_TDD_ENABLE,
+					       ADI_TDD_ENABLE * !!data,
+					       NULL, false, false);
+	case ADI_TDD_ATTR_BURST_COUNT:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_write(st->regs, ADI_REG_TDD_BURST_COUNT, data);
+	case ADI_TDD_ATTR_STARTUP_DELAY_RAW:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_write(st->regs, ADI_REG_TDD_STARTUP_DELAY, data);
+	case ADI_TDD_ATTR_STARTUP_DELAY_MS:
+		ret = adi_axi_tdd_parse_ms(st, buf, &data64);
+		if (ret)
+			return ret;
+		if (FIELD_GET(GENMASK_ULL(63, 32), data64))
+			return -EINVAL;
+		return regmap_write(st->regs, ADI_REG_TDD_STARTUP_DELAY,
+				    (u32)data64);
+	case ADI_TDD_ATTR_FRAME_LENGTH_RAW:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_write(st->regs, ADI_REG_TDD_FRAME_LENGTH, data);
+	case ADI_TDD_ATTR_FRAME_LENGTH_MS:
+		ret = adi_axi_tdd_parse_ms(st, buf, &data64);
+		if (ret)
+			return ret;
+		if (FIELD_GET(GENMASK_ULL(63, 32), data64))
+			return -EINVAL;
+		return regmap_write(st->regs, ADI_REG_TDD_FRAME_LENGTH,
+				    (u32)data64);
+	case ADI_TDD_ATTR_INTERNAL_SYNC_PERIOD_RAW:
+		ret = kstrtou64(buf, 0, &data64);
+		if (ret)
+			return ret;
+		return regmap_bulk_write(st->regs, ADI_REG_TDD_SYNC_COUNTER_LOW,
+					 &data64, 2);
+	case ADI_TDD_ATTR_INTERNAL_SYNC_PERIOD_MS:
+		ret = adi_axi_tdd_parse_ms(st, buf, &data64);
+		if (ret)
+			return ret;
+		return regmap_bulk_write(st->regs, ADI_REG_TDD_SYNC_COUNTER_LOW,
+					 &data64, 2);
+	case ADI_TDD_ATTR_CHANNEL_ENABLE:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_update_bits_base(st->regs,
+					       ADI_REG_TDD_CHANNEL_ENABLE,
+					       BIT(channel),
+					       BIT(channel) * !!data,
+					       NULL, false, false);
+	case ADI_TDD_ATTR_CHANNEL_POLARITY:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_update_bits_base(st->regs,
+					       ADI_REG_TDD_CHANNEL_POLARITY,
+					       BIT(channel),
+					       BIT(channel) * !!data,
+					       NULL, false, false);
+	case ADI_TDD_ATTR_CHANNEL_ON_RAW:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_write(st->regs,
+				    ADI_REG_TDD_CHANNEL(channel,
+							ADI_TDD_CHANNEL_ON),
+				    data);
+	case ADI_TDD_ATTR_CHANNEL_ON_MS:
+		ret = adi_axi_tdd_parse_ms(st, buf, &data64);
+		if (ret)
+			return ret;
+		if (FIELD_GET(GENMASK_ULL(63, 32), data64))
+			return -EINVAL;
+		return regmap_write(st->regs,
+				    ADI_REG_TDD_CHANNEL(channel,
+							ADI_TDD_CHANNEL_ON),
+				    (u32)data64);
+	case ADI_TDD_ATTR_CHANNEL_OFF_RAW:
+		ret = kstrtou32(buf, 0, &data);
+		if (ret)
+			return ret;
+		return regmap_write(st->regs,
+				    ADI_REG_TDD_CHANNEL(channel,
+							ADI_TDD_CHANNEL_OFF),
+				    data);
+	case ADI_TDD_ATTR_CHANNEL_OFF_MS:
+		ret = adi_axi_tdd_parse_ms(st, buf, &data64);
+		if (ret)
+			return ret;
+		if (FIELD_GET(GENMASK_ULL(63, 32), data64))
+			return -EINVAL;
+		return regmap_write(st->regs,
+				    ADI_REG_TDD_CHANNEL(channel,
+							ADI_TDD_CHANNEL_OFF),
+				    (u32)data64);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t adi_axi_tdd_store(struct device *dev,
+				 struct device_attribute *dev_attr,
+				 const char *buf, size_t count)
+{
+	const struct adi_axi_tdd_attribute *attr = to_tdd_attribute(dev_attr);
+	struct adi_axi_tdd_state *st = dev_get_drvdata(dev);
+
+	return adi_axi_tdd_write_regs(attr, st, buf) ?: count;
+}
+
+static int adi_axi_tdd_init_synthesis_parameters(struct adi_axi_tdd_state *st)
+{
+	u32 interface_config;
+	int ret;
+
+	ret = regmap_read(st->regs, ADI_REG_TDD_INTERFACE_DESCRIPTION,
+			  &interface_config);
+	if (ret)
+		return ret;
+
+	st->sync_count_width = FIELD_GET(ADI_TDD_SYNC_COUNT_WIDTH,
+					 interface_config);
+	st->burst_count_width = FIELD_GET(ADI_TDD_BURST_COUNT_WIDTH,
+					  interface_config);
+	st->reg_width = FIELD_GET(ADI_TDD_REG_WIDTH, interface_config);
+	st->sync_external_cdc = ADI_TDD_SYNC_EXTERNAL_CDC & interface_config;
+	st->sync_external = ADI_TDD_SYNC_EXTERNAL & interface_config;
+	st->sync_internal = ADI_TDD_SYNC_INTERNAL & interface_config;
+	st->channel_count = FIELD_GET(ADI_TDD_CHANNEL_COUNT,
+				      interface_config) + 1;
+
+	if (!st->burst_count_width || !st->reg_width)
+		return -EINVAL;
+
+	st->sync_count_mask = GENMASK_ULL(st->sync_count_width - 1, 0);
+	st->burst_count_mask = GENMASK_ULL(st->burst_count_width - 1, 0);
+	st->reg_mask = GENMASK_ULL(st->reg_width - 1, 0);
+
+	return ret;
+}
+
+#define __TDD_ATTR(_name, _id, _channel, _mode)                         \
+	{								\
+		.attr = __ATTR(_name, _mode, adi_axi_tdd_show,          \
+				adi_axi_tdd_store),                     \
+		.id = _id,                                              \
+		.channel = _channel                                     \
+	}
+
+#define TDD_ATTR(_name, _id, _mode)                                     \
+	struct adi_axi_tdd_attribute dev_attr_##_name =                 \
+		__TDD_ATTR(_name, _id, 0, _mode)                        \
+
+static const TDD_ATTR(version, ADI_TDD_ATTR_VERSION, 0444);
+static const TDD_ATTR(core_id, ADI_TDD_ATTR_CORE_ID, 0444);
+static const TDD_ATTR(scratch, ADI_TDD_ATTR_SCRATCH, 0644);
+static const TDD_ATTR(magic, ADI_TDD_ATTR_MAGIC, 0444);
+
+static const TDD_ATTR(sync_soft, ADI_TDD_ATTR_SYNC_SOFT, 0200);
+static const TDD_ATTR(sync_external, ADI_TDD_ATTR_SYNC_EXT, 0644);
+static const TDD_ATTR(sync_internal, ADI_TDD_ATTR_SYNC_INT, 0644);
+static const TDD_ATTR(sync_reset, ADI_TDD_ATTR_SYNC_RST, 0644);
+static const TDD_ATTR(enable, ADI_TDD_ATTR_ENABLE, 0644);
+
+static const TDD_ATTR(burst_count, ADI_TDD_ATTR_BURST_COUNT, 0644);
+static const TDD_ATTR(startup_delay_raw, ADI_TDD_ATTR_STARTUP_DELAY_RAW, 0644);
+static const TDD_ATTR(startup_delay_ms, ADI_TDD_ATTR_STARTUP_DELAY_MS, 0644);
+static const TDD_ATTR(frame_length_raw, ADI_TDD_ATTR_FRAME_LENGTH_RAW, 0644);
+static const TDD_ATTR(frame_length_ms, ADI_TDD_ATTR_FRAME_LENGTH_MS, 0644);
+static const TDD_ATTR(internal_sync_period_raw,
+		      ADI_TDD_ATTR_INTERNAL_SYNC_PERIOD_RAW, 0644);
+static const TDD_ATTR(internal_sync_period_ms,
+		      ADI_TDD_ATTR_INTERNAL_SYNC_PERIOD_MS, 0644);
+
+static const TDD_ATTR(state, ADI_TDD_ATTR_STATE, 0444);
+
+static const struct attribute *adi_axi_tdd_base_attributes[] = {
+	&dev_attr_version.attr.attr,
+	&dev_attr_core_id.attr.attr,
+	&dev_attr_scratch.attr.attr,
+	&dev_attr_magic.attr.attr,
+	&dev_attr_sync_soft.attr.attr,
+	&dev_attr_sync_external.attr.attr,
+	&dev_attr_sync_internal.attr.attr,
+	&dev_attr_sync_reset.attr.attr,
+	&dev_attr_enable.attr.attr,
+	&dev_attr_burst_count.attr.attr,
+	&dev_attr_startup_delay_raw.attr.attr,
+	&dev_attr_startup_delay_ms.attr.attr,
+	&dev_attr_frame_length_raw.attr.attr,
+	&dev_attr_frame_length_ms.attr.attr,
+	&dev_attr_internal_sync_period_raw.attr.attr,
+	&dev_attr_internal_sync_period_ms.attr.attr,
+	&dev_attr_state.attr.attr,
+	/* NOT TERMINATED */
+};
+
+static const char * const channel_names[] = {
+	"out_channel%u_enable", "out_channel%u_polarity",
+	"out_channel%u_on_raw", "out_channel%u_off_raw",
+	"out_channel%u_on_ms", "out_channel%u_off_ms"
+};
+
+static const enum adi_axi_tdd_attribute_id channel_ids[] = {
+	ADI_TDD_ATTR_CHANNEL_ENABLE, ADI_TDD_ATTR_CHANNEL_POLARITY,
+	ADI_TDD_ATTR_CHANNEL_ON_RAW, ADI_TDD_ATTR_CHANNEL_OFF_RAW,
+	ADI_TDD_ATTR_CHANNEL_ON_MS, ADI_TDD_ATTR_CHANNEL_OFF_MS
+};
+
+static int adi_axi_tdd_init_sysfs(struct platform_device *pdev,
+				  struct adi_axi_tdd_state *st)
+{
+	size_t base_attr_count = ARRAY_SIZE(adi_axi_tdd_base_attributes);
+	size_t attribute_count = base_attr_count + 6 * st->channel_count + 1;
+	struct adi_axi_tdd_attribute *channel_attributes;
+	struct adi_axi_tdd_attribute *channel_iter;
+	struct attribute_group *attr_group;
+	struct attribute **tdd_attrs;
+	u32 i, j;
+
+	channel_attributes = devm_kcalloc(&pdev->dev, 6 * st->channel_count,
+					  sizeof(*channel_attributes),
+					  GFP_KERNEL);
+	if (!channel_attributes)
+		return -ENOMEM;
+
+	tdd_attrs = devm_kcalloc(&pdev->dev, attribute_count,
+				 sizeof(*tdd_attrs), GFP_KERNEL);
+	if (!tdd_attrs)
+		return -ENOMEM;
+
+	memcpy(tdd_attrs, adi_axi_tdd_base_attributes,
+	       sizeof(adi_axi_tdd_base_attributes));
+
+	channel_iter = channel_attributes;
+
+	for (i = 0; i < st->channel_count; i++) {
+		for (j = 0; j < ARRAY_SIZE(channel_names); j++) {
+			snprintf(channel_iter->name,
+				 sizeof(channel_iter->name), channel_names[j],
+				 i);
+			channel_iter->id = channel_ids[j];
+			channel_iter->channel = i;
+			channel_iter->attr.attr.name = channel_iter->name;
+			channel_iter->attr.attr.mode = 0644;
+			channel_iter->attr.show = adi_axi_tdd_show;
+			channel_iter->attr.store = adi_axi_tdd_store;
+
+			tdd_attrs[base_attr_count + 6 * i + j] =
+				&channel_iter->attr.attr;
+			channel_iter++;
+		}
+	}
+
+	attr_group = devm_kzalloc(&pdev->dev, sizeof(attr_group), GFP_KERNEL);
+	if (!attr_group)
+		return -ENOMEM;
+
+	attr_group->attrs = tdd_attrs;
+
+	return devm_device_add_group(&pdev->dev, attr_group);
+}
+
+static int adi_axi_tdd_rate_change(struct notifier_block *nb,
+				   unsigned long flags, void *data)
+{
+	struct adi_axi_tdd_clk *clk =
+		container_of(nb, struct adi_axi_tdd_clk, nb);
+	struct clk_notifier_data *cnd = data;
+
+	/* cache the new rate */
+	WRITE_ONCE(clk->rate, cnd->new_rate);
+
+	return NOTIFY_OK;
+}
+
+static void adi_axi_tdd_clk_notifier_unreg(void *data)
+{
+	struct adi_axi_tdd_clk *clk = data;
+
+	clk_notifier_unregister(clk->clk, &clk->nb);
+}
+
+static int adi_axi_tdd_probe(struct platform_device *pdev)
+{
+	unsigned int expected_version, version, data;
+	struct adi_axi_tdd_state *st;
+	struct clk *aclk;
+	int ret;
+
+	st = devm_kzalloc(&pdev->dev, sizeof(*st), GFP_KERNEL);
+	if (!st)
+		return -ENOMEM;
+
+	st->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(st->base))
+		return PTR_ERR(st->base);
+
+	platform_set_drvdata(pdev, st);
+
+	aclk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
+	if (IS_ERR(aclk))
+		return PTR_ERR(aclk);
+
+	st->clk.clk = devm_clk_get_enabled(&pdev->dev, "intf_clk");
+	if (IS_ERR(st->clk.clk))
+		return PTR_ERR(st->clk.clk);
+
+	st->clk.rate = clk_get_rate(st->clk.clk);
+	st->clk.nb.notifier_call = adi_axi_tdd_rate_change;
+	ret = clk_notifier_register(st->clk.clk, &st->clk.nb);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       adi_axi_tdd_clk_notifier_unreg,
+				       st->clk.clk);
+	if (ret)
+		return ret;
+
+	st->regs = devm_regmap_init_mmio(&pdev->dev, st->base,
+					 &adi_axi_tdd_regmap_cfg);
+	if (IS_ERR(st->regs))
+		return PTR_ERR(st->regs);
+
+	ret = regmap_read(st->regs, ADI_AXI_REG_VERSION, &version);
+	if (ret)
+		return ret;
+
+	expected_version = ADI_AXI_PCORE_VER(2, 0, 'a');
+
+	if (ADI_AXI_PCORE_VER_MAJOR(version) !=
+	    ADI_AXI_PCORE_VER_MAJOR(expected_version))
+		return dev_err_probe(&pdev->dev,
+				     -ENODEV,
+				     "Major version mismatch between PCORE and driver. Driver expected %d.%.2d.%c, PCORE reported %d.%.2d.%c\n",
+				     ADI_AXI_PCORE_VER_MAJOR(expected_version),
+				     ADI_AXI_PCORE_VER_MINOR(expected_version),
+				     ADI_AXI_PCORE_VER_PATCH(expected_version),
+				     ADI_AXI_PCORE_VER_MAJOR(version),
+				     ADI_AXI_PCORE_VER_MINOR(version),
+				     ADI_AXI_PCORE_VER_PATCH(version));
+
+	ret = adi_axi_tdd_init_synthesis_parameters(st);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to load synthesis parameters, make sure the device is configured correctly.\n");
+
+	ret = regmap_read(st->regs, ADI_REG_TDD_CONTROL, &data);
+	if (ret)
+		return ret;
+
+	st->enabled =  data & ADI_TDD_ENABLE;
+
+	ret = adi_axi_tdd_init_sysfs(pdev, st);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to init sysfs, aborting ...\n");
+
+	dev_dbg(&pdev->dev, "Probed Analog Devices AXI TDD (%d.%.2d.%c)",
+		ADI_AXI_PCORE_VER_MAJOR(version),
+		ADI_AXI_PCORE_VER_MINOR(version),
+		ADI_AXI_PCORE_VER_PATCH(version));
+
+	return 0;
+}
+
+/* Match table for of_platform binding */
+static const struct of_device_id adi_axi_tdd_of_match[] = {
+	{ .compatible = "adi,axi-tdd" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, adi_axi_tdd_of_match);
+
+static struct platform_driver adi_axi_tdd_driver = {
+	.driver = {
+		.name = "adi-axi-tdd",
+		.of_match_table = adi_axi_tdd_of_match,
+	},
+	.probe = adi_axi_tdd_probe,
+};
+module_platform_driver(adi_axi_tdd_driver);
+
+MODULE_AUTHOR("Eliza Balas <eliza.balas@analog.com>");
+MODULE_AUTHOR("David Winter <david.winter@analog.com>");
+MODULE_DESCRIPTION("Analog Devices TDD HDL CORE driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Greg Kroah-Hartman]

On Thu, Oct 19, 2023 at 03:56:46PM +0300, Eliza Balas wrote:
> +config ADI_AXI_TDD
> +	tristate "Analog Devices TDD Engine support"
> +	depends on HAS_IOMEM
> +	select REGMAP_MMIO
> +	help
> +	  The ADI AXI TDD core is the reworked and generic TDD engine which
> +	  was developed for general use in Analog Devices HDL projects. Unlike
> +	  the previous TDD engine, this core can only be used standalone mode,
> +	  and is not embedded into other devices.

What does "previous" mean here?  That's not relevant for a kernel help
text, is it?

Also, what is the module name?  Why would someone enable this?  What
userspace tools use it?


> +
>  config DUMMY_IRQ
>  	tristate "Dummy IRQ handler"
>  	help

Why put your entry in this specific location in the file?

> +static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
> +				const char *buf,
> +				u64 *res)
> +{
> +	u64 clk_rate = READ_ONCE(st->clk.rate);
> +	char *orig_str, *modf_str, *int_part, frac_part[7];
> +	long ival, frac;
> +	int ret;
> +
> +	orig_str = kstrdup(buf, GFP_KERNEL);
> +	int_part = strsep(&orig_str, ".");

Why are we parsing floating point in the kernel?  Please just keep the
api simple so that we don't have to try to do any type of parsing other
than turning a single text number into a value.

> +	ret = kstrtol(int_part, 10, &ival);
> +	if (ret || ival < 0)
> +		return -EINVAL;

You leaked memory :(

Use the new logic in completion.h to make this simpler?

> +	modf_str = strsep(&orig_str, ".");
> +	if (modf_str) {
> +		snprintf(frac_part, 7, "%s00000", modf_str);
> +		ret = kstrtol(frac_part, 10, &frac);
> +		if (ret)
> +			return -EINVAL;

You leaked memory :(

> +	} else {
> +		frac = 0;
> +	}
> +
> +	*res = DIV_ROUND_CLOSEST_ULL((u64)ival * clk_rate, 1000)
> +		+ DIV_ROUND_CLOSEST_ULL((u64)frac * clk_rate, 1000000000);

Why isn't userspace doing this calculation?

I stopped reviewing here, sorry.

greg k-h

RE: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Balas, Eliza]

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Thursday, October 19, 2023 20:57
> To: Balas, Eliza <Eliza.Balas@analog.com>
> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan
> Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> 
> [External]
> 
> On Thu, Oct 19, 2023 at 03:56:46PM +0300, Eliza Balas wrote:
> > +config ADI_AXI_TDD
> > +	tristate "Analog Devices TDD Engine support"
> > +	depends on HAS_IOMEM
> > +	select REGMAP_MMIO
> > +	help
> > +	  The ADI AXI TDD core is the reworked and generic TDD engine which
> > +	  was developed for general use in Analog Devices HDL projects. Unlike
> > +	  the previous TDD engine, this core can only be used standalone mode,
> > +	  and is not embedded into other devices.
> 
> What does "previous" mean here?  That's not relevant for a kernel help
> text, is it?
> 
> Also, what is the module name?  Why would someone enable this?  What
> userspace tools use it?
> 
> 
> > +
> >  config DUMMY_IRQ
> >  	tristate "Dummy IRQ handler"
> >  	help
> 
> Why put your entry in this specific location in the file?
> 
> > +static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
> > +				const char *buf,
> > +				u64 *res)
> > +{
> > +	u64 clk_rate = READ_ONCE(st->clk.rate);
> > +	char *orig_str, *modf_str, *int_part, frac_part[7];
> > +	long ival, frac;
> > +	int ret;
> > +
> > +	orig_str = kstrdup(buf, GFP_KERNEL);
> > +	int_part = strsep(&orig_str, ".");
> 
> Why are we parsing floating point in the kernel?  Please just keep the
> api simple so that we don't have to try to do any type of parsing other
> than turning a single text number into a value.
> 

The adi_axi_tdd_parse_ms function does almost the same thing as the 
iio_str_to_fixpoint() function which already exists in kernel.
It parses a fixed-point number from a string. 
The __iio_str_to_fixpoint is used in a similar way, as I intend to use adi_axi_tdd_parse_ms.
It writes to a channel the corresponding scale (micro,nano) for a value.

Since the device is not an iio device, using an iio function would be confusing.
That is the reason for creating the adi_axi_tdd_parse_ms function, which is easier
to understand since I don't have to make all the multiplications that are made 
in the __iio_str_to_fixpoint function.

Thank you for review,
Eliza

Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Krzysztof Kozlowski]

On 20/10/2023 13:18, Balas, Eliza wrote:
>> -----Original Message-----
>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Sent: Thursday, October 19, 2023 20:57
>> To: Balas, Eliza <Eliza.Balas@analog.com>
>> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan
>> Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
>> Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
>>
>> [External]
>>
>> On Thu, Oct 19, 2023 at 03:56:46PM +0300, Eliza Balas wrote:
>>> +config ADI_AXI_TDD
>>> +	tristate "Analog Devices TDD Engine support"
>>> +	depends on HAS_IOMEM
>>> +	select REGMAP_MMIO
>>> +	help
>>> +	  The ADI AXI TDD core is the reworked and generic TDD engine which
>>> +	  was developed for general use in Analog Devices HDL projects. Unlike
>>> +	  the previous TDD engine, this core can only be used standalone mode,
>>> +	  and is not embedded into other devices.
>>
>> What does "previous" mean here?  That's not relevant for a kernel help
>> text, is it?
>>
>> Also, what is the module name?  Why would someone enable this?  What
>> userspace tools use it?
>>
>>
>>> +
>>>  config DUMMY_IRQ
>>>  	tristate "Dummy IRQ handler"
>>>  	help
>>
>> Why put your entry in this specific location in the file?
>>
>>> +static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
>>> +				const char *buf,
>>> +				u64 *res)
>>> +{
>>> +	u64 clk_rate = READ_ONCE(st->clk.rate);
>>> +	char *orig_str, *modf_str, *int_part, frac_part[7];
>>> +	long ival, frac;
>>> +	int ret;
>>> +
>>> +	orig_str = kstrdup(buf, GFP_KERNEL);
>>> +	int_part = strsep(&orig_str, ".");
>>
>> Why are we parsing floating point in the kernel?  Please just keep the
>> api simple so that we don't have to try to do any type of parsing other
>> than turning a single text number into a value.
>>
> 
> The adi_axi_tdd_parse_ms function does almost the same thing as the 
> iio_str_to_fixpoint() function which already exists in kernel.
> It parses a fixed-point number from a string. 
> The __iio_str_to_fixpoint is used in a similar way, as I intend to use adi_axi_tdd_parse_ms.
> It writes to a channel the corresponding scale (micro,nano) for a value.
> 
> Since the device is not an iio device, using an iio function would be confusing.
> That is the reason for creating the adi_axi_tdd_parse_ms function, which is easier
> to understand since I don't have to make all the multiplications that are made 
> in the __iio_str_to_fixpoint function.

Why did you skip other comments?

Best regards,
Krzysztof

RE: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Balas, Eliza]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, October 20, 2023 14:26
> To: Balas, Eliza <Eliza.Balas@analog.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan
> Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> 
> [External]
> 
> On 20/10/2023 13:18, Balas, Eliza wrote:
> >> -----Original Message-----
> >> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Sent: Thursday, October 19, 2023 20:57
> >> To: Balas, Eliza <Eliza.Balas@analog.com>
> >> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan
> >> Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> >> Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> >>
> >> [External]
> >>
> >> On Thu, Oct 19, 2023 at 03:56:46PM +0300, Eliza Balas wrote:
> >>> +config ADI_AXI_TDD
> >>> +	tristate "Analog Devices TDD Engine support"
> >>> +	depends on HAS_IOMEM
> >>> +	select REGMAP_MMIO
> >>> +	help
> >>> +	  The ADI AXI TDD core is the reworked and generic TDD engine which
> >>> +	  was developed for general use in Analog Devices HDL projects. Unlike
> >>> +	  the previous TDD engine, this core can only be used standalone mode,
> >>> +	  and is not embedded into other devices.
> >>
> >> What does "previous" mean here?  That's not relevant for a kernel help
> >> text, is it?
> >>

I will redo the config help text to make it clearer.

> >> Also, what is the module name?  Why would someone enable this?  What
> >> userspace tools use it?

I will add a better description. Currently there are no userspace tools
that use the device. This platform driver gives the possibility to access
and configure the device using the sysfs interface.

> >>
> >>> +
> >>>  config DUMMY_IRQ
> >>>  	tristate "Dummy IRQ handler"
> >>>  	help
> >>
> >> Why put your entry in this specific location in the file?

I added the entry in the kconfig file based on the alphabetical order.

> >>
> >>> +static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
> >>> +				const char *buf,
> >>> +				u64 *res)
> >>> +{
> >>> +	u64 clk_rate = READ_ONCE(st->clk.rate);
> >>> +	char *orig_str, *modf_str, *int_part, frac_part[7];
> >>> +	long ival, frac;
> >>> +	int ret;
> >>> +
> >>> +	orig_str = kstrdup(buf, GFP_KERNEL);
> >>> +	int_part = strsep(&orig_str, ".");
> >>
> >> Why are we parsing floating point in the kernel?  Please just keep the
> >> api simple so that we don't have to try to do any type of parsing other
> >> than turning a single text number into a value.
> >>
> >
> > The adi_axi_tdd_parse_ms function does almost the same thing as the
> > iio_str_to_fixpoint() function which already exists in kernel.
> > It parses a fixed-point number from a string.
> > The __iio_str_to_fixpoint is used in a similar way, as I intend to use adi_axi_tdd_parse_ms.
> > It writes to a channel the corresponding scale (micro,nano) for a value.
> >
> > Since the device is not an iio device, using an iio function would be confusing.
> > That is the reason for creating the adi_axi_tdd_parse_ms function, which is easier
> > to understand since I don't have to make all the multiplications that are made
> > in the __iio_str_to_fixpoint function.
> 
> Why did you skip other comments?
> 
 
By mistake, I hit the send button. I added them now.

> >>> +	ret = kstrtol(int_part, 10, &ival);
> >>> +	if (ret || ival < 0)
> >>> +		return -EINVAL;
> >>
> >> You leaked memory :(
> >>
> >> Use the new logic in completion.h to make this simpler?
> >>
> >>> +	modf_str = strsep(&orig_str, ".");
> >>>+	if (modf_str) {
> >>>+		snprintf(frac_part, 7, "%s00000", modf_str);
> >>>+		ret = kstrtol(frac_part, 10, &frac);
> >>>+		if (ret)
> >>>+			return -EINVAL;
> >>
> >> You leaked memory :(
> >>
> >>>+	} else {
> >>>+		frac = 0;
> >>>+	}
> >>>+
> >>>+	*res = DIV_ROUND_CLOSEST_ULL((u64)ival * clk_rate, 1000)
> >>>+		+ DIV_ROUND_CLOSEST_ULL((u64)frac * clk_rate, 1000000000);
> >>
> >> Why isn't userspace doing this calculation?

The string contains a fixed-point number for a value in milliseconds,
but the value must be written in the register in clock cycles. 
The clock rate may change, and we must transform the value in
clock cycles, so this is the reason for this calculation here.

> >>
> >> I stopped reviewing here, sorry.
> >>
> >> greg k-h

Thank you,
Eliza

Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Greg Kroah-Hartman]

On Fri, Oct 20, 2023 at 11:18:44AM +0000, Balas, Eliza wrote:
> > > +static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
> > > +				const char *buf,
> > > +				u64 *res)
> > > +{
> > > +	u64 clk_rate = READ_ONCE(st->clk.rate);
> > > +	char *orig_str, *modf_str, *int_part, frac_part[7];
> > > +	long ival, frac;
> > > +	int ret;
> > > +
> > > +	orig_str = kstrdup(buf, GFP_KERNEL);
> > > +	int_part = strsep(&orig_str, ".");
> > 
> > Why are we parsing floating point in the kernel?  Please just keep the
> > api simple so that we don't have to try to do any type of parsing other
> > than turning a single text number into a value.
> > 
> 
> The adi_axi_tdd_parse_ms function does almost the same thing as the 
> iio_str_to_fixpoint() function which already exists in kernel.

That does not mean that this is a valid api for your device as you are
not an iio driver (why aren't you an iio driver?)

> It parses a fixed-point number from a string. 

And as such, you shouldn't duplicate existing logic.

> The __iio_str_to_fixpoint is used in a similar way, as I intend to use adi_axi_tdd_parse_ms.
> It writes to a channel the corresponding scale (micro,nano) for a value.

Why not just have the api accept values in nanoseconds and then no
parsing is needed?

> Since the device is not an iio device, using an iio function would be confusing.

Why isn't this an iio device?

thanks,

greg k-h

RE: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Balas, Eliza]

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Friday, October 20, 2023 17:32
> To: Balas, Eliza <Eliza.Balas@analog.com>
> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan
> Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> 
> [External]
> 
> On Fri, Oct 20, 2023 at 11:18:44AM +0000, Balas, Eliza wrote:
> > > > +static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
> > > > +				const char *buf,
> > > > +				u64 *res)
> > > > +{
> > > > +	u64 clk_rate = READ_ONCE(st->clk.rate);
> > > > +	char *orig_str, *modf_str, *int_part, frac_part[7];
> > > > +	long ival, frac;
> > > > +	int ret;
> > > > +
> > > > +	orig_str = kstrdup(buf, GFP_KERNEL);
> > > > +	int_part = strsep(&orig_str, ".");
> > >
> > > Why are we parsing floating point in the kernel?  Please just keep the
> > > api simple so that we don't have to try to do any type of parsing other
> > > than turning a single text number into a value.
> > >
> >
> > The adi_axi_tdd_parse_ms function does almost the same thing as the
> > iio_str_to_fixpoint() function which already exists in kernel.
> 
> That does not mean that this is a valid api for your device as you are
> not an iio driver (why aren't you an iio driver?)
> 
> > It parses a fixed-point number from a string.
> 
> And as such, you shouldn't duplicate existing logic.
> 
> > The __iio_str_to_fixpoint is used in a similar way, as I intend to use adi_axi_tdd_parse_ms.
> > It writes to a channel the corresponding scale (micro,nano) for a value.
> 
> Why not just have the api accept values in nanoseconds and then no
> parsing is needed?

I thought this would be easier for the user, to work with smaller values,
than using a lot of zeros for nanoseconds. I will make the changes
to accept values in nanoseconds..

> > Since the device is not an iio device, using an iio function would be confusing.
> 
> Why isn't this an iio device?

The device is not registered into the IIO device tree, 
and does not rely on IIO kernel APIs. 
Even though there are a few attributes that resemble the
ones from iio, and the sysfs structure is similar,
this is not an IIO device.
In the previous patch versions 1 and 2 we concluded
that this device fits better in the misc subsystem.

Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Greg Kroah-Hartman]

On Mon, Oct 23, 2023 at 12:54:15PM +0000, Balas, Eliza wrote:
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Friday, October 20, 2023 17:32
> > To: Balas, Eliza <Eliza.Balas@analog.com>
> > Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan
> > Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> > Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> > 
> > [External]
> > 
> > On Fri, Oct 20, 2023 at 11:18:44AM +0000, Balas, Eliza wrote:
> > > > > +static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
> > > > > +				const char *buf,
> > > > > +				u64 *res)
> > > > > +{
> > > > > +	u64 clk_rate = READ_ONCE(st->clk.rate);
> > > > > +	char *orig_str, *modf_str, *int_part, frac_part[7];
> > > > > +	long ival, frac;
> > > > > +	int ret;
> > > > > +
> > > > > +	orig_str = kstrdup(buf, GFP_KERNEL);
> > > > > +	int_part = strsep(&orig_str, ".");
> > > >
> > > > Why are we parsing floating point in the kernel?  Please just keep the
> > > > api simple so that we don't have to try to do any type of parsing other
> > > > than turning a single text number into a value.
> > > >
> > >
> > > The adi_axi_tdd_parse_ms function does almost the same thing as the
> > > iio_str_to_fixpoint() function which already exists in kernel.
> > 
> > That does not mean that this is a valid api for your device as you are
> > not an iio driver (why aren't you an iio driver?)
> > 
> > > It parses a fixed-point number from a string.
> > 
> > And as such, you shouldn't duplicate existing logic.
> > 
> > > The __iio_str_to_fixpoint is used in a similar way, as I intend to use adi_axi_tdd_parse_ms.
> > > It writes to a channel the corresponding scale (micro,nano) for a value.
> > 
> > Why not just have the api accept values in nanoseconds and then no
> > parsing is needed?
> 
> I thought this would be easier for the user, to work with smaller values,
> than using a lot of zeros for nanoseconds. I will make the changes
> to accept values in nanoseconds..

Make the kernel simpler, it's easier to make more complex userspace,
right?

> > > Since the device is not an iio device, using an iio function would be confusing.
> > 
> > Why isn't this an iio device?
> 
> The device is not registered into the IIO device tree, 
> and does not rely on IIO kernel APIs. 
> Even though there are a few attributes that resemble the
> ones from iio, and the sysfs structure is similar,
> this is not an IIO device.
> In the previous patch versions 1 and 2 we concluded
> that this device fits better in the misc subsystem.

Ok, can you point to that in the changelog where the IIO maintainer
agreed that this doesn't fit into that subsystem?

thanks,

greg k-h

RE: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Balas, Eliza]

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Monday, October 23, 2023 16:00
> To: Balas, Eliza <Eliza.Balas@analog.com>
> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan
> Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> 
> [External]
> 
> On Mon, Oct 23, 2023 at 12:54:15PM +0000, Balas, Eliza wrote:
> > > -----Original Message-----
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Sent: Friday, October 20, 2023 17:32
> > > To: Balas, Eliza <Eliza.Balas@analog.com>
> > > Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan
> > > Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> > > Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> > >
> > > [External]
> > >
> > > On Fri, Oct 20, 2023 at 11:18:44AM +0000, Balas, Eliza wrote:
> > > > > > +static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
> > > > > > +				const char *buf,
> > > > > > +				u64 *res)
> > > > > > +{
> > > > > > +	u64 clk_rate = READ_ONCE(st->clk.rate);
> > > > > > +	char *orig_str, *modf_str, *int_part, frac_part[7];
> > > > > > +	long ival, frac;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	orig_str = kstrdup(buf, GFP_KERNEL);
> > > > > > +	int_part = strsep(&orig_str, ".");
> > > > >
> > > > > Why are we parsing floating point in the kernel?  Please just keep the
> > > > > api simple so that we don't have to try to do any type of parsing other
> > > > > than turning a single text number into a value.
> > > > >
> > > >
> > > > The adi_axi_tdd_parse_ms function does almost the same thing as the
> > > > iio_str_to_fixpoint() function which already exists in kernel.
> > >
> > > That does not mean that this is a valid api for your device as you are
> > > not an iio driver (why aren't you an iio driver?)
> > >
> > > > It parses a fixed-point number from a string.
> > >
> > > And as such, you shouldn't duplicate existing logic.
> > >
> > > > The __iio_str_to_fixpoint is used in a similar way, as I intend to use adi_axi_tdd_parse_ms.
> > > > It writes to a channel the corresponding scale (micro,nano) for a value.
> > >
> > > Why not just have the api accept values in nanoseconds and then no
> > > parsing is needed?
> >
> > I thought this would be easier for the user, to work with smaller values,
> > than using a lot of zeros for nanoseconds. I will make the changes
> > to accept values in nanoseconds..
> 
> Make the kernel simpler, it's easier to make more complex userspace,
> right?
> 
> > > > Since the device is not an iio device, using an iio function would be confusing.
> > >
> > > Why isn't this an iio device?
> >
> > The device is not registered into the IIO device tree,
> > and does not rely on IIO kernel APIs.
> > Even though there are a few attributes that resemble the
> > ones from iio, and the sysfs structure is similar,
> > this is not an IIO device.
> > In the previous patch versions 1 and 2 we concluded
> > that this device fits better in the misc subsystem.
> 
> Ok, can you point to that in the changelog where the IIO maintainer
> agreed that this doesn't fit into that subsystem?
> 
This was one of the discussions from previous v2 : https://lore.kernel.org/all/5b6318f16799e6e2575fe541e83e42e0afebe6cf.camel@gmail.com/

I will add it to the changelog the next time I submit the patches.

Thank you,
Eliza

Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Arnd Bergmann]

On Mon, Oct 23, 2023, at 15:30, Balas, Eliza wrote:
>> -----Original Message-----
>> Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
>> Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

>> > > > Since the device is not an iio device, using an iio function would be confusing.
>> > >
>> > > Why isn't this an iio device?
>> >
>> > The device is not registered into the IIO device tree,
>> > and does not rely on IIO kernel APIs.
>> > Even though there are a few attributes that resemble the
>> > ones from iio, and the sysfs structure is similar,
>> > this is not an IIO device.
>> > In the previous patch versions 1 and 2 we concluded
>> > that this device fits better in the misc subsystem.
>> 
>> Ok, can you point to that in the changelog where the IIO maintainer
>> agreed that this doesn't fit into that subsystem?
>> 
> This was one of the discussions from previous v2 : 
> https://lore.kernel.org/all/5b6318f16799e6e2575fe541e83e42e0afebe6cf.camel@gmail.com/
>
> I will add it to the changelog the next time I submit the patches.

It sounds like Jonathan wasn't quite sure either here, and I would
still argue (as I did in that thread), that drivers/iio is probably
a better option than drivers/misc.

In particular, you mention that you actually make this device
appear as an IIO device to user space using the "iio-fake" hack.

I can see that IIO is not a perfect fit if this is the only
device of its kind, but going that way anyway avoids a number
of problems by reusing infrastructure for the IIO ABI and
serialization with in-kernel users, as well as giving you
the option of adding other compatible drivers later.

     Arnd

Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Nuno Sá]

On Mon, 2023-10-23 at 16:19 +0200, Arnd Bergmann wrote:
> On Mon, Oct 23, 2023, at 15:30, Balas, Eliza wrote:
> > > -----Original Message-----
> > > Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> > > Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> 
> > > > > > Since the device is not an iio device, using an iio function would
> > > > > > be confusing.
> > > > > 
> > > > > Why isn't this an iio device?
> > > > 
> > > > The device is not registered into the IIO device tree,
> > > > and does not rely on IIO kernel APIs.
> > > > Even though there are a few attributes that resemble the
> > > > ones from iio, and the sysfs structure is similar,
> > > > this is not an IIO device.
> > > > In the previous patch versions 1 and 2 we concluded
> > > > that this device fits better in the misc subsystem.
> > > 
> > > Ok, can you point to that in the changelog where the IIO maintainer
> > > agreed that this doesn't fit into that subsystem?
> > > 
> > This was one of the discussions from previous v2 : 
> > https://lore.kernel.org/all/5b6318f16799e6e2575fe541e83e42e0afebe6cf.camel@gmail.com/
> > 
> > I will add it to the changelog the next time I submit the patches.
> 
> It sounds like Jonathan wasn't quite sure either here, and I would
> still argue (as I did in that thread), that drivers/iio is probably
> a better option than drivers/misc.
> 

Well, if Jonathan agrees to have this in IIO, it would actually be better for
us... The below hack would not be needed at all and IIO is very familiar.

> In particular, you mention that you actually make this device
> appear as an IIO device to user space using the "iio-fake" hack.
> 

I want to emphasize that is just our hack to make use of libiio RPC so that we
can remotely access this device.
 
- Nuno Sá

Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Jonathan Cameron]

On Mon, 23 Oct 2023 16:36:57 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2023-10-23 at 16:19 +0200, Arnd Bergmann wrote:
> > On Mon, Oct 23, 2023, at 15:30, Balas, Eliza wrote:  
> > > > -----Original Message-----
> > > > Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> > > > Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine  
> >   
> > > > > > > Since the device is not an iio device, using an iio function would
> > > > > > > be confusing.  
> > > > > > 
> > > > > > Why isn't this an iio device?  
> > > > > 
> > > > > The device is not registered into the IIO device tree,
> > > > > and does not rely on IIO kernel APIs.
> > > > > Even though there are a few attributes that resemble the
> > > > > ones from iio, and the sysfs structure is similar,
> > > > > this is not an IIO device.
> > > > > In the previous patch versions 1 and 2 we concluded
> > > > > that this device fits better in the misc subsystem.  
> > > > 
> > > > Ok, can you point to that in the changelog where the IIO maintainer
> > > > agreed that this doesn't fit into that subsystem?
> > > >   
> > > This was one of the discussions from previous v2 : 
> > > https://lore.kernel.org/all/5b6318f16799e6e2575fe541e83e42e0afebe6cf.camel@gmail.com/
> > > 
> > > I will add it to the changelog the next time I submit the patches.  
> > 
> > It sounds like Jonathan wasn't quite sure either here, and I would
> > still argue (as I did in that thread), that drivers/iio is probably
> > a better option than drivers/misc.
> >   
> 
> Well, if Jonathan agrees to have this in IIO, it would actually be better for
> us... The below hack would not be needed at all and IIO is very familiar.

I'm flexible on including this in IIO - we have a few weird and wonderful
devices already that need heavily custom userspace. This looks like it might
be another one of those - though I'm not sure it's all that unusual if you
consider the DDS devices we have drivers for (still in staging after a lot
of years).

A few things in the ABI would need to fit a little better such as matching
units were appropriate and not providing bother msec and raw control of things.

Mapping it as closely as possible onto IIO concepts might be tricky.
Sync is a bit trigger like, but we aren't doing output buffers so it doesn't
fit that well - also the IIO repeat infrastructure is simple and not much used
in IIO so I doubt it's effective enough for this.  Maybe we can expand
the buffer control concepts to include 'hardware generated sequences'.

Magic might map over to label.

Scratch register says useful for debug on the webpage. Don't expose it then,
or if you do only in debugfs.  Overall enable would be controlled by starting
the buffer.

I'm not against having more sophisticate output channel definitions to allow
for delays from trigger etc.  We've talked about doing that for input channels
in the past to be able to identify the timing more accurately in devices with
a sequencer unit. So anything we define should be useful for that case as well.

So brief brainstorming for how it might be sort of aligned with an IIO ABI.
Abusing slightly the voltage channels - make it a buffer (messing heavily with
the definition and adding a 'type' field.

buffer0/type   - userspace, pattern - for all existing devices it's userspace.

For each channel
buffer0/repeats //can't use the scan element spec as it's not writeable.
                //I'd love to fix that but it's horribly invasive to do.
buffer0/repeat_delay //frame length?  Or could map to sampling_frequency :)
buffer0/out_voltageX_en
buffer0/out_voltageX_polarity
buffer0/out_voltageX_index
buffer0/out_voltageX_delay //in seconds I think to match with timeout ABI
                           //not ondelay because I want to reuse this for
                           //the sequencer case mentioned above.
buffer0/out/voltageX_offdelay // also mapped from start of frame.
For the delays, interesting to think about how they map to more general DDS
devices.

startup delay could be considered a trigger characteristic or hammered in here
as a parameter applying to all channels.
Sync timing is a trigger parameter.  Expose separate triggers for internal,
external and maybe soft - that one is up to you.

It's not that horrible, so if you want to give this a go - perhaps
start with a full formal ABI proposal and let's see if the fit is reasonable?
Feel free to take the above suggestions as the brief one person brainstorm that
they are and propose something much more sensible / coherent!

One thing that concerns me a little is mapping it to more similar
devices.  For instances, treating the signals as voltages is misleading.
Maybe we should treat them as code words, thus supporting more general time
division multiplexers.  We walked through some of the symbol type interfaces
a long time ago for PSK / DDS devices though I think they are still in staging
(feel free to deal with that whilst you are here :)

It's always hard to define ABI with one device of a new class...

The st micro folk have some complex ADC setups that aren't entirely dissimilar.
(we have chained triggers to deal with a similar 'repeat' on trigger situation
that looks to me similar to your burst + frame).
Maybe they have considered doing this for DACs or other usecase that overlap
with yours?  +CC Olivier and Fabrice for input.

If there is some common ground here it would be useful to get inputs on the
ABI even if there is no plan to add equivalent support in the near future.

Jonathan




> 
> > In particular, you mention that you actually make this device
> > appear as an IIO device to user space using the "iio-fake" hack.
> >   
> 
> I want to emphasize that is just our hack to make use of libiio RPC so that we
> can remotely access this device.
>  
> - Nuno Sá
>