| 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: <DB6N1SVPVSQJ.15KQKOBOCHDCQ@bootlin.com>
Date: Tue, 08 Jul 2025 13:31:19 +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 v10 04/11] pwm: max7360: Add MAX7360 PWM support
On Wed Jun 18, 2025 at 8:45 PM CEST, Uwe Kleine-König wrote:
> On Fri, May 30, 2025 at 12:00:12PM +0200, mathieu.dubois-briand@...tlin.com wrote:
>> From: Kamel Bouhara <kamel.bouhara@...tlin.com>
> ...
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-max7360.c
>> @@ -0,0 +1,180 @@
>> +// 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);
>> +
>> + return regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm),
>> + MAX7360_PORT_CFG_COMMON_PWM, 0);
>> +}
>
> Do you need to undo that in .free()?
>
No, this is just to make sure we use the individual PWM configuration
register and not the global one, so there is no need to revert it later.
I'm adding a comment explaining that.
> ...
>
>> + wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps);
>> + wfhw->enabled = !!wf->duty_length_ns;
>
> How does the output behave if you clean the respective bit in
> MAX7360_REG_GPIOCTRL? Unless it emits a constant low signal (and not
> e.g. High-Z) you have to do
>
> wfhw->enabled = !!wf->period_length_ns;
>
> here. Please document the behaviour in a paragraph at the top of
> the driver. Look at other drivers for the right format. The questions to
> answer are:
>
> - How does the driver behave on disable? (Typical is constant low or
> High-Z or freezing. Does it stop instantly or does it complete the
> currently running period?)
>
> - How does the driver behave on a (non-disabling) reconfiguration? Can
> it happen that there are glitches? (Consider for example that
> duty_cycle changes from 0.5 ms to 1.5ms while the hardware is just in
> the middle of the 2ms period. Does the output go high immediately
> then producing two 0.5ms pulses during that period?)
>
Ok, I'm fixing the wfhw->enabled value.
About the comment, I believe we already have everything, I'm just adding
that on disable, the output is put in Hi-Z immediately.
>> + 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);
>
> This should be 0 if !wfhw->enabled to make *wf a valid setting.
>
OK.
> A check for that in the core (with CONFIG_PWM_DEBUG) would be great.
>
I can submit a patch, but I'm not sure what that check should be.
So I believe the check would have to be made in __pwm_set_waveform(),
making sure wf_rounded.duty_length_ns is 0 if the PWM is not enabled or
in other words, if wf->period_length_ns is 0. I believe calling
pwm_wf_valid() on wf_rounded would be enough. Maybe I should add that as
a first check in pwm_check_rounding() to cover all call sites.
We already call pwm_check_rounding() code, so me already make sure that
if wf->period_length_ns is 0, wf_rounded->period_length_ns is 0. And
adding pwm_wf_valid(), would make sure that if
wf_rounded->period_length_ns is 0, wf_rounded->duty_length_ns is also 0.
Any opinion?
> ...
>
> Best regards
> Uwe
OK with all other comments.
Thanks for your review!
--
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists