The Sophgo CV1800 chip provides a set of four independent
PWM channel outputs.
This series adds PWM controller support for Sophgo cv1800.
Jingbao Qiu (2):
dt-bindings: pwm: sophgo: add pwm for Sophgo CV1800 series SoC.
pwm: sophgo: add pwm support for Sophgo CV1800 SoC
.../bindings/pwm/sophgo,cv1800-pwm.yaml | 45 ++++
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-cv1800.c | 218 ++++++++++++++++++
4 files changed, 274 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,cv1800-pwm.yaml
create mode 100644 drivers/pwm/pwm-cv1800.c
base-commit: 7afc0e7f681e6efd6b826f003fc14c17b5093643
--
2.25.1
Add devicetree binding to describe the PWM for Sophgo CV1800 SoC.
Signed-off-by: Jingbao Qiu <[email protected]>
---
.../bindings/pwm/sophgo,cv1800-pwm.yaml | 45 +++++++++++++++++++
1 file changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,cv1800-pwm.yaml
diff --git a/Documentation/devicetree/bindings/pwm/sophgo,cv1800-pwm.yaml b/Documentation/devicetree/bindings/pwm/sophgo,cv1800-pwm.yaml
new file mode 100644
index 000000000000..7fcdf98b27fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/sophgo,cv1800-pwm.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/sophgo,cv1800-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo CV1800 PWM controller
+
+description:
+ The chip provides a set of four independent PWM channel outputs.
+
+maintainers:
+ - Jingbao Qiu <[email protected]>
+
+allOf:
+ - $ref: pwm.yaml#
+
+properties:
+ compatible:
+ const: sophgo,cv1800-pwm
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ "#pwm-cells":
+ const: 3
+
+required:
+ - compatible
+ - reg
+ - clocks
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ pwm0: pwm@3060000 {
+ compatible = "sophgo,cv1800-pwm";
+ reg = <0x3060000 0x1000>;
+ clocks = <&clk 60>;
+ #pwm-cells = <3>;
+ };
--
2.25.1
Implement the PWM driver for CV1800.
Signed-off-by: Jingbao Qiu <[email protected]>
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-cv1800.c | 218 +++++++++++++++++++++++++++++++++++++++
3 files changed, 229 insertions(+)
create mode 100644 drivers/pwm/pwm-cv1800.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..455f07af94f7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -186,6 +186,16 @@ config PWM_CROS_EC
PWM driver for exposing a PWM attached to the ChromeOS Embedded
Controller.
+config PWM_CV1800
+ tristate "Sophgo CV1800 PWM driver"
+ depends on ARCH_SOPHGO || COMPILE_TEST
+ help
+ Generic PWM framework driver for the Sophgo CV1800 series
+ SoCs.
+
+ To compile this driver as a module, build the dependecies
+ as modules, this will be called pwm-cv1800.
+
config PWM_DWC_CORE
tristate
depends on HAS_IOMEM
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c5ec9e168ee7..6c3c4a07a316 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLK) += pwm-clk.o
obj-$(CONFIG_PWM_CLPS711X) += pwm-clps711x.o
obj-$(CONFIG_PWM_CRC) += pwm-crc.o
obj-$(CONFIG_PWM_CROS_EC) += pwm-cros-ec.o
+obj-$(CONFIG_PWM_CV1800) += pwm-cv1800.o
obj-$(CONFIG_PWM_DWC_CORE) += pwm-dwc-core.o
obj-$(CONFIG_PWM_DWC) += pwm-dwc.o
obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
diff --git a/drivers/pwm/pwm-cv1800.c b/drivers/pwm/pwm-cv1800.c
new file mode 100644
index 000000000000..4d4f233c9087
--- /dev/null
+++ b/drivers/pwm/pwm-cv1800.c
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * rtc-cv1800.c: PWM driver for Sophgo cv1800 RTC
+ *
+ * Author: Jingbao Qiu <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+#define HLPERIOD_BASE 0x00
+#define PERIOD_BASE 0x04
+#define POLARITY 0x040
+#define PWMSTART 0x044
+#define PWMDONE 0x048
+#define PWMUPDATE 0x4c
+#define PWM_OE 0xd0
+#define HLPERIOD_SHIFT 0x08
+#define PERIOD_SHIFT 0x08
+
+#define HLPERIOD(n) (HLPERIOD_BASE + ((n) * HLPERIOD_SHIFT))
+#define PERIOD(n) (PERIOD_BASE + ((n) * PERIOD_SHIFT))
+#define UPDATE(n) (BIT(0) << (n))
+#define OE_MASK(n) (BIT(0) << (n))
+#define START_MASK(n) (BIT(0) << (n))
+
+#define PERIOD_RESET 0x02
+#define HLPERIOD_RESET 0x1
+#define REG_DISABLE 0x0U
+#define REG_ENABLE BIT(0)
+
+struct soc_info {
+ unsigned int num_pwms;
+};
+
+struct cv1800_pwm {
+ struct pwm_chip chip;
+ struct regmap *map;
+ struct clk *clk;
+};
+
+static inline struct cv1800_pwm *to_cv1800_pwm_dev(struct pwm_chip *chip)
+{
+ return container_of(chip, struct cv1800_pwm, chip);
+}
+
+static int cv1800_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
+ u32 enable)
+{
+ struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
+ u32 pwm_enable;
+
+ regmap_read(priv->map, PWMSTART, &pwm_enable);
+ pwm_enable >>= pwm->hwpwm;
+
+ if (enable)
+ clk_prepare_enable(priv->clk);
+ else
+ clk_disable_unprepare(priv->clk);
+
+ /*
+ * If the parameters are changed during runtime, Register needs
+ * to be updated to take effect.
+ */
+ if (pwm_enable) {
+ regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
+ REG_ENABLE << pwm->hwpwm);
+ regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
+ REG_DISABLE << pwm->hwpwm);
+ } else {
+ regmap_update_bits(priv->map, PWM_OE, OE_MASK(pwm->hwpwm),
+ enable << pwm->hwpwm);
+ regmap_update_bits(priv->map, PWMSTART, START_MASK(pwm->hwpwm),
+ enable << pwm->hwpwm);
+ }
+
+ return 0;
+}
+
+static int cv1800_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
+ u64 period_ns, duty_ns;
+ u32 period_val, hlperiod_val;
+ unsigned long long rate, div;
+
+ period_ns = state->period;
+ duty_ns = state->duty_cycle;
+
+ rate = (unsigned long long)clk_get_rate(priv->clk);
+
+ div = rate * period_ns;
+ do_div(div, NSEC_PER_SEC);
+ period_val = div;
+
+ div = rate * (period_ns - duty_ns);
+ do_div(div, NSEC_PER_SEC);
+ hlperiod_val = div;
+
+ regmap_write(priv->map, PERIOD(pwm->hwpwm), period_val);
+ regmap_write(priv->map, HLPERIOD(pwm->hwpwm), hlperiod_val);
+
+ cv1800_pwm_enable(chip, pwm, state->enabled);
+
+ return 0;
+}
+
+static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
+ u32 period_val, hlperiod_val, tem;
+ u64 rate;
+ u64 period_ns = 0;
+ u64 duty_ns = 0;
+ u32 enable = 0;
+
+ regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
+ regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
+
+ if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
+ rate = (u64)clk_get_rate(priv->clk);
+
+ tem = NSEC_PER_SEC * period_val;
+ do_div(tem, rate);
+ period_ns = tem;
+
+ tem = period_val * period_ns;
+ do_div(tem, hlperiod_val);
+ duty_ns = tem;
+
+ regmap_read(priv->map, PWMSTART, &enable);
+ enable >>= pwm->hwpwm;
+ }
+
+ state->period = period_ns;
+ state->duty_cycle = duty_ns;
+ state->enabled = enable;
+
+ return 0;
+}
+
+static const struct pwm_ops cv1800_pwm_ops = {
+ .apply = cv1800_pwm_apply,
+ .get_state = cv1800_pwm_get_state,
+};
+
+static const struct regmap_config cv1800_pwm_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+};
+
+static int cv1800_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cv1800_pwm *cv_pwm;
+ void __iomem *base;
+ const struct soc_info *info;
+
+ info = device_get_match_data(dev);
+ if (!info)
+ return -EINVAL;
+
+ cv_pwm = devm_kzalloc(dev, sizeof(*cv_pwm), GFP_KERNEL);
+ if (!cv_pwm)
+ return -ENOMEM;
+
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ cv_pwm->map = devm_regmap_init_mmio(&pdev->dev, base,
+ &cv1800_pwm_regmap_config);
+ if (IS_ERR(cv_pwm->map))
+ return PTR_ERR(cv_pwm->map);
+
+ cv_pwm->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(cv_pwm->clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(cv_pwm->clk),
+ "clk not found\n");
+
+ cv_pwm->chip.dev = dev;
+ cv_pwm->chip.ops = &cv1800_pwm_ops;
+ cv_pwm->chip.npwm = info->num_pwms;
+
+ return devm_pwmchip_add(dev, &cv_pwm->chip);
+}
+
+static const struct soc_info cv1800b_soc_info = {
+ .num_pwms = 4,
+};
+
+static const struct of_device_id cv1800_pwm_dt_ids[] = {
+ { .compatible = "sophgo,cv1800-pwm", .data = &cv1800b_soc_info },
+ {},
+};
+MODULE_DEVICE_TABLE(of, cv1800_pwm_dt_ids);
+
+static struct platform_driver cv1800_pwm_driver = {
+ .driver = {
+ .name = "cv1800-pwm",
+ .of_match_table = cv1800_pwm_dt_ids,
+ },
+ .probe = cv1800_pwm_probe,
+};
+module_platform_driver(cv1800_pwm_driver);
+
+MODULE_ALIAS("platform:cv1800-pwm");
+MODULE_AUTHOR("Jingbao Qiu");
+MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
+MODULE_LICENSE("GPL");
--
2.25.1
On 07/02/2024 06:58, Jingbao Qiu wrote:
> Add devicetree binding to describe the PWM for Sophgo CV1800 SoC.
>
> Signed-off-by: Jingbao Qiu <[email protected]>
> ---
> .../bindings/pwm/sophgo,cv1800-pwm.yaml | 45 +++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,cv1800-pwm.yaml
>
If there is going to be resend: drop full stop from subject and re-order
maintainers and description to match convention (look at other schemas
and example-schema).
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Wed, Feb 7, 2024 at 3:45 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 07/02/2024 06:58, Jingbao Qiu wrote:
> > Add devicetree binding to describe the PWM for Sophgo CV1800 SoC.
> >
> > Signed-off-by: Jingbao Qiu <[email protected]>
> > ---
> > .../bindings/pwm/sophgo,cv1800-pwm.yaml | 45 +++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pwm/sophgo,cv1800-pwm.yaml
> >
>
> If there is going to be resend: drop full stop from subject and re-order
> maintainers and description to match convention (look at other schemas
> and example-schema).
Thank you for your suggestion. I will fix it.
Best regards,
Jingbao Qiu
On 07/02/2024 07:09, Jingbao Qiu wrote:
> Implement the PWM driver for CV1800.
>
> Signed-off-by: Jingbao Qiu <[email protected]>
> ---
> +
> +static struct platform_driver cv1800_pwm_driver = {
> + .driver = {
> + .name = "cv1800-pwm",
> + .of_match_table = cv1800_pwm_dt_ids,
> + },
> + .probe = cv1800_pwm_probe,
> +};
> +module_platform_driver(cv1800_pwm_driver);
> +
> +MODULE_ALIAS("platform:cv1800-pwm");
You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.
Best regards,
Krzysztof
Hello Jingbao,
On Wed, Feb 07, 2024 at 02:09:13PM +0800, Jingbao Qiu wrote:
> Implement the PWM driver for CV1800.
>
> Signed-off-by: Jingbao Qiu <[email protected]>
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-cv1800.c | 218 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 229 insertions(+)
> create mode 100644 drivers/pwm/pwm-cv1800.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4b956d661755..455f07af94f7 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -186,6 +186,16 @@ config PWM_CROS_EC
> PWM driver for exposing a PWM attached to the ChromeOS Embedded
> Controller.
>
> +config PWM_CV1800
> + tristate "Sophgo CV1800 PWM driver"
> + depends on ARCH_SOPHGO || COMPILE_TEST
> + help
> + Generic PWM framework driver for the Sophgo CV1800 series
> + SoCs.
> +
> + To compile this driver as a module, build the dependecies
> + as modules, this will be called pwm-cv1800.
> +
> config PWM_DWC_CORE
> tristate
> depends on HAS_IOMEM
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c5ec9e168ee7..6c3c4a07a316 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLK) += pwm-clk.o
> obj-$(CONFIG_PWM_CLPS711X) += pwm-clps711x.o
> obj-$(CONFIG_PWM_CRC) += pwm-crc.o
> obj-$(CONFIG_PWM_CROS_EC) += pwm-cros-ec.o
> +obj-$(CONFIG_PWM_CV1800) += pwm-cv1800.o
> obj-$(CONFIG_PWM_DWC_CORE) += pwm-dwc-core.o
> obj-$(CONFIG_PWM_DWC) += pwm-dwc.o
> obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
> diff --git a/drivers/pwm/pwm-cv1800.c b/drivers/pwm/pwm-cv1800.c
> new file mode 100644
> index 000000000000..4d4f233c9087
> --- /dev/null
> +++ b/drivers/pwm/pwm-cv1800.c
> @@ -0,0 +1,218 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * rtc-cv1800.c: PWM driver for Sophgo cv1800 RTC
> + *
> + * Author: Jingbao Qiu <[email protected]>
Please document the behaviour of the PWM here at the top of the driver.
Things to mention are:
- How does the hardware behave on disable? (Usual behaviours include:
output of inactive state; freeze where it just happens to be; high-z)
- If you reconfigure the hardware, does it complete the previously
running period before emitting the new wave form?
- Are there possible glitches in .apply()? (i.e. can it happen, that
for a short moment a wave form is emitted that has the new period but
the old duty_cycle?)
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define HLPERIOD_BASE 0x00
> +#define PERIOD_BASE 0x04
> +#define POLARITY 0x040
> +#define PWMSTART 0x044
> +#define PWMDONE 0x048
> +#define PWMUPDATE 0x4c
> +#define PWM_OE 0xd0
> +#define HLPERIOD_SHIFT 0x08
> +#define PERIOD_SHIFT 0x08
> +
> +#define HLPERIOD(n) (HLPERIOD_BASE + ((n) * HLPERIOD_SHIFT))
> +#define PERIOD(n) (PERIOD_BASE + ((n) * PERIOD_SHIFT))
> +#define UPDATE(n) (BIT(0) << (n))
> +#define OE_MASK(n) (BIT(0) << (n))
> +#define START_MASK(n) (BIT(0) << (n))
> +
> +#define PERIOD_RESET 0x02
> +#define HLPERIOD_RESET 0x1
> +#define REG_DISABLE 0x0U
> +#define REG_ENABLE BIT(0)
Please use a driver specific prefix for all these defines.
> +struct soc_info {
> + unsigned int num_pwms;
> +};
> +
> +struct cv1800_pwm {
> + struct pwm_chip chip;
> + struct regmap *map;
> + struct clk *clk;
> +};
> +
> +static inline struct cv1800_pwm *to_cv1800_pwm_dev(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct cv1800_pwm, chip);
> +}
> +
> +static int cv1800_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> + u32 enable)
This is called with the enable parameter set to state->enabled which is
a bool. Please use a bool here, too, instead of a u32.
> +{
> + struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> + u32 pwm_enable;
> +
> + regmap_read(priv->map, PWMSTART, &pwm_enable);
> + pwm_enable >>= pwm->hwpwm;
I don't understand why this value is shifted here. I guess this misses a
mask?
> + if (enable)
> + clk_prepare_enable(priv->clk);
> + else
> + clk_disable_unprepare(priv->clk);
This is broken. It might well happen that the first call to .apply() has
state->enabled = false. Please make sure to properly balance clk
enabling.
> + /*
> + * If the parameters are changed during runtime, Register needs
> + * to be updated to take effect.
> + */
> + if (pwm_enable) {
> + regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
> + REG_ENABLE << pwm->hwpwm);
> + regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
> + REG_DISABLE << pwm->hwpwm);
REG_DISABLE is zero. Why is this shifted? Very strange.
> + } else {
> + regmap_update_bits(priv->map, PWM_OE, OE_MASK(pwm->hwpwm),
> + enable << pwm->hwpwm);
> + regmap_update_bits(priv->map, PWMSTART, START_MASK(pwm->hwpwm),
> + enable << pwm->hwpwm);
> + }
> +
> + return 0;
> +}
> +
> +static int cv1800_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> + u64 period_ns, duty_ns;
> + u32 period_val, hlperiod_val;
> + unsigned long long rate, div;
> +
> + period_ns = state->period;
> + duty_ns = state->duty_cycle;
> +
> + rate = (unsigned long long)clk_get_rate(priv->clk);
This cast has no effect, so please drop it. To prevent that the clock
changes rate while the PWM is running, please use
clk_rate_exclusive_get().
> + div = rate * period_ns;
This might overflow. Please use mul_u64_u64_div_u64() or one of its
variants and error out on rate > NSEC_PER_SEC. (There are other pwm
drivers that make it right, took a look into these if this is unclear.)
> + do_div(div, NSEC_PER_SEC);
> + period_val = div;
> +
> + div = rate * (period_ns - duty_ns);
> + do_div(div, NSEC_PER_SEC);
> + hlperiod_val = div;
I think it can happen here, that div it too big to fit into this u32.
> + regmap_write(priv->map, PERIOD(pwm->hwpwm), period_val);
> + regmap_write(priv->map, HLPERIOD(pwm->hwpwm), hlperiod_val);
> +
> + cv1800_pwm_enable(chip, pwm, state->enabled);
> +
> + return 0;
> +}
> +
> +static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> + u32 period_val, hlperiod_val, tem;
> + u64 rate;
> + u64 period_ns = 0;
> + u64 duty_ns = 0;
> + u32 enable = 0;
> +
> + regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
> + regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
> +
> + if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
> + rate = (u64)clk_get_rate(priv->clk);
> +
> + tem = NSEC_PER_SEC * period_val;
This might overflow.
> + do_div(tem, rate);
> + period_ns = tem;
This uses wrong rounding. If you enabled PWM_DEBUG during your tests,
this should be catched and logged.
> +
> + tem = period_val * period_ns;
> + do_div(tem, hlperiod_val);
> + duty_ns = tem;
> +
> + regmap_read(priv->map, PWMSTART, &enable);
> + enable >>= pwm->hwpwm;
Again a missing mask??
> + }
> +
> + state->period = period_ns;
> + state->duty_cycle = duty_ns;
> + state->enabled = enable;
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops cv1800_pwm_ops = {
> + .apply = cv1800_pwm_apply,
> + .get_state = cv1800_pwm_get_state,
> +};
> +
> +static const struct regmap_config cv1800_pwm_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> +};
> +
> +static int cv1800_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cv1800_pwm *cv_pwm;
Please pick always the same name for your driver private variable. In
the other functions it is called "priv".
> + void __iomem *base;
> + const struct soc_info *info;
> +
> + info = device_get_match_data(dev);
> + if (!info)
error message please.
> + return -EINVAL;
> +
> + cv_pwm = devm_kzalloc(dev, sizeof(*cv_pwm), GFP_KERNEL);
> + if (!cv_pwm)
> + return -ENOMEM;
> +
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + cv_pwm->map = devm_regmap_init_mmio(&pdev->dev, base,
> + &cv1800_pwm_regmap_config);
> + if (IS_ERR(cv_pwm->map))
> + return PTR_ERR(cv_pwm->map);
> +
> + cv_pwm->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(cv_pwm->clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(cv_pwm->clk),
> + "clk not found\n");
> +
> + cv_pwm->chip.dev = dev;
> + cv_pwm->chip.ops = &cv1800_pwm_ops;
> + cv_pwm->chip.npwm = info->num_pwms;
Missing clk handling here. Please all clk_prepare_enable once for each
running channel.
> + return devm_pwmchip_add(dev, &cv_pwm->chip);
> +}
> +
> +static const struct soc_info cv1800b_soc_info = {
> + .num_pwms = 4,
> +};
Until you support more variants, I suggest to just hardcode npwm to
four.
> +static const struct of_device_id cv1800_pwm_dt_ids[] = {
> + { .compatible = "sophgo,cv1800-pwm", .data = &cv1800b_soc_info },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, cv1800_pwm_dt_ids);
> +
> +static struct platform_driver cv1800_pwm_driver = {
> + .driver = {
> + .name = "cv1800-pwm",
> + .of_match_table = cv1800_pwm_dt_ids,
> + },
> + .probe = cv1800_pwm_probe,
> +};
> +module_platform_driver(cv1800_pwm_driver);
> +
> +MODULE_ALIAS("platform:cv1800-pwm");
I'm with Krzysztof here, this shouldn't be needed.
> +MODULE_AUTHOR("Jingbao Qiu");
> +MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
> +MODULE_LICENSE("GPL");
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed, Feb 7, 2024 at 4:09 PM Krzysztof Kozlowski <[email protected]> wrote:
>
> On 07/02/2024 07:09, Jingbao Qiu wrote:
> > Implement the PWM driver for CV1800.
> >
> > Signed-off-by: Jingbao Qiu <[email protected]>
> > ---
>
>
> > +
> > +static struct platform_driver cv1800_pwm_driver = {
> > + .driver = {
> > + .name = "cv1800-pwm",
> > + .of_match_table = cv1800_pwm_dt_ids,
> > + },
> > + .probe = cv1800_pwm_probe,
> > +};
> > +module_platform_driver(cv1800_pwm_driver);
> > +
> > +MODULE_ALIAS("platform:cv1800-pwm");
>
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.
>
you're right, I will drop it.
Best regards,
Jingbao Qiu
On Wed, Feb 7, 2024 at 4:39 PM Uwe Kleine-König
<[email protected]> wrote:
>
> Hello Jingbao,
>
> On Wed, Feb 07, 2024 at 02:09:13PM +0800, Jingbao Qiu wrote:
> > Implement the PWM driver for CV1800.
> >
> > Signed-off-by: Jingbao Qiu <[email protected]>
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-cv1800.c | 218 +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 229 insertions(+)
> > create mode 100644 drivers/pwm/pwm-cv1800.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 4b956d661755..455f07af94f7 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -186,6 +186,16 @@ config PWM_CROS_EC
> > PWM driver for exposing a PWM attached to the ChromeOS Embedded
> > Controller.
> >
> > +config PWM_CV1800
> > + tristate "Sophgo CV1800 PWM driver"
> > + depends on ARCH_SOPHGO || COMPILE_TEST
> > + help
> > + Generic PWM framework driver for the Sophgo CV1800 series
> > + SoCs.
> > +
> > + To compile this driver as a module, build the dependecies
> > + as modules, this will be called pwm-cv1800.
> > +
> > config PWM_DWC_CORE
> > tristate
> > depends on HAS_IOMEM
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index c5ec9e168ee7..6c3c4a07a316 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CLK) += pwm-clk.o
> > obj-$(CONFIG_PWM_CLPS711X) += pwm-clps711x.o
> > obj-$(CONFIG_PWM_CRC) += pwm-crc.o
> > obj-$(CONFIG_PWM_CROS_EC) += pwm-cros-ec.o
> > +obj-$(CONFIG_PWM_CV1800) += pwm-cv1800.o
> > obj-$(CONFIG_PWM_DWC_CORE) += pwm-dwc-core.o
> > obj-$(CONFIG_PWM_DWC) += pwm-dwc.o
> > obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
> > diff --git a/drivers/pwm/pwm-cv1800.c b/drivers/pwm/pwm-cv1800.c
> > new file mode 100644
> > index 000000000000..4d4f233c9087
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-cv1800.c
> > @@ -0,0 +1,218 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * rtc-cv1800.c: PWM driver for Sophgo cv1800 RTC
> > + *
> > + * Author: Jingbao Qiu <[email protected]>
>
> Please document the behaviour of the PWM here at the top of the driver.
> Things to mention are:
>
> - How does the hardware behave on disable? (Usual behaviours include:
> output of inactive state; freeze where it just happens to be; high-z)
>
> - If you reconfigure the hardware, does it complete the previously
> running period before emitting the new wave form?
>
> - Are there possible glitches in .apply()? (i.e. can it happen, that
> for a short moment a wave form is emitted that has the new period but
> the old duty_cycle?)
>
Thanks, I will do that.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +
> > +#define HLPERIOD_BASE 0x00
> > +#define PERIOD_BASE 0x04
> > +#define POLARITY 0x040
> > +#define PWMSTART 0x044
> > +#define PWMDONE 0x048
> > +#define PWMUPDATE 0x4c
> > +#define PWM_OE 0xd0
> > +#define HLPERIOD_SHIFT 0x08
> > +#define PERIOD_SHIFT 0x08
> > +
> > +#define HLPERIOD(n) (HLPERIOD_BASE + ((n) * HLPERIOD_SHIFT))
> > +#define PERIOD(n) (PERIOD_BASE + ((n) * PERIOD_SHIFT))
> > +#define UPDATE(n) (BIT(0) << (n))
> > +#define OE_MASK(n) (BIT(0) << (n))
> > +#define START_MASK(n) (BIT(0) << (n))
> > +
> > +#define PERIOD_RESET 0x02
> > +#define HLPERIOD_RESET 0x1
> > +#define REG_DISABLE 0x0U
> > +#define REG_ENABLE BIT(0)
>
> Please use a driver specific prefix for all these defines.
>
I will do that.
> > +struct soc_info {
> > + unsigned int num_pwms;
> > +};
> > +
> > +struct cv1800_pwm {
> > + struct pwm_chip chip;
> > + struct regmap *map;
> > + struct clk *clk;
> > +};
> > +
> > +static inline struct cv1800_pwm *to_cv1800_pwm_dev(struct pwm_chip *chip)
> > +{
> > + return container_of(chip, struct cv1800_pwm, chip);
> > +}
> > +
> > +static int cv1800_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> > + u32 enable)
>
> This is called with the enable parameter set to state->enabled which is
> a bool. Please use a bool here, too, instead of a u32.
>
Thanks, I will use bool instead of a u32.
> > +{
> > + struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> > + u32 pwm_enable;
> > +
> > + regmap_read(priv->map, PWMSTART, &pwm_enable);
> > + pwm_enable >>= pwm->hwpwm;
>
> I don't understand why this value is shifted here. I guess this misses a
> mask?
The nth bit of this register represents the running state, so a shift
representation is required.
I will use macro definitions to represent masks.
>
> > + if (enable)
> > + clk_prepare_enable(priv->clk);
> > + else
> > + clk_disable_unprepare(priv->clk);
>
> This is broken. It might well happen that the first call to .apply() has
> state->enabled = false. Please make sure to properly balance clk
> enabling.
>
I will fix it.
> > + /*
> > + * If the parameters are changed during runtime, Register needs
> > + * to be updated to take effect.
> > + */
> > + if (pwm_enable) {
> > + regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
> > + REG_ENABLE << pwm->hwpwm);
> > + regmap_update_bits(priv->map, PWMUPDATE, UPDATE(pwm->hwpwm),
> > + REG_DISABLE << pwm->hwpwm);
>
> REG_DISABLE is zero. Why is this shifted? Very strange.
The data manual states that for dynamic updates, for the nth bit of
this register
write one first and then write 0.
you're right, just 0 or BIT(n) is ok.
>
> > + } else {
> > + regmap_update_bits(priv->map, PWM_OE, OE_MASK(pwm->hwpwm),
> > + enable << pwm->hwpwm);
> > + regmap_update_bits(priv->map, PWMSTART, START_MASK(pwm->hwpwm),
> > + enable << pwm->hwpwm);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int cv1800_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> > + u64 period_ns, duty_ns;
> > + u32 period_val, hlperiod_val;
> > + unsigned long long rate, div;
> > +
> > + period_ns = state->period;
> > + duty_ns = state->duty_cycle;
> > +
> > + rate = (unsigned long long)clk_get_rate(priv->clk);
>
> This cast has no effect, so please drop it. To prevent that the clock
> changes rate while the PWM is running, please use
> clk_rate_exclusive_get().
I will fix it.
>
> > + div = rate * period_ns;
>
> This might overflow. Please use mul_u64_u64_div_u64() or one of its
> variants and error out on rate > NSEC_PER_SEC. (There are other pwm
> drivers that make it right, took a look into these if this is unclear.)
>
Thanks, I will do that.
> > + do_div(div, NSEC_PER_SEC);
> > + period_val = div;
> > +
> > + div = rate * (period_ns - duty_ns);
> > + do_div(div, NSEC_PER_SEC);
> > + hlperiod_val = div;
>
> I think it can happen here, that div it too big to fit into this u32.
you're right, I will fix it.
>
> > + regmap_write(priv->map, PERIOD(pwm->hwpwm), period_val);
> > + regmap_write(priv->map, HLPERIOD(pwm->hwpwm), hlperiod_val);
> > +
> > + cv1800_pwm_enable(chip, pwm, state->enabled);
> > +
> > + return 0;
> > +}
> > +
> > +static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
> > + u32 period_val, hlperiod_val, tem;
> > + u64 rate;
> > + u64 period_ns = 0;
> > + u64 duty_ns = 0;
> > + u32 enable = 0;
> > +
> > + regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
> > + regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
> > +
> > + if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
> > + rate = (u64)clk_get_rate(priv->clk);
> > +
> > + tem = NSEC_PER_SEC * period_val;
>
> This might overflow.
I will fix it.
>
> > + do_div(tem, rate);
> > + period_ns = tem;
>
> This uses wrong rounding. If you enabled PWM_DEBUG during your tests,
> this should be catched and logged.
I will fix it.
>
> > +
> > + tem = period_val * period_ns;
> > + do_div(tem, hlperiod_val);
> > + duty_ns = tem;
> > +
> > + regmap_read(priv->map, PWMSTART, &enable);
> > + enable >>= pwm->hwpwm;
>
> Again a missing mask??
Yes, I will use a mask.
>
> > + }
> > +
> > + state->period = period_ns;
> > + state->duty_cycle = duty_ns;
> > + state->enabled = enable;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct pwm_ops cv1800_pwm_ops = {
> > + .apply = cv1800_pwm_apply,
> > + .get_state = cv1800_pwm_get_state,
> > +};
> > +
> > +static const struct regmap_config cv1800_pwm_regmap_config = {
> > + .reg_bits = 32,
> > + .val_bits = 32,
> > + .reg_stride = 4,
> > +};
> > +
> > +static int cv1800_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct cv1800_pwm *cv_pwm;
>
> Please pick always the same name for your driver private variable. In
> the other functions it is called "priv".
I will fix it.
>
> > + void __iomem *base;
> > + const struct soc_info *info;
> > +
> > + info = device_get_match_data(dev);
> > + if (!info)
>
> error message please.
I will fix it.
>
> > + return -EINVAL;
> > +
> > + cv_pwm = devm_kzalloc(dev, sizeof(*cv_pwm), GFP_KERNEL);
> > + if (!cv_pwm)
> > + return -ENOMEM;
> > +
> > + base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> > + cv_pwm->map = devm_regmap_init_mmio(&pdev->dev, base,
> > + &cv1800_pwm_regmap_config);
> > + if (IS_ERR(cv_pwm->map))
> > + return PTR_ERR(cv_pwm->map);
> > +
> > + cv_pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(cv_pwm->clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(cv_pwm->clk),
> > + "clk not found\n");
> > +
> > + cv_pwm->chip.dev = dev;
> > + cv_pwm->chip.ops = &cv1800_pwm_ops;
> > + cv_pwm->chip.npwm = info->num_pwms;
>
> Missing clk handling here. Please all clk_prepare_enable once for each
> running channel.
I will fix it.
>
> > + return devm_pwmchip_add(dev, &cv_pwm->chip);
> > +}
> > +
> > +static const struct soc_info cv1800b_soc_info = {
> > + .num_pwms = 4,
> > +};
>
> Until you support more variants, I suggest to just hardcode npwm to
> four.
I will do that.
>
> > +static const struct of_device_id cv1800_pwm_dt_ids[] = {
> > + { .compatible = "sophgo,cv1800-pwm", .data = &cv1800b_soc_info },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, cv1800_pwm_dt_ids);
> > +
> > +static struct platform_driver cv1800_pwm_driver = {
> > + .driver = {
> > + .name = "cv1800-pwm",
> > + .of_match_table = cv1800_pwm_dt_ids,
> > + },
> > + .probe = cv1800_pwm_probe,
> > +};
> > +module_platform_driver(cv1800_pwm_driver);
> > +
> > +MODULE_ALIAS("platform:cv1800-pwm");
>
> I'm with Krzysztof here, this shouldn't be needed.
I will drop it.
>
> > +MODULE_AUTHOR("Jingbao Qiu");
> > +MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver");
> > +MODULE_LICENSE("GPL");
>
Best regards
Jingbao Qiu
Hi Jingbao,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 7afc0e7f681e6efd6b826f003fc14c17b5093643]
url: https://github.com/intel-lab-lkp/linux/commits/Jingbao-Qiu/dt-bindings-pwm-sophgo-add-pwm-for-Sophgo-CV1800-series-SoC/20240207-141135
base: 7afc0e7f681e6efd6b826f003fc14c17b5093643
patch link: https://lore.kernel.org/r/20240207060913.672554-1-qiujingbao.dlmu%40gmail.com
patch subject: [PATCH v1 2/2] pwm: sophgo: add pwm support for Sophgo CV1800 SoC
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20240208/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 7dd790db8b77c4a833c06632e903dc4f13877a64)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240208/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from drivers/pwm/pwm-cv1800.c:14:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:337:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/pwm/pwm-cv1800.c:14:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:337:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/pwm/pwm-cv1800.c:14:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:337:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> drivers/pwm/pwm-cv1800.c:131:3: warning: comparison of distinct pointer types ('typeof ((tem)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
131 | do_div(tem, rate);
| ^~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
drivers/pwm/pwm-cv1800.c:131:3: error: incompatible pointer types passing 'u32 *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
131 | do_div(tem, rate);
| ^~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ^
drivers/pwm/pwm-cv1800.c:135:3: warning: comparison of distinct pointer types ('typeof ((tem)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
135 | do_div(tem, hlperiod_val);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
drivers/pwm/pwm-cv1800.c:135:3: error: incompatible pointer types passing 'u32 *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
135 | do_div(tem, hlperiod_val);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ^
>> drivers/pwm/pwm-cv1800.c:131:3: warning: shift count >= width of type [-Wshift-count-overflow]
131 | do_div(tem, rate);
| ^~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^ ~~
include/linux/compiler.h:76:40: note: expanded from macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/pwm/pwm-cv1800.c:135:3: warning: shift count >= width of type [-Wshift-count-overflow]
135 | do_div(tem, hlperiod_val);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^ ~~
include/linux/compiler.h:76:40: note: expanded from macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
10 warnings and 2 errors generated.
vim +131 drivers/pwm/pwm-cv1800.c
113
114 static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
115 struct pwm_state *state)
116 {
117 struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
118 u32 period_val, hlperiod_val, tem;
119 u64 rate;
120 u64 period_ns = 0;
121 u64 duty_ns = 0;
122 u32 enable = 0;
123
124 regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
125 regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
126
127 if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
128 rate = (u64)clk_get_rate(priv->clk);
129
130 tem = NSEC_PER_SEC * period_val;
> 131 do_div(tem, rate);
132 period_ns = tem;
133
134 tem = period_val * period_ns;
135 do_div(tem, hlperiod_val);
136 duty_ns = tem;
137
138 regmap_read(priv->map, PWMSTART, &enable);
139 enable >>= pwm->hwpwm;
140 }
141
142 state->period = period_ns;
143 state->duty_cycle = duty_ns;
144 state->enabled = enable;
145
146 return 0;
147 }
148
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Jingbao,
kernel test robot noticed the following build errors:
[auto build test ERROR on 7afc0e7f681e6efd6b826f003fc14c17b5093643]
url: https://github.com/intel-lab-lkp/linux/commits/Jingbao-Qiu/dt-bindings-pwm-sophgo-add-pwm-for-Sophgo-CV1800-series-SoC/20240207-141135
base: 7afc0e7f681e6efd6b826f003fc14c17b5093643
patch link: https://lore.kernel.org/r/20240207060913.672554-1-qiujingbao.dlmu%40gmail.com
patch subject: [PATCH v1 2/2] pwm: sophgo: add pwm support for Sophgo CV1800 SoC
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240208/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 7dd790db8b77c4a833c06632e903dc4f13877a64)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240208/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from drivers/pwm/pwm-cv1800.c:14:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:337:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/pwm/pwm-cv1800.c:14:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:337:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/pwm/pwm-cv1800.c:14:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:337:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
drivers/pwm/pwm-cv1800.c:131:3: warning: comparison of distinct pointer types ('typeof ((tem)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
131 | do_div(tem, rate);
| ^~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
>> drivers/pwm/pwm-cv1800.c:131:3: error: incompatible pointer types passing 'u32 *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
131 | do_div(tem, rate);
| ^~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ^
drivers/pwm/pwm-cv1800.c:135:3: warning: comparison of distinct pointer types ('typeof ((tem)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
135 | do_div(tem, hlperiod_val);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
drivers/pwm/pwm-cv1800.c:135:3: error: incompatible pointer types passing 'u32 *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
135 | do_div(tem, hlperiod_val);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ^
drivers/pwm/pwm-cv1800.c:131:3: warning: shift count >= width of type [-Wshift-count-overflow]
131 | do_div(tem, rate);
| ^~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^ ~~
include/linux/compiler.h:76:40: note: expanded from macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/pwm/pwm-cv1800.c:135:3: warning: shift count >= width of type [-Wshift-count-overflow]
135 | do_div(tem, hlperiod_val);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^ ~~
include/linux/compiler.h:76:40: note: expanded from macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
10 warnings and 2 errors generated.
vim +131 drivers/pwm/pwm-cv1800.c
113
114 static int cv1800_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
115 struct pwm_state *state)
116 {
117 struct cv1800_pwm *priv = to_cv1800_pwm_dev(chip);
118 u32 period_val, hlperiod_val, tem;
119 u64 rate;
120 u64 period_ns = 0;
121 u64 duty_ns = 0;
122 u32 enable = 0;
123
124 regmap_read(priv->map, PERIOD(pwm->hwpwm), &period_val);
125 regmap_read(priv->map, HLPERIOD(pwm->hwpwm), &hlperiod_val);
126
127 if (period_val != PERIOD_RESET || hlperiod_val != HLPERIOD_RESET) {
128 rate = (u64)clk_get_rate(priv->clk);
129
130 tem = NSEC_PER_SEC * period_val;
> 131 do_div(tem, rate);
132 period_ns = tem;
133
134 tem = period_val * period_ns;
135 do_div(tem, hlperiod_val);
136 duty_ns = tem;
137
138 regmap_read(priv->map, PWMSTART, &enable);
139 enable >>= pwm->hwpwm;
140 }
141
142 state->period = period_ns;
143 state->duty_cycle = duty_ns;
144 state->enabled = enable;
145
146 return 0;
147 }
148
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki