Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580588 > unrolled thread

Re: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC

Started byMarcin Nowakowski <marcin.nowakowski@imgtec.com>
First post2017-02-14 16:00 +0100
Last post2017-02-16 10:30 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC Marcin Nowakowski <marcin.nowakowski@imgtec.com> - 2017-02-14 16:00 +0100
    Re: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC Yang Ling <gnaygnil@gmail.com> - 2017-02-15 14:10 +0100
      Re: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC Marcin Nowakowski <marcin.nowakowski@imgtec.com> - 2017-02-16 10:30 +0100

#1580588 — Re: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC

FromMarcin Nowakowski <marcin.nowakowski@imgtec.com>
Date2017-02-14 16:00 +0100
SubjectRe: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC
Message-ID<taMKe-38T-13@gated-at.bofh.it>
Hi Yang,


On 13.02.2017 16:28, Yang Ling wrote:
> Add support for the PWM controller present in Loongson1 family of SoCs.
>
> Signed-off-by: Yang Ling <gnaygnil@gmail.com>
> ---
>  drivers/pwm/Kconfig         |   9 +++
>  drivers/pwm/Makefile        |   1 +
>  drivers/pwm/pwm-loongson1.c | 169 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 179 insertions(+)
>  create mode 100644 drivers/pwm/pwm-loongson1.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index f92dd41..985f2fe 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -216,6 +216,15 @@ config PWM_JZ4740
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-jz4740.
>
> +config PWM_LOONGSON1
> +	tristate "Loongson1 PWM support"
> +	depends on MACH_LOONGSON32
> +	help
> +	  Generic PWM framework driver for Loongson1 based machines.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-loongson1.
> +
>  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 a48bdb5..1979453 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
>  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
>  obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> +obj-$(CONFIG_PWM_LOONGSON1)	+= pwm-loongson1.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-loongson1.c b/drivers/pwm/pwm-loongson1.c
> new file mode 100644
> index 0000000..72e3fe3
> --- /dev/null
> +++ b/drivers/pwm/pwm-loongson1.c
> @@ -0,0 +1,169 @@
> +/*
> + * Copyright (c) 2017 Yang Ling <gnaygnil@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <loongson1.h>
> +
> +struct ls1x_pwm_chip {
> +	struct clk *clk;
> +	void __iomem *base;
> +	struct pwm_chip chip;
> +};
> +
> +struct ls1x_pwm_channel {
> +	u32 period_ns;
> +	u32 duty_ns;
> +};
> +static inline struct ls1x_pwm_chip *to_ls1x_pwm_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct ls1x_pwm_chip, chip);
> +}
> +
> +static int ls1x_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ls1x_pwm_channel *chan = NULL;
> +
> +	chan = devm_kzalloc(chip->dev, sizeof(*chan), GFP_KERNEL);
> +	if (!chan)
> +		return -ENOMEM;
> +
> +	pwm_set_chip_data(pwm, chan);
> +
> +	return 0;
> +}
> +
> +static void ls1x_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	devm_kfree(chip->dev, pwm_get_chip_data(pwm));
> +	pwm_set_chip_data(pwm, NULL);
> +}
> +

Period and duty are stored in the pwm_device already, so you're just 
duplicating the same data here. If you remove ls1x_pwm_channel then all 
of the code above becomes unnecessary ...

> +static int ls1x_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			int duty_ns, int period_ns)
> +{
> +	struct ls1x_pwm_chip *pc = to_ls1x_pwm_chip(chip);
> +	struct ls1x_pwm_channel *chan = pwm_get_chip_data(pwm);
> +	unsigned long long tmp;
> +	unsigned long period, duty;
> +
> +	if (period_ns == chan->period_ns && duty_ns == chan->duty_ns)
> +		return 0;
> +

above check is already handled in pwm core

> +	tmp = (unsigned long long)clk_get_rate(pc->clk) * period_ns;
> +	do_div(tmp, 1000000000);
> +	period = tmp;
> +
> +	tmp = (unsigned long long)period * duty_ns;
> +	do_div(tmp, period_ns);
> +	duty = period - tmp;
> +
> +	if (duty >= period)
> +		duty = period - 1;
> +
> +	if (duty >> 24 || period >> 24)
> +		return -EINVAL;
> +
> +	chan->period_ns = period_ns;
> +	chan->duty_ns = duty_ns;
> +
> +	writel(duty, pc->base + PWM_HRC(pwm->hwpwm));
> +	writel(period, pc->base + PWM_LRC(pwm->hwpwm));
> +	writel(0x00, pc->base + PWM_CNT(pwm->hwpwm));
> +

PWM_HRC and PWM_LRC names suggest that you're using high/low state 
counters here rather than duty/period - but with no documentation I'm 
just guessing here.

> +	return 0;
> +}
> +
> +static int ls1x_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ls1x_pwm_chip *pc = to_ls1x_pwm_chip(chip);
> +
> +	writel(CNT_RST, pc->base + PWM_CTRL(pwm->hwpwm));
> +	writel(CNT_EN, pc->base + PWM_CTRL(pwm->hwpwm));
> +
> +	return 0;
> +}
> +
> +static void ls1x_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ls1x_pwm_chip *pc = to_ls1x_pwm_chip(chip);
> +
> +	writel(PWM_OE, pc->base + PWM_CTRL(pwm->hwpwm));
> +}
> +
> +static const struct pwm_ops ls1x_pwm_ops = {
> +	.request = ls1x_pwm_request,
> +	.free = ls1x_pwm_free,
> +	.config = ls1x_pwm_config,
> +	.enable = ls1x_pwm_enable,
> +	.disable = ls1x_pwm_disable,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int ls1x_pwm_probe(struct platform_device *pdev)
> +{
> +	struct ls1x_pwm_chip *pc = NULL;
> +	struct resource *res = NULL;
> +
> +	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> +	if (!pc)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pc->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pc->base))
> +		return PTR_ERR(pc->base);
> +
> +	pc->clk = devm_clk_get(&pdev->dev, "ls1x-pwmtimer");
> +	if (IS_ERR(pc->clk)) {
> +		dev_err(&pdev->dev, "failed to get %s clock\n", pdev->name);
> +		return PTR_ERR(pc->clk);
> +	}
> +	clk_prepare_enable(pc->clk);

Should check for errors here

> +	pc->chip.ops = &ls1x_pwm_ops;
> +	pc->chip.dev = &pdev->dev;
> +	pc->chip.base = -1;
> +	pc->chip.npwm = 4;
> +
> +	platform_set_drvdata(pdev, pc);
> +
> +	return pwmchip_add(&pc->chip);

Should check for errors (and disable clk appropriately)

> +}
> +
> +static int ls1x_pwm_remove(struct platform_device *pdev)
> +{
> +	struct ls1x_pwm_chip *pc = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&pc->chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	clk_disable_unprepare(pc->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver ls1x_pwm_driver = {
> +	.driver = {
> +		.name = "ls1x-pwm",
> +	},
> +	.probe = ls1x_pwm_probe,
> +	.remove = ls1x_pwm_remove,
> +};
> +module_platform_driver(ls1x_pwm_driver);
> +
> +MODULE_AUTHOR("Yang Ling <gnaygnil@gmail.com>");
> +MODULE_DESCRIPTION("Loongson1 PWM driver");
> +MODULE_ALIAS("platform:loongson1-pwm");
> +MODULE_LICENSE("GPL");
>


Best regards,
Marcin

[toc] | [next] | [standalone]

#1581299

FromYang Ling <gnaygnil@gmail.com>
Date2017-02-15 14:10 +0100
Message-ID<tb7vk-nR-23@gated-at.bofh.it>
In reply to#1580588
Hi Marcin,

On Tue, Feb 14, 2017 at 03:54:44PM +0100, Marcin Nowakowski wrote:
> Hi Yang,
> 
> 
> On 13.02.2017 16:28, Yang Ling wrote:
> >Add support for the PWM controller present in Loongson1 family of SoCs.
> >
> >Signed-off-by: Yang Ling <gnaygnil@gmail.com>
> >---
> > drivers/pwm/Kconfig         |   9 +++
> > drivers/pwm/Makefile        |   1 +
> > drivers/pwm/pwm-loongson1.c | 169 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 179 insertions(+)
> > create mode 100644 drivers/pwm/pwm-loongson1.c
> >
> >diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >index f92dd41..985f2fe 100644
> >--- a/drivers/pwm/Kconfig
> >+++ b/drivers/pwm/Kconfig
> >@@ -216,6 +216,15 @@ config PWM_JZ4740
> > 	  To compile this driver as a module, choose M here: the module
> > 	  will be called pwm-jz4740.
> >
> >+config PWM_LOONGSON1
> >+	tristate "Loongson1 PWM support"
> >+	depends on MACH_LOONGSON32
> >+	help
> >+	  Generic PWM framework driver for Loongson1 based machines.
> >+
> >+	  To compile this driver as a module, choose M here: the module
> >+	  will be called pwm-loongson1.
> >+
> > 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 a48bdb5..1979453 100644
> >--- a/drivers/pwm/Makefile
> >+++ b/drivers/pwm/Makefile
> >@@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
> > obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
> > obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
> > obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> >+obj-$(CONFIG_PWM_LOONGSON1)	+= pwm-loongson1.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-loongson1.c b/drivers/pwm/pwm-loongson1.c
> >new file mode 100644
> >index 0000000..72e3fe3
> >--- /dev/null
> >+++ b/drivers/pwm/pwm-loongson1.c
> >@@ -0,0 +1,169 @@
> >+/*
> >+ * Copyright (c) 2017 Yang Ling <gnaygnil@gmail.com>
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify it
> >+ * under the terms of the GNU General Public License as published by the
> >+ * Free Software Foundation; either version 2 of the License, or (at your
> >+ * option) any later version.
> >+ */
> >+
> >+#include <linux/clk.h>
> >+#include <linux/module.h>
> >+#include <linux/platform_device.h>
> >+#include <linux/pwm.h>
> >+#include <loongson1.h>
> >+
> >+struct ls1x_pwm_chip {
> >+	struct clk *clk;
> >+	void __iomem *base;
> >+	struct pwm_chip chip;
> >+};
> >+
> >+struct ls1x_pwm_channel {
> >+	u32 period_ns;
> >+	u32 duty_ns;
> >+};
> >+static inline struct ls1x_pwm_chip *to_ls1x_pwm_chip(struct pwm_chip *chip)
> >+{
> >+	return container_of(chip, struct ls1x_pwm_chip, chip);
> >+}
> >+
> >+static int ls1x_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> >+{
> >+	struct ls1x_pwm_channel *chan = NULL;
> >+
> >+	chan = devm_kzalloc(chip->dev, sizeof(*chan), GFP_KERNEL);
> >+	if (!chan)
> >+		return -ENOMEM;
> >+
> >+	pwm_set_chip_data(pwm, chan);
> >+
> >+	return 0;
> >+}
> >+
> >+static void ls1x_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >+{
> >+	devm_kfree(chip->dev, pwm_get_chip_data(pwm));
> >+	pwm_set_chip_data(pwm, NULL);
> >+}
> >+
> 
> Period and duty are stored in the pwm_device already, so you're just
> duplicating the same data here. If you remove ls1x_pwm_channel then
> all of the code above becomes unnecessary ...

Remove ls1x_pwm_channel.

> 
> >+static int ls1x_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >+			int duty_ns, int period_ns)
> >+{
> >+	struct ls1x_pwm_chip *pc = to_ls1x_pwm_chip(chip);
> >+	struct ls1x_pwm_channel *chan = pwm_get_chip_data(pwm);
> >+	unsigned long long tmp;
> >+	unsigned long period, duty;
> >+
> >+	if (period_ns == chan->period_ns && duty_ns == chan->duty_ns)
> >+		return 0;
> >+
> 
> above check is already handled in pwm core

Remove it.

> 
> >+	tmp = (unsigned long long)clk_get_rate(pc->clk) * period_ns;
> >+	do_div(tmp, 1000000000);
> >+	period = tmp;
> >+
> >+	tmp = (unsigned long long)period * duty_ns;
> >+	do_div(tmp, period_ns);
> >+	duty = period - tmp;
> >+
> >+	if (duty >= period)
> >+		duty = period - 1;
> >+
> >+	if (duty >> 24 || period >> 24)
> >+		return -EINVAL;
> >+
> >+	chan->period_ns = period_ns;
> >+	chan->duty_ns = duty_ns;
> >+
> >+	writel(duty, pc->base + PWM_HRC(pwm->hwpwm));
> >+	writel(period, pc->base + PWM_LRC(pwm->hwpwm));
> >+	writel(0x00, pc->base + PWM_CNT(pwm->hwpwm));
> >+
> 
> PWM_HRC and PWM_LRC names suggest that you're using high/low state
> counters here rather than duty/period - but with no documentation
> I'm just guessing here.

Indeed, the high/low state counters is used here.
Change the name to duty_cnt/period_cnt.

> 
> >+	return 0;
> >+}
> >+
> >+static int ls1x_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >+{
> >+	struct ls1x_pwm_chip *pc = to_ls1x_pwm_chip(chip);
> >+
> >+	writel(CNT_RST, pc->base + PWM_CTRL(pwm->hwpwm));
> >+	writel(CNT_EN, pc->base + PWM_CTRL(pwm->hwpwm));
> >+
> >+	return 0;
> >+}
> >+
> >+static void ls1x_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >+{
> >+	struct ls1x_pwm_chip *pc = to_ls1x_pwm_chip(chip);
> >+
> >+	writel(PWM_OE, pc->base + PWM_CTRL(pwm->hwpwm));
> >+}
> >+
> >+static const struct pwm_ops ls1x_pwm_ops = {
> >+	.request = ls1x_pwm_request,
> >+	.free = ls1x_pwm_free,
> >+	.config = ls1x_pwm_config,
> >+	.enable = ls1x_pwm_enable,
> >+	.disable = ls1x_pwm_disable,
> >+	.owner = THIS_MODULE,
> >+};
> >+
> >+static int ls1x_pwm_probe(struct platform_device *pdev)
> >+{
> >+	struct ls1x_pwm_chip *pc = NULL;
> >+	struct resource *res = NULL;
> >+
> >+	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> >+	if (!pc)
> >+		return -ENOMEM;
> >+
> >+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+	pc->base = devm_ioremap_resource(&pdev->dev, res);
> >+	if (IS_ERR(pc->base))
> >+		return PTR_ERR(pc->base);
> >+
> >+	pc->clk = devm_clk_get(&pdev->dev, "ls1x-pwmtimer");
> >+	if (IS_ERR(pc->clk)) {
> >+		dev_err(&pdev->dev, "failed to get %s clock\n", pdev->name);
> >+		return PTR_ERR(pc->clk);
> >+	}
> >+	clk_prepare_enable(pc->clk);
> 
> Should check for errors here
> 
> >+	pc->chip.ops = &ls1x_pwm_ops;
> >+	pc->chip.dev = &pdev->dev;
> >+	pc->chip.base = -1;
> >+	pc->chip.npwm = 4;
> >+
> >+	platform_set_drvdata(pdev, pc);
> >+
> >+	return pwmchip_add(&pc->chip);
> 
> Should check for errors (and disable clk appropriately)

These will be modified.

> 
> >+}
> >+
> >+static int ls1x_pwm_remove(struct platform_device *pdev)
> >+{
> >+	struct ls1x_pwm_chip *pc = platform_get_drvdata(pdev);
> >+	int ret;
> >+
> >+	ret = pwmchip_remove(&pc->chip);
> >+	if (ret < 0)
> >+		return ret;
> >+
> >+	clk_disable_unprepare(pc->clk);
> >+
> >+	return 0;
> >+}
> >+
> >+static struct platform_driver ls1x_pwm_driver = {
> >+	.driver = {
> >+		.name = "ls1x-pwm",
> >+	},
> >+	.probe = ls1x_pwm_probe,
> >+	.remove = ls1x_pwm_remove,
> >+};
> >+module_platform_driver(ls1x_pwm_driver);
> >+
> >+MODULE_AUTHOR("Yang Ling <gnaygnil@gmail.com>");
> >+MODULE_DESCRIPTION("Loongson1 PWM driver");
> >+MODULE_ALIAS("platform:loongson1-pwm");
> >+MODULE_LICENSE("GPL");
> >
> 
> 
> Best regards,
> Marcin

Thanks for your friendly reminder.

Yang

[toc] | [prev] | [next] | [standalone]

#1582408

FromMarcin Nowakowski <marcin.nowakowski@imgtec.com>
Date2017-02-16 10:30 +0100
Message-ID<tbqxX-4dp-5@gated-at.bofh.it>
In reply to#1581299
Hi Yang,

On 15.02.2017 14:09, Yang Ling wrote:

>>> +	tmp = (unsigned long long)clk_get_rate(pc->clk) * period_ns;
>>> +	do_div(tmp, 1000000000);

NSEC_PER_SEC ?

>>> +	period = tmp;
>>> +
>>> +	tmp = (unsigned long long)period * duty_ns;
>>> +	do_div(tmp, period_ns);
>>> +	duty = period - tmp;
>>> +
>>> +	if (duty >= period)
>>> +		duty = period - 1;
>>> +
>>> +	if (duty >> 24 || period >> 24)
>>> +		return -EINVAL;
>>> +
>>> +	chan->period_ns = period_ns;
>>> +	chan->duty_ns = duty_ns;
>>> +
>>> +	writel(duty, pc->base + PWM_HRC(pwm->hwpwm));
>>> +	writel(period, pc->base + PWM_LRC(pwm->hwpwm));
>>> +	writel(0x00, pc->base + PWM_CNT(pwm->hwpwm));
>>> +
>>
>> PWM_HRC and PWM_LRC names suggest that you're using high/low state
>> counters here rather than duty/period - but with no documentation
>> I'm just guessing here.
>
> Indeed, the high/low state counters is used here.
> Change the name to duty_cnt/period_cnt.
>
>

What I was referring to here is that if you have a high/low value 
counters that you enter then these are not the same as duty/period, in 
simple terms:
high_cnt = duty_cnt
low_cnt = period_cnt - duty_cnt

so please double check that this is what you want to be doing? As the 
names used suggest that this code may be wrong. Or maybe what you're 
doing is correct but the register access macros have misleading names?

Marcin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web