From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 302E12E7166 for ; Tue, 17 Jun 2025 19:43:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=210.118.77.12 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750189391; cv=none; b=dkVIOYfHkRMA5xpwTpYmzAregedOas4h7nW/FJWtZTXhE4gxn2Z+KlzGB6FXKDw4b3ZOK2FAkhjFweDFjsdGxyjf8rU7jCwShLpbwPS4l2B+tN+GRthYoq0zPvVNJrOvF2asSFFtgG/SBOgr4Gb94cB/uOEWUn7Du4S2re0O6Ks= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750189391; c=relaxed/simple; bh=l7SzB8NO+rrP5k4367W9aS0ArpEC+aDkSTC3pfTFz30=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:From:In-Reply-To: Content-Type:References; b=XDWPiLdnogVAvfOPJrjO2t1SsW7f2ENDiSSWS5GOSm0wIwGS4GfdEYuZBRBHJXZmr8W8hgFOC6orqgSQlvsdsbx/+eDsVwltN/JR/MYSHTAw+Bt8lKsE1Nta2WnIVwtgNIAcQe4oMHK5Xgywo4L78UPCiXCsjturmSzueUNBUn0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=samsung.com; spf=pass smtp.mailfrom=samsung.com; dkim=pass (1024-bit key) header.d=samsung.com header.i=@samsung.com header.b=tQn+1aMb; arc=none smtp.client-ip=210.118.77.12 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=samsung.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=samsung.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=samsung.com header.i=@samsung.com header.b="tQn+1aMb" Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20250617194308euoutp02857ea47871a6cd763e88838ab161204a~J6-nL49Nt2272722727euoutp02D for ; Tue, 17 Jun 2025 19:43:08 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20250617194308euoutp02857ea47871a6cd763e88838ab161204a~J6-nL49Nt2272722727euoutp02D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1750189388; bh=SsrrapZlggFpVYw95Drc6h8mJo8KmYhpbhG7VYywjIs=; h=Date:Subject:To:Cc:From:In-Reply-To:References:From; b=tQn+1aMbwyFepAeZyTjCE0hzZXQjkbcCqJBGARftZbkUFKJLWTc9EfdDMG3TezNAh DIlbOcm9eVbr72Cc5FNc0HMpIvgf9sldMIsyFi1JfXTsCWsUDcCuEV/qJQQp0PfA/q NJT7rOLr+MUFVmvLKM0BTaZbJfXq8aja9MxXgRbc= Received: from eusmtip1.samsung.com (unknown [203.254.199.221]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20250617194306eucas1p118ec57136b8265b7e13aeb1ea93a14b7~J6-l2QnM52077420774eucas1p1s; Tue, 17 Jun 2025 19:43:06 +0000 (GMT) Received: from [192.168.1.44] (unknown [106.210.136.40]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20250617194305eusmtip14ca5682a8b9fab2b3547be7105427287~J6-kraHC01904619046eusmtip1_; Tue, 17 Jun 2025 19:43:05 +0000 (GMT) Message-ID: <98d6b8e5-4694-44df-9ba0-33e6c00d8183@samsung.com> Date: Tue, 17 Jun 2025 21:43:05 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 4/9] pwm: Add Rust driver for T-HEAD TH1520 SoC To: Danilo Krummrich Cc: =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= , Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , Alice Ryhl , Trevor Gross , Drew Fustini , Guo Ren , Fu Wei , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Paul Walmsley , Palmer Dabbelt , Albert Ou , Alexandre Ghiti , Marek Szyprowski , Benno Lossin , Michael Turquette , Stephen Boyd , linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org, rust-for-linux@vger.kernel.org, linux-riscv@lists.infradead.org, devicetree@vger.kernel.org, linux-clk@vger.kernel.org Content-Language: en-US From: Michal Wilczynski In-Reply-To: Content-Transfer-Encoding: 7bit X-CMS-MailID: 20250617194306eucas1p118ec57136b8265b7e13aeb1ea93a14b7 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20250617140839eucas1p2d13775f8e6d34a516e93d3b426d5fb16 X-EPHeader: CA X-CMS-RootMailID: 20250617140839eucas1p2d13775f8e6d34a516e93d3b426d5fb16 References: <20250617-rust-next-pwm-working-fan-for-sending-v3-0-1cca847c6f9f@samsung.com> <20250617-rust-next-pwm-working-fan-for-sending-v3-4-1cca847c6f9f@samsung.com> On 6/17/25 16:28, Danilo Krummrich wrote: > On Tue, Jun 17, 2025 at 04:07:27PM +0200, Michal Wilczynski wrote: >> + fn write_waveform( >> + chip: &mut pwm::Chip, >> + pwm: &mut pwm::Device, > > I think you can't hand out mutable references here. This would allow things like > mem::swap(), which I think are not valid on those structures. > >> + wfhw: &Self::WfHw, >> + parent_dev: &Device, >> + ) -> Result { >> + let data: &Self = chip.drvdata().ok_or(EINVAL)?; >> + let hwpwm = pwm.hwpwm(); >> + let iomem_guard = data.iomem.access(parent_dev)?; > > Technically, this isn't a guard, hence would't call it that way. > >> + let iomap = iomem_guard.deref(); >> + let was_enabled = pwm.state().enabled(); >> + >> + if !wfhw.enabled { >> + if was_enabled { >> + iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?; >> + iomap.try_write32(0, th1520_pwm_fp(hwpwm))?; >> + iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?; >> + } >> + return Ok(()); >> + } >> + >> + iomap.try_write32(wfhw.ctrl_val, th1520_pwm_ctrl(hwpwm))?; >> + iomap.try_write32(wfhw.period_cycles, th1520_pwm_per(hwpwm))?; >> + iomap.try_write32(wfhw.duty_cycles, th1520_pwm_fp(hwpwm))?; >> + iomap.try_write32(wfhw.ctrl_val | PWM_CFG_UPDATE, th1520_pwm_ctrl(hwpwm))?; > > None of the offsets are known at compile time? :( Sadly they are computed based on runtime parameter hwpwm, so Rust can't guarantee correctness during compilation :-(. Thank you for your other feedback, appreciate it ! > >> + >> + // The `PWM_START` bit must be written in a separate, final transaction, and >> + // only when enabling the channel from a disabled state. >> + if !was_enabled { >> + iomap.try_write32(wfhw.ctrl_val | PWM_START, th1520_pwm_ctrl(hwpwm))?; >> + } >> + >> + dev_dbg!( >> + chip.device(), >> + "PWM-{}: Wrote (per: {}, duty: {})", >> + hwpwm, >> + wfhw.period_cycles, >> + wfhw.duty_cycles, >> + ); >> + >> + Ok(()) >> + } >> +} > > > >> +impl platform::Driver for Th1520PwmPlatformDriver { >> + type IdInfo = (); >> + const OF_ID_TABLE: Option> = Some(&OF_TABLE); >> + >> + fn probe( >> + pdev: &platform::Device, >> + _id_info: Option<&Self::IdInfo>, >> + ) -> Result>> { >> + let dev = pdev.as_ref(); >> + let resource = pdev.resource(0).ok_or(ENODEV)?; >> + let iomem = pdev.ioremap_resource_sized::(resource)?; >> + let clk = Clk::get(pdev.as_ref(), None)?; >> + >> + clk.prepare_enable()?; >> + >> + // TODO: Get exclusive ownership of the clock to prevent rate changes. >> + // The Rust equivalent of `clk_rate_exclusive_get()` is not yet available. >> + // This should be updated once it is implemented. >> + let rate_hz = clk.rate().as_hz(); >> + if rate_hz == 0 { >> + dev_err!(dev, "Clock rate is zero\n"); >> + return Err(EINVAL); >> + } >> + >> + if rate_hz > time::NSEC_PER_SEC as usize { >> + dev_err!( >> + dev, >> + "Clock rate {} Hz is too high, not supported.\n", >> + rate_hz >> + ); >> + return Err(ERANGE); >> + } >> + >> + let chip = pwm::Chip::new(dev, MAX_PWM_NUM, 0)?; >> + >> + let drvdata = KBox::new(Th1520PwmDriverData { iomem, clk }, GFP_KERNEL)?; >> + chip.set_drvdata(drvdata); >> + >> + let registration = pwm::Registration::new(chip, &TH1520_PWM_OPS)?; >> + >> + Ok(KBox::new( >> + Th1520PwmPlatformDriver { >> + _registration: registration, >> + }, >> + GFP_KERNEL, >> + )? >> + .into()) > > Here you are setting up the registration for the correct lifetime, however > drivers could extend the lifetime of the registration arbitrarily, which would > break the guarantee of the &Device we rely on in the callbacks above > (e.g. write_waveform()). > > Hence, pwm::Registration's lifetime has to be controlled by Devres. I'll also > add a corresponding comment in your registration patch. > >> + } >> +} > Best regards, -- Michal Wilczynski