From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com [209.85.221.51]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id ADA86391859 for ; Thu, 16 Apr 2026 10:27:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776335258; cv=none; b=HtBp8jHPEsU4/6/DbpwrQPtlMyTaRJ5QJhKHT5JVHDBrTwB20cN9v50aC/t5PkdqozMkHD7ZsYWf/PQ3XlbN6wlOkuNxYJICnLTICK8Yno9+kVdrQBuG7R4KCbwPSA3Qo1ynYnYNKp3Lhfz2X1jH45Cbv7OmcGRzctY2Q+7sZ2I= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776335258; c=relaxed/simple; bh=w9q388Vzqwl8rvLdEaZHWTqCR0j9/zn7j9pEumntPY4=; h=From:Date:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=n3L9D2ty4140c125CTv66L9f49z4AJUJCDKL2uer+QcGBxSw3hHZ44M6+XTm+O4kxxkXXWyF4xAzEEbYov7Zd8uvUHjZLr+udc00uz32A4kMxpk8wTy6WAQi1plzWDJHj0yOKLxsMEm5tlHrFsCMIPKX/JFJe4JA8MgFxwTmTL8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com; spf=pass smtp.mailfrom=suse.com; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b=bzDaiW1i; arc=none smtp.client-ip=209.85.221.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=suse.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b="bzDaiW1i" Received: by mail-wr1-f51.google.com with SMTP id ffacd0b85a97d-43eada6d900so1434698f8f.0 for ; Thu, 16 Apr 2026 03:27:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1776335255; x=1776940055; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:date:from:from:to :cc:subject:date:message-id:reply-to; bh=4HCRqhH2DLFveExlKyLVA9G9WBAn4h6tRZdd4chEffg=; b=bzDaiW1i5OZaAZnjzclrCjivvi/q76gjygkY5Rvg/nSfLF/MMJnasGzoK7uTsGA53J JR3l8j3byiAudfEGyhuUPwWhe0UlEIaFu6EBod8eb9m+r2hD3EjIbXxKRmV86oddNNRd yT9rQyrAMFqeOoCRWmlVgJ9XhHv63TXjfxOnNzTd9kGvPzql+5ZwkW5ROX+GkhG2uSGy gj4Ep14yZr23Dp+CJuSGUpr1FtKD5fkUludeKQ3N6XgLA6W8uU6KnB52TNO99YSrYLvk HN15BG6Nt4qbat0B5lwiaBc5dE/7behB1RwAxhrb93d4YQtO4XGjkU19d6hypjzgcUsS V+tA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1776335255; x=1776940055; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:date:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=4HCRqhH2DLFveExlKyLVA9G9WBAn4h6tRZdd4chEffg=; b=Wx+xPI8ojHE6aKYtulozGfi/fUCHCMTU0CsWYpNIdPbEFj+AGI7FZwsBNSu1mOpjbJ LKgqh2IG3rGMefA49xp/g5e0g5y0s9GdiYgr6oXeYmJFH8h9WigVF3nTuC6jrsxu/+sr O4/2fCvdNjFLbwIe/gHz9LVK38GRlFB91DYPmzKYo6QipqQ1+sWs3Y+OLbF7BaXq+CGp 33UZg2SEHEg5nEB1T+0yipRdNMDP41rjLJW/E1/rf2GzlN4AQOM4QOVcEYWRBpbMDB7e nVdHvtKxuCoEVFpZ+rKpd27xw8F7gmjXJ8f02u9Es1S8L2fiz04my1Vr3H+RisDuW8Gu OhQg== X-Forwarded-Encrypted: i=1; AFNElJ/6ONB4jbwOUZQc8etD32e6MKArMOof+BYDw8h21pFH/9MxTUUFL5pa5rYvrmCETS3WRtsuXotKgVcjC4s=@vger.kernel.org X-Gm-Message-State: AOJu0YzAfysx+ZvdyCRKyYEZR6X5BFTkl7tqKDXerObaVpJKECJGc2J0 KzD5WMeTkPatqCy0Ou356wshJGm6XxX+/KHNTjdcRs2gg3aPDO/KORbteFxfZc2vCDc= X-Gm-Gg: AeBDievo63Vxw8GIPT/MyrHbMFYeMws9IX+Rw9H4PN4qNsErpN5ceQdJmDbKKSsZNVS QbkguIiweFC1m2lyLp4T4Y15FVn6DJjJqbGjyuZMQ4Sko8PphHTh8Y3zVpNepwMA7IbbZQx26/+ 1eiOIc1GFkYyMV57wATFIuATfPYJG1dbz4JwqjnCYnQan3St7LCHlrYS6seXxCNG3+01zxzDKCp fy3AO74jrra6ySA0CDaZVlHK31f7IGKq1wUYSszxc7MYwvfOIfvcX35iiFqOV5GvhBWPD4fEBV3 Bb6DSOM4mbkRWg2aW2qrmYKN2tbEqdX5a5vLwyuUm8fwsA7Xf0UjeNEMxgImGwwHYUmZJwEbuc5 zrJEbzHVuhFAsQdwRz0xKawZkDDjPUfEg8uXz45jDh6D4FSP7mTJvAmDqG+Ahy9ocX01EU8oDfw P7Ux1uFQlYxsTG+DrIciObDDMIm8Rm7b1MF0trM2pWTeYx4CxjuzYBu+K2E0LuvrRCRzLMt5pRs EXAkZg= X-Received: by 2002:a05:6000:25c2:b0:439:c18f:5aaf with SMTP id ffacd0b85a97d-43d642ccd7dmr38851820f8f.34.1776335254939; Thu, 16 Apr 2026 03:27:34 -0700 (PDT) Received: from localhost (host-79-33-140-232.retail.telecomitalia.it. [79.33.140.232]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-43ead3566b7sm13011883f8f.11.2026.04.16.03.27.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Apr 2026 03:27:34 -0700 (PDT) From: Andrea della Porta X-Google-Original-From: Andrea della Porta Date: Thu, 16 Apr 2026 12:30:43 +0200 To: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Cc: Andrea della Porta , linux-pwm@vger.kernel.org, Rob Herring , Krzysztof Kozlowski , Conor Dooley , Florian Fainelli , Broadcom internal kernel review list , devicetree@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Naushir Patuck , Stanimir Varbanov , mbrugger@suse.com Subject: Re: [PATCH v2 2/3] pwm: rp1: Add RP1 PWM controller driver Message-ID: References: <0d99317b9150310dfbd98de1cb2a890f0bffe7cd.1775829499.git.andrea.porta@suse.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Hi Uwe, On 19:31 Fri 10 Apr , Uwe Kleine-König wrote: > Hello Andrea, > > nice work for a v2! Thanks! > > On Fri, Apr 10, 2026 at 04:09:58PM +0200, Andrea della Porta wrote: <...snip...> > > +#define RP1_PWM_GLOBAL_CTRL 0x000 > > +#define RP1_PWM_CHANNEL_CTRL(x) (0x014 + ((x) * 0x10)) > > +#define RP1_PWM_RANGE(x) (0x018 + ((x) * 0x10)) > > +#define RP1_PWM_PHASE(x) (0x01C + ((x) * 0x10)) > > +#define RP1_PWM_DUTY(x) (0x020 + ((x) * 0x10)) > > + > > +/* 8:FIFO_POP_MASK + 0:Trailing edge M/S modulation */ > > +#define RP1_PWM_CHANNEL_DEFAULT (BIT(8) + BIT(0)) > > Please add a #define for BIT(8) and then use that and > FIELD_PREP(RP1_PWM_MODE, RP1_PWM_MODE_SOMENICENAME) to define the > constant. Also I would define it below the register defines. Ack. > > > +#define RP1_PWM_CHANNEL_ENABLE(x) BIT(x) > > +#define RP1_PWM_POLARITY BIT(3) > > +#define RP1_PWM_SET_UPDATE BIT(31) > > +#define RP1_PWM_MODE_MASK GENMASK(1, 0) > > s/_MASK// please > > It would be great if the bitfield's names started with the register > name. Ack. > > > + > > +#define RP1_PWM_NUM_PWMS 4 > > + > > +struct rp1_pwm { > > + struct regmap *regmap; > > + struct clk *clk; > > + unsigned long clk_rate; > > + bool clk_enabled; > > +}; > > + > > +struct rp1_pwm_waveform { > > + u32 period_ticks; > > + u32 duty_ticks; > > + bool enabled; > > + bool inverted_polarity; > > +}; > > + > > +static const struct regmap_config rp1_pwm_regmap_config = { > > + .reg_bits = 32, > > + .val_bits = 32, > > + .reg_stride = 4, > > + .max_register = 0x60, > > I'm not a fan of aligning the = in a struct, still more if it fails like > here. Please consistently align all =s, or even better, use a single > space before each =. (Same for the struct definitions above, but I won't > insist.) Let's use the single space. > > > +}; > > + > > +static void rp1_pwm_apply_config(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > > + u32 value; > > + > > + /* update the changed registers on the next strobe to avoid glitches */ > > + regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value); > > + value |= RP1_PWM_SET_UPDATE; > > + regmap_write(rp1->regmap, RP1_PWM_GLOBAL_CTRL, value); > > I assume there is a glitch if I update two channels and the old > configuration of the first channel ends while I'm in the middle of > configuring the second? The configuration registers are per-channel but the update flag is global. I don't have details of the hw insights, my best guess is that anything that you set in the registers before updating the flag will take effect, so there should be no glitches. > > > +} > > + > > +static int rp1_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > > + > > + /* init channel to reset defaults */ > > + regmap_write(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), RP1_PWM_CHANNEL_DEFAULT); > > + return 0; > > +} > > + > > +static int rp1_pwm_round_waveform_tohw(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const struct pwm_waveform *wf, > > + void *_wfhw) > > +{ > > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > > + struct rp1_pwm_waveform *wfhw = _wfhw; > > + u64 clk_rate = rp1->clk_rate; > > + u64 ticks; > > if (!wf->period_length_ns) > wfhw->enabled = false > return 0; > > > + ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC); > > To ensure this doesn't overflow please fail to probe the driver if > clk_rate > 1 GHz with an explaining comment. (Or alternatively calculate > the length of period_ticks = U32_MAX and skip the calculation if > wf->period_length_ns is bigger.) Ack. > > > + if (ticks > U32_MAX) > > + ticks = U32_MAX; > > + wfhw->period_ticks = ticks; > > What happens if wf->period_length_ns > 0 but ticks == 0? I've added a check, returning 1 to signal teh round-up, and a minimum tick of 1 in this case. > > > + if (wf->duty_offset_ns + wf->duty_length_ns >= wf->period_length_ns) { > > The maybe surprising effect here is that in the two cases > > wf->duty_offset_ns == wf->period_length_ns and wf->duty_length_ns == 0 > > and > > wf->duty_length_ns == wf->period_length_ns and wf->duty_offset_ns == 0 > > you're configuring inverted polarity. I doesn't matter technically > because the result is the same, but for consumers still using pwm_state > this is irritating. That's why pwm-stm32 uses inverted polarity only if > also wf->duty_length_ns and wf->duty_offset_ns are non-zero. Ack. > > > + ticks = mul_u64_u64_div_u64(wf->period_length_ns - wf->duty_length_ns, > > + clk_rate, NSEC_PER_SEC); > > The rounding is wrong here. You should pick the biggest duty_length not > bigger than wf->duty_length_ns, so you have to use > > ticks = wfhw->period_ticks - mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC): > > . I see this is a hole in the pwmtestperf coverage. Ack. > > > + wfhw->inverted_polarity = true; > > + } else { > > + ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC); > > + wfhw->inverted_polarity = false; > > + } > > + > > + if (ticks > wfhw->period_ticks) > > + ticks = wfhw->period_ticks; > > You can and should assume that wf->duty_length_ns <= > wf->period_length_ns. Then the if condition can never become true. Ack. > > > + wfhw->duty_ticks = ticks; > > + > > + wfhw->enabled = !!wfhw->duty_ticks; > > + > > + return 0; > > +} > > + > > +static int rp1_pwm_round_waveform_fromhw(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const void *_wfhw, > > + struct pwm_waveform *wf) > > +{ > > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > > + const struct rp1_pwm_waveform *wfhw = _wfhw; > > + u64 clk_rate = rp1->clk_rate; > > + u32 ticks; > > + > > + memset(wf, 0, sizeof(*wf)); > > wf = (struct pwm_waveform){ }; > > is usually more efficient. Ack. > > > + if (!wfhw->enabled) > > + return 0; > > + > > + wf->period_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->period_ticks * NSEC_PER_SEC, clk_rate); > > + > > + if (wfhw->inverted_polarity) { > > + wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC, > > + clk_rate); > > + } else { > > + wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC, > > + clk_rate); > > + ticks = wfhw->period_ticks - wfhw->duty_ticks; > > + wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)ticks * NSEC_PER_SEC, clk_rate); > > + } > > This needs adaption after the rounding issue in tohw is fixed. Ack. > > > + return 0; > > +} > > + > > +static int rp1_pwm_write_waveform(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const void *_wfhw) > > +{ > > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > > + const struct rp1_pwm_waveform *wfhw = _wfhw; > > + u32 value; > > + > > + /* set period and duty cycle */ > > + regmap_write(rp1->regmap, > > + RP1_PWM_RANGE(pwm->hwpwm), wfhw->period_ticks); > > + regmap_write(rp1->regmap, > > + RP1_PWM_DUTY(pwm->hwpwm), wfhw->duty_ticks); > > + > > + /* set polarity */ > > + regmap_read(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), &value); > > + if (!wfhw->inverted_polarity) > > + value &= ~RP1_PWM_POLARITY; > > + else > > + value |= RP1_PWM_POLARITY; > > + regmap_write(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), value); > > + > > + /* enable/disable */ > > + regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value); > > + if (wfhw->enabled) > > + value |= RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm); > > + else > > + value &= ~RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm); > > + regmap_write(rp1->regmap, RP1_PWM_GLOBAL_CTRL, value); > > You can exit early if wfhw->enabled is false after clearing the channel > enable bit. Ack. > > > + rp1_pwm_apply_config(chip, pwm); > > + > > + return 0; > > +} > > + <,...snip...> > > + } > > + > > + return 0; > > + > > +err_disable_clk: > > + clk_disable_unprepare(rp1->clk); > > + > > + return ret; > > +} > > On remove you miss to balance the call to clk_prepare_enable() (if no > failed call to clk_prepare_enable() in rp1_pwm_resume() happend). Since this driver now exports a syscon, it's only builtin (=Y) so it cannot be unloaded. I've also avoided the .remove callback via .suppress_bind_attrs. > > > + > > +static int rp1_pwm_suspend(struct device *dev) > > +{ > > + struct rp1_pwm *rp1 = dev_get_drvdata(dev); > > + > > + if (rp1->clk_enabled) { > > + clk_disable_unprepare(rp1->clk); > > + rp1->clk_enabled = false; > > + } > > + > > + return 0; > > +} > > + > > +static int rp1_pwm_resume(struct device *dev) > > +{ > > + struct rp1_pwm *rp1 = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = clk_prepare_enable(rp1->clk); > > + if (ret) { > > + dev_err(dev, "Failed to enable clock on resume: %d\n", ret); > > Please use %pe for error codes. Ack. Best regards, Andrea > > > + return ret; > > + } > > + > > + rp1->clk_enabled = true; > > + > > + return 0; > > +} > > Best regards > Uwe