[PATCH v2 0/5] Add PWM for MStar SoCs

[PATCH v2 0/5] Add PWM for MStar SoCs

[Romain Perier]

This patches series adds a new driver for the PWM found in the Mstar
MSC313e SoCs and newer. It adds a basic pwm driver, the corresponding
devicetree bindings and its documentation.

Changes since v1:
- Fixed commit message for the dt-bindings doc
- Removed "OneOf" from the dt-bindings doc
- Re-ordered alphabetically in Kconfig and remove
  unseless empty lines
- Explain and adds comment in _writecounter() (hw
  constrainst)
- Reworked the msc313e_pwm_config() function
- Fixed clk handling
- Removed extra callbacks, only keep .apply and .get_state
- Implement .get_state completly, this fixes the driver with PWM_DEBUG
  (the whole driver has been tested with PWM_DEBUG).
- Dropped useless lines in _probe
- I have kept regmap_field() because it is more clean and helpful, it
  avoids to do too much of offset and mask and shift all over the place.

Daniel Palmer (1):
  pwm: Add support for the MSTAR MSC313 PWM

Romain Perier (4):
  dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings
    documentation
  ARM: dts: mstar: Add pwm device node to infinity
  ARM: dts: mstar: Add pwm device node to infinity3
  ARM: dts: mstar: Add pwm device node to infinity2m

 .../bindings/pwm/mstar,msc313e-pwm.yaml       |  46 +++
 MAINTAINERS                                   |   1 +
 arch/arm/boot/dts/mstar-infinity.dtsi         |  10 +
 arch/arm/boot/dts/mstar-infinity2m.dtsi       |   8 +
 arch/arm/boot/dts/mstar-infinity3.dtsi        |  10 +
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-msc313e.c                     | 269 ++++++++++++++++++
 8 files changed, 355 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
 create mode 100644 drivers/pwm/pwm-msc313e.c

-- 
2.35.1

[PATCH v2 0/5] Add PWM for MStar SoCs

[Romain Perier]

This patches series adds a new driver for the PWM found in the Mstar
MSC313e SoCs and newer. It adds a basic pwm driver, the corresponding
devicetree bindings and its documentation.

Changes since v1:
- Fixed commit message for the dt-bindings doc
- Removed "OneOf" from the dt-bindings doc
- Re-ordered alphabetically in Kconfig and remove
  unseless empty lines
- Explain and adds comment in _writecounter() (hw
  constrainst)
- Reworked the msc313e_pwm_config() function
- Fixed clk handling
- Removed extra callbacks, only keep .apply and .get_state
- Implement .get_state completly, this fixes the driver with PWM_DEBUG
  (the whole driver has been tested with PWM_DEBUG).
- Dropped useless lines in _probe
- I have kept regmap_field() because it is more clean and helpful, it
  avoids to do too much of offset and mask and shift all over the place.

Daniel Palmer (1):
  pwm: Add support for the MSTAR MSC313 PWM

Romain Perier (4):
  dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings
    documentation
  ARM: dts: mstar: Add pwm device node to infinity
  ARM: dts: mstar: Add pwm device node to infinity3
  ARM: dts: mstar: Add pwm device node to infinity2m

 .../bindings/pwm/mstar,msc313e-pwm.yaml       |  46 +++
 MAINTAINERS                                   |   1 +
 arch/arm/boot/dts/mstar-infinity.dtsi         |  10 +
 arch/arm/boot/dts/mstar-infinity2m.dtsi       |   8 +
 arch/arm/boot/dts/mstar-infinity3.dtsi        |  10 +
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-msc313e.c                     | 269 ++++++++++++++++++
 8 files changed, 355 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
 create mode 100644 drivers/pwm/pwm-msc313e.c

-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/5] dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings documentation

[Romain Perier]

This adds the documentation for the devicetree bindings of the Mstar
MSC313e PWM driver, it includes MSC313e SoCs and SSD20xd.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 .../bindings/pwm/mstar,msc313e-pwm.yaml       | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml b/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
new file mode 100644
index 000000000000..07f3f576f21b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.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/mstar,msc313e-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mstar MSC313e PWM controller
+
+allOf:
+  - $ref: "pwm.yaml#"
+
+maintainers:
+  - Daniel Palmer <daniel@0x0f.com>
+  - Romain Perier <romain.perier@gmail.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - mstar,msc313e-pwm
+          - mstar,ssd20xd-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm: pwm@3400 {
+      compatible = "mstar,msc313e-pwm";
+      reg = <0x3400 0x400>;
+      #pwm-cells = <2>;
+      clocks = <&xtal_div2>;
+    };
-- 
2.35.1

[PATCH v2 1/5] dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings documentation

[Romain Perier]

This adds the documentation for the devicetree bindings of the Mstar
MSC313e PWM driver, it includes MSC313e SoCs and SSD20xd.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 .../bindings/pwm/mstar,msc313e-pwm.yaml       | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml b/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
new file mode 100644
index 000000000000..07f3f576f21b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.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/mstar,msc313e-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mstar MSC313e PWM controller
+
+allOf:
+  - $ref: "pwm.yaml#"
+
+maintainers:
+  - Daniel Palmer <daniel@0x0f.com>
+  - Romain Perier <romain.perier@gmail.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - mstar,msc313e-pwm
+          - mstar,ssd20xd-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm: pwm@3400 {
+      compatible = "mstar,msc313e-pwm";
+      reg = <0x3400 0x400>;
+      #pwm-cells = <2>;
+      clocks = <&xtal_div2>;
+    };
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/5] dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings documentation

[Rob Herring]

On Wed, 07 Sep 2022 15:12:37 +0200, Romain Perier wrote:
> This adds the documentation for the devicetree bindings of the Mstar
> MSC313e PWM driver, it includes MSC313e SoCs and SSD20xd.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  .../bindings/pwm/mstar,msc313e-pwm.yaml       | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 1/5] dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings documentation

[Rob Herring]

On Wed, 07 Sep 2022 15:12:37 +0200, Romain Perier wrote:
> This adds the documentation for the devicetree bindings of the Mstar
> MSC313e PWM driver, it includes MSC313e SoCs and SSD20xd.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  .../bindings/pwm/mstar,msc313e-pwm.yaml       | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/5] dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings documentation

[Krzysztof Kozlowski]

On 07/09/2022 09:12, Romain Perier wrote:
> This adds the documentation for the devicetree bindings of the Mstar
> MSC313e PWM driver, it includes MSC313e SoCs and SSD20xd.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  .../bindings/pwm/mstar,msc313e-pwm.yaml       | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml b/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
> new file mode 100644
> index 000000000000..07f3f576f21b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.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/mstar,msc313e-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mstar MSC313e PWM controller
> +
> +allOf:
> +  - $ref: "pwm.yaml#"
> +
> +maintainers:
> +  - Daniel Palmer <daniel@0x0f.com>
> +  - Romain Perier <romain.perier@gmail.com>
> +
> +properties:
> +  compatible:
> +    items:

Drop items, you have only one item.

Would be nice if you used Linux tools for getting list of people to Cc...

Best regards,
Krzysztof

Re: [PATCH v2 1/5] dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings documentation

[Krzysztof Kozlowski]

On 07/09/2022 09:12, Romain Perier wrote:
> This adds the documentation for the devicetree bindings of the Mstar
> MSC313e PWM driver, it includes MSC313e SoCs and SSD20xd.
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  .../bindings/pwm/mstar,msc313e-pwm.yaml       | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml b/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
> new file mode 100644
> index 000000000000..07f3f576f21b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.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/mstar,msc313e-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mstar MSC313e PWM controller
> +
> +allOf:
> +  - $ref: "pwm.yaml#"
> +
> +maintainers:
> +  - Daniel Palmer <daniel@0x0f.com>
> +  - Romain Perier <romain.perier@gmail.com>
> +
> +properties:
> +  compatible:
> +    items:

Drop items, you have only one item.

Would be nice if you used Linux tools for getting list of people to Cc...

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Romain Perier]

From: Daniel Palmer <daniel@0x0f.com>

This adds support for the PWM block on the Mstar MSC313e SoCs and newer.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
Co-developed-by: Romain Perier <romain.perier@gmail.com>
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 MAINTAINERS               |   1 +
 drivers/pwm/Kconfig       |   9 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 280 insertions(+)
 create mode 100644 drivers/pwm/pwm-msc313e.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9d7f64dc0efe..c3b39b09097c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2439,6 +2439,7 @@ F:	arch/arm/mach-mstar/
 F:	drivers/clk/mstar/
 F:	drivers/clocksource/timer-msc313e.c
 F:	drivers/gpio/gpio-msc313.c
+F:	drivers/pwm/pwm-msc313e.c
 F:	drivers/rtc/rtc-msc313.c
 F:	drivers/watchdog/msc313e_wdt.c
 F:	include/dt-bindings/clock/mstar-*
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..8049fd03a821 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -372,6 +372,15 @@ config PWM_MESON
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-meson.
 
+config PWM_MSC313E
+	tristate "MStar MSC313e PWM support"
+	depends on ARCH_MSTARV7 || COMPILE_TEST
+	help
+	  Generic PWM framework driver for MSTAR MSC313e.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-msc313e.
+
 config PWM_MTK_DISP
 	tristate "MediaTek display PWM driver"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7bf1a29f02b8..bc285c054f2a 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
+obj-$(CONFIG_PWM_MSC313E)	+= pwm-msc313e.o
 obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
new file mode 100644
index 000000000000..a71f39ba66c3
--- /dev/null
+++ b/drivers/pwm/pwm-msc313e.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
+ * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/of_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define DRIVER_NAME "msc313e-pwm"
+
+#define CHANNEL_OFFSET	0x80
+#define REG_DUTY	0x8
+#define REG_PERIOD	0x10
+#define REG_DIV		0x18
+#define REG_CTRL	0x1c
+#define REG_SWRST	0x1fc
+
+struct msc313e_pwm_channel {
+	struct regmap_field *clkdiv;
+	struct regmap_field *polarity;
+	struct regmap_field *dutyl;
+	struct regmap_field *dutyh;
+	struct regmap_field *periodl;
+	struct regmap_field *periodh;
+	struct regmap_field *swrst;
+};
+
+struct msc313e_pwm {
+	struct regmap *regmap;
+	struct pwm_chip pwmchip;
+	struct clk *clk;
+	struct msc313e_pwm_channel channels[];
+};
+
+struct msc313e_pwm_info {
+	unsigned int channels;
+};
+
+#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
+
+static const struct regmap_config msc313e_pwm_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 16,
+	.reg_stride = 4,
+};
+
+static const struct msc313e_pwm_info msc313e_data = {
+	.channels = 8,
+};
+
+static const struct msc313e_pwm_info ssd20xd_data = {
+	.channels = 4,
+};
+
+static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
+{
+	/* The bus that connects the CPU to the peripheral registers splits 32 bit registers into
+	 * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter
+	 * we are about to write has this contrainst.
+	 */
+	regmap_field_write(low, value & 0xffff);
+	regmap_field_write(high, value >> 16);
+}
+
+static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value)
+{
+	unsigned int val = 0;
+
+	regmap_field_read(low, &val);
+	*value = val;
+	regmap_field_read(high, &val);
+	*value = (val << 16) | *value;
+}
+
+static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device,
+			      int duty_ns, int period_ns)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	unsigned long long div = 1;
+
+	/* Fit the period into the period register by prescaling the clk */
+	while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) {
+		div++;
+		if (div > (0xffff + 1)) {
+			/* Force clk div to the maximum allowed value */
+			div = 0xffff;
+			break;
+		}
+		nspertick = DIV_ROUND_DOWN_ULL(nspertick, div);
+	}
+
+	regmap_field_write(channel->clkdiv, div - 1);
+	msc313e_pwm_writecounter(channel->dutyl, channel->dutyh,
+				 DIV_ROUND_DOWN_ULL(duty_ns, nspertick));
+	msc313e_pwm_writecounter(channel->periodl, channel->periodh,
+				 DIV_ROUND_DOWN_ULL(period_ns, nspertick));
+	return 0;
+};
+
+static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device,
+				    enum pwm_polarity polarity)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	unsigned int pol = 0;
+
+	if (polarity == PWM_POLARITY_INVERSED)
+		pol = 1;
+	regmap_field_update_bits(channel->polarity, 1, pol);
+
+	return 0;
+}
+
+static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	int ret;
+
+	ret = clk_prepare_enable(pwm->clk);
+	if (ret)
+		return ret;
+	return regmap_field_write(channel->swrst, 0);
+}
+
+static int msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	int ret;
+
+	ret = regmap_field_write(channel->swrst, 1);
+	clk_disable_unprepare(pwm->clk);
+	return ret;
+}
+
+static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	int ret;
+
+	if (state->enabled) {
+		if (!pwm->state.enabled) {
+			ret = msc313e_pwm_enable(chip, pwm);
+			if (ret)
+				return ret;
+		}
+		msc313e_pwm_set_polarity(chip, pwm, state->polarity);
+		msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	} else if (pwm->state.enabled) {
+		ret = msc313e_pwm_disable(chip, pwm);
+	}
+	return 0;
+}
+
+static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device,
+			      struct pwm_state *state)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
+	unsigned int val = 0;
+
+	regmap_field_read(channel->polarity, &val);
+	state->polarity = val ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+
+	regmap_field_read(channel->swrst, &val);
+	state->enabled = val == 0 ? true : false;
+
+	msc313e_pwm_readcounter(channel->dutyl, channel->dutyh, &val);
+	state->duty_cycle = val * nspertick;
+
+	msc313e_pwm_readcounter(channel->periodl, channel->periodh, &val);
+	state->period = val * nspertick;
+}
+
+static const struct pwm_ops msc313e_pwm_ops = {
+	.apply = msc313e_apply,
+	.get_state = msc313e_get_state,
+	.owner = THIS_MODULE
+};
+
+static int msc313e_pwm_probe(struct platform_device *pdev)
+{
+	const struct msc313e_pwm_info *match_data;
+	struct device *dev = &pdev->dev;
+	struct msc313e_pwm *pwm;
+	__iomem void *base;
+	int i;
+
+	match_data = of_device_get_match_data(dev);
+	if (!match_data)
+		return -EINVAL;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n");
+
+	pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config);
+	if (IS_ERR(pwm->regmap))
+		return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n");
+
+	for (i = 0; i < match_data->channels; i++) {
+		unsigned int offset = CHANNEL_OFFSET * i;
+		struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7);
+		struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4);
+		struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15);
+		struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2);
+		struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15);
+		struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2);
+		struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i);
+
+		pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap,
+								  div_clkdiv_field);
+		pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap,
+								    ctrl_polarity_field);
+		pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field);
+		pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field);
+		pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field);
+		pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field);
+		pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field);
+
+		/* Channels are enabled on boot, disable it until the pwm subsystem re-enable it
+		 * explicitly
+		 */
+		regmap_field_write(pwm->channels[i].swrst, 1);
+	}
+
+	pwm->pwmchip.dev = dev;
+	pwm->pwmchip.ops = &msc313e_pwm_ops;
+	pwm->pwmchip.npwm = match_data->channels;
+
+	platform_set_drvdata(pdev, pwm);
+
+	return devm_pwmchip_add(dev, &pwm->pwmchip);
+}
+
+static const struct of_device_id msc313e_pwm_dt_ids[] = {
+	{ .compatible = "mstar,msc313e-pwm", .data = &msc313e_data },
+	{ .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data },
+	{},
+};
+MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids);
+
+static struct platform_driver msc313e_pwm_driver = {
+	.probe = msc313e_pwm_probe,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = msc313e_pwm_dt_ids,
+	},
+};
+module_platform_driver(msc313e_pwm_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Mstar MSC313e PWM driver");
+MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
-- 
2.35.1

[PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Romain Perier]

From: Daniel Palmer <daniel@0x0f.com>

This adds support for the PWM block on the Mstar MSC313e SoCs and newer.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
Co-developed-by: Romain Perier <romain.perier@gmail.com>
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 MAINTAINERS               |   1 +
 drivers/pwm/Kconfig       |   9 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 280 insertions(+)
 create mode 100644 drivers/pwm/pwm-msc313e.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9d7f64dc0efe..c3b39b09097c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2439,6 +2439,7 @@ F:	arch/arm/mach-mstar/
 F:	drivers/clk/mstar/
 F:	drivers/clocksource/timer-msc313e.c
 F:	drivers/gpio/gpio-msc313.c
+F:	drivers/pwm/pwm-msc313e.c
 F:	drivers/rtc/rtc-msc313.c
 F:	drivers/watchdog/msc313e_wdt.c
 F:	include/dt-bindings/clock/mstar-*
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..8049fd03a821 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -372,6 +372,15 @@ config PWM_MESON
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-meson.
 
+config PWM_MSC313E
+	tristate "MStar MSC313e PWM support"
+	depends on ARCH_MSTARV7 || COMPILE_TEST
+	help
+	  Generic PWM framework driver for MSTAR MSC313e.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-msc313e.
+
 config PWM_MTK_DISP
 	tristate "MediaTek display PWM driver"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7bf1a29f02b8..bc285c054f2a 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
 obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
 obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
 obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
+obj-$(CONFIG_PWM_MSC313E)	+= pwm-msc313e.o
 obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
new file mode 100644
index 000000000000..a71f39ba66c3
--- /dev/null
+++ b/drivers/pwm/pwm-msc313e.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
+ * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/of_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define DRIVER_NAME "msc313e-pwm"
+
+#define CHANNEL_OFFSET	0x80
+#define REG_DUTY	0x8
+#define REG_PERIOD	0x10
+#define REG_DIV		0x18
+#define REG_CTRL	0x1c
+#define REG_SWRST	0x1fc
+
+struct msc313e_pwm_channel {
+	struct regmap_field *clkdiv;
+	struct regmap_field *polarity;
+	struct regmap_field *dutyl;
+	struct regmap_field *dutyh;
+	struct regmap_field *periodl;
+	struct regmap_field *periodh;
+	struct regmap_field *swrst;
+};
+
+struct msc313e_pwm {
+	struct regmap *regmap;
+	struct pwm_chip pwmchip;
+	struct clk *clk;
+	struct msc313e_pwm_channel channels[];
+};
+
+struct msc313e_pwm_info {
+	unsigned int channels;
+};
+
+#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
+
+static const struct regmap_config msc313e_pwm_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 16,
+	.reg_stride = 4,
+};
+
+static const struct msc313e_pwm_info msc313e_data = {
+	.channels = 8,
+};
+
+static const struct msc313e_pwm_info ssd20xd_data = {
+	.channels = 4,
+};
+
+static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
+{
+	/* The bus that connects the CPU to the peripheral registers splits 32 bit registers into
+	 * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter
+	 * we are about to write has this contrainst.
+	 */
+	regmap_field_write(low, value & 0xffff);
+	regmap_field_write(high, value >> 16);
+}
+
+static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value)
+{
+	unsigned int val = 0;
+
+	regmap_field_read(low, &val);
+	*value = val;
+	regmap_field_read(high, &val);
+	*value = (val << 16) | *value;
+}
+
+static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device,
+			      int duty_ns, int period_ns)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	unsigned long long div = 1;
+
+	/* Fit the period into the period register by prescaling the clk */
+	while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) {
+		div++;
+		if (div > (0xffff + 1)) {
+			/* Force clk div to the maximum allowed value */
+			div = 0xffff;
+			break;
+		}
+		nspertick = DIV_ROUND_DOWN_ULL(nspertick, div);
+	}
+
+	regmap_field_write(channel->clkdiv, div - 1);
+	msc313e_pwm_writecounter(channel->dutyl, channel->dutyh,
+				 DIV_ROUND_DOWN_ULL(duty_ns, nspertick));
+	msc313e_pwm_writecounter(channel->periodl, channel->periodh,
+				 DIV_ROUND_DOWN_ULL(period_ns, nspertick));
+	return 0;
+};
+
+static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device,
+				    enum pwm_polarity polarity)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	unsigned int pol = 0;
+
+	if (polarity == PWM_POLARITY_INVERSED)
+		pol = 1;
+	regmap_field_update_bits(channel->polarity, 1, pol);
+
+	return 0;
+}
+
+static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	int ret;
+
+	ret = clk_prepare_enable(pwm->clk);
+	if (ret)
+		return ret;
+	return regmap_field_write(channel->swrst, 0);
+}
+
+static int msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	int ret;
+
+	ret = regmap_field_write(channel->swrst, 1);
+	clk_disable_unprepare(pwm->clk);
+	return ret;
+}
+
+static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	int ret;
+
+	if (state->enabled) {
+		if (!pwm->state.enabled) {
+			ret = msc313e_pwm_enable(chip, pwm);
+			if (ret)
+				return ret;
+		}
+		msc313e_pwm_set_polarity(chip, pwm, state->polarity);
+		msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	} else if (pwm->state.enabled) {
+		ret = msc313e_pwm_disable(chip, pwm);
+	}
+	return 0;
+}
+
+static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device,
+			      struct pwm_state *state)
+{
+	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
+	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
+	unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
+	unsigned int val = 0;
+
+	regmap_field_read(channel->polarity, &val);
+	state->polarity = val ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+
+	regmap_field_read(channel->swrst, &val);
+	state->enabled = val == 0 ? true : false;
+
+	msc313e_pwm_readcounter(channel->dutyl, channel->dutyh, &val);
+	state->duty_cycle = val * nspertick;
+
+	msc313e_pwm_readcounter(channel->periodl, channel->periodh, &val);
+	state->period = val * nspertick;
+}
+
+static const struct pwm_ops msc313e_pwm_ops = {
+	.apply = msc313e_apply,
+	.get_state = msc313e_get_state,
+	.owner = THIS_MODULE
+};
+
+static int msc313e_pwm_probe(struct platform_device *pdev)
+{
+	const struct msc313e_pwm_info *match_data;
+	struct device *dev = &pdev->dev;
+	struct msc313e_pwm *pwm;
+	__iomem void *base;
+	int i;
+
+	match_data = of_device_get_match_data(dev);
+	if (!match_data)
+		return -EINVAL;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n");
+
+	pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config);
+	if (IS_ERR(pwm->regmap))
+		return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n");
+
+	for (i = 0; i < match_data->channels; i++) {
+		unsigned int offset = CHANNEL_OFFSET * i;
+		struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7);
+		struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4);
+		struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15);
+		struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2);
+		struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15);
+		struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2);
+		struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i);
+
+		pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap,
+								  div_clkdiv_field);
+		pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap,
+								    ctrl_polarity_field);
+		pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field);
+		pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field);
+		pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field);
+		pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field);
+		pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field);
+
+		/* Channels are enabled on boot, disable it until the pwm subsystem re-enable it
+		 * explicitly
+		 */
+		regmap_field_write(pwm->channels[i].swrst, 1);
+	}
+
+	pwm->pwmchip.dev = dev;
+	pwm->pwmchip.ops = &msc313e_pwm_ops;
+	pwm->pwmchip.npwm = match_data->channels;
+
+	platform_set_drvdata(pdev, pwm);
+
+	return devm_pwmchip_add(dev, &pwm->pwmchip);
+}
+
+static const struct of_device_id msc313e_pwm_dt_ids[] = {
+	{ .compatible = "mstar,msc313e-pwm", .data = &msc313e_data },
+	{ .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data },
+	{},
+};
+MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids);
+
+static struct platform_driver msc313e_pwm_driver = {
+	.probe = msc313e_pwm_probe,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = msc313e_pwm_dt_ids,
+	},
+};
+module_platform_driver(msc313e_pwm_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Mstar MSC313e PWM driver");
+MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Uwe Kleine-König]

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

Hello Romain, hello Daniel,

adding Mark Brown to Cc: for the regmap stuff.

On Wed, Sep 07, 2022 at 03:12:38PM +0200, Romain Perier wrote:
> From: Daniel Palmer <daniel@0x0f.com>
> 
> This adds support for the PWM block on the Mstar MSC313e SoCs and newer.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> Co-developed-by: Romain Perier <romain.perier@gmail.com>
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |   9 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 280 insertions(+)
>  create mode 100644 drivers/pwm/pwm-msc313e.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9d7f64dc0efe..c3b39b09097c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2439,6 +2439,7 @@ F:	arch/arm/mach-mstar/
>  F:	drivers/clk/mstar/
>  F:	drivers/clocksource/timer-msc313e.c
>  F:	drivers/gpio/gpio-msc313.c
> +F:	drivers/pwm/pwm-msc313e.c
>  F:	drivers/rtc/rtc-msc313.c
>  F:	drivers/watchdog/msc313e_wdt.c
>  F:	include/dt-bindings/clock/mstar-*
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 60d13a949bc5..8049fd03a821 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -372,6 +372,15 @@ config PWM_MESON
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-meson.
>  
> +config PWM_MSC313E
> +	tristate "MStar MSC313e PWM support"
> +	depends on ARCH_MSTARV7 || COMPILE_TEST
> +	help
> +	  Generic PWM framework driver for MSTAR MSC313e.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-msc313e.
> +
>  config PWM_MTK_DISP
>  	tristate "MediaTek display PWM driver"
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 7bf1a29f02b8..bc285c054f2a 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_MSC313E)	+= pwm-msc313e.o
>  obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
> diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
> new file mode 100644
> index 000000000000..a71f39ba66c3
> --- /dev/null
> +++ b/drivers/pwm/pwm-msc313e.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
> + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define DRIVER_NAME "msc313e-pwm"
> +
> +#define CHANNEL_OFFSET	0x80
> +#define REG_DUTY	0x8
> +#define REG_PERIOD	0x10
> +#define REG_DIV		0x18
> +#define REG_CTRL	0x1c
> +#define REG_SWRST	0x1fc
> +
> +struct msc313e_pwm_channel {
> +	struct regmap_field *clkdiv;
> +	struct regmap_field *polarity;
> +	struct regmap_field *dutyl;
> +	struct regmap_field *dutyh;
> +	struct regmap_field *periodl;
> +	struct regmap_field *periodh;
> +	struct regmap_field *swrst;
> +};
> +
> +struct msc313e_pwm {
> +	struct regmap *regmap;
> +	struct pwm_chip pwmchip;
> +	struct clk *clk;
> +	struct msc313e_pwm_channel channels[];
> +};
> +
> +struct msc313e_pwm_info {
> +	unsigned int channels;
> +};
> +
> +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
> +
> +static const struct regmap_config msc313e_pwm_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_stride = 4,
> +};
> +
> +static const struct msc313e_pwm_info msc313e_data = {
> +	.channels = 8,
> +};
> +
> +static const struct msc313e_pwm_info ssd20xd_data = {
> +	.channels = 4,
> +};
> +
> +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
> +{
> +	/* The bus that connects the CPU to the peripheral registers splits 32 bit registers into

Please fix the comment style to use /* on a line for itself. Also for
comments staying below 80 chars per line is appreciated.

> +	 * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter
> +	 * we are about to write has this contrainst.

s/contrainst/contraint/

I wonder if that could be abstracted by regmap?!

> +	 */
> +	regmap_field_write(low, value & 0xffff);
> +	regmap_field_write(high, value >> 16);
> +}
> +
> +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value)
> +{
> +	unsigned int val = 0;
> +
> +	regmap_field_read(low, &val);
> +	*value = val;
> +	regmap_field_read(high, &val);
> +	*value = (val << 16) | *value;
> +}
> +
> +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device,
> +			      int duty_ns, int period_ns)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	unsigned long long div = 1;
> +
> +	/* Fit the period into the period register by prescaling the clk */
> +	while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) {

dividing by the result of a division looses precision. Also rounding
down both divisions looks wrong.

> +		div++;
> +		if (div > (0xffff + 1)) {
> +			/* Force clk div to the maximum allowed value */
> +			div = 0xffff;
> +			break;
> +		}
> +		nspertick = DIV_ROUND_DOWN_ULL(nspertick, div);
> +	}
> +
> +	regmap_field_write(channel->clkdiv, div - 1);
> +	msc313e_pwm_writecounter(channel->dutyl, channel->dutyh,
> +				 DIV_ROUND_DOWN_ULL(duty_ns, nspertick));
> +	msc313e_pwm_writecounter(channel->periodl, channel->periodh,
> +				 DIV_ROUND_DOWN_ULL(period_ns, nspertick));
> +	return 0;
> +};
> +
> +static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device,
> +				    enum pwm_polarity polarity)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	unsigned int pol = 0;
> +
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		pol = 1;
> +	regmap_field_update_bits(channel->polarity, 1, pol);
> +
> +	return 0;
> +}
> +
> +static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	int ret;
> +
> +	ret = clk_prepare_enable(pwm->clk);
> +	if (ret)
> +		return ret;
> +	return regmap_field_write(channel->swrst, 0);
> +}
> +
> +static int msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	int ret;
> +
> +	ret = regmap_field_write(channel->swrst, 1);
> +	clk_disable_unprepare(pwm->clk);
> +	return ret;
> +}
> +
> +static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	int ret;
> +
> +	if (state->enabled) {
> +		if (!pwm->state.enabled) {
> +			ret = msc313e_pwm_enable(chip, pwm);
> +			if (ret)
> +				return ret;
> +		}
> +		msc313e_pwm_set_polarity(chip, pwm, state->polarity);
> +		msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period);

Does the output emit a previous setting after msc313e_pwm_enable()? If
so, please do the enable at the end.

Does calling msc313e_pwm_set_polarity() and msc313e_pwm_config() have an
immediate effect? Or is the currently running period completed?

> +	} else if (pwm->state.enabled) {
> +		ret = msc313e_pwm_disable(chip, pwm);

How does the output behave on disable? Typical options are:

 - freezes where ever it currently is
 - goes to the inactive level

Does it complete the currently running period? Please answer these in a
"Limitations:" section similar to e.g. pwm-sl28cpld.c and pwm-iqs620a.

> +	}
> +	return 0;
> +}
> +
> +static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device,

Please s/device/pwm/ to match the naming used in other drivers.

> +			      struct pwm_state *state)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> +	unsigned int val = 0;
> +
> +	regmap_field_read(channel->polarity, &val);
> +	state->polarity = val ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +
> +	regmap_field_read(channel->swrst, &val);
> +	state->enabled = val == 0 ? true : false;
> +
> +	msc313e_pwm_readcounter(channel->dutyl, channel->dutyh, &val);
> +	state->duty_cycle = val * nspertick;

Please do the division at the end. Consider val = 333335 and
clk_get_rate() returning 666666667.

Then the exact duty cycle would be 500002.49974999874 but you would
calculate nspertick = 1 and so duty_cycle = 333335.

Also you need to round up. Did you test your driver with PWM_DEBUG
enabled? The idea is that .get_state should return 500003 here to have
apply ∘ get_state idempotent. As apply is supposed to round down to the
next available setting, get_state has to round up.

> +	msc313e_pwm_readcounter(channel->periodl, channel->periodh, &val);
> +	state->period = val * nspertick;
> +}
> +
> +static const struct pwm_ops msc313e_pwm_ops = {
> +	.apply = msc313e_apply,
> +	.get_state = msc313e_get_state,
> +	.owner = THIS_MODULE
> +};
> +
> +static int msc313e_pwm_probe(struct platform_device *pdev)
> +{
> +	const struct msc313e_pwm_info *match_data;
> +	struct device *dev = &pdev->dev;
> +	struct msc313e_pwm *pwm;
> +	__iomem void *base;
> +	int i;
> +
> +	match_data = of_device_get_match_data(dev);
> +	if (!match_data)
> +		return -EINVAL;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL);

Please don't use "pwm" as a name for your driver data variable. That
name is reserved for struct pwm_device pointers. Something like "ddata"
would be fine, but there is a wide variety in pwm driver land to choose
from.

> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n");
> +
> +	pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config);
> +	if (IS_ERR(pwm->regmap))
> +		return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n");
> +
> +	for (i = 0; i < match_data->channels; i++) {
> +		unsigned int offset = CHANNEL_OFFSET * i;
> +		struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7);
> +		struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4);
> +		struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15);
> +		struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2);
> +		struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15);
> +		struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2);
> +		struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i);
> +
> +		pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap,
> +								  div_clkdiv_field);
> +		pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap,
> +								    ctrl_polarity_field);
> +		pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field);
> +		pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field);
> +		pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field);
> +		pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field);
> +		pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field);
> +
> +		/* Channels are enabled on boot, disable it until the pwm subsystem re-enable it
> +		 * explicitly

You're supposed to keep the PWM running. (Consider a bootloader setting
up a splash screen. You're probably killing the backlight here.) Note
you need improved clk handling to do that right.

> +		 */
> +		regmap_field_write(pwm->channels[i].swrst, 1);
> +	}
> +
> +	pwm->pwmchip.dev = dev;
> +	pwm->pwmchip.ops = &msc313e_pwm_ops;
> +	pwm->pwmchip.npwm = match_data->channels;
> +
> +	platform_set_drvdata(pdev, pwm);

This is unused, please drop.

> +	return devm_pwmchip_add(dev, &pwm->pwmchip);
> +}
> +
> +static const struct of_device_id msc313e_pwm_dt_ids[] = {
> +	{ .compatible = "mstar,msc313e-pwm", .data = &msc313e_data },
> +	{ .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids);
> +
> +static struct platform_driver msc313e_pwm_driver = {
> +	.probe = msc313e_pwm_probe,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = msc313e_pwm_dt_ids,
> +	},
> +};
> +module_platform_driver(msc313e_pwm_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Mstar MSC313e PWM driver");
> +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");

That address doesn't match the author of this patch. Is this on purpose?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 13853 bytes --]

Hello Romain, hello Daniel,

adding Mark Brown to Cc: for the regmap stuff.

On Wed, Sep 07, 2022 at 03:12:38PM +0200, Romain Perier wrote:
> From: Daniel Palmer <daniel@0x0f.com>
> 
> This adds support for the PWM block on the Mstar MSC313e SoCs and newer.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> Co-developed-by: Romain Perier <romain.perier@gmail.com>
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/pwm/Kconfig       |   9 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 280 insertions(+)
>  create mode 100644 drivers/pwm/pwm-msc313e.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9d7f64dc0efe..c3b39b09097c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2439,6 +2439,7 @@ F:	arch/arm/mach-mstar/
>  F:	drivers/clk/mstar/
>  F:	drivers/clocksource/timer-msc313e.c
>  F:	drivers/gpio/gpio-msc313.c
> +F:	drivers/pwm/pwm-msc313e.c
>  F:	drivers/rtc/rtc-msc313.c
>  F:	drivers/watchdog/msc313e_wdt.c
>  F:	include/dt-bindings/clock/mstar-*
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 60d13a949bc5..8049fd03a821 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -372,6 +372,15 @@ config PWM_MESON
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-meson.
>  
> +config PWM_MSC313E
> +	tristate "MStar MSC313e PWM support"
> +	depends on ARCH_MSTARV7 || COMPILE_TEST
> +	help
> +	  Generic PWM framework driver for MSTAR MSC313e.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-msc313e.
> +
>  config PWM_MTK_DISP
>  	tristate "MediaTek display PWM driver"
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 7bf1a29f02b8..bc285c054f2a 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL)		+= pwm-twl.o
>  obj-$(CONFIG_PWM_TWL_LED)	+= pwm-twl-led.o
>  obj-$(CONFIG_PWM_VISCONTI)	+= pwm-visconti.o
>  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> +obj-$(CONFIG_PWM_MSC313E)	+= pwm-msc313e.o
>  obj-$(CONFIG_PWM_XILINX)	+= pwm-xilinx.o
> diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
> new file mode 100644
> index 000000000000..a71f39ba66c3
> --- /dev/null
> +++ b/drivers/pwm/pwm-msc313e.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
> + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/of_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define DRIVER_NAME "msc313e-pwm"
> +
> +#define CHANNEL_OFFSET	0x80
> +#define REG_DUTY	0x8
> +#define REG_PERIOD	0x10
> +#define REG_DIV		0x18
> +#define REG_CTRL	0x1c
> +#define REG_SWRST	0x1fc
> +
> +struct msc313e_pwm_channel {
> +	struct regmap_field *clkdiv;
> +	struct regmap_field *polarity;
> +	struct regmap_field *dutyl;
> +	struct regmap_field *dutyh;
> +	struct regmap_field *periodl;
> +	struct regmap_field *periodh;
> +	struct regmap_field *swrst;
> +};
> +
> +struct msc313e_pwm {
> +	struct regmap *regmap;
> +	struct pwm_chip pwmchip;
> +	struct clk *clk;
> +	struct msc313e_pwm_channel channels[];
> +};
> +
> +struct msc313e_pwm_info {
> +	unsigned int channels;
> +};
> +
> +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
> +
> +static const struct regmap_config msc313e_pwm_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_stride = 4,
> +};
> +
> +static const struct msc313e_pwm_info msc313e_data = {
> +	.channels = 8,
> +};
> +
> +static const struct msc313e_pwm_info ssd20xd_data = {
> +	.channels = 4,
> +};
> +
> +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
> +{
> +	/* The bus that connects the CPU to the peripheral registers splits 32 bit registers into

Please fix the comment style to use /* on a line for itself. Also for
comments staying below 80 chars per line is appreciated.

> +	 * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter
> +	 * we are about to write has this contrainst.

s/contrainst/contraint/

I wonder if that could be abstracted by regmap?!

> +	 */
> +	regmap_field_write(low, value & 0xffff);
> +	regmap_field_write(high, value >> 16);
> +}
> +
> +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value)
> +{
> +	unsigned int val = 0;
> +
> +	regmap_field_read(low, &val);
> +	*value = val;
> +	regmap_field_read(high, &val);
> +	*value = (val << 16) | *value;
> +}
> +
> +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device,
> +			      int duty_ns, int period_ns)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	unsigned long long div = 1;
> +
> +	/* Fit the period into the period register by prescaling the clk */
> +	while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) {

dividing by the result of a division looses precision. Also rounding
down both divisions looks wrong.

> +		div++;
> +		if (div > (0xffff + 1)) {
> +			/* Force clk div to the maximum allowed value */
> +			div = 0xffff;
> +			break;
> +		}
> +		nspertick = DIV_ROUND_DOWN_ULL(nspertick, div);
> +	}
> +
> +	regmap_field_write(channel->clkdiv, div - 1);
> +	msc313e_pwm_writecounter(channel->dutyl, channel->dutyh,
> +				 DIV_ROUND_DOWN_ULL(duty_ns, nspertick));
> +	msc313e_pwm_writecounter(channel->periodl, channel->periodh,
> +				 DIV_ROUND_DOWN_ULL(period_ns, nspertick));
> +	return 0;
> +};
> +
> +static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device,
> +				    enum pwm_polarity polarity)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	unsigned int pol = 0;
> +
> +	if (polarity == PWM_POLARITY_INVERSED)
> +		pol = 1;
> +	regmap_field_update_bits(channel->polarity, 1, pol);
> +
> +	return 0;
> +}
> +
> +static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	int ret;
> +
> +	ret = clk_prepare_enable(pwm->clk);
> +	if (ret)
> +		return ret;
> +	return regmap_field_write(channel->swrst, 0);
> +}
> +
> +static int msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	int ret;
> +
> +	ret = regmap_field_write(channel->swrst, 1);
> +	clk_disable_unprepare(pwm->clk);
> +	return ret;
> +}
> +
> +static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	int ret;
> +
> +	if (state->enabled) {
> +		if (!pwm->state.enabled) {
> +			ret = msc313e_pwm_enable(chip, pwm);
> +			if (ret)
> +				return ret;
> +		}
> +		msc313e_pwm_set_polarity(chip, pwm, state->polarity);
> +		msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period);

Does the output emit a previous setting after msc313e_pwm_enable()? If
so, please do the enable at the end.

Does calling msc313e_pwm_set_polarity() and msc313e_pwm_config() have an
immediate effect? Or is the currently running period completed?

> +	} else if (pwm->state.enabled) {
> +		ret = msc313e_pwm_disable(chip, pwm);

How does the output behave on disable? Typical options are:

 - freezes where ever it currently is
 - goes to the inactive level

Does it complete the currently running period? Please answer these in a
"Limitations:" section similar to e.g. pwm-sl28cpld.c and pwm-iqs620a.

> +	}
> +	return 0;
> +}
> +
> +static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device,

Please s/device/pwm/ to match the naming used in other drivers.

> +			      struct pwm_state *state)
> +{
> +	struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> +	struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> +	unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> +	unsigned int val = 0;
> +
> +	regmap_field_read(channel->polarity, &val);
> +	state->polarity = val ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +
> +	regmap_field_read(channel->swrst, &val);
> +	state->enabled = val == 0 ? true : false;
> +
> +	msc313e_pwm_readcounter(channel->dutyl, channel->dutyh, &val);
> +	state->duty_cycle = val * nspertick;

Please do the division at the end. Consider val = 333335 and
clk_get_rate() returning 666666667.

Then the exact duty cycle would be 500002.49974999874 but you would
calculate nspertick = 1 and so duty_cycle = 333335.

Also you need to round up. Did you test your driver with PWM_DEBUG
enabled? The idea is that .get_state should return 500003 here to have
apply ∘ get_state idempotent. As apply is supposed to round down to the
next available setting, get_state has to round up.

> +	msc313e_pwm_readcounter(channel->periodl, channel->periodh, &val);
> +	state->period = val * nspertick;
> +}
> +
> +static const struct pwm_ops msc313e_pwm_ops = {
> +	.apply = msc313e_apply,
> +	.get_state = msc313e_get_state,
> +	.owner = THIS_MODULE
> +};
> +
> +static int msc313e_pwm_probe(struct platform_device *pdev)
> +{
> +	const struct msc313e_pwm_info *match_data;
> +	struct device *dev = &pdev->dev;
> +	struct msc313e_pwm *pwm;
> +	__iomem void *base;
> +	int i;
> +
> +	match_data = of_device_get_match_data(dev);
> +	if (!match_data)
> +		return -EINVAL;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL);

Please don't use "pwm" as a name for your driver data variable. That
name is reserved for struct pwm_device pointers. Something like "ddata"
would be fine, but there is a wide variety in pwm driver land to choose
from.

> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n");
> +
> +	pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config);
> +	if (IS_ERR(pwm->regmap))
> +		return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n");
> +
> +	for (i = 0; i < match_data->channels; i++) {
> +		unsigned int offset = CHANNEL_OFFSET * i;
> +		struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7);
> +		struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4);
> +		struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15);
> +		struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2);
> +		struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15);
> +		struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2);
> +		struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i);
> +
> +		pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap,
> +								  div_clkdiv_field);
> +		pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap,
> +								    ctrl_polarity_field);
> +		pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field);
> +		pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field);
> +		pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field);
> +		pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field);
> +		pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field);
> +
> +		/* Channels are enabled on boot, disable it until the pwm subsystem re-enable it
> +		 * explicitly

You're supposed to keep the PWM running. (Consider a bootloader setting
up a splash screen. You're probably killing the backlight here.) Note
you need improved clk handling to do that right.

> +		 */
> +		regmap_field_write(pwm->channels[i].swrst, 1);
> +	}
> +
> +	pwm->pwmchip.dev = dev;
> +	pwm->pwmchip.ops = &msc313e_pwm_ops;
> +	pwm->pwmchip.npwm = match_data->channels;
> +
> +	platform_set_drvdata(pdev, pwm);

This is unused, please drop.

> +	return devm_pwmchip_add(dev, &pwm->pwmchip);
> +}
> +
> +static const struct of_device_id msc313e_pwm_dt_ids[] = {
> +	{ .compatible = "mstar,msc313e-pwm", .data = &msc313e_data },
> +	{ .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids);
> +
> +static struct platform_driver msc313e_pwm_driver = {
> +	.probe = msc313e_pwm_probe,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = msc313e_pwm_dt_ids,
> +	},
> +};
> +module_platform_driver(msc313e_pwm_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Mstar MSC313e PWM driver");
> +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");

That address doesn't match the author of this patch. Is this on purpose?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Daniel Palmer]

Hi Uwe,

On Wed, 28 Sept 2022 at 01:33, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Romain, hello Daniel,

Romain is doing the fixes to this so I'll let him answer the other
bits but on all of the "what does the hardware do?" questions I'll
have to test that on the hardware and follow up. The only
documentation we have is very rough listings of registers and bits and
not much else.

>
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Mstar MSC313e PWM driver");
> > +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
>
> That address doesn't match the author of this patch. Is this on purpose?

My intention is to put all of the stuff for this MStar (Now Sigmastar)
platform under this domain including the kernel, u-boot and PCB files
so that at some point I can handover the whole thing to someone else
or maybe a community that want to take over by giving them the domain.
I should really set my email address in git for these commits to match
but keep forgetting.

Cheers,

Daniel

Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Daniel Palmer]

Hi Uwe,

On Wed, 28 Sept 2022 at 01:33, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Romain, hello Daniel,

Romain is doing the fixes to this so I'll let him answer the other
bits but on all of the "what does the hardware do?" questions I'll
have to test that on the hardware and follow up. The only
documentation we have is very rough listings of registers and bits and
not much else.

>
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Mstar MSC313e PWM driver");
> > +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
>
> That address doesn't match the author of this patch. Is this on purpose?

My intention is to put all of the stuff for this MStar (Now Sigmastar)
platform under this domain including the kernel, u-boot and PCB files
so that at some point I can handover the whole thing to someone else
or maybe a community that want to take over by giving them the domain.
I should really set my email address in git for these commits to match
but keep forgetting.

Cheers,

Daniel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Romain Perier]

Hi,


Le mar. 27 sept. 2022 à 18:33, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> a écrit :
>
> Hello Romain, hello Daniel,
>
> adding Mark Brown to Cc: for the regmap stuff.
>
> On Wed, Sep 07, 2022 at 03:12:38PM +0200, Romain Perier wrote:
> > From: Daniel Palmer <daniel@0x0f.com>
> >
> > This adds support for the PWM block on the Mstar MSC313e SoCs and newer.
> >
> > Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> > Co-developed-by: Romain Perier <romain.perier@gmail.com>
> > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > ---
> >  MAINTAINERS               |   1 +
> >  drivers/pwm/Kconfig       |   9 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 280 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-msc313e.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9d7f64dc0efe..c3b39b09097c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2439,6 +2439,7 @@ F:      arch/arm/mach-mstar/
> >  F:   drivers/clk/mstar/
> >  F:   drivers/clocksource/timer-msc313e.c
> >  F:   drivers/gpio/gpio-msc313.c
> > +F:   drivers/pwm/pwm-msc313e.c
> >  F:   drivers/rtc/rtc-msc313.c
> >  F:   drivers/watchdog/msc313e_wdt.c
> >  F:   include/dt-bindings/clock/mstar-*
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 60d13a949bc5..8049fd03a821 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -372,6 +372,15 @@ config PWM_MESON
> >         To compile this driver as a module, choose M here: the module
> >         will be called pwm-meson.
> >
> > +config PWM_MSC313E
> > +     tristate "MStar MSC313e PWM support"
> > +     depends on ARCH_MSTARV7 || COMPILE_TEST
> > +     help
> > +       Generic PWM framework driver for MSTAR MSC313e.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called pwm-msc313e.
> > +
> >  config PWM_MTK_DISP
> >       tristate "MediaTek display PWM driver"
> >       depends on ARCH_MEDIATEK || COMPILE_TEST
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 7bf1a29f02b8..bc285c054f2a 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL)               += pwm-twl.o
> >  obj-$(CONFIG_PWM_TWL_LED)    += pwm-twl-led.o
> >  obj-$(CONFIG_PWM_VISCONTI)   += pwm-visconti.o
> >  obj-$(CONFIG_PWM_VT8500)     += pwm-vt8500.o
> > +obj-$(CONFIG_PWM_MSC313E)    += pwm-msc313e.o
> >  obj-$(CONFIG_PWM_XILINX)     += pwm-xilinx.o
> > diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
> > new file mode 100644
> > index 000000000000..a71f39ba66c3
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-msc313e.c
> > @@ -0,0 +1,269 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
> > + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define DRIVER_NAME "msc313e-pwm"
> > +
> > +#define CHANNEL_OFFSET       0x80
> > +#define REG_DUTY     0x8
> > +#define REG_PERIOD   0x10
> > +#define REG_DIV              0x18
> > +#define REG_CTRL     0x1c
> > +#define REG_SWRST    0x1fc
> > +
> > +struct msc313e_pwm_channel {
> > +     struct regmap_field *clkdiv;
> > +     struct regmap_field *polarity;
> > +     struct regmap_field *dutyl;
> > +     struct regmap_field *dutyh;
> > +     struct regmap_field *periodl;
> > +     struct regmap_field *periodh;
> > +     struct regmap_field *swrst;
> > +};
> > +
> > +struct msc313e_pwm {
> > +     struct regmap *regmap;
> > +     struct pwm_chip pwmchip;
> > +     struct clk *clk;
> > +     struct msc313e_pwm_channel channels[];
> > +};
> > +
> > +struct msc313e_pwm_info {
> > +     unsigned int channels;
> > +};
> > +
> > +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
> > +
> > +static const struct regmap_config msc313e_pwm_regmap_config = {
> > +     .reg_bits = 16,
> > +     .val_bits = 16,
> > +     .reg_stride = 4,
> > +};
> > +
> > +static const struct msc313e_pwm_info msc313e_data = {
> > +     .channels = 8,
> > +};
> > +
> > +static const struct msc313e_pwm_info ssd20xd_data = {
> > +     .channels = 4,
> > +};
> > +
> > +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
> > +{
> > +     /* The bus that connects the CPU to the peripheral registers splits 32 bit registers into
>
> Please fix the comment style to use /* on a line for itself. Also for
> comments staying below 80 chars per line is appreciated.

even if check-patch.pl --strict passed ? ^^

>
> > +      * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter
> > +      * we are about to write has this contrainst.
>
> s/contrainst/contraint/
>
> I wonder if that could be abstracted by regmap?!

I had the same thought, not from what I have read/found, but perhaps
the regmap maintainer has an opinion.

>
> > +      */
> > +     regmap_field_write(low, value & 0xffff);
> > +     regmap_field_write(high, value >> 16);
> > +}
> > +
> > +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value)
> > +{
> > +     unsigned int val = 0;
> > +
> > +     regmap_field_read(low, &val);
> > +     *value = val;
> > +     regmap_field_read(high, &val);
> > +     *value = (val << 16) | *value;
> > +}
> > +
> > +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device,
> > +                           int duty_ns, int period_ns)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     unsigned long long div = 1;
> > +
> > +     /* Fit the period into the period register by prescaling the clk */
> > +     while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) {
>
> dividing by the result of a division looses precision. Also rounding
> down both divisions looks wrong.

Such cases are not supposed to be covered by PWM_DEBUG ? (because
everything passed with PWM_DEBUG)


>
> > +             div++;
> > +             if (div > (0xffff + 1)) {
> > +                     /* Force clk div to the maximum allowed value */
> > +                     div = 0xffff;
> > +                     break;
> > +             }
> > +             nspertick = DIV_ROUND_DOWN_ULL(nspertick, div);
> > +     }
> > +
> > +     regmap_field_write(channel->clkdiv, div - 1);
> > +     msc313e_pwm_writecounter(channel->dutyl, channel->dutyh,
> > +                              DIV_ROUND_DOWN_ULL(duty_ns, nspertick));
> > +     msc313e_pwm_writecounter(channel->periodl, channel->periodh,
> > +                              DIV_ROUND_DOWN_ULL(period_ns, nspertick));
> > +     return 0;
> > +};
> > +
> > +static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device,
> > +                                 enum pwm_polarity polarity)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     unsigned int pol = 0;
> > +
> > +     if (polarity == PWM_POLARITY_INVERSED)
> > +             pol = 1;
> > +     regmap_field_update_bits(channel->polarity, 1, pol);
> > +
> > +     return 0;
> > +}
> > +
> > +static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     int ret;
> > +
> > +     ret = clk_prepare_enable(pwm->clk);
> > +     if (ret)
> > +             return ret;
> > +     return regmap_field_write(channel->swrst, 0);
> > +}
> > +
> > +static int msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     int ret;
> > +
> > +     ret = regmap_field_write(channel->swrst, 1);
> > +     clk_disable_unprepare(pwm->clk);
> > +     return ret;
> > +}
> > +
> > +static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                      const struct pwm_state *state)
> > +{
> > +     int ret;
> > +
> > +     if (state->enabled) {
> > +             if (!pwm->state.enabled) {
> > +                     ret = msc313e_pwm_enable(chip, pwm);
> > +                     if (ret)
> > +                             return ret;
> > +             }
> > +             msc313e_pwm_set_polarity(chip, pwm, state->polarity);
> > +             msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period);
>
> Does the output emit a previous setting after msc313e_pwm_enable()? If
> so, please do the enable at the end.
>
> Does calling msc313e_pwm_set_polarity() and msc313e_pwm_config() have an
> immediate effect? Or is the currently running period completed?
>
> > +     } else if (pwm->state.enabled) {
> > +             ret = msc313e_pwm_disable(chip, pwm);
>
> How does the output behave on disable? Typical options are:
>
>  - freezes where ever it currently is
>  - goes to the inactive level

I let Daniel answer these questions.


>
> Does it complete the currently running period? Please answer these in a
> "Limitations:" section similar to e.g. pwm-sl28cpld.c and pwm-iqs620a.
>
> > +     }
> > +     return 0;
> > +}
> > +
> > +static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device,
>
> Please s/device/pwm/ to match the naming used in other drivers.
>
> > +                           struct pwm_state *state)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> > +     unsigned int val = 0;
> > +
> > +     regmap_field_read(channel->polarity, &val);
> > +     state->polarity = val ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > +
> > +     regmap_field_read(channel->swrst, &val);
> > +     state->enabled = val == 0 ? true : false;
> > +
> > +     msc313e_pwm_readcounter(channel->dutyl, channel->dutyh, &val);
> > +     state->duty_cycle = val * nspertick;
>
> Please do the division at the end. Consider val = 333335 and
> clk_get_rate() returning 666666667.
>
> Then the exact duty cycle would be 500002.49974999874 but you would
> calculate nspertick = 1 and so duty_cycle = 333335.
>
> Also you need to round up. Did you test your driver with PWM_DEBUG
> enabled?

Take a look at my cover letter, yeah I have tested everything with
PWM_DEBUG, everything passed ^^

The idea is that .get_state should return 500003 here to have
> apply ∘ get_state idempotent. As apply is supposed to round down to the
> next available setting, get_state has to round up.
>
> > +     msc313e_pwm_readcounter(channel->periodl, channel->periodh, &val);
> > +     state->period = val * nspertick;
> > +}
> > +
> > +static const struct pwm_ops msc313e_pwm_ops = {
> > +     .apply = msc313e_apply,
> > +     .get_state = msc313e_get_state,
> > +     .owner = THIS_MODULE
> > +};
> > +
> > +static int msc313e_pwm_probe(struct platform_device *pdev)
> > +{
> > +     const struct msc313e_pwm_info *match_data;
> > +     struct device *dev = &pdev->dev;
> > +     struct msc313e_pwm *pwm;
> > +     __iomem void *base;
> > +     int i;
> > +
> > +     match_data = of_device_get_match_data(dev);
> > +     if (!match_data)
> > +             return -EINVAL;
> > +
> > +     base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL);
>
> Please don't use "pwm" as a name for your driver data variable. That
> name is reserved for struct pwm_device pointers. Something like "ddata"
> would be fine, but there is a wide variety in pwm driver land to choose
> from.
>
> > +     if (!pwm)
> > +             return -ENOMEM;
> > +
> > +     pwm->clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(pwm->clk))
> > +             return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n");
> > +
> > +     pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config);
> > +     if (IS_ERR(pwm->regmap))
> > +             return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n");
> > +
> > +     for (i = 0; i < match_data->channels; i++) {
> > +             unsigned int offset = CHANNEL_OFFSET * i;
> > +             struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7);
> > +             struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4);
> > +             struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15);
> > +             struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2);
> > +             struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15);
> > +             struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2);
> > +             struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i);
> > +
> > +             pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap,
> > +                                                               div_clkdiv_field);
> > +             pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap,
> > +                                                                 ctrl_polarity_field);
> > +             pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field);
> > +             pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field);
> > +             pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field);
> > +             pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field);
> > +             pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field);
> > +
> > +             /* Channels are enabled on boot, disable it until the pwm subsystem re-enable it
> > +              * explicitly
>
> You're supposed to keep the PWM running. (Consider a bootloader setting
> up a splash screen. You're probably killing the backlight here.) Note
> you need improved clk handling to do that right.
>
> > +              */
> > +             regmap_field_write(pwm->channels[i].swrst, 1);
> > +     }
> > +
> > +     pwm->pwmchip.dev = dev;
> > +     pwm->pwmchip.ops = &msc313e_pwm_ops;
> > +     pwm->pwmchip.npwm = match_data->channels;
> > +
> > +     platform_set_drvdata(pdev, pwm);
>
> This is unused, please drop.
>
> > +     return devm_pwmchip_add(dev, &pwm->pwmchip);
> > +}
> > +
> > +static const struct of_device_id msc313e_pwm_dt_ids[] = {
> > +     { .compatible = "mstar,msc313e-pwm", .data = &msc313e_data },
> > +     { .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids);
> > +
> > +static struct platform_driver msc313e_pwm_driver = {
> > +     .probe = msc313e_pwm_probe,
> > +     .driver = {
> > +             .name = DRIVER_NAME,
> > +             .of_match_table = msc313e_pwm_dt_ids,
> > +     },
> > +};
> > +module_platform_driver(msc313e_pwm_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Mstar MSC313e PWM driver");
> > +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
>
> That address doesn't match the author of this patch. Is this on purpose?
>
> Best regards
> Uwe


Regards,
Romain

Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Romain Perier]

Hi,


Le mar. 27 sept. 2022 à 18:33, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> a écrit :
>
> Hello Romain, hello Daniel,
>
> adding Mark Brown to Cc: for the regmap stuff.
>
> On Wed, Sep 07, 2022 at 03:12:38PM +0200, Romain Perier wrote:
> > From: Daniel Palmer <daniel@0x0f.com>
> >
> > This adds support for the PWM block on the Mstar MSC313e SoCs and newer.
> >
> > Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> > Co-developed-by: Romain Perier <romain.perier@gmail.com>
> > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > ---
> >  MAINTAINERS               |   1 +
> >  drivers/pwm/Kconfig       |   9 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 280 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-msc313e.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9d7f64dc0efe..c3b39b09097c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2439,6 +2439,7 @@ F:      arch/arm/mach-mstar/
> >  F:   drivers/clk/mstar/
> >  F:   drivers/clocksource/timer-msc313e.c
> >  F:   drivers/gpio/gpio-msc313.c
> > +F:   drivers/pwm/pwm-msc313e.c
> >  F:   drivers/rtc/rtc-msc313.c
> >  F:   drivers/watchdog/msc313e_wdt.c
> >  F:   include/dt-bindings/clock/mstar-*
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 60d13a949bc5..8049fd03a821 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -372,6 +372,15 @@ config PWM_MESON
> >         To compile this driver as a module, choose M here: the module
> >         will be called pwm-meson.
> >
> > +config PWM_MSC313E
> > +     tristate "MStar MSC313e PWM support"
> > +     depends on ARCH_MSTARV7 || COMPILE_TEST
> > +     help
> > +       Generic PWM framework driver for MSTAR MSC313e.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called pwm-msc313e.
> > +
> >  config PWM_MTK_DISP
> >       tristate "MediaTek display PWM driver"
> >       depends on ARCH_MEDIATEK || COMPILE_TEST
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 7bf1a29f02b8..bc285c054f2a 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL)               += pwm-twl.o
> >  obj-$(CONFIG_PWM_TWL_LED)    += pwm-twl-led.o
> >  obj-$(CONFIG_PWM_VISCONTI)   += pwm-visconti.o
> >  obj-$(CONFIG_PWM_VT8500)     += pwm-vt8500.o
> > +obj-$(CONFIG_PWM_MSC313E)    += pwm-msc313e.o
> >  obj-$(CONFIG_PWM_XILINX)     += pwm-xilinx.o
> > diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
> > new file mode 100644
> > index 000000000000..a71f39ba66c3
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-msc313e.c
> > @@ -0,0 +1,269 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
> > + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define DRIVER_NAME "msc313e-pwm"
> > +
> > +#define CHANNEL_OFFSET       0x80
> > +#define REG_DUTY     0x8
> > +#define REG_PERIOD   0x10
> > +#define REG_DIV              0x18
> > +#define REG_CTRL     0x1c
> > +#define REG_SWRST    0x1fc
> > +
> > +struct msc313e_pwm_channel {
> > +     struct regmap_field *clkdiv;
> > +     struct regmap_field *polarity;
> > +     struct regmap_field *dutyl;
> > +     struct regmap_field *dutyh;
> > +     struct regmap_field *periodl;
> > +     struct regmap_field *periodh;
> > +     struct regmap_field *swrst;
> > +};
> > +
> > +struct msc313e_pwm {
> > +     struct regmap *regmap;
> > +     struct pwm_chip pwmchip;
> > +     struct clk *clk;
> > +     struct msc313e_pwm_channel channels[];
> > +};
> > +
> > +struct msc313e_pwm_info {
> > +     unsigned int channels;
> > +};
> > +
> > +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
> > +
> > +static const struct regmap_config msc313e_pwm_regmap_config = {
> > +     .reg_bits = 16,
> > +     .val_bits = 16,
> > +     .reg_stride = 4,
> > +};
> > +
> > +static const struct msc313e_pwm_info msc313e_data = {
> > +     .channels = 8,
> > +};
> > +
> > +static const struct msc313e_pwm_info ssd20xd_data = {
> > +     .channels = 4,
> > +};
> > +
> > +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
> > +{
> > +     /* The bus that connects the CPU to the peripheral registers splits 32 bit registers into
>
> Please fix the comment style to use /* on a line for itself. Also for
> comments staying below 80 chars per line is appreciated.

even if check-patch.pl --strict passed ? ^^

>
> > +      * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter
> > +      * we are about to write has this contrainst.
>
> s/contrainst/contraint/
>
> I wonder if that could be abstracted by regmap?!

I had the same thought, not from what I have read/found, but perhaps
the regmap maintainer has an opinion.

>
> > +      */
> > +     regmap_field_write(low, value & 0xffff);
> > +     regmap_field_write(high, value >> 16);
> > +}
> > +
> > +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value)
> > +{
> > +     unsigned int val = 0;
> > +
> > +     regmap_field_read(low, &val);
> > +     *value = val;
> > +     regmap_field_read(high, &val);
> > +     *value = (val << 16) | *value;
> > +}
> > +
> > +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device,
> > +                           int duty_ns, int period_ns)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     unsigned long long div = 1;
> > +
> > +     /* Fit the period into the period register by prescaling the clk */
> > +     while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) {
>
> dividing by the result of a division looses precision. Also rounding
> down both divisions looks wrong.

Such cases are not supposed to be covered by PWM_DEBUG ? (because
everything passed with PWM_DEBUG)


>
> > +             div++;
> > +             if (div > (0xffff + 1)) {
> > +                     /* Force clk div to the maximum allowed value */
> > +                     div = 0xffff;
> > +                     break;
> > +             }
> > +             nspertick = DIV_ROUND_DOWN_ULL(nspertick, div);
> > +     }
> > +
> > +     regmap_field_write(channel->clkdiv, div - 1);
> > +     msc313e_pwm_writecounter(channel->dutyl, channel->dutyh,
> > +                              DIV_ROUND_DOWN_ULL(duty_ns, nspertick));
> > +     msc313e_pwm_writecounter(channel->periodl, channel->periodh,
> > +                              DIV_ROUND_DOWN_ULL(period_ns, nspertick));
> > +     return 0;
> > +};
> > +
> > +static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device,
> > +                                 enum pwm_polarity polarity)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     unsigned int pol = 0;
> > +
> > +     if (polarity == PWM_POLARITY_INVERSED)
> > +             pol = 1;
> > +     regmap_field_update_bits(channel->polarity, 1, pol);
> > +
> > +     return 0;
> > +}
> > +
> > +static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     int ret;
> > +
> > +     ret = clk_prepare_enable(pwm->clk);
> > +     if (ret)
> > +             return ret;
> > +     return regmap_field_write(channel->swrst, 0);
> > +}
> > +
> > +static int msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     int ret;
> > +
> > +     ret = regmap_field_write(channel->swrst, 1);
> > +     clk_disable_unprepare(pwm->clk);
> > +     return ret;
> > +}
> > +
> > +static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                      const struct pwm_state *state)
> > +{
> > +     int ret;
> > +
> > +     if (state->enabled) {
> > +             if (!pwm->state.enabled) {
> > +                     ret = msc313e_pwm_enable(chip, pwm);
> > +                     if (ret)
> > +                             return ret;
> > +             }
> > +             msc313e_pwm_set_polarity(chip, pwm, state->polarity);
> > +             msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period);
>
> Does the output emit a previous setting after msc313e_pwm_enable()? If
> so, please do the enable at the end.
>
> Does calling msc313e_pwm_set_polarity() and msc313e_pwm_config() have an
> immediate effect? Or is the currently running period completed?
>
> > +     } else if (pwm->state.enabled) {
> > +             ret = msc313e_pwm_disable(chip, pwm);
>
> How does the output behave on disable? Typical options are:
>
>  - freezes where ever it currently is
>  - goes to the inactive level

I let Daniel answer these questions.


>
> Does it complete the currently running period? Please answer these in a
> "Limitations:" section similar to e.g. pwm-sl28cpld.c and pwm-iqs620a.
>
> > +     }
> > +     return 0;
> > +}
> > +
> > +static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device,
>
> Please s/device/pwm/ to match the naming used in other drivers.
>
> > +                           struct pwm_state *state)
> > +{
> > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > +     unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> > +     unsigned int val = 0;
> > +
> > +     regmap_field_read(channel->polarity, &val);
> > +     state->polarity = val ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > +
> > +     regmap_field_read(channel->swrst, &val);
> > +     state->enabled = val == 0 ? true : false;
> > +
> > +     msc313e_pwm_readcounter(channel->dutyl, channel->dutyh, &val);
> > +     state->duty_cycle = val * nspertick;
>
> Please do the division at the end. Consider val = 333335 and
> clk_get_rate() returning 666666667.
>
> Then the exact duty cycle would be 500002.49974999874 but you would
> calculate nspertick = 1 and so duty_cycle = 333335.
>
> Also you need to round up. Did you test your driver with PWM_DEBUG
> enabled?

Take a look at my cover letter, yeah I have tested everything with
PWM_DEBUG, everything passed ^^

The idea is that .get_state should return 500003 here to have
> apply ∘ get_state idempotent. As apply is supposed to round down to the
> next available setting, get_state has to round up.
>
> > +     msc313e_pwm_readcounter(channel->periodl, channel->periodh, &val);
> > +     state->period = val * nspertick;
> > +}
> > +
> > +static const struct pwm_ops msc313e_pwm_ops = {
> > +     .apply = msc313e_apply,
> > +     .get_state = msc313e_get_state,
> > +     .owner = THIS_MODULE
> > +};
> > +
> > +static int msc313e_pwm_probe(struct platform_device *pdev)
> > +{
> > +     const struct msc313e_pwm_info *match_data;
> > +     struct device *dev = &pdev->dev;
> > +     struct msc313e_pwm *pwm;
> > +     __iomem void *base;
> > +     int i;
> > +
> > +     match_data = of_device_get_match_data(dev);
> > +     if (!match_data)
> > +             return -EINVAL;
> > +
> > +     base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL);
>
> Please don't use "pwm" as a name for your driver data variable. That
> name is reserved for struct pwm_device pointers. Something like "ddata"
> would be fine, but there is a wide variety in pwm driver land to choose
> from.
>
> > +     if (!pwm)
> > +             return -ENOMEM;
> > +
> > +     pwm->clk = devm_clk_get(dev, NULL);
> > +     if (IS_ERR(pwm->clk))
> > +             return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n");
> > +
> > +     pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config);
> > +     if (IS_ERR(pwm->regmap))
> > +             return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n");
> > +
> > +     for (i = 0; i < match_data->channels; i++) {
> > +             unsigned int offset = CHANNEL_OFFSET * i;
> > +             struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7);
> > +             struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4);
> > +             struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15);
> > +             struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2);
> > +             struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15);
> > +             struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2);
> > +             struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i);
> > +
> > +             pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap,
> > +                                                               div_clkdiv_field);
> > +             pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap,
> > +                                                                 ctrl_polarity_field);
> > +             pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field);
> > +             pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field);
> > +             pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field);
> > +             pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field);
> > +             pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field);
> > +
> > +             /* Channels are enabled on boot, disable it until the pwm subsystem re-enable it
> > +              * explicitly
>
> You're supposed to keep the PWM running. (Consider a bootloader setting
> up a splash screen. You're probably killing the backlight here.) Note
> you need improved clk handling to do that right.
>
> > +              */
> > +             regmap_field_write(pwm->channels[i].swrst, 1);
> > +     }
> > +
> > +     pwm->pwmchip.dev = dev;
> > +     pwm->pwmchip.ops = &msc313e_pwm_ops;
> > +     pwm->pwmchip.npwm = match_data->channels;
> > +
> > +     platform_set_drvdata(pdev, pwm);
>
> This is unused, please drop.
>
> > +     return devm_pwmchip_add(dev, &pwm->pwmchip);
> > +}
> > +
> > +static const struct of_device_id msc313e_pwm_dt_ids[] = {
> > +     { .compatible = "mstar,msc313e-pwm", .data = &msc313e_data },
> > +     { .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids);
> > +
> > +static struct platform_driver msc313e_pwm_driver = {
> > +     .probe = msc313e_pwm_probe,
> > +     .driver = {
> > +             .name = DRIVER_NAME,
> > +             .of_match_table = msc313e_pwm_dt_ids,
> > +     },
> > +};
> > +module_platform_driver(msc313e_pwm_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Mstar MSC313e PWM driver");
> > +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
>
> That address doesn't match the author of this patch. Is this on purpose?
>
> Best regards
> Uwe


Regards,
Romain

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Uwe Kleine-König]

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

Hello Romain,

On Thu, Oct 27, 2022 at 10:36:10AM +0200, Romain Perier wrote:
> Le mar. 27 sept. 2022 à 18:33, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> >
> > Hello Romain, hello Daniel,
> >
> > adding Mark Brown to Cc: for the regmap stuff.
> >
> > On Wed, Sep 07, 2022 at 03:12:38PM +0200, Romain Perier wrote:
> > > From: Daniel Palmer <daniel@0x0f.com>
> > >
> > > This adds support for the PWM block on the Mstar MSC313e SoCs and newer.
> > >
> > > Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> > > Co-developed-by: Romain Perier <romain.perier@gmail.com>
> > > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > > ---
> > >  MAINTAINERS               |   1 +
> > >  drivers/pwm/Kconfig       |   9 ++
> > >  drivers/pwm/Makefile      |   1 +
> > >  drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 280 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-msc313e.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 9d7f64dc0efe..c3b39b09097c 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -2439,6 +2439,7 @@ F:      arch/arm/mach-mstar/
> > >  F:   drivers/clk/mstar/
> > >  F:   drivers/clocksource/timer-msc313e.c
> > >  F:   drivers/gpio/gpio-msc313.c
> > > +F:   drivers/pwm/pwm-msc313e.c
> > >  F:   drivers/rtc/rtc-msc313.c
> > >  F:   drivers/watchdog/msc313e_wdt.c
> > >  F:   include/dt-bindings/clock/mstar-*
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 60d13a949bc5..8049fd03a821 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -372,6 +372,15 @@ config PWM_MESON
> > >         To compile this driver as a module, choose M here: the module
> > >         will be called pwm-meson.
> > >
> > > +config PWM_MSC313E
> > > +     tristate "MStar MSC313e PWM support"
> > > +     depends on ARCH_MSTARV7 || COMPILE_TEST
> > > +     help
> > > +       Generic PWM framework driver for MSTAR MSC313e.
> > > +
> > > +       To compile this driver as a module, choose M here: the module
> > > +       will be called pwm-msc313e.
> > > +
> > >  config PWM_MTK_DISP
> > >       tristate "MediaTek display PWM driver"
> > >       depends on ARCH_MEDIATEK || COMPILE_TEST
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 7bf1a29f02b8..bc285c054f2a 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL)               += pwm-twl.o
> > >  obj-$(CONFIG_PWM_TWL_LED)    += pwm-twl-led.o
> > >  obj-$(CONFIG_PWM_VISCONTI)   += pwm-visconti.o
> > >  obj-$(CONFIG_PWM_VT8500)     += pwm-vt8500.o
> > > +obj-$(CONFIG_PWM_MSC313E)    += pwm-msc313e.o
> > >  obj-$(CONFIG_PWM_XILINX)     += pwm-xilinx.o
> > > diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
> > > new file mode 100644
> > > index 000000000000..a71f39ba66c3
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-msc313e.c
> > > @@ -0,0 +1,269 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
> > > + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#define DRIVER_NAME "msc313e-pwm"
> > > +
> > > +#define CHANNEL_OFFSET       0x80
> > > +#define REG_DUTY     0x8
> > > +#define REG_PERIOD   0x10
> > > +#define REG_DIV              0x18
> > > +#define REG_CTRL     0x1c
> > > +#define REG_SWRST    0x1fc
> > > +
> > > +struct msc313e_pwm_channel {
> > > +     struct regmap_field *clkdiv;
> > > +     struct regmap_field *polarity;
> > > +     struct regmap_field *dutyl;
> > > +     struct regmap_field *dutyh;
> > > +     struct regmap_field *periodl;
> > > +     struct regmap_field *periodh;
> > > +     struct regmap_field *swrst;
> > > +};
> > > +
> > > +struct msc313e_pwm {
> > > +     struct regmap *regmap;
> > > +     struct pwm_chip pwmchip;
> > > +     struct clk *clk;
> > > +     struct msc313e_pwm_channel channels[];
> > > +};
> > > +
> > > +struct msc313e_pwm_info {
> > > +     unsigned int channels;
> > > +};
> > > +
> > > +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
> > > +
> > > +static const struct regmap_config msc313e_pwm_regmap_config = {
> > > +     .reg_bits = 16,
> > > +     .val_bits = 16,
> > > +     .reg_stride = 4,
> > > +};
> > > +
> > > +static const struct msc313e_pwm_info msc313e_data = {
> > > +     .channels = 8,
> > > +};
> > > +
> > > +static const struct msc313e_pwm_info ssd20xd_data = {
> > > +     .channels = 4,
> > > +};
> > > +
> > > +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
> > > +{
> > > +     /* The bus that connects the CPU to the peripheral registers splits 32 bit registers into
> >
> > Please fix the comment style to use /* on a line for itself. Also for
> > comments staying below 80 chars per line is appreciated.
> 
> even if check-patch.pl --strict passed ? ^^

I also already wondered about check-patch not demanding this. *shrug*

> > > +      * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter
> > > +      * we are about to write has this contrainst.
> >
> > s/contrainst/contraint/
> >
> > I wonder if that could be abstracted by regmap?!
> 
> I had the same thought, not from what I have read/found, but perhaps
> the regmap maintainer has an opinion.
> 
> >
> > > +      */
> > > +     regmap_field_write(low, value & 0xffff);
> > > +     regmap_field_write(high, value >> 16);
> > > +}
> > > +
> > > +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value)
> > > +{
> > > +     unsigned int val = 0;
> > > +
> > > +     regmap_field_read(low, &val);
> > > +     *value = val;
> > > +     regmap_field_read(high, &val);
> > > +     *value = (val << 16) | *value;
> > > +}
> > > +
> > > +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device,
> > > +                           int duty_ns, int period_ns)
> > > +{
> > > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > > +     unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> > > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > > +     unsigned long long div = 1;
> > > +
> > > +     /* Fit the period into the period register by prescaling the clk */
> > > +     while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) {
> >
> > dividing by the result of a division looses precision. Also rounding
> > down both divisions looks wrong.
> 
> Such cases are not supposed to be covered by PWM_DEBUG ? (because
> everything passed with PWM_DEBUG)

Note that PWM_DEBUG being silent isn't an indicator that everything is
fine. It cannot catch everything and so doesn't replace human review.

If you tell me what clk_get_rate() returns for you, I might be able to
tell you a procedure that makes PWM_DEBUG unhappy.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 7467 bytes --]

