| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f22l3uqgt65utxehv2zmozqixjkktp4trpr42xr5arvp6o5zcf@g5iriaeskqa5>
Date: Fri, 17 Jan 2025 10:33:27 +0100
From: Uwe Kleine-König <ukleinek@...nel.org>
To: mathieu.dubois-briand@...tlin.com
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Kamel Bouhara <kamel.bouhara@...tlin.com>, Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>, Dmitry Torokhov <dmitry.torokhov@...il.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-input@...r.kernel.org, linux-pwm@...r.kernel.org,
Grégory Clement <gregory.clement@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support
Hello Mathieu,
On Mon, Jan 13, 2025 at 01:42:27PM +0100, mathieu.dubois-briand@...tlin.com wrote:
> From: Kamel Bouhara <kamel.bouhara@...tlin.com>
>
> Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to
> 8 independent PWM outputs.
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@...tlin.com>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
> ---
> drivers/pwm/Kconfig | 11 +++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-max7360.c | 220 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 232 insertions(+)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0915c1e7df16..399dc3f76e92 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -745,4 +745,15 @@ config PWM_XILINX
> To compile this driver as a module, choose M here: the module
> will be called pwm-xilinx.
>
> +config PWM_MAX7360
> + tristate "MAX7360 PWMs"
> + depends on MFD_MAX7360
> + depends on OF_GPIO
> + help
> + PWM driver for Maxim Integrated MAX7360 multifunction device, with
> + support for up to 8 PWM outputs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-max7360.
> +
> endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9081e0c0e9e0..ae8908ffc892 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
> obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
> obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
> obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o
> +obj-$(CONFIG_PWM_MAX7360) += pwm-max7360.o
> obj-$(CONFIG_PWM_MESON) += pwm-meson.o
> obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o
> obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o
> diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
> new file mode 100644
> index 000000000000..e76a8aa05fc4
> --- /dev/null
> +++ b/drivers/pwm/pwm-max7360.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2024 Bootlin
> + *
> + * Author: Kamel BOUHARA <kamel.bouhara@...tlin.com>
> + *
> + * Limitations:
> + * - Only supports normal polarity.
> + * - The period is fixed to 2 ms.
> + * - Only the duty cycle can be changed, new values are applied at the beginning
> + * of the next cycle.
> + * - When disabled, the output is put in Hi-Z.
> + */
> +#include <linux/math.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +#define MAX7360_NUM_PWMS 8
> +#define MAX7360_PWM_MAX_RES 256
> +#define MAX7360_PWM_PERIOD_NS 2000000 /* 500 Hz */
> +#define MAX7360_PWM_COMMON_PWN BIT(5)
> +#define MAX7360_PWM_CTRL_ENABLE(n) BIT(n)
> +#define MAX7360_PWM_PORT(n) BIT(n)
> +
> +struct max7360_pwm {
> + struct device *parent;
> + struct regmap *regmap;
> +};
> +
> +static inline struct max7360_pwm *to_max7360_pwm(struct pwm_chip *chip)
Please stick to the common function prefix also here. So something like
max7360_pwm_from_chip would work.
> +{
> + return pwmchip_get_drvdata(chip);
> +}
> +
> +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct max7360_pwm *max7360_pwm;
> + int ret;
> +
> + max7360_pwm = to_max7360_pwm(chip);
> + ret = max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm,
> + true);
The statement fits on a single line just fine.
> + if (ret) {
> + dev_warn(&chip->dev, "failed to request pwm-%d\n", pwm->hwpwm);
> + return ret;
> + }
> +
> + ret = regmap_write_bits(max7360_pwm->regmap,
> + MAX7360_REG_PWMCFG + pwm->hwpwm,
Can you make MAX7360_REG_PWMCFG a macro accepting pwm->hwpwm as
parameter please?
> + MAX7360_PWM_COMMON_PWN,
> + 0);
> + if (ret) {
> + dev_warn(&chip->dev,
> + "failed to write pwm-%d cfg register, error %d\n",
> + pwm->hwpwm, ret);
> + return ret;
> + }
> +
> + ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_PORTS,
> + MAX7360_PWM_PORT(pwm->hwpwm),
> + MAX7360_PWM_PORT(pwm->hwpwm));
> + if (ret) {
> + dev_warn(&chip->dev,
> + "failed to write pwm-%d ports register, error %d\n",
> + pwm->hwpwm, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct max7360_pwm *max7360_pwm;
> + int ret;
> +
> + max7360_pwm = to_max7360_pwm(chip);
> + ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
> + MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
> + 0);
> + if (ret)
> + dev_warn(&chip->dev, "failed to disable pwm-%d , error %d\n",
> + pwm->hwpwm, ret);
.free is not supposed to stop the PWM. Though I admit this concept is a
bit fuzzy, because when unconfiguring the pin function this is kind of
moot. Still it's the responsibility of the consumer to stop the PWM
before pwm_put().
Also s/ ,/,/ and use %pe for error codes.
> + max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm,
> + false);
> +}
> +
> +static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct max7360_pwm *max7360_pwm;
> + u64 duty_steps;
> + int ret;
> +
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
> +
> + if (state->period != MAX7360_PWM_PERIOD_NS) {
> + dev_warn(&chip->dev,
> + "unsupported pwm period: %llu, should be %u\n",
> + state->period, MAX7360_PWM_PERIOD_NS);
> + return -EINVAL;
Please don't emit error messages in .apply(). Also a driver is supposed
to round down .period, so any value >= MAX7360_PWM_PERIOD_NS should be
accepted.
Also note that you might want to implement the waveform callbacks
instead of .apply() and .get_state() for the more modern abstraction
(with slightly different rounding rules).
> + }
> +
> + duty_steps = mul_u64_u64_div_u64(state->duty_cycle, MAX7360_PWM_MAX_RES,
> + MAX7360_PWM_PERIOD_NS);
> +
> + max7360_pwm = to_max7360_pwm(chip);
> + ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
> + MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm),
> + MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm));
> + if (ret) {
> + dev_warn(&chip->dev, "failed to enable pwm-%d , error %d\n",
> + pwm->hwpwm, ret);
> + return ret;
> + }
> +
> + ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm,
> + duty_steps >= 255 ? 255 : duty_steps);
> + if (ret) {
> + dev_warn(&chip->dev,
> + "failed to apply pwm duty_cycle %llu on pwm-%d, error %d\n",
> + duty_steps, pwm->hwpwm, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int max7360_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct max7360_pwm *max7360_pwm;
> + unsigned int val;
> + int ret;
> +
> + max7360_pwm = to_max7360_pwm(chip);
> +
> + state->period = MAX7360_PWM_PERIOD_NS;
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL, &val);
> + if (ret) {
> + dev_warn(&chip->dev,
> + "failed to read pwm configuration on pwm-%d, error %d\n",
> + pwm->hwpwm, ret);
Similar to .apply() please no messages in .get_state(). Just fail
silently.
> + return ret;
> + }
> + state->enabled = !!(val & MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm));
> +
> + ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_PWMBASE + pwm->hwpwm,
> + &val);
> + if (ret) {
> + dev_warn(&chip->dev,
> + "failed to read pwm duty_cycle on pwm-%d, error %d\n",
> + pwm->hwpwm, ret);
> + return ret;
> + }
> + state->duty_cycle = mul_u64_u64_div_u64(val, MAX7360_PWM_PERIOD_NS,
> + MAX7360_PWM_MAX_RES);
You have to round up here. I would expect that the checks in the core
(with PWM_DEBUG=1) help you catching this type of error. In your case
changing the configuration to
.period = 2000000,
.duty_cycle = 234379,
should yield some hint in the kernel log.
> + return 0;
> +}
Best regards
Uwe
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists