| 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: <D9WJRVV500O3.GUV0MKHRGTH2@bootlin.com>
Date: Thu, 15 May 2025 09:14:40 +0200
From: "Mathieu Dubois-Briand" <mathieu.dubois-briand@...tlin.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
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>, "Michael Walle"
<mwalle@...nel.org>, "Mark Brown" <broonie@...nel.org>, "Greg
Kroah-Hartman" <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
<rafael@...nel.org>, "Danilo Krummrich" <dakr@...nel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-gpio@...r.kernel.org>, <linux-input@...r.kernel.org>,
<linux-pwm@...r.kernel.org>, <andriy.shevchenko@...el.com>,
Grégory Clement <gregory.clement@...tlin.com>, "Thomas
Petazzoni" <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v8 04/11] pwm: max7360: Add MAX7360 PWM support
On Tue May 13, 2025 at 12:08 PM CEST, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, May 09, 2025 at 11:14:38AM +0200, 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>
>> Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
>> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
>> ---
>> drivers/pwm/Kconfig | 10 +++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-max7360.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 197 insertions(+)
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 4731d5b90d7e..0b22141cbf85 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -755,4 +755,14 @@ 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
>> + 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 539e0def3f82..9c7701d8070b 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..af2006ec7a96
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-max7360.c
>> @@ -0,0 +1,186 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright 2025 Bootlin
>> + *
>> + * Author: Kamel BOUHARA <kamel.bouhara@...tlin.com>
>> + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@...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/bits.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/err.h>
>> +#include <linux/math64.h>
>> +#include <linux/mfd/max7360.h>
>> +#include <linux/minmax.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/regmap.h>
>> +#include <linux/time.h>
>> +#include <linux/types.h>
>> +
>> +#define MAX7360_NUM_PWMS 8
>> +#define MAX7360_PWM_MAX_RES 255
>> +#define MAX7360_PWM_PERIOD_NS (2 * NSEC_PER_MSEC)
>> +
>> +struct max7360_pwm_waveform {
>> + u8 duty_steps;
>> + bool enabled;
>> +};
>> +
>> +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct regmap *regmap = pwmchip_get_drvdata(chip);
>> + int ret;
>> +
>> + ret = regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm),
>> + MAX7360_PORT_CFG_COMMON_PWM, 0);
>> + if (ret)
>> + return ret;
>> +
>> + return regmap_write_bits(regmap, MAX7360_REG_PORTS, BIT(pwm->hwpwm), BIT(pwm->hwpwm));
>
> What is the effect of these writes? It doesn't need to be undone in a
> matching .free()?
>
The first one (MAX7360_PORT_CFG_COMMON_PWM) asks to use a specific duty
cycle for this PWM output and not a value shared across all PWMs. I
believe this one have no reason to be ever reverted.
About the second one, it does switch the output value. Reading the
datasheet, it's not clear if and why setting this here is required. I
will make some tests on the hardware a bit later this week. Still, I
believe there is no need to revert it later.
>> +}
>> +
>> +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip,
>> + struct pwm_device *pwm,
>> + const struct pwm_waveform *wf,
>> + void *_wfhw)
>> +{
>> + struct max7360_pwm_waveform *wfhw = _wfhw;
>> + u64 duty_steps;
>> +
>> + /*
>> + * Ignore user provided values for period_length_ns and duty_offset_ns:
>> + * we only support fixed period of MAX7360_PWM_PERIOD_NS and offset of 0.
>> + */
>> + duty_steps = mul_u64_u64_div_u64(wf->duty_length_ns, MAX7360_PWM_MAX_RES,
>> + MAX7360_PWM_PERIOD_NS);
>> +
>> + wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps);
>> + wfhw->enabled = !!wf->duty_length_ns;
>> +
>> + return 0;
>> +}
>> +
>> +static int max7360_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const void *_wfhw, struct pwm_waveform *wf)
>> +{
>> + const struct max7360_pwm_waveform *wfhw = _wfhw;
>> +
>> + wf->period_length_ns = wfhw->enabled ? MAX7360_PWM_PERIOD_NS : 0;
>> + wf->duty_offset_ns = 0;
>> + wf->duty_length_ns = DIV_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS,
>> + MAX7360_PWM_MAX_RES);
>> +
>> + return 0;
>> +}
>> +
>> +static int max7360_pwm_write_waveform(struct pwm_chip *chip,
>> + struct pwm_device *pwm,
>> + const void *_wfhw)
>> +{
>> + struct regmap *regmap = pwmchip_get_drvdata(chip);
>> + const struct max7360_pwm_waveform *wfhw = _wfhw;
>> + unsigned int val;
>> + int ret;
>> +
>> + val = wfhw->enabled ? BIT(pwm->hwpwm) : 0;
>> + ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCTRL, BIT(pwm->hwpwm), val);
>> + if (ret)
>> + return ret;
>> +
>> + if (wfhw->duty_steps)
>> + return regmap_write(regmap, MAX7360_REG_PWM(pwm->hwpwm), wfhw->duty_steps);
>
> Would it make sense to first write duty_steps and only then enable?
> Otherwise it might happen that you enable and still have a wrong duty
> configuration in the MAX7360_REG_PWM register and emit a wrong period?
>
Yes, I believe it does make sense: I will try to invert them.
> Do you need to write duty_steps = 0 if enabled is false?
>
No, this is not needed: output will be in hi-Z mode. As we have
"wfhw->enabled = !!wf->duty_length_ns", this should be correct here. But
reading this, I believe I could modify above code to be more clear with:
if (wfhw->enabled)
return regmap_write(regmap, MAX7360_REG_PWM(pwm->hwpwm), wfhw->duty_steps);
>> + return 0;
>> +}
>
> Best regards
> Uwe
--
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists