[PATCH v7 0/2] Add PWM support for Intel Keem Bay SoC

[PATCH v7 0/2] Add PWM support for Intel Keem Bay SoC

[vijayakannan.ayyathurai]

From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

Hi,

This patch set enables support for PWM on the Intel Keem Bay SoC.
Keem Bay is an ARM based SoC, and the GPIO module allows
configuration of 6 PWM outputs.

Patch 1 adds the PWM driver and Patch 2 is for the required
Device Tree bindings documentation.

This driver was tested on the Keem Bay evaluation module board.

Thank you.

Regards,
Vijay

Changes since v6:
- Add reviewed-by tag

Changes since v5:
-Reorder symbols/Kconfig in drivers/pwm/Kconfig and drivers/pwm/Makefile
-Use "Limitations" for consistency
-Add clk_prepare_enable()
-Reorder keembay_pwm_get_state() function call
-Rework if conditional for channel disablement in .apply()
-Remove channel disabling from .probe(), and clear LEADIN register bits
in .apply instead
-Update commit message for Patch 1

Changes since v4:
-Add co-developed-by tag
-Include mod_devicetable.h and remove of.h
-Update comment with correct calulation for high/low time
-Fix missing return from dev_err_probe

Changes since v3:
-Removed variable for address and calculate in place instead
-Utilized u32_replace_bits() when updating KMB_PWM_LEADIN_OFFSET
-Utilized dev_err_probe() for error reporting
-Updated comments to use physical units
-Updated error check for pwmchip_add()

Changes since v2:
-Include documentation about HW limitation/behaviour
-Use hex values for KMB_PWM_COUNT_MAX
-Redefine register macros
-Utilize FIELD_GET/FIELD_PREP for calculating pwm_l/h_count and
pwm_count
-Round up duty cycle/period values
-Get current hardware state in .apply instead of cached values
-Do a polarity check before .enabled
-Round high time/low time to closest value
-Set enable bit in KMB_PWM_LEADIN_OFFSET to 0 in probe
-Correct the naming for MODULE_ALIAS
-Add additionalProperties: false in DT bindings

Changes since v1:
-Updated licensing info, "clocks" property and example in DT bindings
-Updated name of DT bindings document to match compatible string
-Removed 1 patch for addition of new sysfs attribute "count"
-Added support for COMPILE_TEST in Kconfig
-Updated naming of defines and regmap attribute
-Updated calculation of waveform high time and low time
-Added range checking for waveform high/low time
-Implemented .get_state
-Removed register writes for lead-in and count values (left to default)
-Updated register access to single-access
-Folded keembay_pwm_enable/disable_channel,
keembay_pwm_config_period/duty_cycle,
 and keembay_pwm_config into keembay_pwm_apply
-Updated error messages/error codes
-Removed pwm_disable from keembay_pwm_remove
-Removed clk_prepare/clk_enable/clk_disable from driver

Lai, Poey Seng (1):
  pwm: Add PWM driver for Intel Keem Bay

Vineetha G. Jaya Kumaran (1):
  dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM

.../bindings/pwm/intel,keembay-pwm.yaml       |  47 ++++
 drivers/pwm/Kconfig                           |   9 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-keembay.c                     | 232 ++++++++++++++++++
 4 files changed, 289 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml
 create mode 100644 drivers/pwm/pwm-keembay.c

-- 
2.17.1

[PATCH v7 1/2] pwm: Add PWM driver for Intel Keem Bay

[vijayakannan.ayyathurai]

From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

The Intel Keem Bay SoC requires PWM support.
Add the pwm-keembay driver to enable this.

Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com>
Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
Co-developed-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pwm/Kconfig       |   9 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-keembay.c | 232 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 242 insertions(+)
 create mode 100644 drivers/pwm/pwm-keembay.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7dbcf6973d33..4f1fdbc2e5e3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -254,6 +254,15 @@ config PWM_JZ4740
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-jz4740.
 
+config PWM_KEEMBAY
+	tristate "Intel Keem Bay PWM driver"
+	depends on ARM64 || COMPILE_TEST
+	help
+	  The platform driver for Intel Keem Bay PWM controller.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-keembay.
+
 config PWM_LP3943
 	tristate "TI/National Semiconductor LP3943 PWM support"
 	depends on MFD_LP3943
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 2c2ba0a03557..a1051122eb07 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
 obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
 obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
+obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
 obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
 obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
 obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
new file mode 100644
index 000000000000..6f41b1c0ae9a
--- /dev/null
+++ b/drivers/pwm/pwm-keembay.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Keem Bay PWM driver
+ *
+ * Copyright (C) 2020 Intel Corporation
+ * Authors: Lai Poey Seng <poey.seng.lai@intel.com>
+ *          Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
+ *
+ * Limitations:
+ * - Upon disabling a channel, the currently running
+ *   period will not be completed. However, upon
+ *   reconfiguration of the duty cycle/period, the
+ *   currently running period will be completed first.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define KMB_TOTAL_PWM_CHANNELS		6
+#define KMB_PWM_COUNT_MAX		0xffff
+#define KMB_PWM_EN_BIT			BIT(31)
+
+/* Mask */
+#define KMB_PWM_HIGH_MASK		GENMASK(31, 16)
+#define KMB_PWM_LOW_MASK		GENMASK(15, 0)
+#define KMB_PWM_LEADIN_MASK		GENMASK(30, 0)
+
+/* PWM Register offset */
+#define KMB_PWM_LEADIN_OFFSET(ch)	(0x00 + 4 * (ch))
+#define KMB_PWM_HIGHLOW_OFFSET(ch)	(0x20 + 4 * (ch))
+
+struct keembay_pwm {
+	struct pwm_chip chip;
+	struct device *dev;
+	struct clk *clk;
+	void __iomem *base;
+};
+
+static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip)
+{
+	return container_of(chip, struct keembay_pwm, chip);
+}
+
+static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask,
+					   u32 val, u32 offset)
+{
+	u32 buff = readl(priv->base + offset);
+
+	buff = u32_replace_bits(buff, val, mask);
+	writel(buff, priv->base + offset);
+}
+
+static void keembay_pwm_enable(struct keembay_pwm *priv, int ch)
+{
+	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1,
+				KMB_PWM_LEADIN_OFFSET(ch));
+}
+
+static void keembay_pwm_disable(struct keembay_pwm *priv, int ch)
+{
+	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0,
+				KMB_PWM_LEADIN_OFFSET(ch));
+}
+
+static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
+	unsigned long long pwm_h_count, pwm_l_count;
+	unsigned long clk_rate;
+	u32 buff;
+
+	clk_rate = clk_get_rate(priv->clk);
+
+	/* Read channel enabled status */
+	buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
+	if (buff & KMB_PWM_EN_BIT)
+		state->enabled = true;
+	else
+		state->enabled = false;
+
+	/* Read period and duty cycle */
+	buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
+	pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC;
+	pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC;
+	state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate);
+	state->period = DIV_ROUND_UP_ULL(pwm_h_count + pwm_l_count, clk_rate);
+}
+
+static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
+{
+	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
+	struct pwm_state current_state;
+	u16 pwm_h_count, pwm_l_count;
+	unsigned long long div;
+	unsigned long clk_rate;
+	u32 pwm_count = 0;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -ENOSYS;
+
+	keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0,
+				KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
+
+	keembay_pwm_get_state(chip, pwm, &current_state);
+
+	if (!state->enabled) {
+		if (current_state.enabled)
+			keembay_pwm_disable(priv, pwm->hwpwm);
+		return 0;
+	}
+
+	/*
+	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register contain
+	 * the high time of the waveform, while the last 16 bits contain
+	 * the low time of the waveform, in terms of clock cycles.
+	 *
+	 * high time = clock rate * duty cycle
+	 * low time =  clock rate * (period - duty cycle)
+	 *
+	 * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
+	 * high time = 500MHz * 30us = 0x3A98
+	 * low time = 500MHz * 20us = 0x2710
+	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
+	 */
+
+	clk_rate = clk_get_rate(priv->clk);
+
+	/* Configure waveform high time */
+	div = clk_rate * state->duty_cycle;
+	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
+	if (div > KMB_PWM_COUNT_MAX)
+		return -ERANGE;
+
+	pwm_h_count = div;
+
+	/* Configure waveform low time */
+	div = clk_rate * (state->period - state->duty_cycle);
+	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
+	if (div > KMB_PWM_COUNT_MAX)
+		return -ERANGE;
+
+	pwm_l_count = div;
+
+	pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) |
+		    FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count);
+
+	writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
+
+	if (state->enabled && !current_state.enabled)
+		keembay_pwm_enable(priv, pwm->hwpwm);
+
+	return 0;
+}
+
+static const struct pwm_ops keembay_pwm_ops = {
+	.owner = THIS_MODULE,
+	.apply = keembay_pwm_apply,
+	.get_state = keembay_pwm_get_state,
+};
+
+static int keembay_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct keembay_pwm *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock\n");
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->chip.base = -1;
+	priv->chip.dev = dev;
+	priv->chip.ops = &keembay_pwm_ops;
+	priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
+
+	ret = pwmchip_add(&priv->chip);
+	if (ret) {
+		dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int keembay_pwm_remove(struct platform_device *pdev)
+{
+	struct keembay_pwm *priv = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&priv->chip);
+}
+
+static const struct of_device_id keembay_pwm_of_match[] = {
+	{ .compatible = "intel,keembay-pwm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, keembay_pwm_of_match);
+
+static struct platform_driver keembay_pwm_driver = {
+	.probe	= keembay_pwm_probe,
+	.remove	= keembay_pwm_remove,
+	.driver	= {
+		.name = "pwm-keembay",
+		.of_match_table = keembay_pwm_of_match,
+	},
+};
+module_platform_driver(keembay_pwm_driver);
+
+MODULE_ALIAS("platform:pwm-keembay");
+MODULE_DESCRIPTION("Intel Keem Bay PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH v7 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM

[vijayakannan.ayyathurai]

From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.

Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 .../bindings/pwm/intel,keembay-pwm.yaml       | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml
new file mode 100644
index 000000000000..a37433487632
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2020 Intel Corporation
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/intel,keembay-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Keem Bay PWM Device Tree Bindings
+
+maintainers:
+  - Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    enum:
+      - intel,keembay-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - '#pwm-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #define KEEM_BAY_A53_GPIO
+
+    pwm@203200a0 {
+      compatible = "intel,keembay-pwm";
+      reg = <0x203200a0 0xe8>;
+      clocks = <&scmi_clk KEEM_BAY_A53_GPIO>;
+      #pwm-cells = <2>;
+    };
-- 
2.17.1

Re: [PATCH v7 0/2] Add PWM support for Intel Keem Bay SoC

[mark gross]

Have I reviewed this yet?
are you waiting on feedback?
--mark

On Thu, Sep 10, 2020 at 12:27:17AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 
> Hi,
> 
> This patch set enables support for PWM on the Intel Keem Bay SoC.
> Keem Bay is an ARM based SoC, and the GPIO module allows
> configuration of 6 PWM outputs.
> 
> Patch 1 adds the PWM driver and Patch 2 is for the required
> Device Tree bindings documentation.
> 
> This driver was tested on the Keem Bay evaluation module board.
> 
> Thank you.
> 
> Regards,
> Vijay
> 
> Changes since v6:
> - Add reviewed-by tag
> 
> Changes since v5:
> -Reorder symbols/Kconfig in drivers/pwm/Kconfig and drivers/pwm/Makefile
> -Use "Limitations" for consistency
> -Add clk_prepare_enable()
> -Reorder keembay_pwm_get_state() function call
> -Rework if conditional for channel disablement in .apply()
> -Remove channel disabling from .probe(), and clear LEADIN register bits
> in .apply instead
> -Update commit message for Patch 1
> 
> Changes since v4:
> -Add co-developed-by tag
> -Include mod_devicetable.h and remove of.h
> -Update comment with correct calulation for high/low time
> -Fix missing return from dev_err_probe
> 
> Changes since v3:
> -Removed variable for address and calculate in place instead
> -Utilized u32_replace_bits() when updating KMB_PWM_LEADIN_OFFSET
> -Utilized dev_err_probe() for error reporting
> -Updated comments to use physical units
> -Updated error check for pwmchip_add()
> 
> Changes since v2:
> -Include documentation about HW limitation/behaviour
> -Use hex values for KMB_PWM_COUNT_MAX
> -Redefine register macros
> -Utilize FIELD_GET/FIELD_PREP for calculating pwm_l/h_count and
> pwm_count
> -Round up duty cycle/period values
> -Get current hardware state in .apply instead of cached values
> -Do a polarity check before .enabled
> -Round high time/low time to closest value
> -Set enable bit in KMB_PWM_LEADIN_OFFSET to 0 in probe
> -Correct the naming for MODULE_ALIAS
> -Add additionalProperties: false in DT bindings
> 
> Changes since v1:
> -Updated licensing info, "clocks" property and example in DT bindings
> -Updated name of DT bindings document to match compatible string
> -Removed 1 patch for addition of new sysfs attribute "count"
> -Added support for COMPILE_TEST in Kconfig
> -Updated naming of defines and regmap attribute
> -Updated calculation of waveform high time and low time
> -Added range checking for waveform high/low time
> -Implemented .get_state
> -Removed register writes for lead-in and count values (left to default)
> -Updated register access to single-access
> -Folded keembay_pwm_enable/disable_channel,
> keembay_pwm_config_period/duty_cycle,
>  and keembay_pwm_config into keembay_pwm_apply
> -Updated error messages/error codes
> -Removed pwm_disable from keembay_pwm_remove
> -Removed clk_prepare/clk_enable/clk_disable from driver
> 
> Lai, Poey Seng (1):
>   pwm: Add PWM driver for Intel Keem Bay
> 
> Vineetha G. Jaya Kumaran (1):
>   dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM
> 
> .../bindings/pwm/intel,keembay-pwm.yaml       |  47 ++++
>  drivers/pwm/Kconfig                           |   9 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-keembay.c                     | 232 ++++++++++++++++++
>  4 files changed, 289 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml
>  create mode 100644 drivers/pwm/pwm-keembay.c
> 
> -- 
> 2.17.1
> 

RE: [PATCH v7 0/2] Add PWM support for Intel Keem Bay SoC

[Ayyathurai, Vijayakannan]

Hi Mark,
It is an ongoing patch handled by Vineetha(earlier PWM developer with IOTG). I am filling the rest of the review comments after she has gone for her higher Studies. 

If required, let me push another thread internally for your review.
 
Thanks,
Vijay 
-----Original Message-----
From: mark gross <mgross@linux.intel.com> 
Sent: Friday, 18 September, 2020 5:00 AM
To: Ayyathurai, Vijayakannan <vijayakannan.ayyathurai@intel.com>
Cc: thierry.reding@gmail.com; u.kleine-koenig@pengutronix.de; robh+dt@kernel.org; linux-pwm@vger.kernel.org; devicetree@vger.kernel.org; Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; andriy.shevchenko@linux.intel.com; Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
Subject: Re: [PATCH v7 0/2] Add PWM support for Intel Keem Bay SoC

Have I reviewed this yet?
are you waiting on feedback?
--mark

On Thu, Sep 10, 2020 at 12:27:17AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 
> Hi,
> 
> This patch set enables support for PWM on the Intel Keem Bay SoC.
> Keem Bay is an ARM based SoC, and the GPIO module allows configuration 
> of 6 PWM outputs.
> 
> Patch 1 adds the PWM driver and Patch 2 is for the required Device 
> Tree bindings documentation.
> 
> This driver was tested on the Keem Bay evaluation module board.
> 
> Thank you.
> 
> Regards,
> Vijay
> 
> Changes since v6:
> - Add reviewed-by tag
> 
> Changes since v5:
> -Reorder symbols/Kconfig in drivers/pwm/Kconfig and 
> drivers/pwm/Makefile -Use "Limitations" for consistency -Add 
> clk_prepare_enable() -Reorder keembay_pwm_get_state() function call 
> -Rework if conditional for channel disablement in .apply() -Remove 
> channel disabling from .probe(), and clear LEADIN register bits in 
> .apply instead -Update commit message for Patch 1
> 
> Changes since v4:
> -Add co-developed-by tag
> -Include mod_devicetable.h and remove of.h -Update comment with 
> correct calulation for high/low time -Fix missing return from 
> dev_err_probe
> 
> Changes since v3:
> -Removed variable for address and calculate in place instead -Utilized 
> u32_replace_bits() when updating KMB_PWM_LEADIN_OFFSET -Utilized 
> dev_err_probe() for error reporting -Updated comments to use physical 
> units -Updated error check for pwmchip_add()
> 
> Changes since v2:
> -Include documentation about HW limitation/behaviour -Use hex values 
> for KMB_PWM_COUNT_MAX -Redefine register macros -Utilize 
> FIELD_GET/FIELD_PREP for calculating pwm_l/h_count and pwm_count 
> -Round up duty cycle/period values -Get current hardware state in 
> .apply instead of cached values -Do a polarity check before .enabled 
> -Round high time/low time to closest value -Set enable bit in 
> KMB_PWM_LEADIN_OFFSET to 0 in probe -Correct the naming for 
> MODULE_ALIAS -Add additionalProperties: false in DT bindings
> 
> Changes since v1:
> -Updated licensing info, "clocks" property and example in DT bindings 
> -Updated name of DT bindings document to match compatible string 
> -Removed 1 patch for addition of new sysfs attribute "count"
> -Added support for COMPILE_TEST in Kconfig -Updated naming of defines 
> and regmap attribute -Updated calculation of waveform high time and 
> low time -Added range checking for waveform high/low time -Implemented 
> .get_state -Removed register writes for lead-in and count values (left 
> to default) -Updated register access to single-access -Folded 
> keembay_pwm_enable/disable_channel,
> keembay_pwm_config_period/duty_cycle,
>  and keembay_pwm_config into keembay_pwm_apply -Updated error 
> messages/error codes -Removed pwm_disable from keembay_pwm_remove 
> -Removed clk_prepare/clk_enable/clk_disable from driver
> 
> Lai, Poey Seng (1):
>   pwm: Add PWM driver for Intel Keem Bay
> 
> Vineetha G. Jaya Kumaran (1):
>   dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM
> 
> .../bindings/pwm/intel,keembay-pwm.yaml       |  47 ++++
>  drivers/pwm/Kconfig                           |   9 +
>  drivers/pwm/Makefile                          |   1 +
>  drivers/pwm/pwm-keembay.c                     | 232 ++++++++++++++++++
>  4 files changed, 289 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/pwm/intel,keembay-pwm.yaml
>  create mode 100644 drivers/pwm/pwm-keembay.c
> 
> --
> 2.17.1
> 

Re: [PATCH v7 1/2] pwm: Add PWM driver for Intel Keem Bay

[Uwe Kleine-König]

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

On Thu, Sep 10, 2020 at 12:27:18AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 
> The Intel Keem Bay SoC requires PWM support.
> Add the pwm-keembay driver to enable this.
> 
> Signed-off-by: Lai, Poey Seng <poey.seng.lai@intel.com>
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> Co-developed-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pwm/Kconfig       |   9 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-keembay.c | 232 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 242 insertions(+)
>  create mode 100644 drivers/pwm/pwm-keembay.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf6973d33..4f1fdbc2e5e3 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -254,6 +254,15 @@ config PWM_JZ4740
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-jz4740.
>  
> +config PWM_KEEMBAY
> +	tristate "Intel Keem Bay PWM driver"
> +	depends on ARM64 || COMPILE_TEST
> +	help
> +	  The platform driver for Intel Keem Bay PWM controller.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-keembay.

I wonder about the dependency. If this is an external chip it should be
possible to be used on platforms other than ARM64. If this is only part
of a certain SoC: Isn't there a more specific symbol than ARM64 to
depend on?

> +
>  config PWM_LP3943
>  	tristate "TI/National Semiconductor LP3943 PWM support"
>  	depends on MFD_LP3943
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a03557..a1051122eb07 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
>  obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
>  obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> +obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
>  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
>  obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
>  obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
> diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
> new file mode 100644
> index 000000000000..6f41b1c0ae9a
> --- /dev/null
> +++ b/drivers/pwm/pwm-keembay.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Keem Bay PWM driver
> + *
> + * Copyright (C) 2020 Intel Corporation
> + * Authors: Lai Poey Seng <poey.seng.lai@intel.com>
> + *          Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> + *
> + * Limitations:
> + * - Upon disabling a channel, the currently running
> + *   period will not be completed. However, upon
> + *   reconfiguration of the duty cycle/period, the
> + *   currently running period will be completed first.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define KMB_TOTAL_PWM_CHANNELS		6
> +#define KMB_PWM_COUNT_MAX		0xffff
> +#define KMB_PWM_EN_BIT			BIT(31)
> +
> +/* Mask */
> +#define KMB_PWM_HIGH_MASK		GENMASK(31, 16)
> +#define KMB_PWM_LOW_MASK		GENMASK(15, 0)
> +#define KMB_PWM_LEADIN_MASK		GENMASK(30, 0)
> +
> +/* PWM Register offset */
> +#define KMB_PWM_LEADIN_OFFSET(ch)	(0x00 + 4 * (ch))
> +#define KMB_PWM_HIGHLOW_OFFSET(ch)	(0x20 + 4 * (ch))
> +
> +struct keembay_pwm {
> +	struct pwm_chip chip;
> +	struct device *dev;
> +	struct clk *clk;
> +	void __iomem *base;
> +};
> +
> +static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct keembay_pwm, chip);
> +}
> +
> +static inline void keembay_pwm_update_bits(struct keembay_pwm *priv, u32 mask,
> +					   u32 val, u32 offset)
> +{
> +	u32 buff = readl(priv->base + offset);
> +
> +	buff = u32_replace_bits(buff, val, mask);
> +	writel(buff, priv->base + offset);
> +}
> +
> +static void keembay_pwm_enable(struct keembay_pwm *priv, int ch)
> +{
> +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 1,
> +				KMB_PWM_LEADIN_OFFSET(ch));
> +}
> +
> +static void keembay_pwm_disable(struct keembay_pwm *priv, int ch)
> +{
> +	keembay_pwm_update_bits(priv, KMB_PWM_EN_BIT, 0,
> +				KMB_PWM_LEADIN_OFFSET(ch));
> +}
> +
> +static void keembay_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				  struct pwm_state *state)
> +{
> +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> +	unsigned long long pwm_h_count, pwm_l_count;
> +	unsigned long clk_rate;
> +	u32 buff;
> +
> +	clk_rate = clk_get_rate(priv->clk);
> +
> +	/* Read channel enabled status */
> +	buff = readl(priv->base + KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
> +	if (buff & KMB_PWM_EN_BIT)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;
> +
> +	/* Read period and duty cycle */
> +	buff = readl(priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> +	pwm_l_count = FIELD_GET(KMB_PWM_LOW_MASK, buff) * NSEC_PER_SEC;
> +	pwm_h_count = FIELD_GET(KMB_PWM_HIGH_MASK, buff) * NSEC_PER_SEC;
> +	state->duty_cycle = DIV_ROUND_UP_ULL(pwm_h_count, clk_rate);
> +	state->period = DIV_ROUND_UP_ULL(pwm_h_count + pwm_l_count, clk_rate);
> +}
> +
> +static int keembay_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct keembay_pwm *priv = to_keembay_pwm_dev(chip);
> +	struct pwm_state current_state;
> +	u16 pwm_h_count, pwm_l_count;
> +	unsigned long long div;
> +	unsigned long clk_rate;
> +	u32 pwm_count = 0;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -ENOSYS;
> +
> +	keembay_pwm_update_bits(priv, KMB_PWM_LEADIN_MASK, 0,
> +				KMB_PWM_LEADIN_OFFSET(pwm->hwpwm));
> +
> +	keembay_pwm_get_state(chip, pwm, &current_state);
> +
> +	if (!state->enabled) {
> +		if (current_state.enabled)
> +			keembay_pwm_disable(priv, pwm->hwpwm);
> +		return 0;
> +	}
> +
> +	/*
> +	 * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register contain
> +	 * the high time of the waveform, while the last 16 bits contain
> +	 * the low time of the waveform, in terms of clock cycles.
> +	 *
> +	 * high time = clock rate * duty cycle
> +	 * low time =  clock rate * (period - duty cycle)
> +	 *
> +	 * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz:
> +	 * high time = 500MHz * 30us = 0x3A98
> +	 * low time = 500MHz * 20us = 0x2710
> +	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
> +	 */
> +
> +	clk_rate = clk_get_rate(priv->clk);
> +
> +	/* Configure waveform high time */
> +	div = clk_rate * state->duty_cycle;
> +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);

Please round down here.

> +	if (div > KMB_PWM_COUNT_MAX)
> +		return -ERANGE;
> +
> +	pwm_h_count = div;
> +
> +	/* Configure waveform low time */
> +	div = clk_rate * (state->period - state->duty_cycle);
> +	div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC);
> +	if (div > KMB_PWM_COUNT_MAX)
> +		return -ERANGE;
> +
> +	pwm_l_count = div;

This isn't right, too.

I think the right formula is:

	div = (clk_rate * state->period) / NSEC_PER_SEC - pwm_h_count

Consider:

  clk_rate = 333334 [Hz]

  state.duty_cycle = 1000500 [ns]
  state.period = 2001000 [ns]

With your calculation (and rounding down in the divisions calculation)
this results in 

pwm_h_count = 333
pwm_l_count = 333

and so a period length of 666 clock ticks (1997996 ns) while it should be
667 clock ticks (2000995 ns).

> +
> +	pwm_count = FIELD_PREP(KMB_PWM_HIGH_MASK, pwm_h_count) |
> +		    FIELD_PREP(KMB_PWM_LOW_MASK, pwm_l_count);
> +
> +	writel(pwm_count, priv->base + KMB_PWM_HIGHLOW_OFFSET(pwm->hwpwm));
> +
> +	if (state->enabled && !current_state.enabled)
> +		keembay_pwm_enable(priv, pwm->hwpwm);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops keembay_pwm_ops = {
> +	.owner = THIS_MODULE,
> +	.apply = keembay_pwm_apply,
> +	.get_state = keembay_pwm_get_state,
> +};
> +
> +static int keembay_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct keembay_pwm *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock\n");
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		return ret;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->chip.base = -1;
> +	priv->chip.dev = dev;
> +	priv->chip.ops = &keembay_pwm_ops;
> +	priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
> +
> +	ret = pwmchip_add(&priv->chip);
> +	if (ret) {
> +		dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;

clk_disable_unprepare is missing in the two last error paths.

> +}
> +
> +static int keembay_pwm_remove(struct platform_device *pdev)
> +{
> +	struct keembay_pwm *priv = platform_get_drvdata(pdev);
> +
> +	return pwmchip_remove(&priv->chip);

clk_disable_unprepare is missing here, too.

> +}

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 v7 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM

[Uwe Kleine-König]

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

On Thu, Sep 10, 2020 at 12:27:19AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 
> Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.
> 
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

nitpick: Your S-o-b should always be last. This way it becomes clear who
added the other tags.

(No need to resend for this, but as patch 1 need some love, please fix
this, too.)

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 v7 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM

[Andy Shevchenko]

On Mon, Sep 21, 2020 at 10:44:01AM +0200, Uwe Kleine-König wrote:
> On Thu, Sep 10, 2020 at 12:27:19AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > 
> > Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.
> > 
> > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> > Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> nitpick: Your S-o-b should always be last. This way it becomes clear who
> added the other tags.

I think it should reflect chronological order. If SoB has been given before
e.g. Ack then SoB should be followed by Ack and not other way around.

But it's my interpretation of the chapter 12. Actually it doesn't say anything
about placement (ordering) of Ack.

So, formally the above follows the letter of law.

What did you have in mind when commenting that? Perhaps I missed documentation.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM

[Uwe Kleine-König]

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

Hello Andy,

On Mon, Sep 21, 2020 at 01:37:56PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 21, 2020 at 10:44:01AM +0200, Uwe Kleine-König wrote:
> > On Thu, Sep 10, 2020 at 12:27:19AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > > 
> > > Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.
> > > 
> > > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> > > Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > nitpick: Your S-o-b should always be last. This way it becomes clear who
> > added the other tags.
> 
> I think it should reflect chronological order. If SoB has been given before
> e.g. Ack then SoB should be followed by Ack and not other way around.

This is how I interpret the rules even though I admit it is not
formalized explicitly. The idea is just what I wrote, when the patch
ends up in git with:

	Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
	Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
	Reviewed-by: Rob Herring <robh@kernel.org>
	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
	Signed-off-by: Peter Maintainer <p.maintainer@tralala>

I'd expect that is was Peter M. who added Rob's and my tag, while when
it is

	Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
	Reviewed-by: Rob Herring <robh@kernel.org>
	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
	Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
	Signed-off-by: Peter Maintainer <p.maintainer@tralala>

it was Vijayakannan who added them.

IMHO this makes sense as Vijayakannan modified the commit log and then
it is usual to add the signature at the end. In my eyes this is more
sensible than the date order, but it seems this is subjective.

I'm aware that most people don't care; and I don't care enough to argue
this case any further.

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 v7 2/2] dt-bindings: pwm: keembay: Add bindings for Intel Keem Bay PWM

[Andy Shevchenko]

On Tue, Sep 22, 2020 at 08:34:03AM +0200, Uwe Kleine-König wrote:
> On Mon, Sep 21, 2020 at 01:37:56PM +0300, Andy Shevchenko wrote:
> > On Mon, Sep 21, 2020 at 10:44:01AM +0200, Uwe Kleine-König wrote:
> > > On Thu, Sep 10, 2020 at 12:27:19AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> > > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > > > 
> > > > Add PWM Device Tree bindings documentation for the Intel Keem Bay SoC.
> > > > 
> > > > Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> > > > Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > nitpick: Your S-o-b should always be last. This way it becomes clear who
> > > added the other tags.
> > 
> > I think it should reflect chronological order. If SoB has been given before
> > e.g. Ack then SoB should be followed by Ack and not other way around.
> 
> This is how I interpret the rules even though I admit it is not
> formalized explicitly. The idea is just what I wrote, when the patch
> ends up in git with:
> 
> 	Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> 	Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 	Reviewed-by: Rob Herring <robh@kernel.org>
> 	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 	Signed-off-by: Peter Maintainer <p.maintainer@tralala>
> 
> I'd expect that is was Peter M. who added Rob's and my tag, while when
> it is
> 
> 	Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@intel.com>
> 	Reviewed-by: Rob Herring <robh@kernel.org>
> 	Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 	Signed-off-by: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> 	Signed-off-by: Peter Maintainer <p.maintainer@tralala>
> 
> it was Vijayakannan who added them.
> 
> IMHO this makes sense as Vijayakannan modified the commit log and then
> it is usual to add the signature at the end. In my eyes this is more
> sensible than the date order, but it seems this is subjective.
> 
> I'm aware that most people don't care; and I don't care enough to argue
> this case any further.

This makes sense. Consider that we are on the same page.
Thanks!

-- 
With Best Regards,
Andy Shevchenko