From: "Alexandre Courbot" <acourbot@nvidia.com>
To: "Kohei Ito" <koheiito.dev@gmail.com>
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
"Boqun Feng" <boqun@kernel.org>, "Gary Guo" <gary@garyguo.net>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Benno Lossin" <lossin@kernel.org>,
"Andreas Hindborg" <a.hindborg@kernel.org>,
"Alice Ryhl" <aliceryhl@google.com>,
"Trevor Gross" <tmgross@umich.edu>,
"Danilo Krummrich" <dakr@kernel.org>,
"Daniel Almeida" <daniel.almeida@collabora.com>,
"Tamir Duberstein" <tamird@kernel.org>,
"Onur Özkan" <work@onurozkan.dev>,
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
Date: Sun, 13 Sep 2026 17:58:43 +0900 [thread overview]
Message-ID: <DLE2CBE8FJ9B.23HDU37C95ROK@nvidia.com> (raw)
In-Reply-To: <20260906-add-rust-gpio-consumer-v1-2-24d192f93760@gmail.com>
On Sun Sep 6, 2026 at 5:45 PM JST, Kohei Ito wrote:
> Add basic abstractions for GPIO consumer APIs.
Wow, GPIO! That brings some good memories back. :_)
>
> 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?
>
> Signed-off-by: Kohei Ito <koheiito.dev@gmail.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/gpio.rs | 2 +
> rust/kernel/gpio/consumer.rs | 437 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 440 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 98b048b36771..30985c102c70 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -26,6 +26,7 @@
> * This workaround may not be possible in some cases, depending on how the C
> * headers are set up.
> */
> +#include <linux/gpio/consumer.h>
> #include <linux/hrtimer_types.h>
>
> #include <linux/acpi.h>
> diff --git a/rust/kernel/gpio.rs b/rust/kernel/gpio.rs
> index 819efc8a0c05..40b6c64e8f5e 100644
> --- a/rust/kernel/gpio.rs
> +++ b/rust/kernel/gpio.rs
> @@ -11,6 +11,8 @@
> prelude::*, //
> };
>
> +pub mod consumer;
> +
> /// Describes GPIO direction.
> #[derive(Clone, Copy, PartialEq, Eq)]
> #[repr(u32)]
> diff --git a/rust/kernel/gpio/consumer.rs b/rust/kernel/gpio/consumer.rs
> new file mode 100644
> index 000000000000..f81c7381c075
> --- /dev/null
> +++ b/rust/kernel/gpio/consumer.rs
> @@ -0,0 +1,437 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// This file is based on rust/kernel/clk.rs.
> +
> +//! GPIO consumer abstractions.
> +//!
> +//! C header: [`include/linux/gpio/consumer.h`](srctree/include/linux/gpio/consumer.h)
> +//!
> +//! Reference: <https://docs.kernel.org/driver-api/gpio/consumer.html>
> +
> +use crate::{
> + device::Device,
> + error::{
> + from_err_ptr,
> + to_result,
> + Error,
> + Result, //
> + },
> + gpio::{
> + LineDirection,
> + LogicalLineLevel,
> + PhysicalLineLevel, //
> + },
> + prelude::*, //
> +};
> +
> +use core::{ops::Deref, ptr};
> +
> +/// 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.
> +///
> +/// Values can be used from the associated constants such as
> +/// [`Flags::GPIOD_ASIS`].
> +#[derive(Clone, Copy, PartialEq)]
> +pub struct GpiodFlags(bindings::gpiod_flags);
> +
> +impl GpiodFlags {
> + /// Don't change anything.
> + pub const ASIS: Self = Self::new(bindings::gpiod_flags_GPIOD_ASIS);
> +
> + /// Set lines to input mode.
> + pub const IN: Self = Self::new(bindings::gpiod_flags_GPIOD_IN);
> +
> + /// Set lines to output and drive them low.
> + pub const OUT_LOW: Self = Self::new(bindings::gpiod_flags_GPIOD_OUT_LOW);
> +
> + /// Set lines to output and drive them high.
> + pub const OUT_HIGH: Self = Self::new(bindings::gpiod_flags_GPIOD_OUT_HIGH);
> +
> + /// Set lines to open-drain output and drive them low.
> + pub const OUT_LOW_OPEN_DRAIN: Self = Self::new(bindings::gpiod_flags_GPIOD_OUT_LOW_OPEN_DRAIN);
> +
> + /// Set lines to open-drain output and drive them high.
> + pub const OUT_HIGH_OPEN_DRAIN: Self =
> + Self::new(bindings::gpiod_flags_GPIOD_OUT_HIGH_OPEN_DRAIN);
> +
> + fn into_inner(self) -> bindings::gpiod_flags {
> + self.0
> + }
> +
> + // 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.
> + Self(value)
> + }
> +}
> +
> +/// 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`.
> +///
> +/// Rust abstraction for the C [`struct gpio_desc`].
> +///
> +/// # Invariants
> +///
> +/// A [`GpioDesc`] instance holds either a pointer to a valid [`struct gpio_desc`] created by the C
> +/// portion of the kernel or a `NULL` pointer.
> +///
> +/// Instances of this type are reference-counted. Calling [`GpioDesc::get`] ensures that the
> +/// allocation remains valid for the lifetime of the [`GpioDesc`].
> +///
> +/// # Examples
> +///
> +/// The following example demonstrates how to obtain a GPIO line for a device.
> +///
> +/// ```
> +/// 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
> +/// device::Device,
> +/// error::Result,
> +/// gpio::{
> +/// consumer::{
> +/// GpioDesc,
> +/// GpiodFlags, //
> +/// },
> +/// LogicalLineLevel, //
> +/// }, //
> +/// };
> +///
> +/// fn examine_gpio(dev: &Device) -> Result {
> +/// let gpiod = GpioDesc::get(dev, Some(c"reset"), GpiodFlags::ASIS)?;
> +///
> +/// gpiod.set_value(LogicalLineLevel::Inactive)?;
> +///
> +/// gpiod.set_value(LogicalLineLevel::Active)?;
> +///
> +/// Ok(())
> +/// }
> +/// ```
> +///
> +/// [`struct gpio_desc`]: https://docs.kernel.org/driver-api/gpio/consumer.html
> +#[repr(transparent)]
> +pub struct GpioDesc(*mut bindings::gpio_desc);
> +
> +// 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.
> +
> +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<Self> {
`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.
> + let con_id = name.map_or(ptr::null(), |n| n.as_char_ptr());
> +
> + // SAFETY: It is safe to call [`gpiod_get`] for a valid device pointer.
> + //
> + // INVARIANT: The reference-count is decremented when [`GpioDesc`] goes out of scope.
> + Ok(Self(from_err_ptr(unsafe {
> + bindings::gpiod_get(dev.as_raw(), con_id, flags.into_inner())
> + })?))
> + }
> +
> + /// Obtain the raw [`struct gpio_desc`] pointer.
> + #[inline]
> + fn as_raw(&self) -> *mut bindings::gpio_desc {
> + self.0
> + }
> +
> + /// 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<LineDirection> {
> + // 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<Input>`, `GpioDesc<Output>`, 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.
> +
> + /// Set the GPIO direction to input.
> + ///
> + /// Equivalent to the kernel's [`gpiod_direction_input`] API.
> + ///
> + /// [`gpiod_direction_input`]:
> + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_direction_input
> + #[inline]
> + pub fn direction_input(&self) -> Result {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_direction_input`].
> + to_result(unsafe { bindings::gpiod_direction_input(self.as_raw()) })
> + }
> +
> + /// Set the GPIO direction to output and assign the logical value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_direction_output`] API.
> + ///
> + /// [`gpiod_direction_output`]:
> + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_direction_output
> + #[inline]
> + pub fn direction_output(&self, value: LogicalLineLevel) -> Result {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_direction_output`].
> + to_result(unsafe { bindings::gpiod_direction_output(self.as_raw(), value.as_c_int()) })
> + }
> +
> + /// Set the GPIO direction to output and assign the physical value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_direction_output_raw`] API.
> + ///
> + /// [`gpiod_direction_output_raw`]:
> + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_direction_output_raw
> + #[inline]
> + pub fn direction_output_raw(&self, value: PhysicalLineLevel) -> Result {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_direction_output_raw`].
> + to_result(unsafe { bindings::gpiod_direction_output_raw(self.as_raw(), value.as_c_int()) })
> + }
> +
> + /// Get the logical GPIO value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_get_value`] API.
> + ///
> + /// [`gpiod_get_value`]: https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_get_value
> + #[inline]
> + pub fn get_value(&self) -> Result<LogicalLineLevel> {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_get_value`].
> + let ret = unsafe { bindings::gpiod_get_value(self.as_raw()) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + LogicalLineLevel::try_from(ret)
> + }
> + }
> +
> + /// Assign the logical value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_set_value`] API.
> + ///
> + /// [`gpiod_set_value`]: https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_set_value
> + #[inline]
> + pub fn set_value(&self, value: LogicalLineLevel) -> Result {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_set_value`].
> + to_result(unsafe { bindings::gpiod_set_value(self.as_raw(), value.as_c_int()) })
> + }
> +
> + /// Get the physical GPIO value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_get_raw_value`] API.
> + ///
> + /// [`gpiod_get_raw_value`]:
> + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_get_raw_value
> + #[inline]
> + pub fn get_raw_value(&self) -> Result<PhysicalLineLevel> {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_get_raw_value`].
> + let ret = unsafe { bindings::gpiod_get_raw_value(self.as_raw()) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + PhysicalLineLevel::try_from(ret)
> + }
> + }
> +
> + /// Assign the physical value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_set_raw_value`] API.
> + ///
> + /// [`gpiod_set_raw_value`]:
> + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_set_raw_value
> + #[inline]
> + pub fn set_raw_value(&self, value: PhysicalLineLevel) -> Result {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_set_raw_value`].
> + to_result(unsafe { bindings::gpiod_set_raw_value(self.as_raw(), value.as_c_int()) })
> + }
> +
> + /// Get the logical GPIO value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_get_value_cansleep`] API.
> + ///
> + /// [`gpiod_get_value_cansleep`]:
> + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_get_value_cansleep
> + #[inline]
> + pub fn get_value_cansleep(&self) -> Result<LogicalLineLevel> {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_get_value_cansleep`].
> + let ret = unsafe { bindings::gpiod_get_value_cansleep(self.as_raw()) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + LogicalLineLevel::try_from(ret)
> + }
> + }
Here as well it would have been nice if we could avoid having
`_cansleep` variants, but I am not sure there is anything we can do for
that so I guess we'll need to keep all the variants.
> +
> + /// Assign the logical value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_set_value_cansleep`] API.
> + ///
> + /// [`gpiod_set_value_cansleep`]:
> + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_set_value_cansleep
> + #[inline]
> + pub fn set_value_cansleep(&self, value: LogicalLineLevel) -> Result {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_set_value_cansleep`].
> + to_result(unsafe { bindings::gpiod_set_value_cansleep(self.as_raw(), value.as_c_int()) })
> + }
> +
> + /// Get the physical GPIO value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_get_raw_value_cansleep`] API.
> + ///
> + /// [`gpiod_get_raw_value_cansleep`]:
> + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_get_raw_value_cansleep
> + #[inline]
> + pub fn get_raw_value_cansleep(&self) -> Result<PhysicalLineLevel> {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_get_raw_value_cansleep`].
> + let ret = unsafe { bindings::gpiod_get_raw_value_cansleep(self.as_raw()) };
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + PhysicalLineLevel::try_from(ret)
> + }
> + }
> +
> + /// Assign the physical value.
> + ///
> + /// Equivalent to the kernel's [`gpiod_set_raw_value_cansleep`] API.
> + ///
> + /// [`gpiod_set_raw_value_cansleep`]:
> + /// https://docs.kernel.org/driver-api/gpio/index.html#c.gpiod_set_raw_value_cansleep
> + #[inline]
> + pub fn set_raw_value_cansleep(&self, value: PhysicalLineLevel) -> Result {
> + // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> + // [`gpiod_set_raw_value_cansleep`].
> + to_result(unsafe {
> + bindings::gpiod_set_raw_value_cansleep(self.as_raw(), value.as_c_int())
> + })
> + }
> +
> + /// 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<bool> {
> + // 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`.
> + }
> +
> + /// 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<bool> {
> + // 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.
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?
next prev parent reply other threads:[~2026-09-13 8:58 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-06 8:45 [PATCH 0/3] rust: Add basic GPIO " Kohei Ito
2026-09-06 8:45 ` [PATCH 1/3] rust: gpio: add GPIO module with common definitions Kohei Ito
2026-09-06 9:56 ` Miguel Ojeda
2026-09-06 13:09 ` Gary Guo
2026-09-06 15:53 ` Kohei Ito
2026-09-06 8:45 ` [PATCH 2/3] rust: gpio: Add basic consumer abstractions Kohei Ito
2026-09-10 7:38 ` Bartosz Golaszewski
2026-09-13 8:58 ` Alexandre Courbot [this message]
2026-09-06 8:45 ` [PATCH 3/3] sample: rust: Add GPIO consumer sample driver Kohei Ito
2026-09-10 7:37 ` Bartosz Golaszewski
2026-09-13 8:46 ` Kohei Ito
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DLE2CBE8FJ9B.23HDU37C95ROK@nvidia.com \
--to=acourbot@nvidia.com \
--cc=a.hindborg@kernel.org \
--cc=aliceryhl@google.com \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun@kernel.org \
--cc=dakr@kernel.org \
--cc=daniel.almeida@collabora.com \
--cc=gary@garyguo.net \
--cc=koheiito.dev@gmail.com \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lossin@kernel.org \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=tamird@kernel.org \
--cc=tmgross@umich.edu \
--cc=work@onurozkan.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®