| 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: <aczpsiqyh4qsbvnqhqdnvkj2j3fihkltafop5ajkxm57sehbx5@mn4vp7avpeac>
Date: Mon, 29 Jul 2024 23:28:56 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, linux-pwm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] pwm: add support for NXPs high-side switch
MC33XS2410
Hello,
On Wed, May 15, 2024 at 01:20:34PM +0200, Dimitri Fedrau wrote:
> diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
> new file mode 100644
> index 000000000000..1904d1ee0652
> --- /dev/null
> +++ b/drivers/pwm/pwm-mc33xs2410.c
> @@ -0,0 +1,410 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
> + *
> + * Limitations:
> + * - Supports frequencies between 0.5Hz and 2048Hz with following steps:
> + * - 0.5 Hz steps from 0.5 Hz to 32 Hz
> + * - 2 Hz steps from 2 Hz to 128 Hz
> + * - 8 Hz steps from 8 Hz to 512 Hz
> + * - 32 Hz steps from 32 Hz to 2048 Hz
> + * - Cannot generate a 0 % duty cycle.
> + * - Always produces low output if disabled.
> + * - Configuration isn't atomic. When changing polarity, duty cycle or period
> + * the data is taken immediately, counters not being affected, resulting in a
> + * behavior of the output pin that is neither the old nor the new state,
> + * rather something in between.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/math.h>
> +#include <linux/math64.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/pwm.h>
> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/spi/spi.h>
> +
> +#define MC33XS2410_GLB_CTRL 0x00
> +#define MC33XS2410_GLB_CTRL_MODE_MASK GENMASK(7, 6)
> +#define MC33XS2410_GLB_CTRL_NORMAL_MODE BIT(6)
> +#define MC33XS2410_PWM_CTRL1 0x05
> +#define MC33XS2410_PWM_CTRL1_POL_INV(x) BIT(x)
> +#define MC33XS2410_PWM_CTRL3 0x07
> +/* x in { 0 ... 3 } */
> +#define MC33XS2410_PWM_CTRL3_EN(x) BIT(4 + (x))
> +#define MC33XS2410_PWM_FREQ1 0x08
> +/* x in { 1 ... 4 } */
> +#define MC33XS2410_PWM_FREQ(x) (MC33XS2410_PWM_FREQ1 + (x - 1))
> +#define MC33XS2410_PWM_FREQ_STEP_MASK GENMASK(7, 6)
> +#define MC33XS2410_PWM_FREQ_COUNT_MASK GENMASK(5, 0)
> +#define MC33XS2410_PWM_DC1 0x0c
> +/* x in { 1 ... 4 } */
> +#define MC33XS2410_PWM_DC(x) (MC33XS2410_PWM_DC1 + (x - 1))
> +#define MC33XS2410_WDT 0x14
> +
> +#define MC33XS2410_WR BIT(7)
> +#define MC33XS2410_RD_CTRL BIT(7)
> +#define MC33XS2410_RD_DATA_MASK GENMASK(13, 0)
> +
> +#define MC33XS2410_MIN_PERIOD_STEP0 31250000
> +#define MC33XS2410_MAX_PERIOD_STEP0 2000000000
> +/* x in { 0 ... 3 } */
> +#define MC33XS2410_MIN_PERIOD_STEP(x) (MC33XS2410_MIN_PERIOD_STEP0 >> (2 * x))
> +/* x in { 0 ... 3 } */
> +#define MC33XS2410_MAX_PERIOD_STEP(x) (MC33XS2410_MAX_PERIOD_STEP0 >> (2 * x))
> +
> +#define MC33XS2410_MAX_TRANSFERS 5
> +#define MC33XS2410_WORD_LEN 2
> +
> +struct mc33xs2410_pwm {
> + struct spi_device *spi;
> +};
> +
> +static
> +inline struct mc33xs2410_pwm *to_pwm_mc33xs2410_chip(struct pwm_chip *chip)
> +{
> + return pwmchip_get_drvdata(chip);
> +}
> +
> +static int mc33xs2410_xfer_regs(struct spi_device *spi, bool read, u8 *reg,
> + u16 *val, bool *ctrl, int len)
> +{
> + struct spi_transfer t[MC33XS2410_MAX_TRANSFERS] = { { 0 } };
> + u8 tx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
> + u8 rx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN];
> + int i, ret, reg_i, val_i;
> +
> + if (!len)
> + return 0;
> +
> + if (read)
> + len++;
> +
> + if (len > MC33XS2410_MAX_TRANSFERS)
> + return -EINVAL;
> +
> + for (i = 0; i < len; i++) {
> + reg_i = i * MC33XS2410_WORD_LEN;
> + val_i = reg_i + 1;
> + if (read) {
> + if (i < len - 1) {
> + tx[reg_i] = reg[i];
> + tx[val_i] = ctrl[i] ? MC33XS2410_RD_CTRL : 0;
> + t[i].tx_buf = &tx[reg_i];
> + }
> +
> + if (i > 0)
> + t[i].rx_buf = &rx[reg_i - MC33XS2410_WORD_LEN];
> + } else {
> + tx[reg_i] = reg[i] | MC33XS2410_WR;
> + tx[val_i] = val[i];
> + t[i].tx_buf = &tx[reg_i];
> + }
> +
> + t[i].len = MC33XS2410_WORD_LEN;
> + t[i].cs_change = 1;
> + }
> +
> + t[len - 1].cs_change = 0;
> +
> + ret = spi_sync_transfer(spi, &t[0], len);
> + if (ret < 0)
> + return ret;
> +
> + if (read) {
> + for (i = 0; i < len - 1; i++) {
> + reg_i = i * MC33XS2410_WORD_LEN;
> + val[i] = FIELD_GET(MC33XS2410_RD_DATA_MASK,
> + get_unaligned_be16(&rx[reg_i]));
> + }
> + }
> +
> + return 0;
Huh, this is complicated. Isn't that covered by regmap somehow?
> +}
> +
> [...]
> +
> +static u8 mc33xs2410_pwm_get_freq(u64 period)
> +{
> + u8 step, count;
> +
> + /*
> + * Check if period is within the limits of each of the four frequency
> + * ranges, starting with the highest frequency(lowest period). Higher
> + * frequencies are represented with better resolution by the device.
> + * Therefore favor frequency range with the better resolution to
> + * minimize error introduced by the frequency steps.
I'm not a native English speaker, but I find that misleading. That
period is in the "possible" range is already asserted by the caller. So
the switch is about "Check which step is appropriate for the given
period", right?
> + */
> +
> + switch (period) {
> + case MC33XS2410_MIN_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(3):
> + step = 3;
> + break;
> + case MC33XS2410_MAX_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(2):
> + step = 2;
> + break;
> + case MC33XS2410_MAX_PERIOD_STEP(2) + 1 ... MC33XS2410_MAX_PERIOD_STEP(1):
> + step = 1;
> + break;
> + case MC33XS2410_MAX_PERIOD_STEP(1) + 1 ... MC33XS2410_MAX_PERIOD_STEP(0):
> + step = 0;
> + break;
> + }
> +
> + count = DIV_ROUND_UP(MC33XS2410_MAX_PERIOD_STEP(step), period) - 1;
> +
> + return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) |
> + FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count);
> +}
> +
> [...]
> +
> +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
> +{
> + if (!period)
> + return 0;
> +
> + duty_cycle *= 256;
This might overflow.
> + duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period);
round-closest is most probably wrong. Please test your driver with
PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and
period.
> +
> + /* Device is not able to generate 0% duty cycle */
> + if (!duty_cycle)
> + return -ERANGE;
Given that the hardware emits a low level when disabled, please disable
if duty_cycle = 0 is requested.
> + return duty_cycle - 1;
> +}
> +
> [...]
> +static int mc33xs2410_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct mc33xs2410_pwm *mc33xs2410 = to_pwm_mc33xs2410_chip(chip);
> + struct spi_device *spi = mc33xs2410->spi;
> + u8 reg[4] = {
> + MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
> + MC33XS2410_PWM_DC(pwm->hwpwm + 1),
> + MC33XS2410_PWM_CTRL1,
> + MC33XS2410_PWM_CTRL3,
> + };
> + bool ctrl[4] = { true, true, true, true };
> + u16 val[4];
> + int ret;
> +
> + ret = mc33xs2410_read_regs(spi, reg, ctrl, val, 4);
> + if (ret < 0)
> + return ret;
> +
> + state->period = mc33xs2410_pwm_get_period(val[0]);
> + pwm_set_relative_duty_cycle(state, val[1] + 1, 256);
pwm_set_relative_duty_cycle doesn't use the right rounding for
.get_state().
> + state->polarity = (val[2] & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
> + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +
> + state->enabled = !!(val[3] & MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm));
> +
> + return 0;
> +}
> +
> [...]
> +static int mc33xs2410_probe(struct spi_device *spi)
> +{
> [...]
> + /* Disable watchdog */
> + ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to disable watchdog\n");
Wouldn't the watchdog functionality better be handled by a dedicated
watchdog driver? Disabling it here unconditionally looks wrong.
> + /* Transition to normal mode */
> + ret = mc33xs2410_modify_reg(spi, MC33XS2410_GLB_CTRL,
> + MC33XS2410_GLB_CTRL_MODE_MASK,
> + MC33XS2410_GLB_CTRL_NORMAL_MODE);
> [...]
Best regards
Uwe
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists