[PATCH 0/6] Add Mule PWM-over-I2C support

[PATCH 0/6] Add Mule PWM-over-I2C support

[Farouk Bouabid]

Mule is an MCU that emulates a set of I2C devices which are reachable
through an I2C-mux that is implemented in a different patch-series[1].

Device #1 on the mux is a PWM controller that allows users to I2C-configure
the PWM output signal.

On rk3399-puma-haikou, px30-ringneck-haikou, rk3588-tiger-haikou and
rk3588-jaguar boards, this PWM controller is connected to a PWM beeper.

      +-----------------------------------------------+
      |  Mule                                         |
      |        +---------------+                      |
    ----+----->|Config register|                      |
      | |      +--------|------+                      |
      | |               |                             |
      | |               V                             |
      | |               __           +--------------+ |
      | |              |   \-------->| amc6821      | |
      | |              |   |         +--------------+ |      +--------+
      | |              | M |-------->| PWM over I2C |------->| Beeper |
      | +------------->| U |         +--------------+ |      +--------+
      |                | X |-------->| dev #2       | |
      |                |   |         +--------------+ |
      |                |   /-------->| dev #3       | |
      |                |__/          +--------------+ |
      +-----------------------------------------------+

This patch-series add support for Mule PWM-over-I2C controller as well
as the PWM-beeper on theses boards.

The device-tree patches are to be merged after the other patch-series.
The dt-bindings and driver patches can be merged regardless of the state
of the other series.

[1] https://lore.kernel.org/lkml/20240506-dev-mule-i2c-mux-v2-0-a91c954f65d7@cherry.de/

Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
---
Farouk Bouabid (6):
      dt-bindings: pwm: add dt-bindings for mule pwm-over-i2c controller
      pwm: add mule pwm-over-i2c driver
      arm64: dts: rockchip: add pwm-beeper to rk3399-puma-haikou
      arm64: dts: rockchip: add pwm-beeper to px30-ringneck-haikou
      arm64: dts: rockchip: add pwm-beeper to rk3588-tiger-haikou
      arm64: dts: rockchip: add pwm-beeper to rk3588-jaguar

 .../devicetree/bindings/pwm/tsd,pwm-mule.yaml      |  46 +++++++++
 .../boot/dts/rockchip/px30-ringneck-haikou.dts     |   5 +
 arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi    |  13 +++
 .../arm64/boot/dts/rockchip/rk3399-puma-haikou.dts |   5 +
 arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi      |  13 +++
 arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts     |  18 ++++
 .../boot/dts/rockchip/rk3588-tiger-haikou.dts      |   6 ++
 arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi     |  13 +++
 drivers/pwm/Kconfig                                |  10 ++
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-mule.c                             | 115 +++++++++++++++++++++
 11 files changed, 245 insertions(+)
---
base-commit: fd8c3f3cd1b029f1851393839f7ce558db9cf202
change-id: 20240515-buzzer_support-33d93c9d0f1b

Best regards,
-- 
Farouk Bouabid <farouk.bouabid@cherry.de>

[PATCH 1/6] dt-bindings: pwm: add dt-bindings for mule pwm-over-i2c controller

[Farouk Bouabid]

Mule is a device that controls a PWM output signal based on I2C commands.

Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
---
 .../devicetree/bindings/pwm/tsd,pwm-mule.yaml      | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/tsd,pwm-mule.yaml b/Documentation/devicetree/bindings/pwm/tsd,pwm-mule.yaml
new file mode 100644
index 000000000000..71a940a2a644
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/tsd,pwm-mule.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/tsd,pwm-mule.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mule PWM-over-I2C controller
+
+description: |
+  A device that outputs a PWM signal based on I2C commands.
+
+maintainers:
+  - Farouk Bouabid <farouk.bouabid@cherry.de>
+  - Quentin Schulz <quentin.schulz@cherry.de>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: tsd,pwm-mule
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      pwm@18 {
+        compatible = "tsd,pwm-mule";
+        reg = <0x18>;
+        #pwm-cells = <2>;
+      };
+    };

-- 
2.34.1

[PATCH 2/6] pwm: add mule pwm-over-i2c driver

[Farouk Bouabid]

Mule is a device that can output a PWM signal based on I2C commands.

Add pwm driver for Mule PWM-over-I2C controller.

Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
---
 drivers/pwm/Kconfig    |  10 +++++
 drivers/pwm/Makefile   |   1 +
 drivers/pwm/pwm-mule.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..eb8cfa113ec7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -425,6 +425,16 @@ config PWM_MICROCHIP_CORE
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-microchip-core.
 
+config PWM_MULE
+	tristate "Mule PWM-over-I2C support"
+	depends on I2C && OF
+	help
+	  PWM driver for Mule PWM-over-I2C controller. Mule is a device
+	  that can output a PWM signal based on I2C commands.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-mule.
+
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
 	depends on ARCH_MXS || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c5ec9e168ee7..cdd736ea3244 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
 obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
 obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
 obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
+obj-$(CONFIG_PWM_MULE)		+= pwm-mule.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
 obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
diff --git a/drivers/pwm/pwm-mule.c b/drivers/pwm/pwm-mule.c
new file mode 100644
index 000000000000..e8593a48b16e
--- /dev/null
+++ b/drivers/pwm/pwm-mule.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Mule PWM-over-I2C controller driver
+ *
+ * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+struct mule_pwm {
+	struct mutex lock;
+	struct regmap *regmap;
+};
+
+static const struct regmap_config pwm_mule_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+#define MULE_PWM_DCY_REG	0x0
+#define MULE_PWM_FREQ_L_REG	0x1	/* LSB register */
+#define MULE_PWM_FREQ_H_REG	0x2	/* MSB register */
+
+#define NANOSECONDS_TO_HZ(x) (1000000000UL/(x))
+
+static int pwm_mule_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      const struct pwm_state *state)
+{
+	struct mule_pwm *priv = pwmchip_get_drvdata(chip);
+	u8 duty_cycle;
+	u64 freq;
+	int ret;
+
+	freq = NANOSECONDS_TO_HZ(state->period);
+
+	if (freq > U16_MAX) /* Frequency is 16-bit wide */ {
+		dev_err(chip->dev,
+			"Failed to set frequency: %llu Hz: out of 16-bit range\n", freq);
+		return -EINVAL;
+	}
+
+	if (state->enabled)
+		duty_cycle = pwm_get_relative_duty_cycle(state, 100);
+	else
+		duty_cycle = 0;
+
+	mutex_lock(&priv->lock);
+
+	ret = regmap_bulk_write(priv->regmap, MULE_PWM_FREQ_L_REG, &freq, 2);
+	if (ret) {
+		dev_err(chip->dev,
+			"Failed to set frequency: %llu Hz: %d\n", freq, ret);
+		goto out;
+	}
+
+	ret = regmap_write(priv->regmap, MULE_PWM_DCY_REG, duty_cycle);
+	if (ret)
+		dev_err(chip->dev,
+			"Failed to set duty cycle: %u: %d\n", duty_cycle, ret);
+
+out:
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
+static const struct pwm_ops pwm_mule_ops = {
+	.apply = pwm_mule_apply,
+};
+
+static int pwm_mule_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct pwm_chip *chip;
+	struct mule_pwm *priv;
+
+	chip = devm_pwmchip_alloc(dev, 1, sizeof(*priv));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	priv = pwmchip_get_drvdata(chip);
+
+	mutex_init(&priv->lock);
+
+	priv->regmap = devm_regmap_init_i2c(client, &pwm_mule_config);
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(dev, PTR_ERR(priv->regmap),
+				     "Failed to allocate i2c register map\n");
+
+	chip->ops = &pwm_mule_ops;
+
+	return devm_pwmchip_add(dev, chip);
+}
+
+static const struct of_device_id pwm_mule_of_match[] = {
+	{ .compatible = "tsd,pwm-mule", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pwm_mule_of_match);
+
+static struct i2c_driver pwm_mule_driver = {
+	.driver = {
+		.name = "pwm-mule",
+		.of_match_table = pwm_mule_of_match,
+	},
+	.probe = pwm_mule_probe,
+};
+module_i2c_driver(pwm_mule_driver);
+
+MODULE_AUTHOR("Farouk Bouabid <farouk.bouabid@cherry.de>");
+MODULE_DESCRIPTION("Mule PWM driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

[PATCH 3/6] arm64: dts: rockchip: add pwm-beeper to rk3399-puma-haikou

[Farouk Bouabid]

Add PWM-beeper that uses Mule PWM-over-I2C controller on i2c-mux (0x18).

Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
---
 arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts |  5 +++++
 arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi       | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts b/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts
index f6f15946579e..3d57c606707b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts
@@ -15,6 +15,11 @@ aliases {
 		mmc1 = &sdmmc;
 	};
 
+	beeper {
+		compatible = "pwm-beeper";
+		pwms = <&mule_pwm 0 250000>;
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
index 69b57cde7d78..e3d2d27d4ca3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
@@ -11,6 +11,7 @@ / {
 	aliases {
 		ethernet0 = &gmac;
 		i2c10 = &i2c10;
+		i2c11 = &i2c11;
 		mmc0 = &sdhci;
 	};
 
@@ -395,6 +396,18 @@ fan: fan@18 {
 				#cooling-cells = <2>;
 			};
 		};
+
+		i2c11: i2c@1 {
+			reg = <0x1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			mule_pwm: pwm@18 {
+				compatible = "tsd,pwm-mule";
+				reg = <0x18>;
+				#pwm-cells = <2>;
+			};
+		};
 	};
 
 	rtc_twi: rtc@6f {

-- 
2.34.1

[PATCH 4/6] arm64: dts: rockchip: add pwm-beeper to px30-ringneck-haikou

[Farouk Bouabid]

Add PWM-beeper that uses Mule PWM-over-I2C controller on i2c-mux (0x18).

Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
---
 arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts |  5 +++++
 arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi       | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
index ae398acdcf45..513ecb382ccd 100644
--- a/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck-haikou.dts
@@ -17,6 +17,11 @@ aliases {
 		mmc2 = &sdmmc;
 	};
 
+	beeper {
+		compatible = "pwm-beeper";
+		pwms = <&mule_pwm 0 250000>;
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
index eea906379983..0eee06024484 100644
--- a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
+++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi
@@ -10,6 +10,7 @@
 / {
 	aliases {
 		i2c10 = &i2c10;
+		i2c11 = &i2c11;
 		mmc0 = &emmc;
 		mmc1 = &sdio;
 		rtc0 = &rtc_twi;
@@ -309,6 +310,18 @@ fan: fan@18 {
 				#cooling-cells = <2>;
 			};
 		};
+
+		i2c11: i2c@1 {
+			reg = <0x1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			mule_pwm: pwm@18 {
+				compatible = "tsd,pwm-mule";
+				reg = <0x18>;
+				#pwm-cells = <2>;
+			};
+		};
 	};
 
 	rtc_twi: rtc@6f {

-- 
2.34.1

[PATCH 5/6] arm64: dts: rockchip: add pwm-beeper to rk3588-tiger-haikou

[Farouk Bouabid]

Add PWM-beeper that uses Mule PWM-over-I2C controller on i2c-mux (0x18).

Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
---
 arch/arm64/boot/dts/rockchip/rk3588-tiger-haikou.dts |  6 ++++++
 arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi       | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-tiger-haikou.dts b/arch/arm64/boot/dts/rockchip/rk3588-tiger-haikou.dts
index d672198c6b64..cd5f07ef70c5 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-tiger-haikou.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-tiger-haikou.dts
@@ -16,6 +16,12 @@ aliases {
 		mmc1 = &sdmmc;
 	};
 
+
+	beeper {
+		compatible = "pwm-beeper";
+		pwms = <&mule_pwm 0 250000>;
+	};
+
 	chosen {
 		stdout-path = "serial2:115200n8";
 	};
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi
index 5ed7d51717bb..7b95bede4621 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-tiger.dtsi
@@ -13,6 +13,7 @@ / {
 
 	aliases {
 		i2c10 = &i2c10;
+		i2c11 = &i2c11;
 		mmc0 = &sdhci;
 		rtc0 = &rtc_twi;
 	};
@@ -228,6 +229,18 @@ fan: fan@18 {
 				#cooling-cells = <2>;
 			};
 		};
+
+		i2c11: i2c@1 {
+			reg = <0x1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			mule_pwm: pwm@18 {
+				compatible = "tsd,pwm-mule";
+				reg = <0x18>;
+				#pwm-cells = <2>;
+			};
+		};
 	};
 
 	rtc_twi: rtc@6f {

-- 
2.34.1

[PATCH 6/6] arm64: dts: rockchip: add pwm-beeper to rk3588-jaguar

[Farouk Bouabid]

Add PWM-beeper that uses Mule PWM-over-I2C controller on i2c-mux (0x18).

Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
---
 arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
index 14f1322c162f..c7c8683dc1ef 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
@@ -33,11 +33,17 @@ button-bios-disable {
 	aliases {
 		ethernet0 = &gmac0;
 		i2c10 = &i2c10;
+		i2c11 = &i2c11;
 		mmc0 = &sdhci;
 		mmc1 = &sdmmc;
 		rtc0 = &rtc_twi;
 	};
 
+	beeper {
+		compatible = "pwm-beeper";
+		pwms = <&mule_pwm 0 250000>;
+	};
+
 	chosen {
 		stdout-path = "serial2:115200n8";
 	};
@@ -267,6 +273,18 @@ fan: fan@18 {
 				#cooling-cells = <2>;
 			};
 		};
+
+		i2c11: i2c@1 {
+			reg = <0x1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			mule_pwm: pwm@18 {
+				compatible = "tsd,pwm-mule";
+				reg = <0x18>;
+				#pwm-cells = <2>;
+			};
+		};
 	};
 
 	vdd_npu_s0: regulator@42 {

-- 
2.34.1

Re: [PATCH 1/6] dt-bindings: pwm: add dt-bindings for mule pwm-over-i2c controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

On Wed, May 29, 2024 at 12:10:30PM +0200, Farouk Bouabid wrote:
> Mule is a device that controls a PWM output signal based on I2C commands.
> 
> Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
> ---
>  .../devicetree/bindings/pwm/tsd,pwm-mule.yaml      | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/tsd,pwm-mule.yaml b/Documentation/devicetree/bindings/pwm/tsd,pwm-mule.yaml
> new file mode 100644
> index 000000000000..71a940a2a644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/tsd,pwm-mule.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/tsd,pwm-mule.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mule PWM-over-I2C controller
> +
> +description: |

This | is not needed.

> +  A device that outputs a PWM signal based on I2C commands.
> +
> +maintainers:
> +  - Farouk Bouabid <farouk.bouabid@cherry.de>
> +  - Quentin Schulz <quentin.schulz@cherry.de>
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    const: tsd,pwm-mule
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 2

No clocks or supplies?
I tried to google for some documentation for this device, but only found
archives of this mail thread..

Thanks,
Conor.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      pwm@18 {
> +        compatible = "tsd,pwm-mule";
> +        reg = <0x18>;
> +        #pwm-cells = <2>;
> +      };
> +    };
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/6] dt-bindings: pwm: add dt-bindings for mule pwm-over-i2c controller

[Farouk Bouabid]

Hi Conor,

On 29.05.24 22:29, Conor Dooley wrote:
> On Wed, May 29, 2024 at 12:10:30PM +0200, Farouk Bouabid wrote:


...


>> +  A device that outputs a PWM signal based on I2C commands.
>> +
>> +maintainers:
>> +  - Farouk Bouabid <farouk.bouabid@cherry.de>
>> +  - Quentin Schulz <quentin.schulz@cherry.de>
>> +
>> +allOf:
>> +  - $ref: pwm.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: tsd,pwm-mule
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#pwm-cells":
>> +    const: 2
> No clocks or supplies?
> I tried to google for some documentation for this device, but only found
> archives of this mail thread..

You can't find documentation for this device because we don't sell it 
standalone. It is part of our boards.

This device handles its clock internally. So there is no need for clock. 
As for the power-supply, it is so far

always enabled but I will add a property.


Cheers,

Farouk

Re: [PATCH 2/6] pwm: add mule pwm-over-i2c driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 6757 bytes --]

Hello,

On Wed, May 29, 2024 at 12:10:31PM +0200, Farouk Bouabid wrote:
> Mule is a device that can output a PWM signal based on I2C commands.
> 
> Add pwm driver for Mule PWM-over-I2C controller.
> 
> Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
> ---
>  drivers/pwm/Kconfig    |  10 +++++
>  drivers/pwm/Makefile   |   1 +
>  drivers/pwm/pwm-mule.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 126 insertions(+)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..eb8cfa113ec7 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -425,6 +425,16 @@ config PWM_MICROCHIP_CORE
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-microchip-core.
>  
> +config PWM_MULE
> +	tristate "Mule PWM-over-I2C support"
> +	depends on I2C && OF

It would be easy to drop the hard dependency on OF. Please do that.

Given that that part doesn't seem to be available for individual sale, I
suggest to add something like:

	depends on (ARM64 && ARCH_ROCKCHIP) || COMPILE_TEST

to not annoy people configuring a kernel for x86 or s390.

> +	help
> +	  PWM driver for Mule PWM-over-I2C controller. Mule is a device
> +	  that can output a PWM signal based on I2C commands.

How is that different from a PWM that is an ordinary I2C device? If
there is no difference, I'd just call this "an I2C device". Also "can
output a PWM signal" is a given for PWM drivers :-)

> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-mule.
> +
>  config PWM_MXS
>  	tristate "Freescale MXS PWM support"
>  	depends on ARCH_MXS || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c5ec9e168ee7..cdd736ea3244 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
>  obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
>  obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
>  obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
> +obj-$(CONFIG_PWM_MULE)		+= pwm-mule.o
>  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
>  obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
>  obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
> diff --git a/drivers/pwm/pwm-mule.c b/drivers/pwm/pwm-mule.c
> new file mode 100644
> index 000000000000..e8593a48b16e
> --- /dev/null
> +++ b/drivers/pwm/pwm-mule.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Mule PWM-over-I2C controller driver
> + *
> + * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH

Is there a publicly available datasheet? I guess not. (I ask because
adding a link there to such a document would be nice.)

> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +struct mule_pwm {
> +	struct mutex lock;
> +	struct regmap *regmap;
> +};
> +
> +static const struct regmap_config pwm_mule_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +#define MULE_PWM_DCY_REG	0x0
> +#define MULE_PWM_FREQ_L_REG	0x1	/* LSB register */
> +#define MULE_PWM_FREQ_H_REG	0x2	/* MSB register */
> +
> +#define NANOSECONDS_TO_HZ(x) (1000000000UL/(x))

Don't introduce such a macro if you only use it once. Having the
division in the function results in code that is easier to read (IMHO).

> +static int pwm_mule_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      const struct pwm_state *state)
> +{
> +	struct mule_pwm *priv = pwmchip_get_drvdata(chip);
> +	u8 duty_cycle;
> +	u64 freq;
> +	int ret;
> +
> +	freq = NANOSECONDS_TO_HZ(state->period);
> +
> +	if (freq > U16_MAX) /* Frequency is 16-bit wide */ {
> +		dev_err(chip->dev,
> +			"Failed to set frequency: %llu Hz: out of 16-bit range\n", freq);
> +		return -EINVAL;
> +	}

You're supposed to configure the biggest possible period not bigger than
the requested period. So this should be:

	/*
	 * The period is configured using a 16 bit wide register holding
	 * the frequency in Hz.
	 */
	unsigned int period = min_t(u64, state->period, NSEC_PER_SEC);
	unsigned int freq = DIV_ROUND_UP(NSEC_PER_SEC, period);

	if (freq > U16_MAX)
		return -EINVAL;

> +	if (state->enabled)
> +		duty_cycle = pwm_get_relative_duty_cycle(state, 100);

This is wrong for two reasons:

 - It uses rounding to the nearest duty_cycle, however you're supposed
   to round down.
 - It uses the requested period instead of the real one.

I wonder why the hardware doesn't use the whole 8 bits here.

> +	else
> +		duty_cycle = 0;
> +
> +	mutex_lock(&priv->lock);

If you use the guard helper, you don't need to resort to goto for error
handling.

> +	ret = regmap_bulk_write(priv->regmap, MULE_PWM_FREQ_L_REG, &freq, 2);
> +	if (ret) {
> +		dev_err(chip->dev,
> +			"Failed to set frequency: %llu Hz: %d\n", freq, ret);
> +		goto out;
> +	}
> +
> +	ret = regmap_write(priv->regmap, MULE_PWM_DCY_REG, duty_cycle);
> +	if (ret)
> +		dev_err(chip->dev,
> +			"Failed to set duty cycle: %u: %d\n", duty_cycle, ret);

Please document how the hardware behaves here in a "Limitations" section
as several other drivers do. Questions to answer include: Does it
complete a period when the parameters are updated? Can it happen that a
glitch is emitted while MULE_PWM_FREQ_[LH]_REG is updated but
MULE_PWM_DCY_REG isn't yet? Maybe updating MULE_PWM_FREQ_[LH]_REG isn't
even atomic? "Doesn't support disabling, configures duty_cycle=0 when
disabled is requested."

Maybe write all three registers in a bulk write, then you might even be
able to drop the lock.

Also please fail silently.

> +out:
> +	mutex_unlock(&priv->lock);
> +	return ret;
> +}
> +
> +static const struct pwm_ops pwm_mule_ops = {
> +	.apply = pwm_mule_apply,

Is .get_state not possible to implement (then please document that with a reason)?

> +};
> +
> +static int pwm_mule_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct pwm_chip *chip;
> +	struct mule_pwm *priv;
> +
> +	chip = devm_pwmchip_alloc(dev, 1, sizeof(*priv));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	priv = pwmchip_get_drvdata(chip);
> +
> +	mutex_init(&priv->lock);
> +
> +	priv->regmap = devm_regmap_init_i2c(client, &pwm_mule_config);
> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
> +				     "Failed to allocate i2c register map\n");
> +
> +	chip->ops = &pwm_mule_ops;
> +
> +	return devm_pwmchip_add(dev, chip);

Error message if this fails, please.

> +}

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/6] pwm: add mule pwm-over-i2c driver

[Quentin Schulz]

Hi Uwe,

Answering since Farouk's on PTO.

Just to clarify, below when I say HW, I mean the actual HW, the MCU 
basically. There are two flavours of those, ATtiny816 and STM32F072CB. 
They need to be swappable, so same API. When I saw FW, I mean the 
firmware running on both MCUs (though we do have two different FW, one 
for each MCU, but they expose the same API and we expect the same 
behavior, which can be challenging).

The FW is only supposed to run on Cherry products fitted with one of 
those MCUs, we do not plan on selling them separately or releasing the 
FW for consumption in other devices. As such, there's no need on our 
side for public documentation. I'll try to answer the FW questions to 
the best of my ability though.

On 7/12/24 10:54 AM, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, May 29, 2024 at 12:10:31PM +0200, Farouk Bouabid wrote:
>> Mule is a device that can output a PWM signal based on I2C commands.
>>
>> Add pwm driver for Mule PWM-over-I2C controller.
>>
>> Signed-off-by: Farouk Bouabid <farouk.bouabid@cherry.de>
>> ---
>>   drivers/pwm/Kconfig    |  10 +++++
>>   drivers/pwm/Makefile   |   1 +
>>   drivers/pwm/pwm-mule.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 126 insertions(+)
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 4b956d661755..eb8cfa113ec7 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -425,6 +425,16 @@ config PWM_MICROCHIP_CORE
>>   	  To compile this driver as a module, choose M here: the module
>>   	  will be called pwm-microchip-core.
>>   
>> +config PWM_MULE
>> +	tristate "Mule PWM-over-I2C support"
>> +	depends on I2C && OF
> 
> It would be easy to drop the hard dependency on OF. Please do that.
> 

Just being curious here, what would be the benefit?

[...]

>> diff --git a/drivers/pwm/pwm-mule.c b/drivers/pwm/pwm-mule.c
>> new file mode 100644
>> index 000000000000..e8593a48b16e
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-mule.c
>> @@ -0,0 +1,115 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Mule PWM-over-I2C controller driver
>> + *
>> + * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH
> 
> Is there a publicly available datasheet? I guess not. (I ask because
> adding a link there to such a document would be nice.)
> 

Unfortunately no. It's also only part of our product line and there's no 
plan to start selling it standalone or selling the IP.

>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/pwm.h>
>> +#include <linux/regmap.h>
>> +
>> +struct mule_pwm {
>> +	struct mutex lock;
>> +	struct regmap *regmap;
>> +};
>> +
>> +static const struct regmap_config pwm_mule_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +};
>> +
>> +#define MULE_PWM_DCY_REG	0x0
>> +#define MULE_PWM_FREQ_L_REG	0x1	/* LSB register */
>> +#define MULE_PWM_FREQ_H_REG	0x2	/* MSB register */
>> +
>> +#define NANOSECONDS_TO_HZ(x) (1000000000UL/(x))
> 
> Don't introduce such a macro if you only use it once. Having the
> division in the function results in code that is easier to read (IMHO).
> 
>> +static int pwm_mule_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			      const struct pwm_state *state)
>> +{
>> +	struct mule_pwm *priv = pwmchip_get_drvdata(chip);
>> +	u8 duty_cycle;
>> +	u64 freq;
>> +	int ret;
>> +
>> +	freq = NANOSECONDS_TO_HZ(state->period);
>> +
>> +	if (freq > U16_MAX) /* Frequency is 16-bit wide */ {
>> +		dev_err(chip->dev,
>> +			"Failed to set frequency: %llu Hz: out of 16-bit range\n", freq);
>> +		return -EINVAL;
>> +	}
> 
> You're supposed to configure the biggest possible period not bigger than
> the requested period. So this should be:
> 
> 	/*
> 	 * The period is configured using a 16 bit wide register holding
> 	 * the frequency in Hz.
> 	 */
> 	unsigned int period = min_t(u64, state->period, NSEC_PER_SEC);
> 	unsigned int freq = DIV_ROUND_UP(NSEC_PER_SEC, period);
> 
> 	if (freq > U16_MAX)
> 		return -EINVAL;
> 
>> +	if (state->enabled)
>> +		duty_cycle = pwm_get_relative_duty_cycle(state, 100);
> 
> This is wrong for two reasons:
> 
>   - It uses rounding to the nearest duty_cycle, however you're supposed
>     to round down.
>   - It uses the requested period instead of the real one.
> 

I assume you want:

unsigned int real_period = ((u64) NSEC_PER_SEC * 100) / freq;

which rounds down?

> I wonder why the hardware doesn't use the whole 8 bits here.
> 

It's even a 16b register that the HW uses. I guess we just went with the 
most human-friendly API :) I believe it's something we should be able to 
change in the FW before releasing if this is something that really makes 
sense. FYI, the register stores the number of clock ticks for the signal 
to be high, once reached, put it low (or the opposite). So it's 
necessarily a fraction of the clock frequency. 100% was easy because we 
know that every clock frequency we support is a multiple of 100 so 
there's no issue around rounding for example since we definitely do not 
want to do float maths in MCUs :)

>> +	else
>> +		duty_cycle = 0;
>> +
>> +	mutex_lock(&priv->lock);
> 
> If you use the guard helper, you don't need to resort to goto for error
> handling.
> 

I would have liked a link or more precise hint at what this "guard 
helper" was, but found something myself so here it is for anyone wondering:

https://lwn.net/Articles/934679/

Had never heard of that one before, neat!

The suggested implementation would then be:

guard(mutex)(&priv->lock);

I believe.

>> +	ret = regmap_bulk_write(priv->regmap, MULE_PWM_FREQ_L_REG, &freq, 2);
>> +	if (ret) {
>> +		dev_err(chip->dev,
>> +			"Failed to set frequency: %llu Hz: %d\n", freq, ret);
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(priv->regmap, MULE_PWM_DCY_REG, duty_cycle);
>> +	if (ret)
>> +		dev_err(chip->dev,
>> +			"Failed to set duty cycle: %u: %d\n", duty_cycle, ret);
> 
> Please document how the hardware behaves here in a "Limitations" section
> as several other drivers do. Questions to answer include: Does it
> complete a period when the parameters are updated? Can it happen that a
> glitch is emitted while MULE_PWM_FREQ_[LH]_REG is updated but
> MULE_PWM_DCY_REG isn't yet? Maybe updating MULE_PWM_FREQ_[LH]_REG isn't
> even atomic? "Doesn't support disabling, configures duty_cycle=0 when
> disabled is requested."
> 

Updating MULE_PWM_FREQ_[LH]_REG is atomic (L is stored in SRAM until H 
reg is written, when LH are then written to the hardware IP).

We use double-buffering (supported by the HW directly) for the period 
and comparator registers. I believe we still have a possible glitch 
during a small time-frame in the current version of the FW. Basically, 
trying to change the period AND duty cycle at the same time, the 
following could happen:

- period A + duty-cycle AA
- period B + duty-cycle AA
- period B + duty-cycle BB

Depending on what we consider a glitch, the second element in the list 
could be one. Even if we do a multibyte write to the actual HW, I'm not 
sure if this window can be eliminated.

To give a bit more info on this, there are two possible flavors of the 
MCU, ATtiny 816 (datasheet: 
https://ww1.microchip.com/downloads/en/DeviceDoc/ATtiny416-816-DataSheet-DS40001913B.pdf) 
and STM32F072CB (datasheet: 
https://www.st.com/content/ccc/resource/technical/document/reference_manual/c2/f8/8a/f2/18/e6/43/96/DM00031936.pdf/files/DM00031936.pdf/jcr:content/translations/en.DM00031936.pdf).

FYI, on ATtiny, we use TCA in single-slope PWM generation mode and 
PERBUF and CMP2BUF as period and duty-cycle registers. On STM32, we use 
TIM15 in PWM mode and ARR and CCR1 as period and duty-cycle registers.

Re-reading both datasheets, and if I understand correctly, we could have 
glitch-free transitions by controlling the ARPE bit on STM32 and LUPD 
bit on ATtiny816.

@Farouk, please confirm but the above makes sense to me and I guess we 
could implement something in the FW. The question is how to detect if we 
want to change both the duty-cycle and period at the same time (we could 
decide to document this as a requirement for the API user though: 
"changes to MULE_PWM_FREQ_[LH]_REG are only applied when 
MULE_PWM_DCY_REG is written to").

> Maybe write all three registers in a bulk write, then you might even be
> able to drop the lock.
> 

The bulk write wouldn't help with the glitch, but it's a good idea for 
getting rid of the lock.

> Also please fail silently.
> 

Would dev_dbg() be fine here or would you rather see them gone entirely?

Cheers,
Quentin

Re: [PATCH 2/6] pwm: add mule pwm-over-i2c driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 8422 bytes --]

Hello Quentin,

On Mon, Jul 15, 2024 at 02:16:15PM +0200, Quentin Schulz wrote:
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -425,6 +425,16 @@ config PWM_MICROCHIP_CORE
> > >   	  To compile this driver as a module, choose M here: the module
> > >   	  will be called pwm-microchip-core.
> > > +config PWM_MULE
> > > +	tristate "Mule PWM-over-I2C support"
> > > +	depends on I2C && OF
> > 
> > It would be easy to drop the hard dependency on OF. Please do that.
> > 
> 
> Just being curious here, what would be the benefit?

Increasing easy compile coverage.

> [...]
> 
> > > diff --git a/drivers/pwm/pwm-mule.c b/drivers/pwm/pwm-mule.c
> > > new file mode 100644
> > > index 000000000000..e8593a48b16e
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-mule.c
> > > @@ -0,0 +1,115 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Mule PWM-over-I2C controller driver
> > > + *
> > > + * Copyright (C) 2024 Theobroma Systems Design und Consulting GmbH
> > 
> > Is there a publicly available datasheet? I guess not. (I ask because
> > adding a link there to such a document would be nice.)
> > 
> 
> Unfortunately no. It's also only part of our product line and there's no
> plan to start selling it standalone or selling the IP.
> 
> > > + */
> > > +
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +struct mule_pwm {
> > > +	struct mutex lock;
> > > +	struct regmap *regmap;
> > > +};
> > > +
> > > +static const struct regmap_config pwm_mule_config = {
> > > +	.reg_bits = 8,
> > > +	.val_bits = 8,
> > > +};
> > > +
> > > +#define MULE_PWM_DCY_REG	0x0
> > > +#define MULE_PWM_FREQ_L_REG	0x1	/* LSB register */
> > > +#define MULE_PWM_FREQ_H_REG	0x2	/* MSB register */
> > > +
> > > +#define NANOSECONDS_TO_HZ(x) (1000000000UL/(x))
> > 
> > Don't introduce such a macro if you only use it once. Having the
> > division in the function results in code that is easier to read (IMHO).
> > 
> > > +static int pwm_mule_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			      const struct pwm_state *state)
> > > +{
> > > +	struct mule_pwm *priv = pwmchip_get_drvdata(chip);
> > > +	u8 duty_cycle;
> > > +	u64 freq;
> > > +	int ret;
> > > +
> > > +	freq = NANOSECONDS_TO_HZ(state->period);
> > > +
> > > +	if (freq > U16_MAX) /* Frequency is 16-bit wide */ {
> > > +		dev_err(chip->dev,
> > > +			"Failed to set frequency: %llu Hz: out of 16-bit range\n", freq);
> > > +		return -EINVAL;
> > > +	}
> > 
> > You're supposed to configure the biggest possible period not bigger than
> > the requested period. So this should be:
> > 
> > 	/*
> > 	 * The period is configured using a 16 bit wide register holding
> > 	 * the frequency in Hz.
> > 	 */
> > 	unsigned int period = min_t(u64, state->period, NSEC_PER_SEC);
> > 	unsigned int freq = DIV_ROUND_UP(NSEC_PER_SEC, period);
> > 
> > 	if (freq > U16_MAX)
> > 		return -EINVAL;
> > 
> > > +	if (state->enabled)
> > > +		duty_cycle = pwm_get_relative_duty_cycle(state, 100);
> > 
> > This is wrong for two reasons:
> > 
> >   - It uses rounding to the nearest duty_cycle, however you're supposed
> >     to round down.
> >   - It uses the requested period instead of the real one.
> > 
> 
> I assume you want:
> 
> unsigned int real_period = ((u64) NSEC_PER_SEC * 100) / freq;
> 
> which rounds down?

Yes. And then to calculate the duty_cycle setting use real_period.

> > I wonder why the hardware doesn't use the whole 8 bits here.
> > 
> 
> It's even a 16b register that the HW uses. I guess we just went with the
> most human-friendly API :) I believe it's something we should be able to
> change in the FW before releasing if this is something that really makes
> sense. FYI, the register stores the number of clock ticks for the signal to
> be high, once reached, put it low (or the opposite). So it's necessarily a
> fraction of the clock frequency. 100% was easy because we know that every
> clock frequency we support is a multiple of 100 so there's no issue around
> rounding for example since we definitely do not want to do float maths in
> MCUs :)

Interesting perspective. I'd still go for using the register completely.

> > > +	else
> > > +		duty_cycle = 0;
> > > +
> > > +	mutex_lock(&priv->lock);
> > 
> > If you use the guard helper, you don't need to resort to goto for error
> > handling.
> > 
> 
> I would have liked a link or more precise hint at what this "guard helper"
> was, but found something myself so here it is for anyone wondering:
> 
> https://lwn.net/Articles/934679/
> 
> Had never heard of that one before, neat!

Right. A conversion example is available at
https://lore.kernel.org/linux-pwm/2102fe8189bdf1f02ff3785b551a69be27a65af4.1719520143.git.u.kleine-koenig@baylibre.com/

> > > +	ret = regmap_bulk_write(priv->regmap, MULE_PWM_FREQ_L_REG, &freq, 2);
> > > +	if (ret) {
> > > +		dev_err(chip->dev,
> > > +			"Failed to set frequency: %llu Hz: %d\n", freq, ret);
> > > +		goto out;
> > > +	}
> > > +
> > > +	ret = regmap_write(priv->regmap, MULE_PWM_DCY_REG, duty_cycle);
> > > +	if (ret)
> > > +		dev_err(chip->dev,
> > > +			"Failed to set duty cycle: %u: %d\n", duty_cycle, ret);
> > 
> > Please document how the hardware behaves here in a "Limitations" section
> > as several other drivers do. Questions to answer include: Does it
> > complete a period when the parameters are updated? Can it happen that a
> > glitch is emitted while MULE_PWM_FREQ_[LH]_REG is updated but
> > MULE_PWM_DCY_REG isn't yet? Maybe updating MULE_PWM_FREQ_[LH]_REG isn't
> > even atomic? "Doesn't support disabling, configures duty_cycle=0 when
> > disabled is requested."
> > 
> 
> Updating MULE_PWM_FREQ_[LH]_REG is atomic (L is stored in SRAM until H reg
> is written, when LH are then written to the hardware IP).
> 
> We use double-buffering (supported by the HW directly) for the period and
> comparator registers. I believe we still have a possible glitch during a
> small time-frame in the current version of the FW. Basically, trying to
> change the period AND duty cycle at the same time, the following could
> happen:
> 
> - period A + duty-cycle AA
> - period B + duty-cycle AA
> - period B + duty-cycle BB
> 
> Depending on what we consider a glitch, the second element in the list could
> be one. Even if we do a multibyte write to the actual HW, I'm not sure if
> this window can be eliminated.

I'd call that a glitch, yes.

> To give a bit more info on this, there are two possible flavors of the MCU,
> ATtiny 816 (datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/ATtiny416-816-DataSheet-DS40001913B.pdf)
> and STM32F072CB (datasheet: https://www.st.com/content/ccc/resource/technical/document/reference_manual/c2/f8/8a/f2/18/e6/43/96/DM00031936.pdf/files/DM00031936.pdf/jcr:content/translations/en.DM00031936.pdf).
> 
> FYI, on ATtiny, we use TCA in single-slope PWM generation mode and PERBUF
> and CMP2BUF as period and duty-cycle registers. On STM32, we use TIM15 in
> PWM mode and ARR and CCR1 as period and duty-cycle registers.

Wouldn't it be more natural with these to have duty in a base-2 register
for duty, in the assumption that your MCUs habe this, too?

> Re-reading both datasheets, and if I understand correctly, we could have
> glitch-free transitions by controlling the ARPE bit on STM32 and LUPD bit on
> ATtiny816.
> 
> @Farouk, please confirm but the above makes sense to me and I guess we could
> implement something in the FW. The question is how to detect if we want to
> change both the duty-cycle and period at the same time (we could decide to
> document this as a requirement for the API user though: "changes to
> MULE_PWM_FREQ_[LH]_REG are only applied when MULE_PWM_DCY_REG is written
> to").
> 
> > Maybe write all three registers in a bulk write, then you might even be
> > able to drop the lock.
> > 
> 
> The bulk write wouldn't help with the glitch, but it's a good idea for
> getting rid of the lock.
> 
> > Also please fail silently.
> 
> Would dev_dbg() be fine here or would you rather see them gone entirely?

dev_dbg is silent by default, so that's fine for me.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/6] pwm: add mule pwm-over-i2c driver

[Quentin Schulz]

Hi Uwe,

On 7/15/24 5:09 PM, Uwe Kleine-König wrote:
> Hello Quentin,
> 
> On Mon, Jul 15, 2024 at 02:16:15PM +0200, Quentin Schulz wrote:

[...]

>> To give a bit more info on this, there are two possible flavors of the MCU,
>> ATtiny 816 (datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/ATtiny416-816-DataSheet-DS40001913B.pdf)
>> and STM32F072CB (datasheet: https://www.st.com/content/ccc/resource/technical/document/reference_manual/c2/f8/8a/f2/18/e6/43/96/DM00031936.pdf/files/DM00031936.pdf/jcr:content/translations/en.DM00031936.pdf).
>>
>> FYI, on ATtiny, we use TCA in single-slope PWM generation mode and PERBUF
>> and CMP2BUF as period and duty-cycle registers. On STM32, we use TIM15 in
>> PWM mode and ARR and CCR1 as period and duty-cycle registers.
> 
> Wouldn't it be more natural with these to have duty in a base-2 register
> for duty, in the assumption that your MCUs habe this, too?
> 

Not sure to understand what you meant by base-2 register here? I am 
guessing you rather wanted to suggest a different unit/representation of 
the duty cycle in the register in the FW API?

Cheers,
Quentin

Re: [PATCH 2/6] pwm: add mule pwm-over-i2c driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

Hello Quentin,

On Wed, Jul 17, 2024 at 10:48:52AM +0200, Quentin Schulz wrote:
> On 7/15/24 5:09 PM, Uwe Kleine-König wrote:
> > On Mon, Jul 15, 2024 at 02:16:15PM +0200, Quentin Schulz wrote:
> > > To give a bit more info on this, there are two possible flavors of the MCU,
> > > ATtiny 816 (datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/ATtiny416-816-DataSheet-DS40001913B.pdf)
> > > and STM32F072CB (datasheet: https://www.st.com/content/ccc/resource/technical/document/reference_manual/c2/f8/8a/f2/18/e6/43/96/DM00031936.pdf/files/DM00031936.pdf/jcr:content/translations/en.DM00031936.pdf).
> > > 
> > > FYI, on ATtiny, we use TCA in single-slope PWM generation mode and PERBUF
> > > and CMP2BUF as period and duty-cycle registers. On STM32, we use TIM15 in
> > > PWM mode and ARR and CCR1 as period and duty-cycle registers.
> > 
> > Wouldn't it be more natural with these to have duty in a base-2 register
> > for duty, in the assumption that your MCUs habe this, too?
> 
> Not sure to understand what you meant by base-2 register here? I am guessing
> you rather wanted to suggest a different unit/representation of the duty
> cycle in the register in the FW API?

For humans 100 as maximal value for a register is natural, but hardware
usually uses binary representation ("base-2") for values and usually a
register (or bit field) is used completely. That is, valid values range
beween 0 and 2^n (or 2^n - 1) for some n.

Note this discussion isn't really relevant to the driver. Just me
wondering about the hardware design. So if you don't want to follow up,
that's fine for me.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]