Hello Romain,

On Thu, Oct 27, 2022 at 10:36:10AM +0200, Romain Perier wrote:
> Le mar. 27 sept. 2022 à 18:33, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> >
> > Hello Romain, hello Daniel,
> >
> > adding Mark Brown to Cc: for the regmap stuff.
> >
> > On Wed, Sep 07, 2022 at 03:12:38PM +0200, Romain Perier wrote:
> > > From: Daniel Palmer <daniel@0x0f.com>
> > >
> > > This adds support for the PWM block on the Mstar MSC313e SoCs and newer.
> > >
> > > Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> > > Co-developed-by: Romain Perier <romain.perier@gmail.com>
> > > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > > ---
> > >  MAINTAINERS               |   1 +
> > >  drivers/pwm/Kconfig       |   9 ++
> > >  drivers/pwm/Makefile      |   1 +
> > >  drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 280 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-msc313e.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 9d7f64dc0efe..c3b39b09097c 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -2439,6 +2439,7 @@ F:      arch/arm/mach-mstar/
> > >  F:   drivers/clk/mstar/
> > >  F:   drivers/clocksource/timer-msc313e.c
> > >  F:   drivers/gpio/gpio-msc313.c
> > > +F:   drivers/pwm/pwm-msc313e.c
> > >  F:   drivers/rtc/rtc-msc313.c
> > >  F:   drivers/watchdog/msc313e_wdt.c
> > >  F:   include/dt-bindings/clock/mstar-*
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 60d13a949bc5..8049fd03a821 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -372,6 +372,15 @@ config PWM_MESON
> > >         To compile this driver as a module, choose M here: the module
> > >         will be called pwm-meson.
> > >
> > > +config PWM_MSC313E
> > > +     tristate "MStar MSC313e PWM support"
> > > +     depends on ARCH_MSTARV7 || COMPILE_TEST
> > > +     help
> > > +       Generic PWM framework driver for MSTAR MSC313e.
> > > +
> > > +       To compile this driver as a module, choose M here: the module
> > > +       will be called pwm-msc313e.
> > > +
> > >  config PWM_MTK_DISP
> > >       tristate "MediaTek display PWM driver"
> > >       depends on ARCH_MEDIATEK || COMPILE_TEST
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 7bf1a29f02b8..bc285c054f2a 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL)               += pwm-twl.o
> > >  obj-$(CONFIG_PWM_TWL_LED)    += pwm-twl-led.o
> > >  obj-$(CONFIG_PWM_VISCONTI)   += pwm-visconti.o
> > >  obj-$(CONFIG_PWM_VT8500)     += pwm-vt8500.o
> > > +obj-$(CONFIG_PWM_MSC313E)    += pwm-msc313e.o
> > >  obj-$(CONFIG_PWM_XILINX)     += pwm-xilinx.o
> > > diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
> > > new file mode 100644
> > > index 000000000000..a71f39ba66c3
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-msc313e.c
> > > @@ -0,0 +1,269 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp>
> > > + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#define DRIVER_NAME "msc313e-pwm"
> > > +
> > > +#define CHANNEL_OFFSET       0x80
> > > +#define REG_DUTY     0x8
> > > +#define REG_PERIOD   0x10
> > > +#define REG_DIV              0x18
> > > +#define REG_CTRL     0x1c
> > > +#define REG_SWRST    0x1fc
> > > +
> > > +struct msc313e_pwm_channel {
> > > +     struct regmap_field *clkdiv;
> > > +     struct regmap_field *polarity;
> > > +     struct regmap_field *dutyl;
> > > +     struct regmap_field *dutyh;
> > > +     struct regmap_field *periodl;
> > > +     struct regmap_field *periodh;
> > > +     struct regmap_field *swrst;
> > > +};
> > > +
> > > +struct msc313e_pwm {
> > > +     struct regmap *regmap;
> > > +     struct pwm_chip pwmchip;
> > > +     struct clk *clk;
> > > +     struct msc313e_pwm_channel channels[];
> > > +};
> > > +
> > > +struct msc313e_pwm_info {
> > > +     unsigned int channels;
> > > +};
> > > +
> > > +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip)
> > > +
> > > +static const struct regmap_config msc313e_pwm_regmap_config = {
> > > +     .reg_bits = 16,
> > > +     .val_bits = 16,
> > > +     .reg_stride = 4,
> > > +};
> > > +
> > > +static const struct msc313e_pwm_info msc313e_data = {
> > > +     .channels = 8,
> > > +};
> > > +
> > > +static const struct msc313e_pwm_info ssd20xd_data = {
> > > +     .channels = 4,
> > > +};
> > > +
> > > +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value)
> > > +{
> > > +     /* The bus that connects the CPU to the peripheral registers splits 32 bit registers into
> >
> > Please fix the comment style to use /* on a line for itself. Also for
> > comments staying below 80 chars per line is appreciated.
> 
> even if check-patch.pl --strict passed ? ^^

I also already wondered about check-patch not demanding this. *shrug*

> > > +      * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter
> > > +      * we are about to write has this contrainst.
> >
> > s/contrainst/contraint/
> >
> > I wonder if that could be abstracted by regmap?!
> 
> I had the same thought, not from what I have read/found, but perhaps
> the regmap maintainer has an opinion.
> 
> >
> > > +      */
> > > +     regmap_field_write(low, value & 0xffff);
> > > +     regmap_field_write(high, value >> 16);
> > > +}
> > > +
> > > +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value)
> > > +{
> > > +     unsigned int val = 0;
> > > +
> > > +     regmap_field_read(low, &val);
> > > +     *value = val;
> > > +     regmap_field_read(high, &val);
> > > +     *value = (val << 16) | *value;
> > > +}
> > > +
> > > +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device,
> > > +                           int duty_ns, int period_ns)
> > > +{
> > > +     struct msc313e_pwm *pwm = to_msc313e_pwm(chip);
> > > +     unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk));
> > > +     struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm];
> > > +     unsigned long long div = 1;
> > > +
> > > +     /* Fit the period into the period register by prescaling the clk */
> > > +     while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) {
> >
> > dividing by the result of a division looses precision. Also rounding
> > down both divisions looks wrong.
> 
> Such cases are not supposed to be covered by PWM_DEBUG ? (because
> everything passed with PWM_DEBUG)

Note that PWM_DEBUG being silent isn't an indicator that everything is
fine. It cannot catch everything and so doesn't replace human review.

If you tell me what clk_get_rate() returns for you, I might be able to
tell you a procedure that makes PWM_DEBUG unhappy.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/5] ARM: dts: mstar: Add pwm device node to infinity

