Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Lee Jones <lee.jones@linaro.org>
To: Michael Walle <michael@walle.cc>
Cc: linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
	linux-pwm@vger.kernel.org, linux-watchdog@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Jean Delvare" <jdelvare@suse.com>,
	"Guenter Roeck" <linux@roeck-us.net>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Wim Van Sebroeck" <wim@linux-watchdog.org>,
	"Shawn Guo" <shawnguo@kernel.org>, "Li Yang" <leoyang.li@nxp.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Jason Cooper" <jason@lakedaemon.net>,
	"Marc Zyngier" <maz@kernel.org>,
	"Mark Brown" <broonie@kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH v4 05/11] pwm: add support for sl28cpld PWM controller
Date: Fri, 5 Jun 2020 09:49:15 +0100
Message-ID: <20200605084915.GE3714@dell> (raw)
In-Reply-To: <20200604211039.12689-6-michael@walle.cc>

On Thu, 04 Jun 2020, Michael Walle wrote:

> Add support for the PWM controller of the sl28cpld board management
> controller. This is part of a multi-function device driver.
> 
> The controller has one PWM channel and can just generate four distinct
> frequencies.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/pwm/Kconfig        |  10 ++
>  drivers/pwm/Makefile       |   1 +
>  drivers/pwm/pwm-sl28cpld.c | 201 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 212 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sl28cpld.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index cb8d739067d2..a39371c11ff6 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -437,6 +437,16 @@ config PWM_SIFIVE
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-sifive.
>  
> +config PWM_SL28CPLD
> +	tristate "Kontron sl28 PWM support"
> +	depends on MFD_SL28CPLD
> +	help
> +	  Generic PWM framework driver for board management controller
> +	  found on the Kontron sl28 CPLD.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-sl28cpld.
> +
>  config PWM_SPEAR
>  	tristate "STMicroelectronics SPEAr PWM support"
>  	depends on PLAT_SPEAR || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index a59c710e98c7..c479623724e8 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
>  obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
> +obj-$(CONFIG_PWM_SL28CPLD)	+= pwm-sl28cpld.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
>  obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> new file mode 100644
> index 000000000000..d82303f509f5
> --- /dev/null
> +++ b/drivers/pwm/pwm-sl28cpld.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sl28cpld PWM driver.
> + *
> + * Copyright 2019 Kontron Europe GmbH

This is out of date.

> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * PWM timer block registers.
> + */
> +#define PWM_CTRL		0x00
> +#define   PWM_ENABLE		BIT(7)
> +#define   PWM_MODE_250HZ	0
> +#define   PWM_MODE_500HZ	1
> +#define   PWM_MODE_1KHZ		2
> +#define   PWM_MODE_2KHZ		3
> +#define   PWM_MODE_MASK		GENMASK(1, 0)
> +#define PWM_CYCLE		0x01
> +#define   PWM_CYCLE_MAX		0x7f
> +
> +struct sl28cpld_pwm {
> +	struct pwm_chip pwm_chip;
> +	struct regmap *regmap;
> +	u32 offset;
> +};
> +
> +struct sl28cpld_pwm_periods {
> +	u8 ctrl;
> +	unsigned long duty_cycle;
> +};
> +
> +struct sl28cpld_pwm_config {
> +	unsigned long period_ns;
> +	u8 max_duty_cycle;
> +};

Also, instead of hand rolling your own structure here, I think it
would be prudent to re-use something that already exists.  Seeing as
this will be used to describe possible state, perhaps 'struct
pwm_state' would be suitable - leaving polarity and enabled
unpopulated of course.

Ah wait (sorry, thinking allowed and on-the-fly here), what is
max_duty_cycle here?  I assume this does not have the same
meaning/value-type as the one in 'struct pwm_state'.  What does
max_duty_cycle represent in your use-case?

> +static struct sl28cpld_pwm_config sl28cpld_pwm_config[] = {
> +	[PWM_MODE_250HZ] = { .period_ns = 4000000, .max_duty_cycle = 0x80 },
> +	[PWM_MODE_500HZ] = { .period_ns = 2000000, .max_duty_cycle = 0x40 },
> +	[PWM_MODE_1KHZ] = { .period_ns = 1000000, .max_duty_cycle = 0x20 },
> +	[PWM_MODE_2KHZ] = { .period_ns =  500000, .max_duty_cycle = 0x10 },
> +};

Tiny nit: If you lined these up from the '{'s it would be easier to
see/compare the period_ns values at first glance, rather than having
to count the ' 's and '0's.

> +static inline struct sl28cpld_pwm *to_sl28cpld_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct sl28cpld_pwm, pwm_chip);
> +}

Why not save yourself the trouble and just:

  struct sl28cpld_pwm *pwm = dev_get_drvdata(chip->dev);

> +static void sl28cpld_pwm_get_state(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   struct pwm_state *state)
> +{
> +	struct sl28cpld_pwm *spc = to_sl28cpld_pwm(chip);
> +	static struct sl28cpld_pwm_config *config;
> +	unsigned int reg;
> +	unsigned long cycle;

Why is this 'long' here and 'long long' in *_apply()?

> +	unsigned int mode;
> +
> +	regmap_read(spc->regmap, spc->offset + PWM_CTRL, &reg);
> +
> +	state->enabled = reg & PWM_ENABLE;
> +
> +	mode = FIELD_GET(PWM_MODE_MASK, reg);
> +	config = &sl28cpld_pwm_config[mode];
> +	state->period = config->period_ns;
> +
> +	regmap_read(spc->regmap, spc->offset + PWM_CYCLE, &reg);
> +	cycle = reg * config->period_ns;
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(cycle,
> +						  config->max_duty_cycle);
> +}
> +
> +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      const struct pwm_state *state)
> +{
> +	struct sl28cpld_pwm *spc = to_sl28cpld_pwm(chip);
> +	struct sl28cpld_pwm_config *config;
> +	unsigned long long cycle;
> +	int ret;
> +	int mode;
> +	u8 ctrl;
> +
> +	/* update config, first search best matching period */

Please use correct grammar (less full stops) in comments.

> +	for (mode = 0; mode < ARRAY_SIZE(sl28cpld_pwm_config); mode++) {
> +		config = &sl28cpld_pwm_config[mode];
> +		if (state->period == config->period_ns)
> +			break;
> +	}
> +
> +	if (mode == ARRAY_SIZE(sl28cpld_pwm_config))
> +		return -EINVAL;
> +
> +	ctrl = FIELD_PREP(PWM_MODE_MASK, mode);
> +	if (state->enabled)
> +		ctrl |= PWM_ENABLE;
> +
> +	cycle = state->duty_cycle * config->max_duty_cycle;
> +	do_div(cycle, state->period);

Forgive my ignorance (I'm new here!), but what are these 2 lines
doing?  Here we are multiplying the current duty_cycle with the
maximum value, then dividing by the period.

So in the case of PWM_MODE_1KHZ with a 50% duty cycle, you'd have:

   (500000 * 0x20[16]) / 1000000 = [0x10]16

Thus, the above gives as a proportional representation of the maximum
valid value for placement into the cycle control register(s), right?

Either way (whether I'm correct or not), I think it would be nice to
mention this in a comment.  Maybe even clarify with a simple example.

> +	/*
> +	 * The hardware doesn't allow to set max_duty_cycle if the
> +	 * 250Hz mode is enabled. But since this is "all-high" output
> +	 * just use the 500Hz mode with the duty cycle to max value.
> +	 */
> +	if (cycle == config->max_duty_cycle) {
> +		ctrl &= ~PWM_MODE_MASK;
> +		ctrl |= FIELD_PREP(PWM_MODE_MASK, PWM_MODE_500HZ);
> +		cycle = PWM_CYCLE_MAX;
> +	}

This is being executed even when 250Hz mode is not enabled.

Is that by design?  If so, it doesn't match the comment.

> +	ret = regmap_write(spc->regmap, spc->offset + PWM_CTRL, ctrl);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(spc->regmap, spc->offset + PWM_CYCLE, (u8)cycle);
> +}
> +
> +static const struct pwm_ops sl28cpld_pwm_ops = {
> +	.apply = sl28cpld_pwm_apply,
> +	.get_state = sl28cpld_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int sl28cpld_pwm_probe(struct platform_device *pdev)
> +{
> +	struct sl28cpld_pwm *pwm;

This is super confusing.  Here you call it 'pwm', but when you bring
the data to the fore for consumption, you call it something different
('spc') for some reason.

Is there logic behind this?

> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	if (!pdev->dev.parent)
> +		return -ENODEV;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!pwm->regmap)
> +		return -ENODEV;
> +
> +	ret = device_property_read_u32(&pdev->dev, "reg", &pwm->offset);

Really?  Can you use the 'reg' property in this way?

Side question:
  Do any of your child address spaces actually overlap/intersect?

> +	if (ret)
> +		return -EINVAL;
> +
> +	/* initialize struct pwm_chip */

Proper grammar please.

> +	chip = &pwm->pwm_chip;
> +	chip->dev = &pdev->dev;
> +	chip->ops = &sl28cpld_pwm_ops;
> +	chip->base = -1;
> +	chip->npwm = 1;
> +
> +	ret = pwmchip_add(&pwm->pwm_chip);
> +	if (ret < 0)

Is '> 0' even valid?

Suggest "!ret" here, as you have done above.

> +		return ret;
> +
> +	platform_set_drvdata(pdev, pwm);
> +
> +	return 0;
> +}
> +
> +static int sl28cpld_pwm_remove(struct platform_device *pdev)
> +{
> +	struct sl28cpld_pwm *pwm = platform_get_drvdata(pdev);
> +
> +	return pwmchip_remove(&pwm->pwm_chip);
> +}
> +
> +static const struct of_device_id sl28cpld_pwm_of_match[] = {
> +	{ .compatible = "kontron,sl28cpld-pwm" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sl28cpld_pwm_of_match);
> +
> +static const struct platform_device_id sl28cpld_pwm_id_table[] = {
> +	{"sl28cpld-pwm"},

Spaces either side of the "'s please.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(platform, sl28cpld_pwm_id_table);

What are you using this for?

> +static struct platform_driver sl28cpld_pwm_driver = {
> +	.probe = sl28cpld_pwm_probe,
> +	.remove	= sl28cpld_pwm_remove,
> +	.id_table = sl28cpld_pwm_id_table,
> +	.driver = {
> +		.name = KBUILD_MODNAME,

Please just use the quoted name in full.

> +		.of_match_table = sl28cpld_pwm_of_match,
> +	},
> +};
> +module_platform_driver(sl28cpld_pwm_driver);
> +
> +MODULE_DESCRIPTION("sl28cpld PWM Driver");

"SL28CPLD" ?

> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
> +MODULE_LICENSE("GPL");

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

  parent reply index

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-04 21:10 [PATCH v4 00/11] Add support for Kontron sl28cpld Michael Walle
2020-06-04 21:10 ` [PATCH v4 01/11] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
2020-06-09 16:28   ` Rob Herring
2020-06-04 21:10 ` [PATCH v4 02/11] mfd: Add support for Kontron sl28cpld management controller Michael Walle
2020-06-05  6:57   ` Lee Jones
2020-06-05  9:51     ` Michael Walle
2020-06-05 10:50     ` Mark Brown
2020-06-05 20:07       ` Michael Walle
2020-06-06 11:46         ` Mark Brown
2020-06-06 12:45           ` Michael Walle
2020-06-08  8:28             ` Lee Jones
2020-06-08 10:02               ` Andy Shevchenko
2020-06-08 15:41                 ` Michael Walle
2020-06-08 18:56                   ` Lee Jones
2020-06-08 21:09                     ` Michael Walle
2020-06-09  6:47                       ` Lee Jones
2020-06-09 14:38                         ` Michael Walle
2020-06-09 14:42                           ` Mark Brown
2020-06-09 15:01                             ` Michael Walle
2020-06-09 17:15                               ` Rob Herring
2020-06-09 17:29                                 ` Mark Brown
2020-06-09 18:41                                 ` Lee Jones
2020-06-09 15:19                           ` Lee Jones
2020-06-09 15:30                             ` Michael Walle
2020-06-09 19:45                               ` Lee Jones
2020-06-10  7:10                                 ` Michael Walle
2020-06-10  7:19                                   ` Lee Jones
2020-06-10  7:49                                     ` Michael Walle
2020-06-10  7:56                                       ` Lee Jones
2020-06-10  9:27                                         ` Michael Walle
2020-06-10 18:30                                           ` Lee Jones
2020-06-10 17:16                                     ` Rob Herring
2020-06-10 18:02                                       ` Lee Jones
2020-06-08 18:20                 ` Lee Jones
2020-06-09 16:54               ` Rob Herring
2020-06-09 18:52                 ` Lee Jones
2020-06-05  8:01   ` Andy Shevchenko
2020-06-05  8:02     ` Andy Shevchenko
2020-06-05 10:09     ` Michael Walle
2020-06-05 10:48       ` Andy Shevchenko
2020-06-05 11:51         ` Michael Walle
2020-06-04 21:10 ` [PATCH v4 03/11] irqchip: add sl28cpld interrupt controller support Michael Walle
2020-06-05  1:26   ` kernel test robot
2020-06-05  8:07   ` Andy Shevchenko
2020-06-08 15:12   ` kernel test robot
2020-06-04 21:10 ` [PATCH v4 04/11] watchdog: add support for sl28cpld watchdog Michael Walle
2020-06-05  8:14   ` Andy Shevchenko
2020-06-05 10:24     ` Michael Walle
2020-06-05 10:50       ` Andy Shevchenko
2020-06-05 13:52         ` Guenter Roeck
2020-06-05 14:09           ` Andy Shevchenko
2020-06-05 15:05             ` Guenter Roeck
2020-06-05 16:04               ` Andy Shevchenko
2020-06-05 16:34                 ` Guenter Roeck
2020-06-04 21:10 ` [PATCH v4 05/11] pwm: add support for sl28cpld PWM controller Michael Walle
2020-06-05  8:15   ` Andy Shevchenko
2020-06-05  8:49   ` Lee Jones [this message]
2020-06-05  9:33     ` Andy Shevchenko
2020-06-05 11:39     ` Michael Walle
2020-06-05 18:17     ` Michael Walle
2020-06-08  7:46       ` Lee Jones
2020-06-04 21:10 ` [PATCH v4 06/11] gpio: add support for the sl28cpld GPIO controller Michael Walle
2020-06-05 12:00   ` Andy Shevchenko
2020-06-05 12:42     ` Michael Walle
2020-06-05 13:15       ` Andy Shevchenko
2020-06-05 18:44         ` Michael Walle
2020-06-05 21:19           ` Andy Shevchenko
2020-06-04 21:10 ` [PATCH v4 07/11] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
2020-06-05 12:06   ` Andy Shevchenko
2020-06-04 21:10 ` [PATCH v4 08/11] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
2020-06-04 21:10 ` [PATCH v4 09/11] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
2020-06-04 21:10 ` [PATCH v4 10/11] arm64: dts: freescale: sl28: enable LED support Michael Walle
2020-06-04 21:10 ` [PATCH v4 11/11] arm64: dts: freescale: sl28: enable fan support Michael Walle
2020-06-05  6:13 ` [PATCH v4 00/11] Add support for Kontron sl28cpld Lee Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200605084915.GE3714@dell \
    --to=lee.jones@linaro.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jason@lakedaemon.net \
    --cc=jdelvare@suse.com \
    --cc=leoyang.li@nxp.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=maz@kernel.org \
    --cc=michael@walle.cc \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git