From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj2-f43.google.com (mail-pj2-f43.google.com [74.125.227.171]) (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 305C74A9D5B for ; Sat, 3 Oct 2026 16:45:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.227.171 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1791045936; cv=none; b=azqHIFv2/R4oPa3jDZNvzF2vGqt8QfUfYAjpXlPp+sKYNMcCaRQD4c5GdTOvVtRJiArGslLv1/t4FHYrAnSbThmm5PNzB3+UXWCuSOwEvP3Wy/oJNSSSYu0/gbwTuPzOSXYgZF36VDTO6i0vr2UorpaXs+orImuxNUPg6UWVI9w= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1791045936; c=relaxed/simple; bh=Z5+z7hNiBBNdVlbXFF76RV5SnZkdJjiqvRRCBvSiz5M=; h=Message-ID:Date:From:To:Cc:Subject:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ZL+RygwADHd5S5F1xxtI+oFxZoYG8MheV7JyC7pjXBOl4eVhyUpTmMHMaDWE6n/+V2HxGGLaoyKGF1JNpPz4cEX4gPFUJv970m+bcI9yH1gSnL0eaduPWit8Pz4tlavQyC97CgXcFFG4x+ttHJDdX0Ofhm9hH9U+Qs2WeHeowpM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=fFphFe5S; arc=none smtp.client-ip=74.125.227.171 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="fFphFe5S" Received: by mail-pj2-f43.google.com with SMTP id 98e67ed59e1d1-3a49573b8bdso302004a91.2 for ; Sat, 03 Oct 2026 09:45:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1791045933; x=1791650733; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:subject:cc:to:from:date:message-id:from:to:cc:subject :date:message-id:reply-to:content-type; bh=XwUxbbpJnDbzL/dxfGXuu2rEGUNZ/u1k+T1w6QXAexY=; b=fFphFe5SokgvKiDThGaJO86hQ7NU1zNQpWyo7Viy3haREWJ9yV9Xnx55mBbQXgksuw fnYpKGgGOoNpudSycsm5bwI+seS6ZwGvuUCJPWx3GxlQWvt47eK8PSFg++Dm1+ue6UTV z88C0e3wXq+OtIHy8NBCVriVTJ/L/j4QPpXQEIW3Uh4j8Fmb+Pz6/Ds5eKenhO7RCVcH SwlGAcvVjxmh8veyzgVMQSpzuAM6aVkRlwUE1JjT+mVrnztHCuBQq/0WNQZizZcGzt+6 wcf4g4vLqyOw/1/anbiNzRJCPc8dRHxs2MHIpxAYwVSiEXwqa84mLzUEjPD/vuRJhMpi mYww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1791045933; x=1791650733; h=in-reply-to:content-disposition:content-type:mime-version :references:subject:cc:to:from:date:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=XwUxbbpJnDbzL/dxfGXuu2rEGUNZ/u1k+T1w6QXAexY=; b=nLfFbZN8jqHo5F9faVUSPZYS3Kz+u7ThkZG8W0L977WhR1C1ZsWZt5CHQCH52Ekv3q OpG6L3Q7D8fb/D0tB0PRc9O16iURK4ZmjbzlepAnUb/hTca8XpDwRcwV1E4mCFXyX0+U esC7d4s+r0oKTDMRDZ/0Fts0d8Jg+Uk70rHaQ/AEzF5U19415qSOP6b8/2sLXuGKk98d 3FCKhUuxIkI4LDqst9bPyyXSIukpXpIKGMp4eugJfzYVtDIO7PQfRJ297OCdZh5YAO7V 1ick5jU9fKyGDnbGfpKPbd60+Pp47t50qn80fEsFsrabUbdOLJmLJW9OGQ7SW9x2ymPb ltjQ== X-Forwarded-Encrypted: i=1; AKwUvBx+dBgEv19QA0NPNDrjeHoEdyGSVjNO2sqd0GLS8i9W1goNDvklsCRNH6reV5hteAPcAIth1ik2bzTeX9E=@vger.kernel.org X-Gm-Message-State: AFq9FYJHKcOFh8M2bKNNqxVK6JX8gkPasr3Vy4KdtMrdk0RAwxNKy46N ZpCcLxPf9V61SN0hTxmZRCan0MK6B+26Pobu8e+F/V/CTjlzcAfZnkFg X-Gm-Gg: AYBFou0OxBVL69xNUq/fCKOHBQAVrJb7Z5oPu16ovtOZeH6EmMWWRmskRFgpEIbAM08 iXiyTO1O+Kh1KiWbUH0r32J0RU5aM+urpv30ykYQfMbdubDQ+UhsJXfGy3AYk2GGm527+zyTkq0 Rq+k86NGH4jRGyyARBnNsPQhoKHRk+dHUVXT7TpczGd5sAf5YgHccpLv0L/RTZUS5LMFOeA4oO9 v/X6HsVGAuwnnOOKbwSW1NANoHBgoOUFgZiwpHmdDDtgqDpZK/42Gnk0kxiT+tadQ94oWgzBkzU qSZpADtZxxPZy84U29OWKgyBG6CoHIzaiSHSx4uo1LkFVHzNf4qBuBw8kSX6xrHEwPgQHmiPq4r j64P9OU2ecAhX6MGmnZTLZxHqgWLrU6hj+OV2u0hlH1L5zvwGzXgMtidG7MCqkHrCSNntZlfV2s yMihnjGbvmonFCPR0nzoZvkcVfpkhYmHqAFafOtY8ZGEE2kkKcCRLNH2j5laUTNR20GSokDWDqv jbLkmwmHUf6u68Kmw== X-Received: by 2002:a17:90a:a82:b0:3a6:e159:a233 with SMTP id 98e67ed59e1d1-3a6e159b3e8mr3430086a91.43.1791045933065; Sat, 03 Oct 2026 09:45:33 -0700 (PDT) Received: from localhost (madb688426.ap.nuro.jp. [219.104.132.38]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-3a78d6878d2sm3528085a91.8.2026.10.03.09.45.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 03 Oct 2026 09:45:32 -0700 (PDT) Message-ID: <6ac1312c.ee551989.ce4ed.e56f@mx.google.com> X-Google-Original-Message-ID: <20261003164530.ay47jwe3xjuyfmsn@DESKTOP-1P5QNTF.> Date: Sun, 4 Oct 2026 01:45:30 +0900 From: Kohei Ito To: Alexandre Courbot Cc: Miguel Ojeda , Boqun Feng , Gary Guo , =?utf-8?B?QmrDtnJu?= Roy Baron , Benno Lossin , Andreas Hindborg , Alice Ryhl , Trevor Gross , Danilo Krummrich , Daniel Almeida , Tamir Duberstein , Onur =?utf-8?B?w5Z6a2Fu?= , linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, linux-gpio@vger.kernel.org Subject: Re: [PATCH 2/3] rust: gpio: Add basic consumer abstractions References: <20260906-add-rust-gpio-consumer-v1-0-24d192f93760@gmail.com> <20260906-add-rust-gpio-consumer-v1-2-24d192f93760@gmail.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=utf-8 Content-Disposition: inline In-Reply-To: Hi Alexandre, thank you for your review. > > Due to a bindgen issue that may generate the wrong type for enum types, > > `gpio/consumer.h` is included at the top of `bindings_helper.h` as a > > temporary workaround. Once the issue is resolved, it can be moved back > > to its proper alphabetical position. > > Can you describe what the issue is, and share any relevant link? `bindgen` can generate the wrong type for the `enum`s when their forward declarations appear before the actual definitions. The details are described in [1]. I haven't confirmed that `gpiod_flags` is actually affected. I placed the include at the top as a precaution, but if it isn't needed I'll drop the workaround. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8cbc95f983bcec7e042266766ffe0d68980e4290 > > +/// The GPIO descriptor flags to configure its direction and output value. > > +/// > > +/// Rust abstraction for the C [`enum gpiod_flags`]. > > +/// > > +/// They can be combined with the operators `|`, and `&`. > > The C comment for `gpiod_flags` says "these values cannot be OR'd" so I > guess this comment isn't true. Besides, there is no `BitOr` impl for > `GpiodFlags` in the patch so it actually cannot be done. Good catch! I'll correct it. > > + // Always inline to optimize out error path of `build_assert`. > > + #[inline(always)] > > + const fn new(value: bindings::gpiod_flags) -> Self { > > + build_assert!(value as u64 <= bindings::gpiod_flags::MAX as u64); > > Better to not use `build_assert` here as it inserts build-time > landmines. > > Since you are only using this to build the constants above, you can just > do `Self(bindings::gpiod_flags_*)` on them. Adding an extra assert for > an bounded enum type doesn't add any extra protection. Sure. I'll remove it. > > +/// A reference-counted gpio descriptor. > > Not really - the GPIO device is reference-counted, but descriptors are > not. Calling `gpiod_get` a second time returns `EBUSY`. Sure. I'll correct it. > > +/// ``` > > +/// use crate::{ > > These doctests won't compile as they are supposed to use `kernel::`, not > `crate::`. > > Please make sure to include the doctests when building > (`CONFIG_RUST_KERNEL_DOCTESTS` build option), and to also build the > `rustdoc` target as per the checklist [1]. > > [1] https://rust-for-linux.com/contributing#submit-checklist-addendum Sure. I'll fix it and make sure to run the doctests and build rustdoc. > > +// SAFETY: It is safe to call `gpiod_put` on another thread than where `gpiod_get` was called. > > +unsafe impl Send for GpioDesc {} > > We should probably also implement `Sync` so GPIOs can be used in > interrupt context. I agree we want `Sync` so that GPIOs can be used from interrupt context. However, the direction setters are not safe to call concurrently on the same descriptor: gpiolib changes the hardware direction and then updates `GPIOD_FLAG_IS_OUT`, so the two can become inconsistent. I think we can add `Sync` if the direction setters take `&mut self` (or use the typestate approach). I'll look into this together with the typestate design. > > + > > +impl GpioDesc { > > + /// Gets [`GpioDesc`] corresponding to a [`Device`] and a connection id. > > + /// > > + /// Equivalent to the kernel's [`gpiod_get`] API. > > + /// > > + /// [`gpiod_get`]: https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_get > > + pub fn get(dev: &Device, name: Option<&CStr>, flags: GpiodFlags) -> Result { > > `dev` here is only used as a lookup key, and the GPIO descriptor can > outlive the device being unbound (the GPIO can actually even be obtained > while the device is unbound!). This is because `dev` is not the provider > of the GPIO, but as the API name implies its consumer - i.e. the device > on which the GPIO is expected to have an effect. > > This is what the GPIO API expects, but it looks a bit counterintuitive > when compared to most other Rust subsystems, where an obtained resource > is typically tied to the device given as parameter being bound. I think > it's worth mentioning in the comment. Sure. I'll add a note explaining how `dev` is used. > > + /// Get the direction. > > + /// > > + /// Equivalent to the kernel's [`gpiod_get_direction`] API. > > + /// > > + /// [`gpiod_get_direction`]: > > + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_get_direction > > + #[inline] > > + pub fn get_direction(&self) -> Result { > > + // SAFETY: By the type invariants, self.as_raw() is a valid argument for > > + // [`gpiod_get_direction`]. > > + let ret = unsafe { bindings::gpiod_get_direction(self.as_raw()) }; > > + if ret < 0 { > > + Err(Error::from_errno(ret)) > > + } else { > > + LineDirection::try_from(ret) > > + } > > + } > > IIUC the direction of a GPIO at a given point in the code is always > statically known, and only a subset of the API really make sense for a > given direction (e.g. `gpiod_set_raw_value_commit` returns `EPERM` if > the direction is not output). So this is a prime candidate for using the > typestate pattern to store the direction in the type. > > I.e. you would have `GpioDesc`, `GpioDesc`, and changing > the direction would consume the descriptor and return the new one with > the requested direction. > > The regulator Rust API makes use of this pattern, you can check it out > for an example if needed. I haven't fully grasped the idea yet. I'll check the regulator Rust API implementation and explore a typestate implementation for GPIO consumer APIs. > > + /// Test whether the GPIO is active-low or not. > > + /// > > + /// Equivalent to the kernel's [`gpiod_is_active_low`] API. > > + /// > > + /// [`gpiod_is_active_low`]: > > + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_is_active_low > > + #[inline] > > + pub fn is_active_low(&self) -> Result { > > + // SAFETY: By the type invariants, self.as_raw() is a valid argument for > > + // [`gpiod_is_active_low`]. > > + match unsafe { bindings::gpiod_is_active_low(self.as_raw()) } { > > + 0 => Ok(false), > > + 1 => Ok(true), > > + err => Err(Error::from_errno(err)), > > + } > > In C this function cannot fail for a valid descriptor, so the Rust one > shouldn't either. Anything != 0 can be considered `true`. Sure. I'll correct it for `GpioDesc`. Should we return Result for OptionalGpioDesc? As I understand, NULL GPIO descriptors can't return the right state. > > + /// Report whether gpio value access may sleep or not. > > + /// > > + /// Equivalent to the kernel's [`gpiod_cansleep`] API. > > + /// > > + /// [`gpiod_cansleep`]: > > + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_cansleep > > + #[inline] > > + pub fn cansleep(&self) -> Result { > > + // SAFETY: By the type invariants, self.as_raw() is a valid argument for > > + // [`gpiod_cansleep`]. > > + match unsafe { bindings::gpiod_cansleep(self.as_raw()) } { > > + 0 => Ok(false), > > + 1 => Ok(true), > > + err => Err(Error::from_errno(err)), > > + } > > + } > > Same here. Sure. I'll fix it in the same way. > Also, as a general guideline, it is good to have a concrete user for new > Rust abstractions. Do you have a project that will make use of this? No, I don't have a specific project that will use this. My motivation is that Rust drivers currently have no way to use GPIO lines, so I expect that providing a basic set of consumer APIs would make it easier for such drivers to appear. That said, I understand the concern about adding APIs without actual users. If you think it should wait until there is a concrete user, please let me know. Best regards, Kohei Ito