[Romain Perier]

This adds the definition of the pwm device node. The PWM being able to
work with the oscillator at 12Mhz for now, it shares the same xtal than
other devices (rtc or watchdog for instance).

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/mstar-infinity.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-infinity.dtsi b/arch/arm/boot/dts/mstar-infinity.dtsi
index 441a917b88ba..752f4c26b31c 100644
--- a/arch/arm/boot/dts/mstar-infinity.dtsi
+++ b/arch/arm/boot/dts/mstar-infinity.dtsi
@@ -38,6 +38,16 @@ opp-800000000 {
 	};
 };
 
+&soc {
+	pm_pwm: pwm@1f001da0 {
+		compatible = "mstar,msc313-pwm";
+		reg = <0x1f001da0 0xc>;
+		#pwm-cells = <2>;
+		clocks = <&xtal_div2>;
+		status = "disabled";
+	};
+};
+
 &cpu0 {
 	operating-points-v2 = <&cpu0_opp_table>;
 };
-- 
2.35.1

[PATCH v2 3/5] ARM: dts: mstar: Add pwm device node to infinity

[Romain Perier]

This adds the definition of the pwm device node. The PWM being able to
work with the oscillator at 12Mhz for now, it shares the same xtal than
other devices (rtc or watchdog for instance).

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/mstar-infinity.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-infinity.dtsi b/arch/arm/boot/dts/mstar-infinity.dtsi
index 441a917b88ba..752f4c26b31c 100644
--- a/arch/arm/boot/dts/mstar-infinity.dtsi
+++ b/arch/arm/boot/dts/mstar-infinity.dtsi
@@ -38,6 +38,16 @@ opp-800000000 {
 	};
 };
 
+&soc {
+	pm_pwm: pwm@1f001da0 {
+		compatible = "mstar,msc313-pwm";
+		reg = <0x1f001da0 0xc>;
+		#pwm-cells = <2>;
+		clocks = <&xtal_div2>;
+		status = "disabled";
+	};
+};
+
 &cpu0 {
 	operating-points-v2 = <&cpu0_opp_table>;
 };
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/5] ARM: dts: mstar: Add pwm device node to infinity3

[Romain Perier]

This adds the definition of the pwm device node. The PWM being able to
work with the oscillator at 12Mhz for now, it shares the same xtal than
other devices (rtc or watchdog for instance).

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/mstar-infinity3.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-infinity3.dtsi b/arch/arm/boot/dts/mstar-infinity3.dtsi
index a56cf29e5d82..aa26f25392d0 100644
--- a/arch/arm/boot/dts/mstar-infinity3.dtsi
+++ b/arch/arm/boot/dts/mstar-infinity3.dtsi
@@ -67,3 +67,13 @@ opp-1512000000 {
 &imi {
 	reg = <0xa0000000 0x20000>;
 };
+
+&riu {
+	pwm: pwm@3400 {
+		compatible = "mstar,msc313e-pwm";
+		reg = <0x3400 0x400>;
+		#pwm-cells = <2>;
+		clocks = <&xtal_div2>;
+		status = "disabled";
+	};
+};
-- 
2.35.1

[PATCH v2 4/5] ARM: dts: mstar: Add pwm device node to infinity3

[Romain Perier]

This adds the definition of the pwm device node. The PWM being able to
work with the oscillator at 12Mhz for now, it shares the same xtal than
other devices (rtc or watchdog for instance).

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/mstar-infinity3.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-infinity3.dtsi b/arch/arm/boot/dts/mstar-infinity3.dtsi
index a56cf29e5d82..aa26f25392d0 100644
--- a/arch/arm/boot/dts/mstar-infinity3.dtsi
+++ b/arch/arm/boot/dts/mstar-infinity3.dtsi
@@ -67,3 +67,13 @@ opp-1512000000 {
 &imi {
 	reg = <0xa0000000 0x20000>;
 };
+
+&riu {
+	pwm: pwm@3400 {
+		compatible = "mstar,msc313e-pwm";
+		reg = <0x3400 0x400>;
+		#pwm-cells = <2>;
+		clocks = <&xtal_div2>;
+		status = "disabled";
+	};
+};
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/5] ARM: dts: mstar: Add pwm device node to infinity2m

[Romain Perier]

This adds definition of the pwm device node, infinity2m has its own
hardware variant, so use the one for ssd20xd.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/mstar-infinity2m.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-infinity2m.dtsi b/arch/arm/boot/dts/mstar-infinity2m.dtsi
index 1b485efd7156..70561e512483 100644
--- a/arch/arm/boot/dts/mstar-infinity2m.dtsi
+++ b/arch/arm/boot/dts/mstar-infinity2m.dtsi
@@ -32,6 +32,14 @@ cpu1: cpu@1 {
 };
 
 &riu {
+	pwm: pwm@3400 {
+		compatible = "mstar,ssd20xd-pwm";
+		reg = <0x3400 0x400>;
+		#pwm-cells = <2>;
+		clocks = <&xtal_div2>;
+		status = "disabled";
+	};
+
 	smpctrl: smpctrl@204000 {
 		reg = <0x204000 0x200>;
 		status = "disabled";
-- 
2.35.1

[PATCH v2 5/5] ARM: dts: mstar: Add pwm device node to infinity2m

[Romain Perier]

This adds definition of the pwm device node, infinity2m has its own
hardware variant, so use the one for ssd20xd.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/mstar-infinity2m.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-infinity2m.dtsi b/arch/arm/boot/dts/mstar-infinity2m.dtsi
index 1b485efd7156..70561e512483 100644
--- a/arch/arm/boot/dts/mstar-infinity2m.dtsi
+++ b/arch/arm/boot/dts/mstar-infinity2m.dtsi
@@ -32,6 +32,14 @@ cpu1: cpu@1 {
 };
 
 &riu {
+	pwm: pwm@3400 {
+		compatible = "mstar,ssd20xd-pwm";
+		reg = <0x3400 0x400>;
+		#pwm-cells = <2>;
+		clocks = <&xtal_div2>;
+		status = "disabled";
+	};
+
 	smpctrl: smpctrl@204000 {
 		reg = <0x204000 0x200>;
 		status = "disabled";
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] Add PWM for MStar SoCs

[Romain Perier]

ping ;)

Regards,

Le mer. 7 sept. 2022 à 15:12, Romain Perier <romain.perier@gmail.com> a écrit :
>
> This patches series adds a new driver for the PWM found in the Mstar
> MSC313e SoCs and newer. It adds a basic pwm driver, the corresponding
> devicetree bindings and its documentation.
>
> Changes since v1:
> - Fixed commit message for the dt-bindings doc
> - Removed "OneOf" from the dt-bindings doc
> - Re-ordered alphabetically in Kconfig and remove
>   unseless empty lines
> - Explain and adds comment in _writecounter() (hw
>   constrainst)
> - Reworked the msc313e_pwm_config() function
> - Fixed clk handling
> - Removed extra callbacks, only keep .apply and .get_state
> - Implement .get_state completly, this fixes the driver with PWM_DEBUG
>   (the whole driver has been tested with PWM_DEBUG).
> - Dropped useless lines in _probe
> - I have kept regmap_field() because it is more clean and helpful, it
>   avoids to do too much of offset and mask and shift all over the place.
>
> Daniel Palmer (1):
>   pwm: Add support for the MSTAR MSC313 PWM
>
> Romain Perier (4):
>   dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings
>     documentation
>   ARM: dts: mstar: Add pwm device node to infinity
>   ARM: dts: mstar: Add pwm device node to infinity3
>   ARM: dts: mstar: Add pwm device node to infinity2m
>
>  .../bindings/pwm/mstar,msc313e-pwm.yaml       |  46 +++
>  MAINTAINERS                                   |   1 +
>  arch/arm/boot/dts/mstar-infinity.dtsi         |  10 +
>  arch/arm/boot/dts/mstar-infinity2m.dtsi       |   8 +
>  arch/arm/boot/dts/mstar-infinity3.dtsi        |  10 +
>  drivers/pwm/Kconfig                           |  10 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-msc313e.c                     | 269 ++++++++++++++++++
>  8 files changed, 355 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
>  create mode 100644 drivers/pwm/pwm-msc313e.c
>
> --
> 2.35.1
>

Re: [PATCH v2 0/5] Add PWM for MStar SoCs

[Romain Perier]

ping ;)

Regards,

Le mer. 7 sept. 2022 à 15:12, Romain Perier <romain.perier@gmail.com> a écrit :
>
> This patches series adds a new driver for the PWM found in the Mstar
> MSC313e SoCs and newer. It adds a basic pwm driver, the corresponding
> devicetree bindings and its documentation.
>
> Changes since v1:
> - Fixed commit message for the dt-bindings doc
> - Removed "OneOf" from the dt-bindings doc
> - Re-ordered alphabetically in Kconfig and remove
>   unseless empty lines
> - Explain and adds comment in _writecounter() (hw
>   constrainst)
> - Reworked the msc313e_pwm_config() function
> - Fixed clk handling
> - Removed extra callbacks, only keep .apply and .get_state
> - Implement .get_state completly, this fixes the driver with PWM_DEBUG
>   (the whole driver has been tested with PWM_DEBUG).
> - Dropped useless lines in _probe
> - I have kept regmap_field() because it is more clean and helpful, it
>   avoids to do too much of offset and mask and shift all over the place.
>
> Daniel Palmer (1):
>   pwm: Add support for the MSTAR MSC313 PWM
>
> Romain Perier (4):
>   dt-bindings: pwm: Add Mstar MSC313e PWM devicetree bindings
>     documentation
>   ARM: dts: mstar: Add pwm device node to infinity
>   ARM: dts: mstar: Add pwm device node to infinity3
>   ARM: dts: mstar: Add pwm device node to infinity2m
>
>  .../bindings/pwm/mstar,msc313e-pwm.yaml       |  46 +++
>  MAINTAINERS                                   |   1 +
>  arch/arm/boot/dts/mstar-infinity.dtsi         |  10 +
>  arch/arm/boot/dts/mstar-infinity2m.dtsi       |   8 +
>  arch/arm/boot/dts/mstar-infinity3.dtsi        |  10 +
>  drivers/pwm/Kconfig                           |  10 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-msc313e.c                     | 269 ++++++++++++++++++
>  8 files changed, 355 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/mstar,msc313e-pwm.yaml
>  create mode 100644 drivers/pwm/pwm-msc313e.c
>
> --
> 2.35.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] Add PWM for MStar SoCs

[Krzysztof Kozlowski]

On 27/09/2022 02:41, Romain Perier wrote:
> ping ;)
> 

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

Best regards,
Krzysztof

Re: [PATCH v2 0/5] Add PWM for MStar SoCs

[Krzysztof Kozlowski]

On 27/09/2022 02:41, Romain Perier wrote:
> ping ;)
> 

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] Add PWM for MStar SoCs

[Krzysztof Kozlowski]

On 29/10/2022 03:48, Romain Perier wrote:
> Hi
> 
> This is what I already do and what I do for every patch series, usually ^^
> And I send To: the maintainers and everyone else in Cc: , as the process or
> other maintainers suggest.
> 
> The command was run on 6.0 for the first v1 I think .
> 
> I have forgot something ?

You forgot to Cc at least me. I did not check other missing entries.

Best regards,
Krzysztof

Re: [PATCH v2 0/5] Add PWM for MStar SoCs

[Krzysztof Kozlowski]

On 29/10/2022 03:48, Romain Perier wrote:
> Hi
> 
> This is what I already do and what I do for every patch series, usually ^^
> And I send To: the maintainers and everyone else in Cc: , as the process or
> other maintainers suggest.
> 
> The command was run on 6.0 for the first v1 I think .
> 
> I have forgot something ?

You forgot to Cc at least me. I did not check other missing entries.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel