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 44BFA26B974 for ; Tue, 27 May 2025 12:45:01 +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=1748349904; cv=none; b=W/sDSPLV6s0rC7i0Bx2afn3eIf7GdLKl6soVkwgNYM3chk/ULw4iNHG8zQttS4f8+S2Mo11iX1Tq7ZSnFlEUky+v9lbePK6Q+yzOBOgzgxvyI8LiB47SuQRZYsoDd8i7bF5qvLzBrfyoLiYHr468JOeeCkgLic7C8LH3aU4HRvE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1748349904; c=relaxed/simple; bh=TEoSJj5pG0vAhGRt8hK9p/l05yeh2Xc+ll+l5NCgMYw=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:From:In-Reply-To: Content-Type:References; b=LM+8aYITk83IJfDjRxwlwYemQgv2l/MHytCLxUFHUdrMMPwOJy3+NDMdfRMF55NAPbIZU8TMNDr3UEO++hsRQCqwek7JI+yopPZd9/6+WIyif5P0woPm8of4MsZR0UB24zA3crcEh6vz64zievjxMYwGlCfu8jGlSR2kD5tv3iU= 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=LoaE5J21; 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="LoaE5J21" Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20250527124459euoutp0221cc7fe5102be3ec0c02eb454292d277~DYvh8Hk1u0821208212euoutp02_ for ; Tue, 27 May 2025 12:44:59 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20250527124459euoutp0221cc7fe5102be3ec0c02eb454292d277~DYvh8Hk1u0821208212euoutp02_ DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1748349899; bh=6MggDmUhFkhh0YiQ7gVTaOXRvovvaDJZMhoGzqh3YHw=; h=Date:Subject:To:Cc:From:In-Reply-To:References:From; b=LoaE5J21GKq0EsZodbMoZpJbZvU0a5uNptGWSRSgSekRchtrp2tIQ2LPkMcb9v3oF Pg+9Mszs4+XXEMBDXqEQwhypatxb3S7Kq8zQCZUIETx6Xg00huVKuyaBE73vDhBl7d hyfcKZOFoVLct4kcKiLIaiuT5nGdYZXao/PWF6+U= Received: from eusmtip1.samsung.com (unknown [203.254.199.221]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20250527124458eucas1p25fc02c83cc0febc8bfccb24116a38937~DYvhIbmaK2867528675eucas1p26; Tue, 27 May 2025 12:44:58 +0000 (GMT) Received: from [106.210.136.40] (unknown [106.210.136.40]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20250527124457eusmtip1e773e74c7b1946257de1f317c434a396~DYvgQd7GN1338813388eusmtip1I; Tue, 27 May 2025 12:44:57 +0000 (GMT) Message-ID: Date: Tue, 27 May 2025 14:44:57 +0200 Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH RFC 2/6] 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?= , Benno Lossin , 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 , 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 Content-Language: pl From: "Michal Wilczynski/Kernel (PLT) /SRPOL/Engineer/Samsung Electronics" In-Reply-To: Content-Transfer-Encoding: 8bit X-CMS-MailID: 20250527124458eucas1p25fc02c83cc0febc8bfccb24116a38937 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20250524211521eucas1p1929a51901c91d1a37e9f4c2da86ff7b0 X-EPHeader: CA X-CMS-RootMailID: 20250524211521eucas1p1929a51901c91d1a37e9f4c2da86ff7b0 References: <20250524-rust-next-pwm-working-fan-for-sending-v1-0-bdd2d5094ff7@samsung.com> <20250524-rust-next-pwm-working-fan-for-sending-v1-2-bdd2d5094ff7@samsung.com> W dniu 25.05.2025 o 14:03, Danilo Krummrich pisze: > On Sat, May 24, 2025 at 11:14:56PM +0200, Michal Wilczynski wrote: >> diff --git a/drivers/pwm/pwm_th1520.rs b/drivers/pwm/pwm_th1520.rs >> new file mode 100644 >> index 0000000000000000000000000000000000000000..4665e293e8d0bdc1a62a4e295cdaf4d47b3dd134 >> --- /dev/null >> +++ b/drivers/pwm/pwm_th1520.rs >> @@ -0,0 +1,272 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2025 Samsung Electronics Co., Ltd. >> +// Author: Michal Wilczynski >> + >> +//! Rust T-HEAD TH1520 PWM driver >> +use kernel::{c_st >> + >> +struct Th1520PwmChipData { >> + clk: Clk, >> + iomem: kernel::devres::Devres>, > Why IoMem<0>? If you put the expected memory region size for this chip instead > all your subsequent accesses can be iomem.write() / iomem.read() rather than the > fallible try_{read,write}() variants. The size of the memory region is not known at the compile time. Instead it's configured via Device Tree. I'm not sure why it should work differently in Rust ? > >> +impl Th1520PwmChipData { >> + fn _config( >> + &self, >> + hwpwm: u32, >> + duty_ns: u64, >> + period_ns: u64, >> + target_polarity: pwm::Polarity, >> + ) -> Result { >> + let regs = self.iomem.try_access().ok_or_else(|| { >> + pr_err!("PWM-{}: Failed to access I/O memory in _config\n", hwpwm); > Here and throughout the whole driver, please use the dev_*!() print macros. > Drivers have no reason to use the pr_*!() macros. > >> +impl pwm::PwmOps for Th1520PwmChipData { >> + // This driver implements get_state >> + fn apply( >> + pwm_chip_ref: &mut pwm::Chip, >> + pwm_dev: &mut pwm::Device, >> + target_state: &pwm::State, >> + ) -> Result { > I assume those callbacks can't race with pwmchip_remove() called from driver > remove()? I.e. the callbacks are guaranteed to complete before pwmchip_remove() > completes? Yeah this is my understanding as well - this is something that the PWM core should guarantee. Fairly recently there was a commit adding even more locking 1cc2e1faafb3 ("pwm: Add more locking") > > If so, this function signature can provide the parent device of the pwm::Chip as > device::Device reference. > > This would allow you to access iomem more efficiently. > > Instead of > > data.iomem.try_access() > > you could do > > data.iomem.access(parent) // [1] > > which does get you rid of the atomic check and the RCU read side critical > section implied by try_access(). > > Actually, I should have added this comment and explanation to the abstraction > patch, but forgot about it. :) Thanks ! Appreciate your review ! Michał > > [1] https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/rust/kernel/devres.rs?ref_type=heads#L213 >