* [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions
@ 2024-12-18 23:36 Fabien Parent
2024-12-18 23:36 ` [PATCH 1/9] rust: i2c: add basic I2C client abstraction Fabien Parent
` (8 more replies)
0 siblings, 9 replies; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent, Fiona Behrens
This patch series adds a new regulator driver used by APQ8039 T2,
as well as all the Rust abstractions needed to write that driver.
This series is adding three abstractions:
* I2C: I do not expect this one to be too controversial since it is
closely following what is done for the platform abstraction.
* Regmap: This might be the abstraction that will trigger most of the
conversations since I took some liberties, and used what Rust has to
offer in order provide something that seems to me user-friendly while
also providing type-safety to ensure correct usage of the APIs.
A problem I faced is that 'struct regmap' is not reference counted,
and pointer to it are being held in several places. In C it is
mostly expected to use devm_ API during allocation and forget about
the pointer lifetime, but this does not seem idiomatic to rust since
one would expect that the `Regmap` structure to be deinited and
deallocated when dropped. I was hesitating between adding a refcount
to the C API, or maintain some form of refcount by wrapping the
structure into a Arc, and making sure that each abstraction for C code
that hold a pointer to a regmap would keep a Arc<Regmap> instance.
I chose to do the later for simplicity.
* Regulator driver: This one contains the bare minimum to write the
ncv6336 drivers. I also have an abstraction for the consumer part, but
it will come later in another patch series for another driver.
Dependencies:
[1] Entire series depends on the patch series from Danilo adding Device
and OF Rust abstractions: 2024121550-palpitate-exhume-348c@gregkh
[2] "rust: add abstraction for regmap" depends on genmask:
CAH5fLgjOKQdKViAGArMjvXjVmYpbo_sA8xoQGwu+DCW1FDFRXA@mail.gmail.com
[3] "rust: regulator: add support for regmap" depends on the Sealed trait:
20230224-rust-iopt-rtkit-v1-1-49ced3391295@asahilina.net
A while ago when Rust support was close to getting merged in the kernel
I decided to learn Rust to see what the fuss was about and see if I
could help contribute to it. Since then I've been working on a few drivers
during my free time, but my code was depending on too much out-of-tree
code. With the patch series [1] getting close to being merged, it was
finally time to send my code to the mailing list, so here it is.
Happy reviewing!
Fabien
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
Fabien Parent (8):
rust: add abstraction for regmap
rust: error: add declaration for ENOTRECOVERABLE error
rust: regulator: add abstraction for Regulator's modes
rust: regulator: add Regulator Driver abstraction
rust: regulator: add support for regmap
dt-bindings: regulator: add binding for ncv6336 regulator
regulator: add driver for ncv6336 regulator
arm64: dts: qcom: apq8039-t2: add node for ncv6336 regulator
Fiona Behrens (1):
rust: i2c: add basic I2C client abstraction
.../bindings/regulator/onnn,ncv6336.yaml | 54 +
MAINTAINERS | 4 +
arch/arm64/boot/dts/qcom/apq8039-t2.dts | 23 +
drivers/regulator/Kconfig | 7 +
drivers/regulator/Makefile | 1 +
drivers/regulator/ncv6336_regulator.rs | 241 +++++
rust/bindings/bindings_helper.h | 4 +
rust/helpers/helpers.c | 2 +
rust/helpers/i2c.c | 13 +
rust/helpers/regmap.c | 48 +
rust/kernel/error.rs | 1 +
rust/kernel/i2c.rs | 288 ++++++
rust/kernel/lib.rs | 6 +
rust/kernel/regmap.rs | 1043 ++++++++++++++++++++
rust/kernel/regulator.rs | 44 +
rust/kernel/regulator/driver.rs | 991 +++++++++++++++++++
16 files changed, 2770 insertions(+)
---
base-commit: 26033691d5821bb493dd3426140939d0a79cd480
change-id: 20241025-ncv6336-12324a94c693
prerequisite-message-id: <2024121550-palpitate-exhume-348c@gregkh>
prerequisite-patch-id: b798fb7308e85f8cd237e3de0c33f3079eec29b8
prerequisite-patch-id: ac93ef5057cbb09cf886223c4f10e53eeec1d234
prerequisite-patch-id: ea80287941ef43f59fa75a28e6798ff10c8e90c1
prerequisite-patch-id: 482d8f7c10b0b79fb5d06e3718fd6da0fdd0dfa2
prerequisite-patch-id: cd9756c9586f394e5b39198497979aa1384ad2b8
prerequisite-patch-id: 313083700e67eab809a6b673d1fd835a6bd18625
prerequisite-patch-id: d0c7198d84ffa229221bbe06541136c97e8aae4e
prerequisite-patch-id: 0c4e157879b92f366feca2e89b5719e0a9bfa36a
prerequisite-patch-id: 515464a50ad216e2e9811043db8ca341262db288
prerequisite-patch-id: c50da45a4d7e62930f78b854f9afc636120dc255
prerequisite-patch-id: 5e32316afbfed41fe68cc096bf5a93201d0c65f9
prerequisite-patch-id: 15b08041af5e8f50619e3098b01ac0c0c0357751
prerequisite-patch-id: 3feaab560cf92641ee1af4e07cd5cb2a50aa36a5
prerequisite-patch-id: 14a272276db1704d58a2afc140da51826347a90e
prerequisite-patch-id: c79292d0686dd843cf25f5a2300445e410e99d13
prerequisite-patch-id: ea5c28331c595ad00929291b483c8848f3ff84cf
prerequisite-patch-id: 69858ba82eb5852a4e7e36d084d2917ba4e8a6cd
prerequisite-patch-id: 3f1b33691d5ad742f689a90d41bfd37244ff4133
Best regards,
--
Fabien Parent <parent.f@gmail.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/9] rust: i2c: add basic I2C client abstraction
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
@ 2024-12-18 23:36 ` Fabien Parent
2024-12-19 13:03 ` Rob Herring
2024-12-18 23:36 ` [PATCH 2/9] rust: add abstraction for regmap Fabien Parent
` (7 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent, Fiona Behrens
From: Fiona Behrens <me@kloenk.dev>
Implement an abstraction to write I2C device drivers. The abstraction
is pretty basic and provides just the infrastructure to probe
a device from I2C/OF device_id and abstract `i2c_client`.
The client will be used by the Regmap abstraction to perform
I/O on the I2C bus.
Signed-off-by: Fiona Behrens <me@kloenk.dev>
Co-developed-by: Fabien Parent <fabien.parent@linaro.org>
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
MAINTAINERS | 1 +
rust/bindings/bindings_helper.h | 1 +
rust/helpers/helpers.c | 1 +
rust/helpers/i2c.c | 13 ++
rust/kernel/i2c.rs | 288 ++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
6 files changed, 306 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 6b9e10551392c185b9314c9f94edeaf6e85af58f..961fe4ed39605bf489d1d9e473f47bccb692ff14 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10796,6 +10796,7 @@ F: include/linux/i2c-smbus.h
F: include/linux/i2c.h
F: include/uapi/linux/i2c-*.h
F: include/uapi/linux/i2c.h
+F: rust/kernel/i2c.rs
I2C SUBSYSTEM HOST DRIVERS
M: Andi Shyti <andi.shyti@kernel.org>
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index e9fdceb568b8f94e602ee498323e5768a40a6cba..a882efb90bfc27960ef1fd5f2dc8cc40533a1c27 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -16,6 +16,7 @@
#include <linux/file.h>
#include <linux/firmware.h>
#include <linux/fs.h>
+#include <linux/i2c.h>
#include <linux/jiffies.h>
#include <linux/jump_label.h>
#include <linux/mdio.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e115be1553549312dcfdf842bcae3bde1b..630e903f516ee14a51f46ff0bcc68e8f9a64021a 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -15,6 +15,7 @@
#include "device.c"
#include "err.c"
#include "fs.c"
+#include "i2c.c"
#include "io.c"
#include "jump_label.c"
#include "kunit.c"
diff --git a/rust/helpers/i2c.c b/rust/helpers/i2c.c
new file mode 100644
index 0000000000000000000000000000000000000000..8ffdc454e7597cc61909da5b3597057aeb5f7299
--- /dev/null
+++ b/rust/helpers/i2c.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/i2c.h>
+
+void *rust_helper_i2c_get_clientdata(const struct i2c_client *client)
+{
+ return i2c_get_clientdata(client);
+}
+
+void rust_helper_i2c_set_clientdata(struct i2c_client *client, void *data)
+{
+ i2c_set_clientdata(client, data);
+}
diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
new file mode 100644
index 0000000000000000000000000000000000000000..efa03335e5b59e72738380e94213976b2464c25b
--- /dev/null
+++ b/rust/kernel/i2c.rs
@@ -0,0 +1,288 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the I2C bus.
+//!
+//! C header: [`include/linux/i2c.h`](srctree/include/linux/i2c.h)
+
+use crate::{
+ bindings, container_of,
+ device::Device,
+ device_id::{self, RawDeviceId},
+ driver,
+ error::{to_result, Result},
+ of,
+ prelude::*,
+ str::CStr,
+ types::{ARef, ForeignOwnable, Opaque},
+ ThisModule,
+};
+
+/// Abstraction for `bindings::i2c_device_id`.
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct DeviceId(bindings::i2c_device_id);
+
+impl DeviceId {
+ /// Create a new device id from an I2C name.
+ pub const fn new(name: &CStr) -> Self {
+ let src = name.as_bytes_with_nul();
+ // TODO: Replace with `bindings::i2c_device_id::default()` once stabilized for `const`.
+ // SAFETY: FFI type is valid to be zero-initialized.
+ let mut i2c: bindings::i2c_device_id = unsafe { core::mem::zeroed() };
+
+ let mut i = 0;
+ while i < src.len() {
+ i2c.name[i] = src[i] as _;
+ i += 1;
+ }
+
+ Self(i2c)
+ }
+}
+
+// SAFETY:
+// * `DeviceId` is a `#[repr(transparent)` wrapper of `i2c_device_id` and does not add
+// additional invariants, so it's safe to transmute to `RawType`.
+// * `DRIVER_DATA_OFFSET` is the offset to the `data` field.
+unsafe impl RawDeviceId for DeviceId {
+ type RawType = bindings::i2c_device_id;
+
+ const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::i2c_device_id, driver_data);
+
+ fn index(&self) -> usize {
+ self.0.driver_data as _
+ }
+}
+
+/// I2C [`DeviceId`] table.
+pub type IdTable<T> = &'static dyn device_id::IdTable<DeviceId, T>;
+
+/// An adapter for the registration of I2C drivers.
+#[doc(hidden)]
+pub struct Adapter<T: Driver + 'static>(T);
+
+impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+ type RegType = bindings::i2c_driver;
+
+ fn register(
+ i2cdrv: &Opaque<Self::RegType>,
+ name: &'static CStr,
+ module: &'static ThisModule,
+ ) -> Result {
+ // SAFETY: It's safe to set the fields of `struct i2c_driver` on initialization.
+ unsafe {
+ (*i2cdrv.get()).driver.name = name.as_char_ptr();
+ (*i2cdrv.get()).probe = Some(Self::probe_callback);
+ (*i2cdrv.get()).remove = Some(Self::remove_callback);
+ if let Some(t) = T::I2C_ID_TABLE {
+ (*i2cdrv.get()).id_table = t.as_ptr();
+ }
+ if let Some(t) = T::OF_ID_TABLE {
+ (*i2cdrv.get()).driver.of_match_table = t.as_ptr();
+ }
+ }
+
+ // SAFETY: `i2cdrv` is guaranteed to be a valid `RegType`.
+ to_result(unsafe { bindings::i2c_register_driver(module.0, i2cdrv.get()) })
+ }
+
+ fn unregister(i2cdrv: &Opaque<Self::RegType>) {
+ // SAFETY: `i2cdrv` is guaranteed to be a valid `RegType`.
+ unsafe { bindings::i2c_del_driver(i2cdrv.get()) };
+ }
+}
+
+impl<T: Driver> Adapter<T> {
+ /// Get the [`Self::IdInfo`] that matched during probe.
+ fn id_info(client: &mut Client) -> Option<&'static T::IdInfo> {
+ let id = <Self as driver::Adapter>::id_info(client.as_ref());
+ if id.is_some() {
+ return id;
+ }
+
+ // SAFETY: `client` and `client.as_raw()` are guaranteed to be valid.
+ let id = unsafe { bindings::i2c_client_get_device_id(client.as_raw()) };
+ if !id.is_null() {
+ // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct i2c_device_id` and
+ // does not add additional invariants, so it's safe to transmute.
+ let id = unsafe { &*id.cast::<DeviceId>() };
+ return Some(T::I2C_ID_TABLE?.info(id.index()));
+ }
+
+ None
+ }
+
+ extern "C" fn probe_callback(client: *mut bindings::i2c_client) -> core::ffi::c_int {
+ // SAFETY: The i2c bus only ever calls the probe callback with a valid `client`.
+ let dev = unsafe { Device::get_device(core::ptr::addr_of_mut!((*client).dev)) };
+ // SAFETY: `dev` is guaranteed to be embedded in a valid `struct i2c_client` by the
+ // call above.
+ let mut client = unsafe { Client::from_dev(dev) };
+
+ let info = Self::id_info(&mut client);
+ match T::probe(&mut client, info) {
+ Ok(data) => {
+ // Let the `struct i2c_client` own a reference of the driver's private data.
+ // SAFETY: By the type invariant `client.as_raw` returns a valid pointer to a
+ // `struct i2c_client`.
+ unsafe { bindings::i2c_set_clientdata(client.as_raw(), data.into_foreign() as _) };
+ }
+ Err(err) => return Error::to_errno(err),
+ }
+
+ 0
+ }
+
+ extern "C" fn remove_callback(client: *mut bindings::i2c_client) {
+ // SAFETY: `client` is a valid pointer to a `struct i2c_client`.
+ let ptr = unsafe { bindings::i2c_get_clientdata(client) };
+
+ // SAFETY: `remove_callback` is only ever called after a successful call to
+ // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
+ // `KBox<T>` pointer created through `KBox::into_foreign`.
+ let _ = unsafe { KBox::<T>::from_foreign(ptr) };
+ }
+}
+
+impl<T: Driver + 'static> driver::Adapter for Adapter<T> {
+ type IdInfo = T::IdInfo;
+
+ fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> {
+ T::OF_ID_TABLE
+ }
+}
+
+/// The I2C driver trait.
+///
+/// Drivers must implement this trait in order to get a i2c driver registered.
+///
+/// # Example
+///
+///```
+/// # use kernel::{bindings, c_str, i2c, of};
+/// #
+/// kernel::of_device_table!(
+/// OF_ID_TABLE,
+/// MODULE_OF_ID_TABLE,
+/// <MyDriver as i2c::Driver>::IdInfo,
+/// [(of::DeviceId::new(c_str!("onnn,ncv6336")), ()),]
+/// );
+///
+/// kernel::i2c_device_table!(
+/// I2C_ID_TABLE,
+/// MODULE_I2C_ID_TABLE,
+/// <MyDriver as i2c::Driver>::IdInfo,
+/// [(i2c::DeviceId::new(c_str!("ncv6336")), ()),]
+/// );
+///
+/// struct MyDriver;
+///
+/// impl i2c::Driver for MyDriver {
+/// type IdInfo = ();
+/// const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_ID_TABLE);
+/// const I2C_ID_TABLE: Option<i2c::IdTable<Self::IdInfo>> = Some(&I2C_ID_TABLE);
+///
+/// fn probe(_client: &mut i2c::Client,
+/// id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
+/// Ok(KBox::new(Self, GFP_KERNEL)?.into())
+/// }
+/// }
+///```
+pub trait Driver {
+ /// The type holding information about each device id supported by the driver.
+ // TODO: Use associated_type_defaults once stabilized:
+ // type IdInfo: 'static = ();
+ type IdInfo: 'static;
+
+ /// An optional table of I2C device ids supported by the driver.
+ const I2C_ID_TABLE: Option<IdTable<Self::IdInfo>>;
+
+ /// An optional table of OF device ids supported by the driver.
+ const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>>;
+
+ /// I2C driver probe.
+ ///
+ /// Called when a new I2C client is added or discovered.
+ fn probe(client: &mut Client, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
+}
+
+/// An I2C Client.
+///
+/// # Invariants
+///
+/// `Client` holds a valid reference of `ARef<device::Device>` whose underlying `struct device` is a
+/// member of a `struct i2c_client`.
+#[derive(Clone)]
+pub struct Client(ARef<Device>);
+
+impl Client {
+ /// Convert a raw kernel device into a `Client`
+ ///
+ /// # Safety
+ ///
+ /// `dev` must be an `Aref<Device>` whose underlying `bindings::device` is a member of a
+ /// `bindings::i2c_client`.
+ unsafe fn from_dev(dev: ARef<Device>) -> Self {
+ Self(dev)
+ }
+
+ /// Returns the raw `struct i2c_client`.
+ pub fn as_raw(&self) -> *mut bindings::i2c_client {
+ // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
+ // embedded in `struct i2c_client`.
+ unsafe { container_of!(self.0.as_raw(), bindings::i2c_client, dev) }.cast_mut()
+ }
+}
+
+impl AsRef<Device> for Client {
+ fn as_ref(&self) -> &Device {
+ &self.0
+ }
+}
+
+/// Declares a kernel module that exposes a single I2C driver.
+///
+/// # Examples
+///
+/// ```ignore
+/// kernel::module_i2c_driver! {
+/// type: MyDriver,
+/// name: "Module name",
+/// author: "Author name",
+/// description: "Description",
+/// license: "GPL v2",
+/// }
+/// ```
+#[macro_export]
+macro_rules! module_i2c_driver {
+ ($($f:tt)*) => {
+ $crate::module_driver!(<T>, $crate::i2c::Adapter<T>, { $($f)* });
+ };
+}
+
+/// Create an I2C `IdTable` with an "alias" for modpost.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::{c_str, i2c};
+///
+/// kernel::i2c_device_table!(
+/// I2C_ID_TABLE,
+/// MODULE_I2C_ID_TABLE,
+/// u32,
+/// [(i2c::DeviceId::new(c_str!("ncv6336")), 0x6336),]
+/// );
+/// ```
+#[macro_export]
+macro_rules! i2c_device_table {
+ ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => {
+ const $table_name: $crate::device_id::IdArray<
+ $crate::i2c::DeviceId,
+ $id_info_type,
+ { $table_data.len() },
+ > = $crate::device_id::IdArray::new($table_data);
+
+ $crate::module_device_table!("i2c", $module_table_name, $table_name);
+ };
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 7fb9858966e8457611d5868783000844ba640db9..71ef7df94302b689be665676a36bd5c2e6effff3 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -48,6 +48,8 @@
#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
pub mod firmware;
pub mod fs;
+#[cfg(CONFIG_I2C)]
+pub mod i2c;
pub mod init;
pub mod ioctl;
pub mod jump_label;
--
2.45.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/9] rust: add abstraction for regmap
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
2024-12-18 23:36 ` [PATCH 1/9] rust: i2c: add basic I2C client abstraction Fabien Parent
@ 2024-12-18 23:36 ` Fabien Parent
2024-12-20 13:16 ` Mark Brown
2024-12-18 23:36 ` [PATCH 3/9] rust: error: add declaration for ENOTRECOVERABLE error Fabien Parent
` (6 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
From: Fabien Parent <fabien.parent@linaro.org>
This regmap abstraction is implemented using only the regmap_field
APIs. This abstraction tries to bring some type-safety features and
ease of use by enhancing the regmap API through extensive macro use
to generate code.
The abstraction is bringing only a small subset of all the features
provided by regmap by only supporting the most vital field from
`struct regmap_config`.
This abstraction is used by the Regulator abstraction as well as the
following driver:
https://github.com/Fabo/linux/blob/b4/ncv6336/drivers/regulator/ncv6336_regulator.rs
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
MAINTAINERS | 1 +
rust/bindings/bindings_helper.h | 1 +
rust/helpers/helpers.c | 1 +
rust/helpers/regmap.c | 48 ++
rust/kernel/lib.rs | 2 +
rust/kernel/regmap.rs | 1043 +++++++++++++++++++++++++++++++++++++++
6 files changed, 1096 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 961fe4ed39605bf489d1d9e473f47bccb692ff14..acb3942eb1b66ec2bc09ac50f51c2054b7b45355 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19790,6 +19790,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
F: Documentation/devicetree/bindings/regmap/
F: drivers/base/regmap/
F: include/linux/regmap.h
+F: rust/kernel/regmap.rs
REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM
M: Bjorn Andersson <andersson@kernel.org>
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index a882efb90bfc27960ef1fd5f2dc8cc40533a1c27..48d2b91b34067e7e9ee9c64c2e42681e988e9aad 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -28,6 +28,7 @@
#include <linux/platform_device.h>
#include <linux/poll.h>
#include <linux/refcount.h>
+#include <linux/regmap.h>
#include <linux/sched.h>
#include <linux/security.h>
#include <linux/slab.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 630e903f516ee14a51f46ff0bcc68e8f9a64021a..f78371d1932939821ecc5f57b065bd63b8bc9dee 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -27,6 +27,7 @@
#include "rbtree.c"
#include "rcu.c"
#include "refcount.c"
+#include "regmap.c"
#include "security.c"
#include "signal.c"
#include "slab.c"
diff --git a/rust/helpers/regmap.c b/rust/helpers/regmap.c
new file mode 100644
index 0000000000000000000000000000000000000000..2426e563df339a9c7c9c52a72f9982eea5a0ed29
--- /dev/null
+++ b/rust/helpers/regmap.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+
+#if IS_BUILTIN(CONFIG_REGMAP_I2C)
+struct regmap *rust_helper_regmap_init_i2c(struct i2c_client *i2c,
+ const struct regmap_config *config)
+{
+ return regmap_init_i2c(i2c, config);
+}
+#endif
+
+int rust_helper_regmap_field_write(struct regmap_field *field, unsigned int val)
+{
+ return regmap_field_write(field, val);
+}
+
+int rust_helper_regmap_field_force_write(struct regmap_field *field,
+ unsigned int val)
+{
+ return regmap_field_force_write(field, val);
+}
+
+int rust_helper_regmap_field_update_bits(struct regmap_field *field,
+ unsigned int mask, unsigned int val)
+{
+ return regmap_field_update_bits(field, mask, val);
+}
+
+int rust_helper_regmap_field_set_bits(struct regmap_field *field,
+ unsigned int bits)
+{
+ return regmap_field_set_bits(field, bits);
+}
+
+int rust_helper_regmap_field_clear_bits(struct regmap_field *field,
+ unsigned int bits)
+{
+ return regmap_field_clear_bits(field, bits);
+}
+
+int rust_helper_regmap_field_force_update_bits(struct regmap_field *field,
+ unsigned int mask,
+ unsigned int val)
+{
+ return regmap_field_force_update_bits(field, mask, val);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 71ef7df94302b689be665676a36bd5c2e6effff3..456e979724d1079045cb157086ff2b2ed0fcca3b 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -66,6 +66,8 @@
pub mod prelude;
pub mod print;
pub mod rbtree;
+#[cfg(CONFIG_REGMAP)]
+pub mod regmap;
pub mod revocable;
pub mod security;
pub mod seq_file;
diff --git a/rust/kernel/regmap.rs b/rust/kernel/regmap.rs
new file mode 100644
index 0000000000000000000000000000000000000000..232fe93df769eee97966703e0ba92c969b8f506e
--- /dev/null
+++ b/rust/kernel/regmap.rs
@@ -0,0 +1,1043 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Register map access API.
+//!
+//! C header: [`include/linux/regmap.h`](srctree/include/linux/regmap.h)
+//!
+//! # Examples
+//!
+//! ```ignore
+//! regmap::define_regmap_field_descs!(FIELD_DESCS, {
+//! (pid, 0x3, READ, { value => raw([7:0], ro) }),
+//! (limconf, 0x16, RW, {
+//! rearm => bit(0, rw),
+//! rststatus => bit(1, rw),
+//! tpwth => enum([5:4], rw, {
+//! Temp83C = 0x0,
+//! Temp94C = 0x1,
+//! Temp105C = 0x2,
+//! Temp116C = 0x3,
+//! }),
+//! })
+//! });
+//!
+//! fn probe(client: &mut i2c::Client) -> Result {
+//! let config = regmap::Config::<AccessOps>::new(8, 8)
+//! .with_max_register(0x16)
+//! .with_cache_type(regmap::CacheType::RbTree);
+//! let regmap = regmap::Regmap::init_i2c(client, &config);
+//! let mut fields = regmap.alloc_fields(&FIELD_DESCS)?;
+//!
+//! dev_info!(client.as_ref(), "PID: {:#x}", pid::value::read(&mut fields)?);
+//! }
+//! ```
+
+use crate::{
+ bindings,
+ error::{code::*, to_result, Error, Result},
+ macros::paste,
+ sync::Arc,
+};
+#[cfg(CONFIG_REGMAP_I2C = "y")]
+use crate::{error::from_err_ptr, i2c};
+use core::{marker::PhantomData, ptr::NonNull};
+
+/// Type of caching
+#[repr(u32)]
+pub enum CacheType {
+ /// Don't cache anything
+ None = bindings::regcache_type_REGCACHE_NONE,
+ /// Use RbTree caching
+ RbTree = bindings::regcache_type_REGCACHE_RBTREE,
+ /// Use Flat caching
+ Flat = bindings::regcache_type_REGCACHE_FLAT,
+ /// Use Maple caching
+ Maple = bindings::regcache_type_REGCACHE_MAPLE,
+}
+
+/// Register map
+///
+/// Note for Rust abstractions using Regmap:
+/// Regmap C structure does not implement reference count, so in order to keep the abstractions
+/// safe it is essential to keep a `Arc<Regmap>` instance whenever the associated C API is holding
+/// on the `struct regmap` pointer.
+///
+/// # Invariants
+///
+/// * `self.0` is valid, non-zero, and the memory is owned by `self`.
+/// * This abstraction does not allow to disable regmap locking.
+pub struct Regmap(NonNull<bindings::regmap>);
+
+impl Regmap {
+ #[cfg(CONFIG_REGMAP_I2C = "y")]
+ /// Initialize a [`Regmap`] instance for an `i2c` client.
+ pub fn init_i2c<T: ConfigOps>(i2c: &i2c::Client, config: &Config<T>) -> Result<Self> {
+ // SAFETY: Type invariants guarantee that `i2c.as_raw` is valid and non-null and
+ // the Config type invariant guarantee that `config.raw` always contains valid data.
+ let regmap = from_err_ptr(unsafe { bindings::regmap_init_i2c(i2c.as_raw(), &config.raw) })?;
+
+ Ok(Regmap(NonNull::new(regmap).ok_or(EINVAL)?))
+ }
+
+ /// Return the raw pointer of this regmap.
+ pub fn as_raw(&self) -> *mut bindings::regmap {
+ self.0.as_ptr()
+ }
+}
+
+impl Drop for Regmap {
+ fn drop(&mut self) {
+ // SAFETY: By the type invariant, `self.as_raw` is a valid pointer and it can be freed
+ // because we own the memory.
+ unsafe { bindings::regmap_exit(self.as_raw()) }
+ }
+}
+
+// SAFETY: The type invariants guarantee that the memory of `bindings::regmap` is owned and
+// guarantee that the C API is using locked accesses.
+unsafe impl Send for Regmap {}
+
+/// Field Descriptors
+///
+/// FieldDescriptors can be created by calling the [`define_regmap_field_descs`] macro.
+///
+/// # Examples
+///
+/// ```ignore
+/// use kernel::regmap::{define_regmap_field_descs, Fields};
+///
+/// define_regmap_field_descs!(DESCS, {
+/// (pid, 0x3, READ, { value => raw([7:0], ro) })
+/// });
+///
+/// struct Registrations {
+/// fields: Fields<{ DESCS.len() }>,
+/// }
+/// ```
+pub struct FieldDescs<const N: usize>([bindings::reg_field; N]);
+
+impl<const N: usize> FieldDescs<N> {
+ // macro use only
+ #[doc(hidden)]
+ pub const fn new(fields: [bindings::reg_field; N]) -> Self {
+ Self(fields)
+ }
+
+ /// Number of fields being held by `FieldDescs<N>`
+ ///
+ /// This function can be used to retrieve the number of fields that were
+ /// created when calling [`define_regmap_field_descs`].
+ #[allow(clippy::len_without_is_empty)]
+ pub const fn len(&self) -> usize {
+ N
+ }
+}
+
+/// Regmap fields
+///
+/// # Invariants
+///
+/// `self.fields` array is garanteed to contains valid and non-null pointers.
+/// `self.fields[0]` memory is owned by `Fields`.
+/// `self.fields[*]` values cannot be modified.
+pub struct Fields<const N: usize> {
+ fields: [NonNull<bindings::regmap_field>; N],
+
+ // Each regmap_field hold a pointer to the `struct regmap` instance, so we need to keep a copy
+ // of the wrapper around.
+ _regmap: Arc<Regmap>,
+}
+impl<const N: usize> Fields<N> {
+ /// Allocate regmap [`Fields`]
+ ///
+ /// This function allocate regmap fields from the `reg_fields` descriptors
+ pub fn new(regmap: &Arc<Regmap>, descs: &'static FieldDescs<N>) -> Result<Self> {
+ let mut fields = [NonNull::<bindings::regmap_field>::dangling(); N];
+ // SAFETY:
+ // * [`Regmap`] type invariants guarantee that `Regmap::as_raw` returns a valid pointer.
+ // * `FieldDescs::<N>` is guaranteed to hold a valid array of size N.
+ to_result(unsafe {
+ bindings::regmap_field_bulk_alloc(
+ regmap.as_raw(),
+ fields.as_mut_ptr().cast(),
+ descs.0.as_ptr().cast(),
+ descs.0.len() as i32,
+ )
+ })?;
+
+ Ok(Fields {
+ fields,
+ _regmap: regmap.clone(),
+ })
+ }
+
+ /// Get field `index`
+ pub fn index(&mut self, index: usize) -> *mut bindings::regmap_field {
+ self.fields[index].as_ptr()
+ }
+
+ // macro use only
+ #[doc(hidden)]
+ pub fn read(&mut self, index: usize) -> Result<core::ffi::c_uint> {
+ let mut val = 0;
+
+ // Make sure we don't panic if the index is out of bound.
+ if index >= N {
+ return Err(EINVAL);
+ }
+
+ // SAFETY: By the type invariants, we are garanteed that all fields entries point
+ // to valid and initialized values, hence it is safe to make this FFI call.
+ let ret = unsafe { bindings::regmap_field_read(self.fields[index].as_ptr(), &mut val) };
+ if ret < 0 {
+ return Err(Error::from_errno(ret));
+ }
+
+ Ok(val)
+ }
+}
+
+impl<const N: usize> Drop for Fields<N> {
+ fn drop(&mut self) {
+ // SAFETY: Per type invariant, `self.fields[0].as_mut` is garanteed to be valid and
+ // are owned by `Fields`.
+ unsafe { bindings::regmap_field_bulk_free(core::ptr::from_mut(self.fields[0].as_mut())) }
+ }
+}
+
+// SAFETY: The type invariants guarantee that we own the `struct regmap_field` data and that they
+// cannot be modified after allocation, and _regmap is Send, so it is safe for `Fields` to be Send.
+unsafe impl<const N: usize> Send for Fields<N> {}
+
+macro_rules! config_with {
+ ($(#[$meta:meta])* $name:ident: $type:ty) => {
+ config_with!($(#[$meta])* $name: $type, $name);
+ };
+
+ ($(#[$meta:meta])* $name:ident: $type:ty, $e:expr) => {
+ paste! {
+ $(#[$meta])*
+ pub const fn [<with_$name>](mut self, $name: $type) -> Self {
+ self.raw.$name = $e;
+ self
+ }
+ }
+ };
+}
+
+// macro use only
+#[doc(hidden)]
+pub trait ConfigOps {
+ fn is_readable_reg(reg: u32) -> bool;
+ fn is_writeable_reg(reg: u32) -> bool;
+ fn is_volatile_reg(reg: u32) -> bool;
+ fn is_precious_reg(reg: u32) -> bool;
+}
+
+/// Regmap Configuration
+///
+/// # Invariants
+///
+/// `self.raw` always contain valid data.
+pub struct Config<T: ConfigOps> {
+ raw: bindings::regmap_config,
+ _phantom: PhantomData<T>,
+}
+impl<T: ConfigOps> Config<T> {
+ /// Create a new regmap Config
+ pub const fn new(reg_bits: i32, val_bits: i32) -> Self {
+ // SAFETY: FFI type is valid to be zero-initialized.
+ let mut cfg: bindings::regmap_config = unsafe { core::mem::zeroed() };
+
+ cfg.reg_bits = reg_bits;
+ cfg.val_bits = val_bits;
+ cfg.writeable_reg = Some(Self::writeable_reg_callback);
+ cfg.readable_reg = Some(Self::readable_reg_callback);
+ cfg.volatile_reg = Some(Self::volatile_reg_callback);
+ cfg.precious_reg = Some(Self::precious_reg_callback);
+
+ Self {
+ raw: cfg,
+ _phantom: PhantomData,
+ }
+ }
+
+ config_with!(
+ /// Specifies the maximum valid register address.
+ max_register: u32
+ );
+
+ config_with!(
+ /// Type of caching being performed.
+ cache_type: CacheType, cache_type as _
+ );
+
+ /// # Safety
+ ///
+ /// `_dev` must be a non-null and valid `struct device` pointer.
+ unsafe extern "C" fn writeable_reg_callback(_dev: *mut bindings::device, reg: u32) -> bool {
+ T::is_writeable_reg(reg)
+ }
+
+ /// # Safety
+ ///
+ /// `_dev` must be a non-null and valid `struct device` pointer.
+ unsafe extern "C" fn readable_reg_callback(_dev: *mut bindings::device, reg: u32) -> bool {
+ T::is_readable_reg(reg)
+ }
+
+ /// # Safety
+ ///
+ /// `_dev` must be a non-null and valid `struct device` pointer.
+ unsafe extern "C" fn volatile_reg_callback(_dev: *mut bindings::device, reg: u32) -> bool {
+ T::is_volatile_reg(reg)
+ }
+
+ /// # Safety
+ ///
+ /// `_dev` must be a non-null and valid `struct device` pointer.
+ unsafe extern "C" fn precious_reg_callback(_dev: *mut bindings::device, reg: u32) -> bool {
+ T::is_precious_reg(reg)
+ }
+}
+
+/// Definitions describing how registers can be accessed.
+pub mod access {
+ /// Register can be read from.
+ pub const READ: u32 = 0b000001;
+ /// Register can be written to.
+ pub const WRITE: u32 = 0b000010;
+ /// Register should not be read outside of a call from the driver.
+ pub const PRECIOUS: u32 = 0b000100;
+ /// Register value can't be cached.
+ pub const VOLATILE: u32 = 0b001000;
+
+ /// Register can be read from and written to.
+ pub const RW: u32 = READ | WRITE;
+}
+
+// macro use only
+#[doc(hidden)]
+#[macro_export]
+macro_rules! regmap_check_access {
+ ($type:ident, $access:expr, $reg:ident, $addr:literal) => {
+ if kernel::regmap::access::$type & $access > 0 && $reg == $addr {
+ return true;
+ }
+ };
+}
+// macro use only
+#[doc(hidden)]
+pub use regmap_check_access;
+
+/// Common operations for all field types
+pub trait FieldCommonOps {
+ /// Get the Mask for the field
+ fn mask() -> u32;
+}
+
+/// Read operations for fields with `bit` type
+pub trait BitFieldReadOps {
+ /// Returns whether the bit is set
+ fn is_set<const N: usize>(fields: &mut Fields<N>) -> Result<bool>;
+}
+
+/// Write operations for fields with `bit` type
+pub trait BitFieldWriteOps {
+ /// Set the bit
+ fn set<const N: usize>(fields: &mut Fields<N>) -> Result;
+
+ /// Force set the bit
+ fn force_set<const N: usize>(fields: &mut Fields<N>) -> Result;
+
+ /// Clear the bit
+ fn clear<const N: usize>(fields: &mut Fields<N>) -> Result;
+
+ /// Force clear the bit
+ fn force_clear<const N: usize>(fields: &mut Fields<N>) -> Result;
+}
+
+/// Read operations for fields with `enum` type
+pub trait EnumFieldReadOps {
+ #[doc(hidden)]
+ /// Underlying enum type reprensenting the field values
+ type EnumType;
+
+ /// Read the field
+ fn read<const N: usize>(fields: &mut Fields<N>) -> Result<Self::EnumType>;
+}
+
+/// Write operations for fields with `enum` type
+pub trait EnumFieldWriteOps {
+ #[doc(hidden)]
+ /// Underlying enum type reprensenting the field values
+ type EnumType;
+
+ /// Write the field
+ fn write<const N: usize>(fields: &mut Fields<N>, val: Self::EnumType) -> Result;
+
+ /// Force write the field
+ fn force_write<const N: usize>(fields: &mut Fields<N>, val: Self::EnumType) -> Result;
+}
+
+/// Read operations for fields with `raw` type
+pub trait RawFieldReadOps {
+ /// Read the field
+ fn read<const N: usize>(fields: &mut Fields<N>) -> Result<core::ffi::c_uint>;
+
+ /// Test the field bits
+ fn test_bits<const N: usize>(fields: &mut Fields<N>, bits: core::ffi::c_uint) -> Result;
+}
+
+/// Write operations for fields with `raw` type
+pub trait RawFieldWriteOps {
+ /// Write the field
+ fn write<const N: usize>(fields: &mut Fields<N>, val: core::ffi::c_uint) -> Result;
+
+ /// Force write the field
+ fn force_write<const N: usize>(fields: &mut Fields<N>, val: core::ffi::c_uint) -> Result;
+
+ /// Update the field using a mask
+ fn update_bits<const N: usize>(
+ fields: &mut Fields<N>,
+ mask: core::ffi::c_uint,
+ val: core::ffi::c_uint,
+ ) -> Result;
+
+ /// Force update the field using a mask
+ fn force_update_bits<const N: usize>(
+ fields: &mut Fields<N>,
+ mask: core::ffi::c_uint,
+ val: core::ffi::c_uint,
+ ) -> Result;
+
+ /// Set field bits
+ fn set_bits<const N: usize>(fields: &mut Fields<N>, bits: core::ffi::c_uint) -> Result;
+
+ /// Clear the field bits
+ fn clear_bits<const N: usize>(fields: &mut Fields<N>, bits: core::ffi::c_uint) -> Result;
+}
+
+/// Bit field
+///
+/// `bit` should be use when a feature is implemented through reading or writing a single bit of
+/// a register.
+///
+/// See [`BitFieldReadOps`] and [`BitFieldWriteOps`] for operations available..
+///
+/// # Syntax
+///
+/// `bit(index, access)`
+///
+/// where
+/// * `index`: bit index starting from 0
+/// * `access`: access of the bit with the following possible values:
+/// - `ro`: read-only ([`BitFieldReadOps`] gets implemented)
+/// - `wo`: write-only ([`BitFieldWriteOps`] gets implemented)
+/// - `rw`: read and write (both [`BitFieldReadOps`] and [`BitFieldWriteOps`] gets
+/// implemented)
+///
+/// # Examples
+///
+/// ```ignore
+/// regmap::define_regmap_field_descs!(FIELD_DESCS, {
+/// (command, 0x14, RW, {
+/// vselgt => bit(0, rw),
+/// pwmvsel1 => bit(6, rw),
+/// pwmvsel0 => bit(7, rw),
+/// })
+/// });
+///
+/// command::pwmvsel0::set(&mut fields);
+/// command::pwmvsel0::is_set(&mut fields);
+/// command::pwmvsel0::clear(&mut fields);
+/// ```
+#[macro_export]
+macro_rules! regmap_field_bit {
+ ($field_name:ident, $access: expr, $reg:literal, $pos:literal, rw) => {
+ kernel::static_assert!($access & kernel::regmap::access::RW == kernel::regmap::access::RW);
+
+ $crate::regmap_field_bit!($field_name, $reg, $pos, reserved);
+ $crate::regmap_field_bit!($field_name, _ro);
+ $crate::regmap_field_bit!($field_name, _wo);
+ };
+
+ ($field_name:ident, $access: expr, $reg:literal, $pos:literal, ro) => {
+ kernel::static_assert!(
+ $access & kernel::regmap::access::READ == kernel::regmap::access::READ
+ );
+
+ $crate::regmap_field_bit!($field_name, $reg, $pos, reserved);
+ $crate::regmap_field_bit!($field_name, _ro);
+ };
+
+ ($field_name:ident, $access: expr, $reg:literal, $pos:literal, wo) => {
+ kernel::static_assert!(
+ $access & kernel::regmap::access::WRITE == kernel::regmap::access::WRITE
+ );
+
+ $crate::regmap_field_bit!($field_name, $reg, $pos, reserved);
+ $crate::regmap_field_bit!($field_name, _wo);
+ };
+
+ ($field_name:ident, $reg:literal, $pos:literal, reserved) => {
+ kernel::macros::paste! {
+ struct [<_Bit $pos >];
+ }
+
+ impl $field_name {
+ pub(crate) const fn reg_field() -> bindings::reg_field {
+ bindings::reg_field {
+ reg: $reg,
+ lsb: $pos,
+ msb: $pos + 1,
+ id_offset: 0,
+ id_size: 0,
+ }
+ }
+
+ #[allow(dead_code)]
+ pub(crate) const fn mask() -> u32 {
+ kernel::genmask!($pos, $pos) as _
+ }
+ }
+ };
+
+ ($field_name:ident, _ro) => {
+ impl super::BitFieldReadOps for $field_name {
+ fn is_set<const N: usize>(fields: &mut regmap::Fields<N>) -> Result<bool> {
+ let field = fields.index(Self::id() as usize);
+ let mut val: core::ffi::c_uint = 0;
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe { bindings::regmap_field_read(field, &mut val) })?;
+ Ok(val == 1)
+ }
+ }
+ };
+
+ ($field_name:ident, _wo) => {
+ impl super::BitFieldWriteOps for $field_name {
+ fn set<const N: usize>(fields: &mut regmap::Fields<N>) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe { bindings::regmap_field_write(field, 1) })
+ }
+
+ fn force_set<const N: usize>(fields: &mut regmap::Fields<N>) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe { bindings::regmap_field_force_write(field, 1) })
+ }
+
+ fn clear<const N: usize>(fields: &mut regmap::Fields<N>) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe { bindings::regmap_field_write(field, 0) })
+ }
+
+ fn force_clear<const N: usize>(fields: &mut regmap::Fields<N>) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe { bindings::regmap_field_force_write(field, 0) })
+ }
+ }
+ };
+}
+
+/// Enum field
+///
+/// `enum` should be used when a series of contineous bits represent possible values that can be
+/// enumerated.
+/// `enum` fields provide type-safety and preventing to write into the fields incorrect values.
+///
+/// See [`EnumFieldReadOps`] and [`EnumFieldWriteOps`] for operations available..
+///
+/// # Syntax
+///
+/// `enum(bits_range, access, { variant_definitions })`
+///
+/// where
+/// * `bits_range`: bit used to store the data.
+/// * `access`: access of the bits with the following possible values:
+/// - `ro`: read-only ([`EnumFieldReadOps`] gets implemented)
+/// - `wo`: write-only ([`EnumFieldWriteOps`] gets implemented)
+/// - `rw`: read and write (both [`EnumFieldReadOps`] and [`EnumFieldWriteOps`] gets
+/// implemented)
+/// * `variant_definitions`: list of all the enum variants using the syntax: `VariantName = Value,`.
+///
+/// # Examples
+///
+/// ```ignore
+/// regmap::define_regmap_field_descs!(FIELD_DESCS, {
+/// (limconf, 0x16, RW, {
+/// ipeak => enum([7:6], rw, {
+/// Peak3p5A = 0x0,
+/// Peak4p0A = 0x1,
+/// Peak4p5A = 0x2,
+/// Peak5p0A = 0x3,
+/// }),
+/// })
+/// });
+///
+/// limconf::ipeak::write(&mut fields, limconf::ipeak::Peak4p0A);
+/// limconf::ipeak::read(&mut fields);
+/// ```
+#[macro_export]
+macro_rules! regmap_field_enum {
+ ($field_name:ident, $access: expr, $reg:literal, [$msb:literal:$lsb:literal], ro, {
+ $($k:ident = $v:literal,)+ }) => {
+ kernel::static_assert!(
+ $access & kernel::regmap::access::READ == kernel::regmap::access::READ
+ );
+
+ $crate::regmap_field_enum!($field_name, $reg, [$msb:$lsb], reserved, { $($k = $v,)+ });
+ $crate::regmap_field_enum!($field_name, _ro);
+ };
+
+ ($field_name:ident, $access: expr, $reg:literal, [$msb:literal:$lsb:literal], rw, {
+ $($k:ident = $v:literal,)+ }) => {
+ kernel::static_assert!($access & kernel::regmap::access::RW == kernel::regmap::access::RW);
+
+ $crate::regmap_field_enum!($field_name, $reg, [$msb:$lsb], reserved, { $($k = $v,)+ });
+ $crate::regmap_field_enum!($field_name, _ro);
+ $crate::regmap_field_enum!($field_name, _wo);
+ };
+
+ ($field_name:ident, $access: expr, $reg:literal, [$msb:literal:$lsb:literal], wo, {
+ $($k:ident = $v:literal,)+ }) => {
+ kernel::static_assert!(
+ $access & kernel::regmap::access::WRITE == kernel::regmap::access::WRITE
+ );
+
+ $crate::regmap_field_enum!($field_name, $reg, [$msb:$lsb], reserved, { $($k = $v,)+ });
+ $crate::regmap_field_enum!($field_name, _wo);
+ };
+
+ ($field_name:ident, $reg:literal, [$msb:literal:$lsb:literal], reserved, {
+ $($k:ident = $v:literal,)+ }) => {
+ kernel::macros::paste! {
+ #[repr(u32)]
+ #[allow(non_camel_case_types)]
+ pub(crate) enum [<$field_name _enum>] {
+ $($k = $v,)+
+ }
+
+ impl TryFrom<core::ffi::c_uint> for [<$field_name _enum>] {
+ type Error = kernel::error::Error;
+
+ fn try_from(raw_value: core::ffi::c_uint) -> Result<Self> {
+ match raw_value {
+ $($v => Ok(Self::$k),)+
+ _ => Err(kernel::error::code::EINVAL),
+ }
+ }
+ }
+
+ impl $field_name {
+ pub(crate) const fn reg_field() -> bindings::reg_field {
+ bindings::reg_field {
+ reg: $reg,
+ lsb: $lsb,
+ msb: $msb,
+ id_offset: 0,
+ id_size: 0,
+ }
+ }
+
+ #[allow(dead_code)]
+ pub(crate) const fn mask() -> u32 {
+ kernel::genmask!($msb, $lsb) as _
+ }
+ }
+ }
+ };
+
+ ($field_name:ident, _ro) => {
+ impl super::EnumFieldReadOps for $field_name {
+ type EnumType = kernel::macros::paste! {[<$field_name _enum>]};
+
+ fn read<const N: usize>(fields: &mut regmap::Fields<N>) -> Result<Self::EnumType> {
+ Self::EnumType::try_from(fields.read(Self::id() as usize)?)
+ }
+ }
+ };
+
+ ($field_name:ident, _wo) => {
+ impl super::EnumFieldWriteOps for $field_name {
+ type EnumType = kernel::macros::paste! {[<$field_name _enum>]};
+
+ fn write<const N: usize>(
+ fields: &mut regmap::Fields<N>,
+ val: Self::EnumType
+ ) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ let ret = unsafe { bindings::regmap_field_write(field, val as _) };
+ kernel::error::to_result(ret)
+ }
+
+ fn force_write<const N: usize>(
+ fields: &mut regmap::Fields<N>,
+ val: Self::EnumType
+ ) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ let ret = unsafe { bindings::regmap_field_force_write(field, val as _) };
+ kernel::error::to_result(ret)
+ }
+ }
+ };
+}
+
+/// Raw field
+///
+/// `raw` should be used when bits cannot be represented by any other field types. It provides
+/// raw access to the register bits.
+///
+/// # Syntax
+///
+/// `raw(bits_range, access)`
+///
+/// where
+/// * `bits_range`: bits used to store the data.
+/// * `access`: access of the bit with the following possible values:
+/// - `ro`: read-only ([`RawFieldReadOps`] gets implemented)
+/// - `wo`: write-only ([`RawFieldWriteOps`] gets implemented)
+/// - `rw`: read and write (both [`RawFieldReadOps`] and [`RawFieldWriteOps`] gets
+/// implemented)
+///
+/// # Examples
+///
+/// ```ignore
+/// regmap::define_regmap_field_descs!(FIELD_DESCS, {
+/// (pid, 0x3, READ, { value => raw([7:0], ro) }),
+/// (progvsel1, 0x10, RW, {
+/// voutvsel1 => raw([6:0], rw),
+/// })
+/// });
+///
+/// pid::value::read(&mut fields);
+/// progvsel1::voutvsel1::write(&mut fields, 0x42);
+/// ```
+#[macro_export]
+macro_rules! regmap_field_raw {
+ ($field_name:ident, $access: expr, $reg:literal, [$msb:literal:$lsb:literal], rw) => {
+ kernel::static_assert!($access & kernel::regmap::access::RW == kernel::regmap::access::RW);
+
+ $crate::regmap_field_raw!($field_name, $reg, [$msb:$lsb], reserved);
+ $crate::regmap_field_raw!($field_name, $reg, [$msb:$lsb], _ro);
+ $crate::regmap_field_raw!($field_name, $reg, [$msb:$lsb], _wo);
+ };
+
+ ($field_name:ident, $access: expr, $reg:literal, [$msb:literal:$lsb:literal], ro) => {
+ kernel::static_assert!(
+ $access & kernel::regmap::access::READ == kernel::regmap::access::READ
+ );
+
+ $crate::regmap_field_raw!($field_name, $reg, [$msb:$lsb], reserved);
+ $crate::regmap_field_raw!($field_name, $reg, [$msb:$lsb], _ro);
+ };
+
+ ($field_name:ident, $access: expr, $reg:literal, [$msb:literal:$lsb:literal], wo) => {
+ kernel::static_assert!(
+ $access & kernel::regmap::access::WRITE == kernel::regmap::access::WRITE
+ );
+
+ $crate::regmap_field_raw!($field_name, $reg, [$msb:$lsb], reserved);
+ $crate::regmap_field_raw!($field_name, $reg, [$msb:$lsb], _wo);
+ };
+
+ ($field_name:ident, $reg:literal, [$msb:literal:$lsb:literal], reserved) => {
+ impl $field_name {
+ pub(crate) const fn reg_field() -> bindings::reg_field {
+ bindings::reg_field {
+ reg: $reg,
+ lsb: $lsb,
+ msb: $msb,
+ id_offset: 0,
+ id_size: 0,
+ }
+ }
+
+ #[allow(dead_code)]
+ pub(crate) const fn mask() -> u32 {
+ kernel::genmask!($msb, $lsb) as _
+ }
+ }
+ };
+
+ ($field_name:ident, $reg:literal, [$msb:literal:$lsb:literal], _ro) => {
+ impl super::RawFieldReadOps for $field_name {
+ fn read<const N: usize>(fields: &mut regmap::Fields<N>) -> Result<core::ffi::c_uint> {
+ fields.read(Self::id() as usize)
+ }
+
+ fn test_bits<const N: usize>(
+ fields: &mut regmap::Fields<N>,
+ bits: core::ffi::c_uint,
+ ) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe { bindings::regmap_field_test_bits(field, bits) })
+ }
+ }
+ };
+
+ ($field_name:ident, $reg:literal, [$msb:literal:$lsb:literal], _wo) => {
+ impl super::RawFieldWriteOps for $field_name {
+ fn write<const N: usize>(
+ fields: &mut regmap::Fields<N>,
+ val: core::ffi::c_uint,
+ ) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe { bindings::regmap_field_write(field, val as _) })
+ }
+
+ fn force_write<const N: usize>(
+ fields: &mut regmap::Fields<N>,
+ val: core::ffi::c_uint,
+ ) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe {
+ bindings::regmap_field_force_write(field, val as _)
+ })
+ }
+
+ fn update_bits<const N: usize>(
+ fields: &mut regmap::Fields<N>,
+ mask: core::ffi::c_uint,
+ val: core::ffi::c_uint,
+ ) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe {
+ bindings::regmap_field_update_bits(field, mask, val)
+ })
+ }
+
+ fn force_update_bits<const N: usize>(
+ fields: &mut regmap::Fields<N>,
+ mask: core::ffi::c_uint,
+ val: core::ffi::c_uint,
+ ) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe {
+ bindings::regmap_field_force_update_bits(field, mask, val)
+ })
+ }
+
+ fn set_bits<const N: usize>(
+ fields: &mut regmap::Fields<N>,
+ bits: core::ffi::c_uint,
+ ) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe { bindings::regmap_field_set_bits(field, bits) })
+ }
+
+ fn clear_bits<const N: usize>(
+ fields: &mut regmap::Fields<N>,
+ bits: core::ffi::c_uint,
+ ) -> Result {
+ let field = fields.index(Self::id() as usize);
+ // SAFETY: `Fields` guarantee that anything returned from `Fields::index` is valid
+ // and non-null, hence it is safe to perform the FFI function call.
+ kernel::error::to_result(unsafe { bindings::regmap_field_clear_bits(field, bits) })
+ }
+ }
+ };
+}
+
+// macro use only
+#[doc(hidden)]
+#[macro_export]
+macro_rules! regmap_fields {
+ ($type:ident, $reg:ident, $access:expr, $name:ident, $($t:tt)*) => {
+ kernel::macros::paste! {
+ #[allow(non_camel_case_types)]
+ pub(crate) struct $name;
+
+ impl $name {
+ #[allow(dead_code)]
+ pub(crate) const fn id() -> super::Fields {
+ super::Fields::[<$reg _ $name>]
+ }
+ }
+
+ $crate::[<regmap_field_ $type>]!($name, $access, $($t)*);
+ }
+ };
+}
+
+// macro use only
+#[doc(hidden)]
+#[macro_export]
+macro_rules! regmap_reg_field {
+ ($reg_name:ident, $field_name:ident) => {
+ register::$reg_name::$field_name::reg_field()
+ };
+}
+
+// macro use only
+#[doc(hidden)]
+#[macro_export]
+macro_rules! regmap_count_fields {
+ () => { 0usize };
+ ($type:ident $($rhs:ident)*) => { 1 + $crate::regmap_count_fields!($($rhs)*) };
+}
+
+/// Define regmap field descriptors
+///
+/// # Syntax
+///
+/// ```ignore
+/// define_regmap_field_desc!(VAR_NAME, { <register_definition>, [<register_definition>, ...] });
+/// ```
+///
+/// where `VAR_NAME`: symbol under which the regmap [`Fields`] are available.
+///
+/// register_definition:
+/// ```ignore
+/// (name, address, access_permission, { <field_definition>, [<field_definition>, ...] })
+/// ```
+/// where
+///
+/// * name: symbol under which this field will be available
+/// * address: register address
+/// * access_permission: [`access`] permission of the register
+///
+/// field_definition:
+/// ```ignore
+/// field_name => <field_type>(...),
+/// ```
+///
+/// where `field_name` is the symbol under which the field will be accessible.
+///
+/// The following `<field_type>`s are available:
+/// * [bit](`regmap_field_bit`)
+/// * [enum](`regmap_field_enum`)
+/// * [raw](`regmap_field_raw`)
+///
+/// # Examples
+///
+/// ```ignore
+/// regmap::define_regmap_field_descs!(FIELD_DESCS, {
+/// (pid, 0x3, READ, { value => raw([7:0], ro) }),
+/// (limconf, 0x16, RW, {
+/// rearm => bit(0, rw),
+/// rststatus => bit(1, rw),
+/// tpwth => enum([5:4], rw, {
+/// Temp83C = 0x0,
+/// Temp94C = 0x1,
+/// Temp105C = 0x2,
+/// Temp116C = 0x3,
+/// }),
+/// })
+/// });
+/// ```
+#[macro_export]
+macro_rules! define_regmap_field_descs {
+ ($name:ident, {
+ $((
+ $reg_name:ident, $reg_addr:literal, $access:expr, {
+ $($field_name:ident => $type:ident($($x:tt),*)),* $(,)?
+ }
+ )),+
+ }) => {
+ mod register {
+ use kernel::regmap::{
+ access::*,
+ BitFieldReadOps, BitFieldWriteOps,
+ ConfigOps,
+ EnumFieldReadOps, EnumFieldWriteOps,
+ RawFieldReadOps, RawFieldWriteOps
+ };
+
+ kernel::macros::paste! {
+ $(
+ pub(crate) mod $reg_name {
+ use kernel::{bindings, error::{Result}, regmap::{self, access::*}};
+ $(
+ $crate::regmap_fields!($type, $reg_name, $access, $field_name,
+ $reg_addr, $($x),*);
+ )*
+
+ #[allow(dead_code)]
+ pub(crate) const fn addr() -> u32 {
+ $reg_addr
+ }
+ }
+ )+
+
+ #[repr(u32)]
+ #[allow(non_camel_case_types)]
+ pub(crate) enum Fields {
+ $($(
+ [<$reg_name _ $field_name>],
+ )*)+
+ }
+
+ pub(crate) struct AccessOps;
+ impl ConfigOps for AccessOps {
+ fn is_readable_reg(reg: u32) -> bool {
+ $(
+ kernel::regmap::regmap_check_access!(READ, $access, reg, $reg_addr);
+ )+
+
+ false
+ }
+
+ fn is_writeable_reg(reg: u32) -> bool {
+ $(
+ kernel::regmap::regmap_check_access!(WRITE, $access, reg, $reg_addr);
+ )+
+
+ false
+ }
+
+ fn is_volatile_reg(reg: u32) -> bool {
+ $(
+ kernel::regmap::regmap_check_access!(VOLATILE, $access, reg, $reg_addr);
+ )+
+
+ false
+ }
+
+ fn is_precious_reg(reg: u32) -> bool {
+ $(
+ kernel::regmap::regmap_check_access!(PRECIOUS, $access, reg, $reg_addr);
+ )+
+
+ false
+ }
+ }
+ }
+ }
+
+ const $name: regmap::FieldDescs<{$crate::regmap_count_fields!($($($type)*)+)}> =
+ regmap::FieldDescs::new([
+ $(
+ $(
+ $crate::regmap_reg_field!($reg_name, $field_name)
+ ),*
+ ),+
+ ]);
+ };
+}
+pub use define_regmap_field_descs;
--
2.45.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/9] rust: error: add declaration for ENOTRECOVERABLE error
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
2024-12-18 23:36 ` [PATCH 1/9] rust: i2c: add basic I2C client abstraction Fabien Parent
2024-12-18 23:36 ` [PATCH 2/9] rust: add abstraction for regmap Fabien Parent
@ 2024-12-18 23:36 ` Fabien Parent
2024-12-18 23:36 ` [PATCH 4/9] rust: regulator: add abstraction for Regulator's modes Fabien Parent
` (5 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
From: Fabien Parent <fabien.parent@linaro.org>
Add a missing errno for ENOTRECOVERABLE.
ENOTRECOVERABLE is returned by get_voltage{_sel} from the Regulator
driver API when a voltage cannot be read at bootup and hasn't been
set yet.
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
rust/kernel/error.rs | 1 +
1 file changed, 1 insertion(+)
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 52c5024324474fc1306047f3fd7516f0023d0313..19c8287032631ee8c4afd3c9c3f1e0709591ba3d 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -82,6 +82,7 @@ macro_rules! declare_err {
declare_err!(EIOCBQUEUED, "iocb queued, will get completion event.");
declare_err!(ERECALLCONFLICT, "Conflict with recalled state.");
declare_err!(ENOGRACE, "NFS file lock reclaim refused.");
+ declare_err!(ENOTRECOVERABLE, "State not recoverable.");
}
/// Generic integer kernel error.
--
2.45.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/9] rust: regulator: add abstraction for Regulator's modes
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
` (2 preceding siblings ...)
2024-12-18 23:36 ` [PATCH 3/9] rust: error: add declaration for ENOTRECOVERABLE error Fabien Parent
@ 2024-12-18 23:36 ` Fabien Parent
2024-12-18 23:36 ` [PATCH 5/9] rust: regulator: add Regulator Driver abstraction Fabien Parent
` (4 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
From: Fabien Parent <fabien.parent@linaro.org>
The type regulator::Mode is used by both the regulator consumer
abstraction and the regulator driver abstraction. This commits
adds a shared abstraction for it.
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
MAINTAINERS | 1 +
rust/bindings/bindings_helper.h | 1 +
rust/kernel/lib.rs | 2 ++
rust/kernel/regulator.rs | 42 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 46 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index acb3942eb1b66ec2bc09ac50f51c2054b7b45355..90c231f0aa7381aa8d206fb94c5d1f013dfcae41 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25159,6 +25159,7 @@ F: Documentation/power/regulator/
F: drivers/regulator/
F: include/dt-bindings/regulator/
F: include/linux/regulator/
+F: rust/kernel/regulator.rs
K: regulator_get_optional
VOLTAGE AND CURRENT REGULATOR IRQ HELPERS
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 48d2b91b34067e7e9ee9c64c2e42681e988e9aad..b18d772bc3a0e78d749cc9e5ae81a4237a57f8c5 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -29,6 +29,7 @@
#include <linux/poll.h>
#include <linux/refcount.h>
#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <linux/sched.h>
#include <linux/security.h>
#include <linux/slab.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 456e979724d1079045cb157086ff2b2ed0fcca3b..3aa36648e9571e305a89f5d1353c0dd44e136384 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -68,6 +68,8 @@
pub mod rbtree;
#[cfg(CONFIG_REGMAP)]
pub mod regmap;
+#[cfg(CONFIG_REGULATOR)]
+pub mod regulator;
pub mod revocable;
pub mod security;
pub mod seq_file;
diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
new file mode 100644
index 0000000000000000000000000000000000000000..d695ac955193efcfda62770784a92d70d606b93d
--- /dev/null
+++ b/rust/kernel/regulator.rs
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! SoC Regulators
+
+use crate::{
+ bindings,
+ error::{code::*, Error, Result},
+};
+
+/// Regulators operating modes
+#[derive(Copy, Clone)]
+#[repr(u32)]
+pub enum Mode {
+ /// Invalid mode
+ Invalid = bindings::REGULATOR_MODE_INVALID,
+ /// Regulator can handle fast changes in it's load
+ Fast = bindings::REGULATOR_MODE_FAST,
+ /// Normal regulator power supply mode
+ Normal = bindings::REGULATOR_MODE_NORMAL,
+ /// Regulator runs in a more efficient mode for light loads
+ Idle = bindings::REGULATOR_MODE_IDLE,
+ /// Regulator runs in the most efficient mode for very light loads
+ Standby = bindings::REGULATOR_MODE_STANDBY,
+}
+
+impl TryFrom<core::ffi::c_uint> for Mode {
+ type Error = Error;
+
+ /// Convert a mode represented as an unsigned integer into its Rust enum equivalent
+ ///
+ /// If the integer does not match any of the [`Mode`], then [`EINVAL`] is returned
+ fn try_from(mode: core::ffi::c_uint) -> Result<Self> {
+ match mode {
+ bindings::REGULATOR_MODE_FAST => Ok(Self::Fast),
+ bindings::REGULATOR_MODE_NORMAL => Ok(Self::Normal),
+ bindings::REGULATOR_MODE_IDLE => Ok(Self::Idle),
+ bindings::REGULATOR_MODE_STANDBY => Ok(Self::Standby),
+ bindings::REGULATOR_MODE_INVALID => Ok(Self::Invalid),
+ _ => Err(EINVAL),
+ }
+ }
+}
--
2.45.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/9] rust: regulator: add Regulator Driver abstraction
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
` (3 preceding siblings ...)
2024-12-18 23:36 ` [PATCH 4/9] rust: regulator: add abstraction for Regulator's modes Fabien Parent
@ 2024-12-18 23:36 ` Fabien Parent
2024-12-19 10:26 ` Danilo Krummrich
2024-12-18 23:36 ` [PATCH 6/9] rust: regulator: add support for regmap Fabien Parent
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
From: Fabien Parent <fabien.parent@linaro.org>
This commit adds a Rust abstraction to write Regulator drivers. Only
the features used by the NCV6336 driver were added to this abstraction.
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
MAINTAINERS | 1 +
rust/bindings/bindings_helper.h | 1 +
rust/kernel/regulator.rs | 4 +-
rust/kernel/regulator/driver.rs | 850 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 855 insertions(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 90c231f0aa7381aa8d206fb94c5d1f013dfcae41..87da43251bf0f20d2b5831345778ead592c407dc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25160,6 +25160,7 @@ F: drivers/regulator/
F: include/dt-bindings/regulator/
F: include/linux/regulator/
F: rust/kernel/regulator.rs
+F: rust/kernel/regulator/
K: regulator_get_optional
VOLTAGE AND CURRENT REGULATOR IRQ HELPERS
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index b18d772bc3a0e78d749cc9e5ae81a4237a57f8c5..124129daea73c143c919d05814fc02bb4460ddfd 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -30,6 +30,7 @@
#include <linux/refcount.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
+#include <linux/regulator/driver.h>
#include <linux/sched.h>
#include <linux/security.h>
#include <linux/slab.h>
diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
index d695ac955193efcfda62770784a92d70d606b93d..bd8202fe5702b944201e76553b9496e1d42cb429 100644
--- a/rust/kernel/regulator.rs
+++ b/rust/kernel/regulator.rs
@@ -2,12 +2,14 @@
//! SoC Regulators
+pub mod driver;
+
use crate::{
bindings,
error::{code::*, Error, Result},
};
-/// Regulators operating modes
+/// [`driver::Device`] operating modes
#[derive(Copy, Clone)]
#[repr(u32)]
pub enum Mode {
diff --git a/rust/kernel/regulator/driver.rs b/rust/kernel/regulator/driver.rs
new file mode 100644
index 0000000000000000000000000000000000000000..8079ea28fd5bf7b6871a0b1d2cea7a6fffcb43ca
--- /dev/null
+++ b/rust/kernel/regulator/driver.rs
@@ -0,0 +1,850 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! SoC Device Driver Interface
+//!
+//! C header: [`include/linux/regulator/driver.h`](srctree/include/linux/regulator/driver.h)
+//!
+//! # Examples
+//!
+//! ```
+//! use kernel::regulator::driver::{Config, Desc, Device, Driver, Type};
+//!
+//! static DESC: Desc =
+//! Desc::new::<MyDeviceDriver>(kernel::c_str!("my-regulator-driver"), Type::Voltage);
+//!
+//! struct MyDeviceDriver;
+//!
+//! #[vtable]
+//! impl Driver for MyDeviceDriver {
+//! type Data = ();
+//!
+//! // Implement supported `Driver`'s operations here.
+//!
+//! // Example:
+//! fn is_enabled(reg: &mut Device<Self::Data>) -> Result<bool> {
+//! Ok(true)
+//! }
+//! }
+//!
+//! impl MyDeviceDriver {
+//! fn probe(dev: &mut kernel::device::Device) {
+//! let _ = Device::register(dev, &DESC, Config::<<Self as Driver>::Data>::new(dev, ()));
+//! }
+//! }
+//! ```
+
+use crate::{
+ device,
+ error::{code::*, from_err_ptr, from_result, Error, Result},
+ macros::vtable,
+ regulator::Mode,
+ str::CStr,
+ types::ForeignOwnable,
+ ThisModule,
+};
+use core::{marker::PhantomData, mem::ManuallyDrop, ptr::NonNull};
+
+/// [`Device`]'s status
+#[derive(Eq, PartialEq)]
+pub enum Status {
+ /// Device is off
+ Off,
+ /// Device is on
+ On,
+ /// Device is in an error state
+ Error,
+ /// Device is on and in Fast mode
+ Fast,
+ /// Device is on and in Normal mode
+ Normal,
+ /// Device is on and in Idle mode
+ Idle,
+ /// Device is on and in Standby mode
+ Standby,
+ /// Device is enabled but not regulating
+ Bypass,
+ /// Device is any other status
+ Undefined,
+}
+
+impl TryFrom<core::ffi::c_uint> for Status {
+ type Error = Error;
+
+ fn try_from(status: core::ffi::c_uint) -> Result<Self> {
+ match status {
+ bindings::regulator_status_REGULATOR_STATUS_OFF => Ok(Self::Off),
+ bindings::regulator_status_REGULATOR_STATUS_ON => Ok(Self::On),
+ bindings::regulator_status_REGULATOR_STATUS_ERROR => Ok(Self::Error),
+ bindings::regulator_status_REGULATOR_STATUS_FAST => Ok(Self::Fast),
+ bindings::regulator_status_REGULATOR_STATUS_NORMAL => Ok(Self::Normal),
+ bindings::regulator_status_REGULATOR_STATUS_IDLE => Ok(Self::Idle),
+ bindings::regulator_status_REGULATOR_STATUS_STANDBY => Ok(Self::Standby),
+ bindings::regulator_status_REGULATOR_STATUS_BYPASS => Ok(Self::Bypass),
+ bindings::regulator_status_REGULATOR_STATUS_UNDEFINED => Ok(Self::Undefined),
+ _ => Err(EINVAL),
+ }
+ }
+}
+
+impl From<Mode> for Status {
+ fn from(mode: Mode) -> Self {
+ // SAFETY: `regulator_mode_to_status` is a `pure function` that is only doing integer
+ // to integer conversion, hence this function call is safe.
+ let status = unsafe { bindings::regulator_mode_to_status(mode as _) };
+
+ if status < 0 {
+ Self::Undefined
+ } else {
+ Self::try_from(status as core::ffi::c_uint).unwrap_or(Self::Undefined)
+ }
+ }
+}
+
+/// [`Device`]'s operations
+#[vtable]
+pub trait Driver {
+ /// User data that will be accessible to all operations
+ type Data: ForeignOwnable + Send + Sync;
+
+ /// Return one of the supported voltages, in microvolt; zero if the selector indicates a
+ /// voltage that is unusable by the system; or negative errno. Selectors range from zero to one
+ /// less than the number of voltages supported by the system.
+ fn list_voltage(_rdev: &mut Device<Self::Data>, _selector: u32) -> Result<i32> {
+ Err(ENOTSUPP)
+ }
+
+ /// Set the voltage for the regulator within the range specified. The driver should select the
+ /// voltage closest to `min_uv`.
+ fn set_voltage(_rdev: &mut Device<Self::Data>, _min_uv: i32, _max_uv: i32) -> Result<i32> {
+ Err(ENOTSUPP)
+ }
+
+ /// Set the voltage for the regulator using the specified selector.
+ fn set_voltage_sel(_rdev: &mut Device<Self::Data>, _selector: u32) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Convert a voltage into a selector.
+ fn map_voltage(_rdev: &mut Device<Self::Data>, _min_uv: i32, _max_uv: i32) -> Result<i32> {
+ Err(ENOTSUPP)
+ }
+
+ /// Get the currently configured voltage for the regulator; Returns
+ /// [`ENOTRECOVERABLE`] if the regulator can't be read at bootup and hasn't been
+ /// set yet.
+ fn get_voltage(_rdev: &mut Device<Self::Data>) -> Result<i32> {
+ Err(ENOTSUPP)
+ }
+
+ /// Get the currently configured voltage selector for the regulator; Returns
+ /// [`ENOTRECOVERABLE`] if the regulator can't be read at bootup and hasn't been
+ /// set yet.
+ fn get_voltage_sel(_rdev: &mut Device<Self::Data>) -> Result<i32> {
+ Err(ENOTSUPP)
+ }
+
+ /// Configure a limit for a current-limited regulator.
+ ///
+ /// The driver should select the current closest to `max_ua`.
+ fn set_current_limit(_rdev: &mut Device<Self::Data>, _min_ua: i32, _max_ua: i32) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Get the configured limit for a current-limited regulator.
+ fn get_current_limit(_rdev: &mut Device<Self::Data>) -> Result<i32> {
+ Err(ENOTSUPP)
+ }
+
+ /// Enable or disable the active discharge of the regulator.
+ fn set_active_discharge(_rdev: &mut Device<Self::Data>, _enable: bool) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Configure the regulator as enabled.
+ fn enable(_rdev: &mut Device<Self::Data>) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Configure the regulator as disabled.
+ fn disable(_rdev: &mut Device<Self::Data>) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Returns enablement state of the regulator.
+ fn is_enabled(_rdev: &mut Device<Self::Data>) -> Result<bool> {
+ Err(ENOTSUPP)
+ }
+
+ /// Set the configured operating [`Mode`] for the regulator.
+ fn set_mode(_rdev: &mut Device<Self::Data>, _mode: Mode) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Get the configured operating [`Mode`] for the regulator.
+ fn get_mode(_rdev: &mut Device<Self::Data>) -> Mode {
+ Mode::Invalid
+ }
+
+ /// Report the regulator [`Status`].
+ fn get_status(_rdev: &mut Device<Self::Data>) -> Result<Status> {
+ Err(ENOTSUPP)
+ }
+
+ /// Set the voltage for the regaultor when the system is suspended.
+ fn set_suspend_voltage(_rdev: &mut Device<Self::Data>, _uv: i32) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Mark the regulator as enabled when the system is suspended.
+ fn set_suspend_enable(_rdev: &mut Device<Self::Data>) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Mark the regulator as disabled when the system is suspended.
+ fn set_suspend_disable(_rdev: &mut Device<Self::Data>) -> Result {
+ Err(ENOTSUPP)
+ }
+
+ /// Set the operating mode for the regulator when the system is suspended.
+ fn set_suspend_mode(_rdev: &mut Device<Self::Data>, _mode: Mode) -> Result {
+ Err(ENOTSUPP)
+ }
+}
+
+/// [`Device`]'s descriptor
+///
+/// # Examples
+///
+/// ```
+/// use kernel::{
+/// c_str,
+/// device,
+/// regulator::driver::{Config, Desc, Device, Driver, Type},
+/// types::ForeignOwnable,
+/// };
+///
+/// struct MyDeviceDriver;
+///
+/// #[vtable]
+/// impl Driver for MyDeviceDriver {
+/// type Data = ();
+/// }
+///
+/// static BUCK_DESC: Desc = Desc::new::<MyDeviceDriver>(c_str!("my_driver"), Type::Voltage)
+/// .with_of_match(c_str!("buck"))
+/// .with_enable(0x24, 0x1, 0x1, 0);
+///
+/// fn example(dev: &mut device::Device, mut config: Config<<MyDeviceDriver as Driver>::Data>) {
+/// let _ = Device::register(dev, &BUCK_DESC, config);
+/// }
+/// ```
+///
+/// # Invariants
+///
+/// `self.0` has always valid data.
+pub struct Desc(bindings::regulator_desc);
+impl Desc {
+ /// Create a new [`Device`] descriptor
+ pub const fn new<T: Driver>(name: &'static CStr, reg_type: Type) -> Self {
+ // SAFETY: `bindings::regulator_desc" is safe to initialize with 0s.
+ let mut desc: bindings::regulator_desc = unsafe { core::mem::zeroed() };
+ desc.name = name.as_char_ptr();
+ desc.type_ = match reg_type {
+ Type::Voltage => bindings::regulator_type_REGULATOR_VOLTAGE,
+ Type::Current => bindings::regulator_type_REGULATOR_CURRENT,
+ };
+ desc.ops = Adapter::<T>::build();
+ Self(desc)
+ }
+
+ /// Setup the register address, mask, and {en,dis}able values
+ pub const fn with_enable(mut self, reg: u32, mask: u32, en_val: u32, dis_val: u32) -> Self {
+ self.0.enable_reg = reg;
+ self.0.enable_mask = mask;
+ self.0.enable_val = en_val;
+ self.0.disable_val = dis_val;
+ self
+ }
+
+ /// Setup the register address, mask, and {en,dis}able values. {En,Dis}able values are
+ /// inverted, i.e. `dis_val` will be use to enable the regulator while `en_val` will be used
+ /// to disable the regulator.
+ pub const fn with_inverted_enable(
+ mut self,
+ reg: u32,
+ mask: u32,
+ en_val: u32,
+ dis_val: u32,
+ ) -> Self {
+ self.0.enable_is_inverted = true;
+ self.with_enable(reg, mask, en_val, dis_val)
+ }
+
+ /// Setup the active discharge regiter address, mask, on/off values.
+ pub const fn with_active_discharge(mut self, reg: u32, mask: u32, on: u32, off: u32) -> Self {
+ self.0.active_discharge_on = on;
+ self.0.active_discharge_off = off;
+ self.0.active_discharge_reg = reg;
+ self.0.active_discharge_mask = mask;
+ self
+ }
+
+ /// Setup the current selection register address, mask, and current table
+ pub const fn with_csel(mut self, reg: u32, mask: u32, table: &'static [u32]) -> Self {
+ self.0.csel_reg = reg;
+ self.0.csel_mask = mask;
+ self.0.curr_table = table.as_ptr();
+ self
+ }
+
+ /// Voltages are a linear mapping
+ pub const fn with_linear_mapping(
+ mut self,
+ reg: u32,
+ mask: u32,
+ min_uv: u32,
+ uv_step: u32,
+ n_voltages: u32,
+ linear_min_sel: u32,
+ ) -> Self {
+ self.0.vsel_reg = reg;
+ self.0.vsel_mask = mask;
+ self.0.n_voltages = n_voltages;
+ self.0.min_uV = min_uv;
+ self.0.uV_step = uv_step;
+ self.0.linear_min_sel = linear_min_sel;
+ self
+ }
+
+ /// Set the regulator owner
+ pub const fn with_owner(mut self, owner: &'static ThisModule) -> Self {
+ self.0.owner = owner.as_ptr();
+ self
+ }
+
+ /// Set the name used to identify the regulator in the DT.
+ pub const fn with_of_match(mut self, of_match: &'static CStr) -> Self {
+ self.0.of_match = of_match.as_char_ptr();
+ self
+ }
+}
+
+// SAFETY: `Desc` cannot be modified after its declaration and owns its data, hence it is safe
+// to share references between threads.
+unsafe impl Sync for Desc {}
+
+/// [`Device`]'s Config
+///
+/// # Examples
+///
+/// ```
+/// use kernel::regulator::driver::Config;
+/// # use kernel::regulator::driver::{Desc, Device};
+/// # use kernel::{device, sync::Arc};
+///
+/// struct DriverData(u32);
+///
+/// # fn probe(dev: &device::Device, desc: &'static Desc) -> Result {
+/// let config = Config::<Arc<DriverData>>::new(dev, Arc::new(DriverData(128), GFP_KERNEL)?);
+/// let reg = Device::register(dev, desc, config)?;
+/// # Ok(())
+/// # }
+/// ```
+///
+/// # Invariants
+///
+/// `self.cfg` always hold valid data.
+pub struct Config<T: ForeignOwnable + Send + Sync = ()> {
+ cfg: bindings::regulator_config,
+ data: T,
+}
+
+impl<T: ForeignOwnable + Send + Sync> Config<T> {
+ /// Create a [`Device`] config.
+ pub fn new(dev: &device::Device, data: T) -> Self {
+ Self {
+ cfg: bindings::regulator_config {
+ dev: dev.as_raw(),
+ ..Default::default()
+ },
+ data,
+ }
+ }
+}
+
+/// Regulator device
+///
+/// Abstraction for `struct regulator_dev`.
+///
+/// # Invariants
+///
+/// * `self.rdev` is valid and non-null.
+/// * [`Self`] has owns `self.rdev` memory allocation.
+/// * [`Self`] has owns memory of type `T` that can be retrieved through `rdev_get_drvdata`.
+pub struct Device<T: ForeignOwnable + Send + Sync> {
+ rdev: NonNull<bindings::regulator_dev>,
+ _data_type: PhantomData<T>,
+}
+
+impl<T: ForeignOwnable + Send + Sync> Device<T> {
+ /// # Safety
+ ///
+ /// `rdev` must be valid and non-null.
+ unsafe fn from_raw(rdev: *mut bindings::regulator_dev) -> ManuallyDrop<Self> {
+ ManuallyDrop::new(Self {
+ // SAFETY: The caller of `Self::from_raw` must garantee that `rdev` is non-null and
+ // valid..
+ rdev: unsafe { NonNull::new_unchecked(rdev) },
+ _data_type: PhantomData::<T>,
+ })
+ }
+
+ /// register a Regulator driver
+ pub fn register(
+ dev: &device::Device,
+ desc: &'static Desc,
+ mut config: Config<T>,
+ ) -> Result<Self> {
+ config.cfg.driver_data = config.data.into_foreign() as _;
+
+ // SAFETY: By the type invariants, we know that `dev.as_ref().as_raw()` is always
+ // valid and non-null, and the descriptor and config are guaranteed to be valid values,
+ // hence it is safe to perform the FFI call.
+ let rdev = from_err_ptr(unsafe {
+ bindings::regulator_register(dev.as_raw(), &desc.0, &config.cfg)
+ })?;
+
+ Ok(Self {
+ rdev: NonNull::new(rdev).ok_or(EINVAL)?,
+ _data_type: PhantomData::<T>,
+ })
+ }
+
+ /// List voltages when the regulator is using linear mapping
+ pub fn list_voltage_linear(&self, selector: u32) -> Result<i32> {
+ // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
+ // The C function is safe to call with any selector values.
+ let ret = unsafe { bindings::regulator_list_voltage_linear(self.rdev.as_ptr(), selector) };
+ if ret < 0 {
+ return Err(Error::from_errno(ret));
+ }
+ Ok(ret)
+ }
+
+ /// Get regulator's name
+ pub fn get_name(&self) -> &'static CStr {
+ // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
+ // The C function is guaranteed to return a valid string.
+ unsafe { CStr::from_char_ptr(bindings::rdev_get_name(self.rdev.as_ptr())) }
+ }
+
+ /// Get regulator's ID
+ pub fn get_id(&self) -> i32 {
+ // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
+ unsafe { bindings::rdev_get_id(self.rdev.as_ptr()) }
+ }
+
+ /// Retrieve driver data associated to `self`
+ pub fn data(&self) -> T::Borrowed<'_> {
+ // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
+ unsafe { T::borrow(bindings::rdev_get_drvdata(self.rdev.as_ptr())) }
+ }
+}
+
+impl<T: ForeignOwnable + Send + Sync> Drop for Device<T> {
+ fn drop(&mut self) {
+ // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
+ // so it is safe to perform the FFI call.
+ unsafe { bindings::regulator_unregister(self.rdev.as_ptr()) };
+
+ // SAFETY: The type invariants garuantee that `self.rdev` is valid and non-null, and
+ // that `rdev_get_drvdata` is valid memory of type `T` stored there by calling
+ // `T::into_foreign`.
+ unsafe { T::from_foreign(bindings::rdev_get_drvdata(self.rdev.as_ptr())) };
+ }
+}
+
+// SAFETY: `Device` has sole ownership of `self.rdev` and is never read outside of the C
+// implementation. It is safe to use it from any thread.
+unsafe impl<T: ForeignOwnable + Send + Sync> Send for Device<T> {}
+
+// SAFETY: It is OK to access `Device` through shared references from other threads because
+// the C code is insuring proper synchronization of `self.rdev`.
+unsafe impl<T: ForeignOwnable + Send + Sync> Sync for Device<T> {}
+
+/// [`Device`] type
+pub enum Type {
+ /// Voltage regulator
+ Voltage,
+ /// Current regulator
+ Current,
+}
+
+pub(crate) struct Adapter<T>(PhantomData<T>);
+
+impl<T: Driver> Adapter<T> {
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn list_voltage_callback(
+ rdev: *mut bindings::regulator_dev,
+ selector: core::ffi::c_uint,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| T::list_voltage(&mut rdev, selector))
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` and `selector` must be non-null and valid.
+ unsafe extern "C" fn set_voltage_callback(
+ rdev: *mut bindings::regulator_dev,
+ min_uv: core::ffi::c_int,
+ max_uv: core::ffi::c_int,
+ selector: *mut core::ffi::c_uint,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ match T::set_voltage(&mut rdev, min_uv, max_uv) {
+ Ok(v) => {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ unsafe { *selector = v as _ };
+ 0
+ }
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn map_voltage_callback(
+ rdev: *mut bindings::regulator_dev,
+ min_uv: core::ffi::c_int,
+ max_uv: core::ffi::c_int,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| T::map_voltage(&mut rdev, min_uv, max_uv))
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn set_voltage_sel_callback(
+ rdev: *mut bindings::regulator_dev,
+ selector: core::ffi::c_uint,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ T::set_voltage_sel(&mut rdev, selector)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn get_voltage_callback(
+ rdev: *mut bindings::regulator_dev,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| T::get_voltage(&mut rdev))
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn get_voltage_sel_callback(
+ rdev: *mut bindings::regulator_dev,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| T::get_voltage_sel(&mut rdev))
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn set_current_limit_callback(
+ rdev: *mut bindings::regulator_dev,
+ min_ua: core::ffi::c_int,
+ max_ua: core::ffi::c_int,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ T::set_current_limit(&mut rdev, min_ua, max_ua)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn get_current_limit_callback(
+ rdev: *mut bindings::regulator_dev,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| T::get_current_limit(&mut rdev))
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn set_active_discharge_callback(
+ rdev: *mut bindings::regulator_dev,
+ enable: bool,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ T::set_active_discharge(&mut rdev, enable)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn enable_callback(rdev: *mut bindings::regulator_dev) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ T::enable(&mut rdev)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn disable_callback(rdev: *mut bindings::regulator_dev) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ T::disable(&mut rdev)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn is_enabled_callback(
+ rdev: *mut bindings::regulator_dev,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ T::is_enabled(&mut rdev)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn set_mode_callback(
+ rdev: *mut bindings::regulator_dev,
+ mode: core::ffi::c_uint,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ let mode = Mode::try_from(mode).unwrap_or(Mode::Invalid);
+ T::set_mode(&mut rdev, mode)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn get_mode_callback(
+ rdev: *mut bindings::regulator_dev,
+ ) -> core::ffi::c_uint {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ T::get_mode(&mut rdev) as _
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn get_status_callback(
+ rdev: *mut bindings::regulator_dev,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| Ok(T::get_status(&mut rdev)? as _))
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn set_suspend_voltage_callback(
+ rdev: *mut bindings::regulator_dev,
+ uv: core::ffi::c_int,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ T::set_suspend_voltage(&mut rdev, uv)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn set_suspend_enable_callback(
+ rdev: *mut bindings::regulator_dev,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ T::set_suspend_enable(&mut rdev)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn set_suspend_disable_callback(
+ rdev: *mut bindings::regulator_dev,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ T::set_suspend_disable(&mut rdev)?;
+ Ok(0)
+ })
+ }
+
+ /// # Safety
+ ///
+ /// `rdev` must be non-null and valid.
+ unsafe extern "C" fn set_suspend_mode_callback(
+ rdev: *mut bindings::regulator_dev,
+ mode: core::ffi::c_uint,
+ ) -> core::ffi::c_int {
+ // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
+ let mut rdev = unsafe { Device::from_raw(rdev) };
+ from_result(|| {
+ let mode = Mode::try_from(mode).unwrap_or(Mode::Invalid);
+ T::set_suspend_mode(&mut rdev, mode)?;
+ Ok(0)
+ })
+ }
+
+ const VTABLE: bindings::regulator_ops = bindings::regulator_ops {
+ list_voltage: if T::HAS_LIST_VOLTAGE {
+ Some(Adapter::<T>::list_voltage_callback)
+ } else {
+ None
+ },
+ set_voltage: if T::HAS_SET_VOLTAGE {
+ Some(Adapter::<T>::set_voltage_callback)
+ } else {
+ None
+ },
+ map_voltage: if T::HAS_MAP_VOLTAGE {
+ Some(Adapter::<T>::map_voltage_callback)
+ } else {
+ None
+ },
+ set_voltage_sel: if T::HAS_SET_VOLTAGE_SEL {
+ Some(Adapter::<T>::set_voltage_sel_callback)
+ } else {
+ None
+ },
+ get_voltage: if T::HAS_GET_VOLTAGE {
+ Some(Adapter::<T>::get_voltage_callback)
+ } else {
+ None
+ },
+ get_voltage_sel: if T::HAS_GET_VOLTAGE_SEL {
+ Some(Adapter::<T>::get_voltage_sel_callback)
+ } else {
+ None
+ },
+ set_current_limit: if T::HAS_SET_CURRENT_LIMIT {
+ Some(Adapter::<T>::set_current_limit_callback)
+ } else {
+ None
+ },
+ get_current_limit: if T::HAS_GET_CURRENT_LIMIT {
+ Some(Adapter::<T>::get_current_limit_callback)
+ } else {
+ None
+ },
+ set_active_discharge: if T::HAS_SET_ACTIVE_DISCHARGE {
+ Some(Adapter::<T>::set_active_discharge_callback)
+ } else {
+ None
+ },
+ enable: if T::HAS_ENABLE {
+ Some(Adapter::<T>::enable_callback)
+ } else {
+ None
+ },
+ disable: if T::HAS_DISABLE {
+ Some(Adapter::<T>::disable_callback)
+ } else {
+ None
+ },
+ is_enabled: if T::HAS_IS_ENABLED {
+ Some(Adapter::<T>::is_enabled_callback)
+ } else {
+ None
+ },
+ set_mode: if T::HAS_SET_MODE {
+ Some(Adapter::<T>::set_mode_callback)
+ } else {
+ None
+ },
+ get_mode: if T::HAS_GET_MODE {
+ Some(Adapter::<T>::get_mode_callback)
+ } else {
+ None
+ },
+ get_status: if T::HAS_GET_STATUS {
+ Some(Adapter::<T>::get_status_callback)
+ } else {
+ None
+ },
+ set_suspend_voltage: if T::HAS_SET_SUSPEND_VOLTAGE {
+ Some(Adapter::<T>::set_suspend_voltage_callback)
+ } else {
+ None
+ },
+ set_suspend_enable: if T::HAS_SET_SUSPEND_ENABLE {
+ Some(Adapter::<T>::set_suspend_enable_callback)
+ } else {
+ None
+ },
+ set_suspend_disable: if T::HAS_SET_SUSPEND_DISABLE {
+ Some(Adapter::<T>::set_suspend_disable_callback)
+ } else {
+ None
+ },
+ set_suspend_mode: if T::HAS_SET_SUSPEND_MODE {
+ Some(Adapter::<T>::set_suspend_mode_callback)
+ } else {
+ None
+ },
+ // SAFETY: The rest is zeroed out to initialize `struct regulator_ops`,
+ // sets `Option<&F>` to be `None`.
+ ..unsafe { core::mem::zeroed() }
+ };
+
+ const fn build() -> &'static bindings::regulator_ops {
+ &Self::VTABLE
+ }
+}
--
2.45.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 6/9] rust: regulator: add support for regmap
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
` (4 preceding siblings ...)
2024-12-18 23:36 ` [PATCH 5/9] rust: regulator: add Regulator Driver abstraction Fabien Parent
@ 2024-12-18 23:36 ` Fabien Parent
2024-12-18 23:36 ` [PATCH 7/9] dt-bindings: regulator: add binding for ncv6336 regulator Fabien Parent
` (2 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
From: Fabien Parent <fabien.parent@linaro.org>
The regulator API offer many helpers to help simplifies drivers that
use the regmap API. This commit adds partial support for it, only the
function needed by the NCV6336 driver were added.
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
rust/kernel/regulator/driver.rs | 141 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 141 insertions(+)
diff --git a/rust/kernel/regulator/driver.rs b/rust/kernel/regulator/driver.rs
index 8079ea28fd5bf7b6871a0b1d2cea7a6fffcb43ca..e79e93122b094e5e086780f18ecdac5105d07153 100644
--- a/rust/kernel/regulator/driver.rs
+++ b/rust/kernel/regulator/driver.rs
@@ -37,13 +37,27 @@
device,
error::{code::*, from_err_ptr, from_result, Error, Result},
macros::vtable,
+ private::Sealed,
regulator::Mode,
str::CStr,
+ sync::Arc,
types::ForeignOwnable,
ThisModule,
};
+#[cfg(CONFIG_REGMAP)]
+use crate::{error::to_result, regmap::Regmap};
use core::{marker::PhantomData, mem::ManuallyDrop, ptr::NonNull};
+#[cfg(not(CONFIG_REGMAP))]
+struct Regmap;
+
+#[cfg(not(CONFIG_REGMAP))]
+impl Regmap {
+ pub fn as_raw(&self) -> *mut bindings::regmap {
+ core::ptr::null_mut()
+ }
+}
+
/// [`Device`]'s status
#[derive(Eq, PartialEq)]
pub enum Status {
@@ -357,6 +371,7 @@ unsafe impl Sync for Desc {}
pub struct Config<T: ForeignOwnable + Send + Sync = ()> {
cfg: bindings::regulator_config,
data: T,
+ regmap: Option<Arc<Regmap>>,
}
impl<T: ForeignOwnable + Send + Sync> Config<T> {
@@ -368,8 +383,16 @@ pub fn new(dev: &device::Device, data: T) -> Self {
..Default::default()
},
data,
+ regmap: None,
}
}
+
+ /// Assign a regmap device to the config
+ #[cfg(CONFIG_REGMAP)]
+ pub fn with_regmap(mut self, regmap: Arc<Regmap>) -> Self {
+ self.regmap = Some(regmap);
+ self
+ }
}
/// Regulator device
@@ -384,6 +407,9 @@ pub fn new(dev: &device::Device, data: T) -> Self {
pub struct Device<T: ForeignOwnable + Send + Sync> {
rdev: NonNull<bindings::regulator_dev>,
_data_type: PhantomData<T>,
+ // The C regmap API does not keep reference count. Keep a reference to the regmap pointer that
+ // is shared to the C regulator API.
+ _regmap: Option<Arc<Regmap>>,
}
impl<T: ForeignOwnable + Send + Sync> Device<T> {
@@ -396,6 +422,7 @@ unsafe fn from_raw(rdev: *mut bindings::regulator_dev) -> ManuallyDrop<Self> {
// valid..
rdev: unsafe { NonNull::new_unchecked(rdev) },
_data_type: PhantomData::<T>,
+ _regmap: None,
})
}
@@ -407,6 +434,11 @@ pub fn register(
) -> Result<Self> {
config.cfg.driver_data = config.data.into_foreign() as _;
+ let regmap = config.regmap.take();
+ if let Some(regmap) = ®map {
+ config.cfg.regmap = regmap.as_raw() as _;
+ };
+
// SAFETY: By the type invariants, we know that `dev.as_ref().as_raw()` is always
// valid and non-null, and the descriptor and config are guaranteed to be valid values,
// hence it is safe to perform the FFI call.
@@ -417,6 +449,7 @@ pub fn register(
Ok(Self {
rdev: NonNull::new(rdev).ok_or(EINVAL)?,
_data_type: PhantomData::<T>,
+ _regmap: regmap,
})
}
@@ -472,6 +505,114 @@ unsafe impl<T: ForeignOwnable + Send + Sync> Send for Device<T> {}
// the C code is insuring proper synchronization of `self.rdev`.
unsafe impl<T: ForeignOwnable + Send + Sync> Sync for Device<T> {}
+impl<T: ForeignOwnable + Send + Sync> Sealed for Device<T> {}
+
+/// Helper functions to implement some of the [`Driver`] trait methods using [`Regmap`].
+///
+/// This trait is implemented by [`Device`] and is Sealed to prevent
+/// to be implemented by anyone else.
+#[cfg(CONFIG_REGMAP)]
+pub trait RegmapHelpers: Sealed {
+ /// Implementation of [`Driver::get_voltage_sel`] using [`Regmap`].
+ fn get_voltage_sel_regmap(&self) -> Result<i32>;
+ /// Implementation of [`Driver::set_voltage_sel`] using [`Regmap`].
+ fn set_voltage_sel_regmap(&self, sel: u32) -> Result;
+
+ /// Implementation of [`Driver::is_enabled`] using [`Regmap`].
+ ///
+ /// [`Desc::with_enable`] or [`Desc::with_inverted_enable`] must have been called
+ /// to setup the fields required by regmap.
+ fn is_enabled_regmap(&self) -> Result<bool>;
+ /// Implementation of [`Driver::enable`] using [`Regmap`].
+ ///
+ /// [`Desc::with_enable`] or [`Desc::with_inverted_enable`] must have been called
+ /// to setup the fields required by regmap.
+ fn enable_regmap(&self) -> Result;
+ /// Implementation of [`Driver::disable`] using [`Regmap`].
+ ///
+ /// [`Desc::with_enable`] or [`Desc::with_inverted_enable`] must have been called
+ /// to setup the fields required by regmap.
+ fn disable_regmap(&self) -> Result;
+
+ /// Implementation of [`Driver::set_active_discharge`] using [`Regmap`].
+ ///
+ /// [`Desc::with_active_discharge`] must have been called to setup the fields required
+ /// by regmap.
+ fn set_active_discharge_regmap(&self, enable: bool) -> Result;
+
+ /// Implementation of [`Driver::set_current_limit`] using [`Regmap`].
+ fn set_current_limit_regmap(&self, min_ua: i32, max_ua: i32) -> Result;
+ /// Implementation of [`Driver::get_current_limit`] using [`Regmap`].
+ fn get_current_limit_regmap(&self) -> Result<i32>;
+}
+
+#[cfg(CONFIG_REGMAP)]
+impl<T: ForeignOwnable + Send + Sync> RegmapHelpers for Device<T> {
+ fn get_voltage_sel_regmap(&self) -> Result<i32> {
+ // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
+ // so it is safe to perform the FFI call.
+ let ret = unsafe { bindings::regulator_get_voltage_sel_regmap(self.rdev.as_ptr()) };
+ if ret < 0 {
+ return Err(Error::from_errno(ret));
+ }
+ Ok(ret)
+ }
+
+ fn set_voltage_sel_regmap(&self, sel: u32) -> Result {
+ // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
+ // so it is safe to perform the FFI call.
+ to_result(unsafe { bindings::regulator_set_voltage_sel_regmap(self.rdev.as_ptr(), sel) })
+ }
+
+ fn is_enabled_regmap(&self) -> Result<bool> {
+ // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
+ // so it is safe to perform the FFI call.
+ let ret = unsafe { bindings::regulator_is_enabled_regmap(self.rdev.as_ptr()) };
+ if ret < 0 {
+ return Err(Error::from_errno(ret));
+ }
+ Ok(ret > 0)
+ }
+
+ fn enable_regmap(&self) -> Result {
+ // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
+ // so it is safe to perform the FFI call.
+ to_result(unsafe { bindings::regulator_enable_regmap(self.rdev.as_ptr()) })
+ }
+
+ fn disable_regmap(&self) -> Result {
+ // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
+ // so it is safe to perform the FFI call.
+ to_result(unsafe { bindings::regulator_disable_regmap(self.rdev.as_ptr()) })
+ }
+
+ fn set_active_discharge_regmap(&self, enable: bool) -> Result {
+ // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
+ // so it is safe to perform the FFI call.
+ to_result(unsafe {
+ bindings::regulator_set_active_discharge_regmap(self.rdev.as_ptr(), enable)
+ })
+ }
+
+ fn set_current_limit_regmap(&self, min_ua: i32, max_ua: i32) -> Result {
+ // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
+ // so it is safe to perform the FFI call.
+ to_result(unsafe {
+ bindings::regulator_set_current_limit_regmap(self.rdev.as_ptr(), min_ua, max_ua)
+ })
+ }
+
+ fn get_current_limit_regmap(&self) -> Result<i32> {
+ // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
+ // so it is safe to perform the FFI call.
+ let ret = unsafe { bindings::regulator_get_current_limit_regmap(self.rdev.as_ptr()) };
+ if ret < 0 {
+ return Err(Error::from_errno(ret));
+ }
+ Ok(ret)
+ }
+}
+
/// [`Device`] type
pub enum Type {
/// Voltage regulator
--
2.45.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 7/9] dt-bindings: regulator: add binding for ncv6336 regulator
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
` (5 preceding siblings ...)
2024-12-18 23:36 ` [PATCH 6/9] rust: regulator: add support for regmap Fabien Parent
@ 2024-12-18 23:36 ` Fabien Parent
2024-12-19 9:28 ` Krzysztof Kozlowski
2024-12-18 23:36 ` [PATCH 8/9] regulator: add driver " Fabien Parent
2024-12-18 23:36 ` [PATCH 9/9] arm64: dts: qcom: apq8039-t2: add node " Fabien Parent
8 siblings, 1 reply; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
From: Fabien Parent <fabien.parent@linaro.org>
Add binding documentation for the Onsemi NCV6336 regulator.
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
.../bindings/regulator/onnn,ncv6336.yaml | 54 ++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/Documentation/devicetree/bindings/regulator/onnn,ncv6336.yaml b/Documentation/devicetree/bindings/regulator/onnn,ncv6336.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..c69d126cab33668febe18e77bb34bd4bef52c993
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/onnn,ncv6336.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/onnn,ncv6336.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Onsemi NCV6336 Buck converter
+
+maintainers:
+ - Fabien Parent <fabien.parent@linaro.org>
+
+description: |
+ The NCV6336 is an I2C programmable BUCK (step-down) converter.
+ It is designed for mobile power applications.
+
+properties:
+ $nodename:
+ pattern: "regulator@[0-9a-f]{2}"
+
+ compatible:
+ const: onnn,ncv6336
+
+ reg:
+ maxItems: 1
+
+ buck:
+ description: buck regulator description
+ type: object
+ $ref: regulator.yaml#
+ unevaluatedProperties: false
+
+required:
+ - compatible
+ - reg
+ - buck
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ regulator@1c {
+ compatible = "onnn,ncv6336";
+ reg = <0x1c>;
+
+ buck {
+ regulator-name = "ncv6336,buck";
+ };
+ };
+ };
+...
--
2.45.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 8/9] regulator: add driver for ncv6336 regulator
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
` (6 preceding siblings ...)
2024-12-18 23:36 ` [PATCH 7/9] dt-bindings: regulator: add binding for ncv6336 regulator Fabien Parent
@ 2024-12-18 23:36 ` Fabien Parent
2024-12-19 10:19 ` Dirk Behme
2024-12-20 14:50 ` Mark Brown
2024-12-18 23:36 ` [PATCH 9/9] arm64: dts: qcom: apq8039-t2: add node " Fabien Parent
8 siblings, 2 replies; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
From: Fabien Parent <fabien.parent@linaro.org>
This commit adds support for the Onsemi NCV6336 buck down converter.
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
drivers/regulator/Kconfig | 7 +
drivers/regulator/Makefile | 1 +
drivers/regulator/ncv6336_regulator.rs | 241 +++++++++++++++++++++++++++++++++
3 files changed, 249 insertions(+)
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 39297f7d8177193e51c99bc2b360c6d9936e62fe..7254a6e1d7539b147b7ba00ebcb6fd92eb6b2616 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -952,6 +952,13 @@ config REGULATOR_MTK_DVFSRC
of Mediatek. It allows for voting on regulator state
between multiple users.
+config REGULATOR_NCV6336
+ tristate "Onsemi NCV6336 regulator driver"
+ depends on RUST && OF && I2C=y
+ select REGMAP_I2C
+ help
+ Say y here to support the Onsemi NCV6336 buck converter.
+
config REGULATOR_PALMAS
tristate "TI Palmas PMIC Regulators"
depends on MFD_PALMAS
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 3d5a803dce8a0556ba9557fa069c6e37593b3c69..0309a78b820cc85883c0c129801ab713e08e4391 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -113,6 +113,7 @@ obj-$(CONFIG_REGULATOR_MT6370) += mt6370-regulator.o
obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
+obj-$(CONFIG_REGULATOR_NCV6336) += ncv6336_regulator.o
obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_PM8008) += qcom-pm8008-regulator.o
obj-$(CONFIG_REGULATOR_QCOM_REFGEN) += qcom-refgen-regulator.o
diff --git a/drivers/regulator/ncv6336_regulator.rs b/drivers/regulator/ncv6336_regulator.rs
new file mode 100644
index 0000000000000000000000000000000000000000..7efd7630792b68fb353ed1b1634980def9e326a1
--- /dev/null
+++ b/drivers/regulator/ncv6336_regulator.rs
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Driver for the Onsemi Buck Converter NCV6336
+//!
+//! Datasheet: https://www.onsemi.com/pdf/datasheet/ncv6336bm-d.pdf
+
+use kernel::{
+ c_str, i2c, of,
+ prelude::*,
+ regmap::{self, BitFieldReadOps, BitFieldWriteOps, RawFieldWriteOps},
+ regulator::{
+ driver::{Config, Desc, Device, Driver, RegmapHelpers, Status, Type},
+ Mode,
+ },
+ sync::{new_mutex, Arc, Mutex},
+};
+use register::*;
+
+kernel::module_i2c_driver! {
+ type: Ncv6336,
+ name: "ncv6336",
+ author: "Fabien Parent <fabien.parent@linaro.org>",
+ license: "GPL",
+}
+
+kernel::i2c_device_table!(
+ I2C_ID_TABLE,
+ MODULE_I2C_ID_TABLE,
+ <Ncv6336 as i2c::Driver>::IdInfo,
+ [(i2c::DeviceId::new(c_str!("ncv6336")), ()),]
+);
+
+kernel::of_device_table!(
+ OF_ID_TABLE,
+ MODULE_OF_ID_TABLE,
+ <Ncv6336 as i2c::Driver>::IdInfo,
+ [(of::DeviceId::new(c_str!("onnn,ncv6336")), ()),]
+);
+
+regmap::define_regmap_field_descs!(FIELD_DESCS, {
+ (pid, 0x3, READ, { value => raw([7:0], ro) }),
+ (rid, 0x4, READ, { value => raw([7:0], ro) }),
+ (fid, 0x5, READ, { value => raw([7:0], ro) }),
+ (progvsel1, 0x10, RW, {
+ voutvsel1 => raw([6:0], rw),
+ envsel1 => bit(7, rw),
+ }),
+ (progvsel0, 0x11, RW, {
+ voutvsel0 => raw([6:0], rw),
+ envsel0 => bit(7, rw),
+ }),
+ (pgood, 0x12, RW, { dischg => bit(4, rw) }),
+ (command, 0x14, RW, {
+ vselgt => bit(0, rw),
+ pwmvsel1 => bit(6, rw),
+ pwmvsel0 => bit(7, rw),
+ }),
+ (limconf, 0x16, RW, {
+ rearm => bit(0, rw),
+ rststatus => bit(1, rw),
+ tpwth => enum([5:4], rw, {
+ Temp83C = 0x0,
+ Temp94C = 0x1,
+ Temp105C = 0x2,
+ Temp116C = 0x3,
+ }),
+ ipeak => enum([7:6], rw, {
+ Peak3p5A = 0x0,
+ Peak4p0A = 0x1,
+ Peak4p5A = 0x2,
+ Peak5p0A = 0x3,
+ }),
+ })
+});
+
+static NCV6336_DESC: Desc = Desc::new::<Ncv6336>(c_str!("ncv6336"), Type::Voltage)
+ .with_owner(&THIS_MODULE)
+ .with_of_match(c_str!("buck"))
+ .with_active_discharge(
+ pgood::addr(),
+ pgood::dischg::mask(),
+ pgood::dischg::mask(),
+ 0,
+ )
+ .with_csel(
+ limconf::addr(),
+ limconf::ipeak::mask(),
+ &[3_500_000, 4_000_000, 4_500_000, 5_000_000],
+ )
+ .with_enable(
+ progvsel0::addr(),
+ progvsel0::envsel0::mask(),
+ progvsel0::envsel0::mask(),
+ 0,
+ )
+ .with_linear_mapping(
+ progvsel0::addr(),
+ progvsel0::voutvsel0::mask(),
+ 600_000,
+ 6250,
+ 128,
+ 0,
+ );
+
+struct Ncv6336RegulatorData {
+ fields: regmap::Fields<{ FIELD_DESCS.len() }>,
+}
+
+struct Ncv6336(#[expect(dead_code)] Device<<Self as Driver>::Data>);
+
+impl i2c::Driver for Ncv6336 {
+ type IdInfo = ();
+
+ const I2C_ID_TABLE: Option<i2c::IdTable<Self::IdInfo>> = Some(&I2C_ID_TABLE);
+ const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_ID_TABLE);
+
+ fn probe(client: &mut i2c::Client, _id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
+ let config = regmap::Config::<AccessOps>::new(8, 8)
+ .with_max_register(0x16)
+ .with_cache_type(regmap::CacheType::RbTree);
+ let regmap = Arc::new(regmap::Regmap::init_i2c(client, &config)?, GFP_KERNEL)?;
+ let fields = regmap::Fields::new(®map, &FIELD_DESCS)?;
+
+ let data = Arc::pin_init(new_mutex!(Ncv6336RegulatorData { fields }), GFP_KERNEL)?;
+ let config = Config::new(client.as_ref(), data.clone()).with_regmap(regmap.clone());
+ let regulator = Device::register(client.as_ref(), &NCV6336_DESC, config)?;
+
+ let drvdata = KBox::new(Self(regulator), GFP_KERNEL)?;
+
+ Ok(drvdata.into())
+ }
+}
+
+#[vtable]
+impl Driver for Ncv6336 {
+ type Data = Arc<Mutex<Ncv6336RegulatorData>>;
+
+ fn list_voltage(reg: &mut Device<Self::Data>, selector: u32) -> Result<i32> {
+ reg.list_voltage_linear(selector)
+ }
+
+ fn enable(reg: &mut Device<Self::Data>) -> Result {
+ reg.enable_regmap()
+ }
+
+ fn disable(reg: &mut Device<Self::Data>) -> Result {
+ reg.disable_regmap()
+ }
+
+ fn is_enabled(reg: &mut Device<Self::Data>) -> Result<bool> {
+ reg.is_enabled_regmap()
+ }
+
+ fn set_active_discharge(reg: &mut Device<Self::Data>, enable: bool) -> Result {
+ reg.set_active_discharge_regmap(enable)
+ }
+
+ fn set_current_limit(reg: &mut Device<Self::Data>, min_ua: i32, max_ua: i32) -> Result {
+ reg.set_current_limit_regmap(min_ua, max_ua)
+ }
+
+ fn get_current_limit(reg: &mut Device<Self::Data>) -> Result<i32> {
+ reg.get_current_limit_regmap()
+ }
+
+ fn set_voltage_sel(reg: &mut Device<Self::Data>, selector: u32) -> Result {
+ reg.set_voltage_sel_regmap(selector)
+ }
+
+ fn get_voltage_sel(reg: &mut Device<Self::Data>) -> Result<i32> {
+ reg.get_voltage_sel_regmap()
+ }
+
+ fn set_mode(reg: &mut Device<Self::Data>, mode: Mode) -> Result {
+ let data = reg.data();
+ let fields = &mut data.lock().fields;
+
+ match mode {
+ Mode::Normal => command::pwmvsel0::clear(fields),
+ Mode::Fast => command::pwmvsel0::set(fields),
+ _ => Err(ENOTSUPP),
+ }
+ }
+
+ fn get_mode(reg: &mut Device<Self::Data>) -> Mode {
+ let data = reg.data();
+ let fields = &mut data.lock().fields;
+
+ match command::pwmvsel0::is_set(fields) {
+ Ok(true) => Mode::Fast,
+ Ok(false) => Mode::Normal,
+ Err(_) => Mode::Invalid,
+ }
+ }
+
+ fn get_status(reg: &mut Device<Self::Data>) -> Result<Status> {
+ if !Self::is_enabled(reg)? {
+ return Ok(Status::Off);
+ }
+
+ Ok(Self::get_mode(reg).into())
+ }
+
+ fn set_suspend_voltage(reg: &mut Device<Self::Data>, uv: i32) -> Result {
+ let data = reg.data();
+ let fields = &mut data.lock().fields;
+
+ let quot = (uv - 600000) / 6250;
+ let rem = (uv - 600000) % 6250;
+ let selector = if rem > 0 { quot + 1 } else { quot };
+
+ progvsel1::voutvsel1::write(fields, selector as _)
+ }
+
+ fn set_suspend_enable(reg: &mut Device<Self::Data>) -> Result {
+ let data = reg.data();
+ let fields = &mut data.lock().fields;
+
+ progvsel1::envsel1::set(fields)?;
+ command::vselgt::clear(fields)
+ }
+
+ fn set_suspend_disable(reg: &mut Device<Self::Data>) -> Result {
+ let data = reg.data();
+ let fields = &mut data.lock().fields;
+
+ progvsel1::envsel1::clear(fields)?;
+ command::vselgt::set(fields)
+ }
+
+ fn set_suspend_mode(reg: &mut Device<Self::Data>, mode: Mode) -> Result {
+ let data = reg.data();
+ let fields = &mut data.lock().fields;
+
+ match mode {
+ Mode::Normal => command::pwmvsel1::clear(fields),
+ Mode::Fast => command::pwmvsel1::set(fields),
+ _ => Err(ENOTSUPP),
+ }
+ }
+}
--
2.45.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 9/9] arm64: dts: qcom: apq8039-t2: add node for ncv6336 regulator
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
` (7 preceding siblings ...)
2024-12-18 23:36 ` [PATCH 8/9] regulator: add driver " Fabien Parent
@ 2024-12-18 23:36 ` Fabien Parent
8 siblings, 0 replies; 21+ messages in thread
From: Fabien Parent @ 2024-12-18 23:36 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Fabien Parent
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
From: Fabien Parent <fabien.parent@linaro.org>
CPR is using the power rail provided by the ncv6336 buck regulator
on the apq8039 t2 board. This commit adds the required regulator.
Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
---
arch/arm64/boot/dts/qcom/apq8039-t2.dts | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/apq8039-t2.dts b/arch/arm64/boot/dts/qcom/apq8039-t2.dts
index 4f82bb668616f942d65f59a6f418cf38f404df32..2da5b7d01521520a6814b76c02329be3bdedb4fd 100644
--- a/arch/arm64/boot/dts/qcom/apq8039-t2.dts
+++ b/arch/arm64/boot/dts/qcom/apq8039-t2.dts
@@ -111,6 +111,23 @@ typec_ep: endpoint {
};
};
+&blsp_i2c4 {
+ status = "okay";
+
+ regulator@1c {
+ compatible = "onnn,ncv6336";
+ reg = <0x1c>;
+ pinctrl-0 = <&ncv6336_vsel>;
+ pinctrl-names = "default";
+
+ buck {
+ regulator-name = "ncv6336,buck";
+ regulator-min-microvolt = <600000>;
+ regulator-max-microvolt = <1393750>;
+ };
+ };
+};
+
&blsp_i2c5 {
status = "okay";
};
@@ -371,6 +388,12 @@ typec_irq: typec-irq-state {
pins = "gpio107";
bias-pull-up;
};
+
+ ncv6336_vsel: ncv6336-state {
+ function = "gpio";
+ pins = "gpio111";
+ output-low;
+ };
};
&usb {
--
2.45.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 7/9] dt-bindings: regulator: add binding for ncv6336 regulator
2024-12-18 23:36 ` [PATCH 7/9] dt-bindings: regulator: add binding for ncv6336 regulator Fabien Parent
@ 2024-12-19 9:28 ` Krzysztof Kozlowski
2024-12-19 16:13 ` Fabien Parent
0 siblings, 1 reply; 21+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-19 9:28 UTC (permalink / raw)
To: Fabien Parent
Cc: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
On Wed, Dec 18, 2024 at 03:36:37PM -0800, Fabien Parent wrote:
> From: Fabien Parent <fabien.parent@linaro.org>
>
> Add binding documentation for the Onsemi NCV6336 regulator.
>
> Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> ---
> .../bindings/regulator/onnn,ncv6336.yaml | 54 ++++++++++++++++++++++
> 1 file changed, 54 insertions(+)
>
subject: regulator first, then dt-bindings.
Please use subject prefixes matching the subsystem. You can get them for
example with 'git log --oneline -- DIRECTORY_OR_FILE' on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> diff --git a/Documentation/devicetree/bindings/regulator/onnn,ncv6336.yaml b/Documentation/devicetree/bindings/regulator/onnn,ncv6336.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..c69d126cab33668febe18e77bb34bd4bef52c993
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/onnn,ncv6336.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/onnn,ncv6336.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Onsemi NCV6336 Buck converter
> +
> +maintainers:
> + - Fabien Parent <fabien.parent@linaro.org>
> +
> +description: |
Do not need '|' unless you need to preserve formatting.
> + The NCV6336 is an I2C programmable BUCK (step-down) converter.
> + It is designed for mobile power applications.
> +
> +properties:
> + $nodename:
> + pattern: "regulator@[0-9a-f]{2}"
Drop nodename, not really needed in device schema.
> +
> + compatible:
> + const: onnn,ncv6336
> +
> + reg:
> + maxItems: 1
> +
> + buck:
> + description: buck regulator description
Why do you need "buck" node? Just merge the properties into this device
node.
> + type: object
> + $ref: regulator.yaml#
> + unevaluatedProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - buck
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + regulator@1c {
> + compatible = "onnn,ncv6336";
> + reg = <0x1c>;
> +
> + buck {
> + regulator-name = "ncv6336,buck";
> + };
> + };
Messed indentation.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 8/9] regulator: add driver for ncv6336 regulator
2024-12-18 23:36 ` [PATCH 8/9] regulator: add driver " Fabien Parent
@ 2024-12-19 10:19 ` Dirk Behme
2024-12-20 14:50 ` Mark Brown
1 sibling, 0 replies; 21+ messages in thread
From: Dirk Behme @ 2024-12-19 10:19 UTC (permalink / raw)
To: Fabien Parent, Rob Herring, Saravana Kannan, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang, Mark Brown,
Liam Girdwood, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio
Cc: devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
Hi Fabien,
you might have noticed that I'm using your development branches of
this since some time. So sending these patches for review is a big
step! Many thanks!
Just one topic to check below:
On 19.12.24 00:36, Fabien Parent wrote:
> From: Fabien Parent <fabien.parent@linaro.org>
>
> This commit adds support for the Onsemi NCV6336 buck down converter.
>
> Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> ---
> drivers/regulator/Kconfig | 7 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/ncv6336_regulator.rs | 241 +++++++++++++++++++++++++++++++++
> 3 files changed, 249 insertions(+)
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 39297f7d8177193e51c99bc2b360c6d9936e62fe..7254a6e1d7539b147b7ba00ebcb6fd92eb6b2616 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -952,6 +952,13 @@ config REGULATOR_MTK_DVFSRC
> of Mediatek. It allows for voting on regulator state
> between multiple users.
>
> +config REGULATOR_NCV6336
> + tristate "Onsemi NCV6336 regulator driver"
> + depends on RUST && OF && I2C=y
> + select REGMAP_I2C
> + help
> + Say y here to support the Onsemi NCV6336 buck converter.
> +
> config REGULATOR_PALMAS
> tristate "TI Palmas PMIC Regulators"
> depends on MFD_PALMAS
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 3d5a803dce8a0556ba9557fa069c6e37593b3c69..0309a78b820cc85883c0c129801ab713e08e4391 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -113,6 +113,7 @@ obj-$(CONFIG_REGULATOR_MT6370) += mt6370-regulator.o
> obj-$(CONFIG_REGULATOR_MT6380) += mt6380-regulator.o
> obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
> obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
> +obj-$(CONFIG_REGULATOR_NCV6336) += ncv6336_regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_PM8008) += qcom-pm8008-regulator.o
> obj-$(CONFIG_REGULATOR_QCOM_REFGEN) += qcom-refgen-regulator.o
> diff --git a/drivers/regulator/ncv6336_regulator.rs b/drivers/regulator/ncv6336_regulator.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..7efd7630792b68fb353ed1b1634980def9e326a1
> --- /dev/null
> +++ b/drivers/regulator/ncv6336_regulator.rs
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Driver for the Onsemi Buck Converter NCV6336
> +//!
> +//! Datasheet: https://www.onsemi.com/pdf/datasheet/ncv6336bm-d.pdf
> +
> +use kernel::{
> + c_str, i2c, of,
> + prelude::*,
> + regmap::{self, BitFieldReadOps, BitFieldWriteOps, RawFieldWriteOps},
> + regulator::{
> + driver::{Config, Desc, Device, Driver, RegmapHelpers, Status, Type},
> + Mode,
> + },
> + sync::{new_mutex, Arc, Mutex},
> +};
> +use register::*;
> +
> +kernel::module_i2c_driver! {
> + type: Ncv6336,
> + name: "ncv6336",
> + author: "Fabien Parent <fabien.parent@linaro.org>",
> + license: "GPL",
> +}
> +
> +kernel::i2c_device_table!(
> + I2C_ID_TABLE,
> + MODULE_I2C_ID_TABLE,
> + <Ncv6336 as i2c::Driver>::IdInfo,
> + [(i2c::DeviceId::new(c_str!("ncv6336")), ()),]
> +);
> +
> +kernel::of_device_table!(
> + OF_ID_TABLE,
> + MODULE_OF_ID_TABLE,
> + <Ncv6336 as i2c::Driver>::IdInfo,
> + [(of::DeviceId::new(c_str!("onnn,ncv6336")), ()),]
> +);
> +
> +regmap::define_regmap_field_descs!(FIELD_DESCS, {
> + (pid, 0x3, READ, { value => raw([7:0], ro) }),
> + (rid, 0x4, READ, { value => raw([7:0], ro) }),
> + (fid, 0x5, READ, { value => raw([7:0], ro) }),
> + (progvsel1, 0x10, RW, {
> + voutvsel1 => raw([6:0], rw),
> + envsel1 => bit(7, rw),
> + }),
> + (progvsel0, 0x11, RW, {
> + voutvsel0 => raw([6:0], rw),
> + envsel0 => bit(7, rw),
> + }),
> + (pgood, 0x12, RW, { dischg => bit(4, rw) }),
> + (command, 0x14, RW, {
> + vselgt => bit(0, rw),
> + pwmvsel1 => bit(6, rw),
> + pwmvsel0 => bit(7, rw),
> + }),
> + (limconf, 0x16, RW, {
> + rearm => bit(0, rw),
> + rststatus => bit(1, rw),
> + tpwth => enum([5:4], rw, {
> + Temp83C = 0x0,
> + Temp94C = 0x1,
> + Temp105C = 0x2,
> + Temp116C = 0x3,
> + }),
> + ipeak => enum([7:6], rw, {
> + Peak3p5A = 0x0,
> + Peak4p0A = 0x1,
> + Peak4p5A = 0x2,
> + Peak5p0A = 0x3,
Could you check to read from or write to the tpwth or ipeak (enum)
above? I've been under the impression that for that Desc & enum need
to Copy & Clone [1]?
[1]
diff --git a/rust/kernel/regmap.rs b/rust/kernel/regmap.rs
index 232fe93df769..d1baf182f53c 100644
--- a/rust/kernel/regmap.rs
+++ b/rust/kernel/regmap.rs
@@ -623,6 +623,7 @@ macro_rules! regmap_field_enum {
kernel::macros::paste! {
#[repr(u32)]
#[allow(non_camel_case_types)]
+ #[derive(Copy, Clone)]
pub(crate) enum [<$field_name _enum>] {
$($k = $v,)+
}
diff --git a/rust/kernel/regulator/driver.rs
b/rust/kernel/regulator/driver.rs
index e79e93122b09..0b6819e46686 100644
--- a/rust/kernel/regulator/driver.rs
+++ b/rust/kernel/regulator/driver.rs
@@ -256,6 +256,7 @@ fn set_suspend_mode(_rdev: &mut
Device<Self::Data>, _mode: Mode) -> Result {
/// # Invariants
///
/// `self.0` has always valid data.
+#[derive(Copy, Clone)]
pub struct Desc(bindings::regulator_desc);
impl Desc {
/// Create a new [`Device`] descriptor
> +static NCV6336_DESC: Desc = Desc::new::<Ncv6336>(c_str!("ncv6336"), Type::Voltage)
> + .with_owner(&THIS_MODULE)
> + .with_of_match(c_str!("buck"))
> + .with_active_discharge(
> + pgood::addr(),
> + pgood::dischg::mask(),
> + pgood::dischg::mask(),
> + 0,
> + )
> + .with_csel(
> + limconf::addr(),
> + limconf::ipeak::mask(),
> + &[3_500_000, 4_000_000, 4_500_000, 5_000_000],
> + )
> + .with_enable(
> + progvsel0::addr(),
> + progvsel0::envsel0::mask(),
> + progvsel0::envsel0::mask(),
> + 0,
> + )
> + .with_linear_mapping(
> + progvsel0::addr(),
> + progvsel0::voutvsel0::mask(),
> + 600_000,
> + 6250,
> + 128,
> + 0,
> + );
> +
> +struct Ncv6336RegulatorData {
> + fields: regmap::Fields<{ FIELD_DESCS.len() }>,
> +}
> +
> +struct Ncv6336(#[expect(dead_code)] Device<<Self as Driver>::Data>);
> +
> +impl i2c::Driver for Ncv6336 {
> + type IdInfo = ();
> +
> + const I2C_ID_TABLE: Option<i2c::IdTable<Self::IdInfo>> = Some(&I2C_ID_TABLE);
> + const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_ID_TABLE);
> +
> + fn probe(client: &mut i2c::Client, _id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
> + let config = regmap::Config::<AccessOps>::new(8, 8)
> + .with_max_register(0x16)
> + .with_cache_type(regmap::CacheType::RbTree);
> + let regmap = Arc::new(regmap::Regmap::init_i2c(client, &config)?, GFP_KERNEL)?;
> + let fields = regmap::Fields::new(®map, &FIELD_DESCS)?;
> +
> + let data = Arc::pin_init(new_mutex!(Ncv6336RegulatorData { fields }), GFP_KERNEL)?;
> + let config = Config::new(client.as_ref(), data.clone()).with_regmap(regmap.clone());
> + let regulator = Device::register(client.as_ref(), &NCV6336_DESC, config)?;
> +
> + let drvdata = KBox::new(Self(regulator), GFP_KERNEL)?;
> +
> + Ok(drvdata.into())
> + }
> +}
> +
> +#[vtable]
> +impl Driver for Ncv6336 {
> + type Data = Arc<Mutex<Ncv6336RegulatorData>>;
> +
> + fn list_voltage(reg: &mut Device<Self::Data>, selector: u32) -> Result<i32> {
> + reg.list_voltage_linear(selector)
> + }
> +
> + fn enable(reg: &mut Device<Self::Data>) -> Result {
> + reg.enable_regmap()
> + }
> +
> + fn disable(reg: &mut Device<Self::Data>) -> Result {
> + reg.disable_regmap()
> + }
> +
> + fn is_enabled(reg: &mut Device<Self::Data>) -> Result<bool> {
> + reg.is_enabled_regmap()
> + }
> +
> + fn set_active_discharge(reg: &mut Device<Self::Data>, enable: bool) -> Result {
> + reg.set_active_discharge_regmap(enable)
> + }
> +
> + fn set_current_limit(reg: &mut Device<Self::Data>, min_ua: i32, max_ua: i32) -> Result {
> + reg.set_current_limit_regmap(min_ua, max_ua)
> + }
> +
> + fn get_current_limit(reg: &mut Device<Self::Data>) -> Result<i32> {
> + reg.get_current_limit_regmap()
> + }
> +
> + fn set_voltage_sel(reg: &mut Device<Self::Data>, selector: u32) -> Result {
> + reg.set_voltage_sel_regmap(selector)
> + }
> +
> + fn get_voltage_sel(reg: &mut Device<Self::Data>) -> Result<i32> {
> + reg.get_voltage_sel_regmap()
> + }
> +
> + fn set_mode(reg: &mut Device<Self::Data>, mode: Mode) -> Result {
> + let data = reg.data();
> + let fields = &mut data.lock().fields;
> +
> + match mode {
> + Mode::Normal => command::pwmvsel0::clear(fields),
> + Mode::Fast => command::pwmvsel0::set(fields),
> + _ => Err(ENOTSUPP),
> + }
> + }
> +
> + fn get_mode(reg: &mut Device<Self::Data>) -> Mode {
> + let data = reg.data();
> + let fields = &mut data.lock().fields;
> +
> + match command::pwmvsel0::is_set(fields) {
> + Ok(true) => Mode::Fast,
> + Ok(false) => Mode::Normal,
> + Err(_) => Mode::Invalid,
> + }
> + }
> +
> + fn get_status(reg: &mut Device<Self::Data>) -> Result<Status> {
> + if !Self::is_enabled(reg)? {
> + return Ok(Status::Off);
> + }
> +
> + Ok(Self::get_mode(reg).into())
> + }
> +
> + fn set_suspend_voltage(reg: &mut Device<Self::Data>, uv: i32) -> Result {
> + let data = reg.data();
> + let fields = &mut data.lock().fields;
> +
> + let quot = (uv - 600000) / 6250;
> + let rem = (uv - 600000) % 6250;
> + let selector = if rem > 0 { quot + 1 } else { quot };
> +
> + progvsel1::voutvsel1::write(fields, selector as _)
> + }
> +
> + fn set_suspend_enable(reg: &mut Device<Self::Data>) -> Result {
> + let data = reg.data();
> + let fields = &mut data.lock().fields;
> +
> + progvsel1::envsel1::set(fields)?;
> + command::vselgt::clear(fields)
> + }
> +
> + fn set_suspend_disable(reg: &mut Device<Self::Data>) -> Result {
> + let data = reg.data();
> + let fields = &mut data.lock().fields;
> +
> + progvsel1::envsel1::clear(fields)?;
> + command::vselgt::set(fields)
> + }
> +
> + fn set_suspend_mode(reg: &mut Device<Self::Data>, mode: Mode) -> Result {
> + let data = reg.data();
> + let fields = &mut data.lock().fields;
> +
> + match mode {
> + Mode::Normal => command::pwmvsel1::clear(fields),
> + Mode::Fast => command::pwmvsel1::set(fields),
> + _ => Err(ENOTSUPP),
> + }
> + }
> +}
>
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 5/9] rust: regulator: add Regulator Driver abstraction
2024-12-18 23:36 ` [PATCH 5/9] rust: regulator: add Regulator Driver abstraction Fabien Parent
@ 2024-12-19 10:26 ` Danilo Krummrich
2024-12-19 16:00 ` Fabien Parent
0 siblings, 1 reply; 21+ messages in thread
From: Danilo Krummrich @ 2024-12-19 10:26 UTC (permalink / raw)
To: Fabien Parent
Cc: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
On Wed, Dec 18, 2024 at 03:36:35PM -0800, Fabien Parent wrote:
> From: Fabien Parent <fabien.parent@linaro.org>
>
> This commit adds a Rust abstraction to write Regulator drivers. Only
> the features used by the NCV6336 driver were added to this abstraction.
>
> Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> ---
> MAINTAINERS | 1 +
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/regulator.rs | 4 +-
> rust/kernel/regulator/driver.rs | 850 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 855 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90c231f0aa7381aa8d206fb94c5d1f013dfcae41..87da43251bf0f20d2b5831345778ead592c407dc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25160,6 +25160,7 @@ F: drivers/regulator/
> F: include/dt-bindings/regulator/
> F: include/linux/regulator/
> F: rust/kernel/regulator.rs
> +F: rust/kernel/regulator/
> K: regulator_get_optional
>
> VOLTAGE AND CURRENT REGULATOR IRQ HELPERS
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index b18d772bc3a0e78d749cc9e5ae81a4237a57f8c5..124129daea73c143c919d05814fc02bb4460ddfd 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -30,6 +30,7 @@
> #include <linux/refcount.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/regulator/driver.h>
> #include <linux/sched.h>
> #include <linux/security.h>
> #include <linux/slab.h>
> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> index d695ac955193efcfda62770784a92d70d606b93d..bd8202fe5702b944201e76553b9496e1d42cb429 100644
> --- a/rust/kernel/regulator.rs
> +++ b/rust/kernel/regulator.rs
> @@ -2,12 +2,14 @@
>
> //! SoC Regulators
Why "SoC", could be a regulator for anything else too, right?
>
> +pub mod driver;
> +
> use crate::{
> bindings,
> error::{code::*, Error, Result},
> };
>
> -/// Regulators operating modes
> +/// [`driver::Device`] operating modes
> #[derive(Copy, Clone)]
> #[repr(u32)]
> pub enum Mode {
> diff --git a/rust/kernel/regulator/driver.rs b/rust/kernel/regulator/driver.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..8079ea28fd5bf7b6871a0b1d2cea7a6fffcb43ca
> --- /dev/null
> +++ b/rust/kernel/regulator/driver.rs
> @@ -0,0 +1,850 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! SoC Device Driver Interface
Should rather be "Regulator Device Driver Interface".
> +//!
> +//! C header: [`include/linux/regulator/driver.h`](srctree/include/linux/regulator/driver.h)
> +//!
> +//! # Examples
> +//!
> +//! ```
> +//! use kernel::regulator::driver::{Config, Desc, Device, Driver, Type};
> +//!
> +//! static DESC: Desc =
> +//! Desc::new::<MyDeviceDriver>(kernel::c_str!("my-regulator-driver"), Type::Voltage);
> +//!
> +//! struct MyDeviceDriver;
> +//!
> +//! #[vtable]
> +//! impl Driver for MyDeviceDriver {
I usually prefer to keep the module prefix for the `Driver` traits, i.e.
`impl regulator::Driver for ...`, this makes things a bit more obvious.
> +//! type Data = ();
> +//!
> +//! // Implement supported `Driver`'s operations here.
> +//!
> +//! // Example:
> +//! fn is_enabled(reg: &mut Device<Self::Data>) -> Result<bool> {
> +//! Ok(true)
> +//! }
> +//! }
> +//!
> +//! impl MyDeviceDriver {
> +//! fn probe(dev: &mut kernel::device::Device) {
> +//! let _ = Device::register(dev, &DESC, Config::<<Self as Driver>::Data>::new(dev, ()));
Like below, this is confusing, the device is immediately unregistered again.
> +//! }
> +//! }
> +//! ```
> +
> +use crate::{
> + device,
> + error::{code::*, from_err_ptr, from_result, Error, Result},
> + macros::vtable,
> + regulator::Mode,
> + str::CStr,
> + types::ForeignOwnable,
> + ThisModule,
> +};
> +use core::{marker::PhantomData, mem::ManuallyDrop, ptr::NonNull};
> +
> +/// [`Device`]'s status
> +#[derive(Eq, PartialEq)]
> +pub enum Status {
> + /// Device is off
> + Off,
> + /// Device is on
> + On,
> + /// Device is in an error state
> + Error,
> + /// Device is on and in Fast mode
> + Fast,
> + /// Device is on and in Normal mode
> + Normal,
> + /// Device is on and in Idle mode
> + Idle,
> + /// Device is on and in Standby mode
> + Standby,
> + /// Device is enabled but not regulating
> + Bypass,
> + /// Device is any other status
> + Undefined,
> +}
> +
> +impl TryFrom<core::ffi::c_uint> for Status {
> + type Error = Error;
> +
> + fn try_from(status: core::ffi::c_uint) -> Result<Self> {
> + match status {
> + bindings::regulator_status_REGULATOR_STATUS_OFF => Ok(Self::Off),
> + bindings::regulator_status_REGULATOR_STATUS_ON => Ok(Self::On),
> + bindings::regulator_status_REGULATOR_STATUS_ERROR => Ok(Self::Error),
> + bindings::regulator_status_REGULATOR_STATUS_FAST => Ok(Self::Fast),
> + bindings::regulator_status_REGULATOR_STATUS_NORMAL => Ok(Self::Normal),
> + bindings::regulator_status_REGULATOR_STATUS_IDLE => Ok(Self::Idle),
> + bindings::regulator_status_REGULATOR_STATUS_STANDBY => Ok(Self::Standby),
> + bindings::regulator_status_REGULATOR_STATUS_BYPASS => Ok(Self::Bypass),
> + bindings::regulator_status_REGULATOR_STATUS_UNDEFINED => Ok(Self::Undefined),
> + _ => Err(EINVAL),
> + }
> + }
> +}
> +
> +impl From<Mode> for Status {
> + fn from(mode: Mode) -> Self {
> + // SAFETY: `regulator_mode_to_status` is a `pure function` that is only doing integer
> + // to integer conversion, hence this function call is safe.
> + let status = unsafe { bindings::regulator_mode_to_status(mode as _) };
> +
> + if status < 0 {
> + Self::Undefined
> + } else {
> + Self::try_from(status as core::ffi::c_uint).unwrap_or(Self::Undefined)
> + }
> + }
> +}
> +
> +/// [`Device`]'s operations
> +#[vtable]
> +pub trait Driver {
> + /// User data that will be accessible to all operations
> + type Data: ForeignOwnable + Send + Sync;
> +
> + /// Return one of the supported voltages, in microvolt; zero if the selector indicates a
> + /// voltage that is unusable by the system; or negative errno. Selectors range from zero to one
> + /// less than the number of voltages supported by the system.
> + fn list_voltage(_rdev: &mut Device<Self::Data>, _selector: u32) -> Result<i32> {
> + Err(ENOTSUPP)
> + }
> +
> + /// Set the voltage for the regulator within the range specified. The driver should select the
> + /// voltage closest to `min_uv`.
> + fn set_voltage(_rdev: &mut Device<Self::Data>, _min_uv: i32, _max_uv: i32) -> Result<i32> {
> + Err(ENOTSUPP)
> + }
> +
> + /// Set the voltage for the regulator using the specified selector.
> + fn set_voltage_sel(_rdev: &mut Device<Self::Data>, _selector: u32) -> Result {
> + Err(ENOTSUPP)
> + }
> +
> + /// Convert a voltage into a selector.
> + fn map_voltage(_rdev: &mut Device<Self::Data>, _min_uv: i32, _max_uv: i32) -> Result<i32> {
> + Err(ENOTSUPP)
> + }
> +
> + /// Get the currently configured voltage for the regulator; Returns
> + /// [`ENOTRECOVERABLE`] if the regulator can't be read at bootup and hasn't been
> + /// set yet.
> + fn get_voltage(_rdev: &mut Device<Self::Data>) -> Result<i32> {
> + Err(ENOTSUPP)
> + }
> +
> + /// Get the currently configured voltage selector for the regulator; Returns
> + /// [`ENOTRECOVERABLE`] if the regulator can't be read at bootup and hasn't been
> + /// set yet.
> + fn get_voltage_sel(_rdev: &mut Device<Self::Data>) -> Result<i32> {
> + Err(ENOTSUPP)
> + }
> +
> + /// Configure a limit for a current-limited regulator.
> + ///
> + /// The driver should select the current closest to `max_ua`.
> + fn set_current_limit(_rdev: &mut Device<Self::Data>, _min_ua: i32, _max_ua: i32) -> Result {
> + Err(ENOTSUPP)
> + }
> +
> + /// Get the configured limit for a current-limited regulator.
> + fn get_current_limit(_rdev: &mut Device<Self::Data>) -> Result<i32> {
> + Err(ENOTSUPP)
> + }
> +
> + /// Enable or disable the active discharge of the regulator.
> + fn set_active_discharge(_rdev: &mut Device<Self::Data>, _enable: bool) -> Result {
> + Err(ENOTSUPP)
> + }
> +
> + /// Configure the regulator as enabled.
> + fn enable(_rdev: &mut Device<Self::Data>) -> Result {
> + Err(ENOTSUPP)
> + }
> +
> + /// Configure the regulator as disabled.
> + fn disable(_rdev: &mut Device<Self::Data>) -> Result {
> + Err(ENOTSUPP)
> + }
> +
> + /// Returns enablement state of the regulator.
> + fn is_enabled(_rdev: &mut Device<Self::Data>) -> Result<bool> {
> + Err(ENOTSUPP)
> + }
> +
> + /// Set the configured operating [`Mode`] for the regulator.
> + fn set_mode(_rdev: &mut Device<Self::Data>, _mode: Mode) -> Result {
> + Err(ENOTSUPP)
> + }
> +
> + /// Get the configured operating [`Mode`] for the regulator.
> + fn get_mode(_rdev: &mut Device<Self::Data>) -> Mode {
> + Mode::Invalid
> + }
> +
> + /// Report the regulator [`Status`].
> + fn get_status(_rdev: &mut Device<Self::Data>) -> Result<Status> {
> + Err(ENOTSUPP)
> + }
> +
> + /// Set the voltage for the regaultor when the system is suspended.
> + fn set_suspend_voltage(_rdev: &mut Device<Self::Data>, _uv: i32) -> Result {
> + Err(ENOTSUPP)
> + }
> +
> + /// Mark the regulator as enabled when the system is suspended.
> + fn set_suspend_enable(_rdev: &mut Device<Self::Data>) -> Result {
> + Err(ENOTSUPP)
> + }
> +
> + /// Mark the regulator as disabled when the system is suspended.
> + fn set_suspend_disable(_rdev: &mut Device<Self::Data>) -> Result {
> + Err(ENOTSUPP)
> + }
> +
> + /// Set the operating mode for the regulator when the system is suspended.
> + fn set_suspend_mode(_rdev: &mut Device<Self::Data>, _mode: Mode) -> Result {
> + Err(ENOTSUPP)
> + }
> +}
> +
> +/// [`Device`]'s descriptor
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::{
> +/// c_str,
> +/// device,
> +/// regulator::driver::{Config, Desc, Device, Driver, Type},
> +/// types::ForeignOwnable,
> +/// };
> +///
> +/// struct MyDeviceDriver;
> +///
> +/// #[vtable]
> +/// impl Driver for MyDeviceDriver {
> +/// type Data = ();
> +/// }
This seems incomplete.
> +///
> +/// static BUCK_DESC: Desc = Desc::new::<MyDeviceDriver>(c_str!("my_driver"), Type::Voltage)
> +/// .with_of_match(c_str!("buck"))
> +/// .with_enable(0x24, 0x1, 0x1, 0);
> +///
> +/// fn example(dev: &mut device::Device, mut config: Config<<MyDeviceDriver as Driver>::Data>) {
> +/// let _ = Device::register(dev, &BUCK_DESC, config);
> +/// }
`example`? This should be within a driver trait with `probe` instead.
> +/// ```
> +///
> +/// # Invariants
> +///
> +/// `self.0` has always valid data.
> +pub struct Desc(bindings::regulator_desc);
I think this needs `#[repr(transparent)]`.
> +impl Desc {
> + /// Create a new [`Device`] descriptor
> + pub const fn new<T: Driver>(name: &'static CStr, reg_type: Type) -> Self {
> + // SAFETY: `bindings::regulator_desc" is safe to initialize with 0s.
> + let mut desc: bindings::regulator_desc = unsafe { core::mem::zeroed() };
> + desc.name = name.as_char_ptr();
> + desc.type_ = match reg_type {
> + Type::Voltage => bindings::regulator_type_REGULATOR_VOLTAGE,
> + Type::Current => bindings::regulator_type_REGULATOR_CURRENT,
> + };
> + desc.ops = Adapter::<T>::build();
> + Self(desc)
> + }
> +
> + /// Setup the register address, mask, and {en,dis}able values
> + pub const fn with_enable(mut self, reg: u32, mask: u32, en_val: u32, dis_val: u32) -> Self {
> + self.0.enable_reg = reg;
> + self.0.enable_mask = mask;
> + self.0.enable_val = en_val;
> + self.0.disable_val = dis_val;
> + self
> + }
> +
> + /// Setup the register address, mask, and {en,dis}able values. {En,Dis}able values are
> + /// inverted, i.e. `dis_val` will be use to enable the regulator while `en_val` will be used
> + /// to disable the regulator.
> + pub const fn with_inverted_enable(
> + mut self,
> + reg: u32,
> + mask: u32,
> + en_val: u32,
> + dis_val: u32,
> + ) -> Self {
> + self.0.enable_is_inverted = true;
> + self.with_enable(reg, mask, en_val, dis_val)
> + }
> +
> + /// Setup the active discharge regiter address, mask, on/off values.
> + pub const fn with_active_discharge(mut self, reg: u32, mask: u32, on: u32, off: u32) -> Self {
> + self.0.active_discharge_on = on;
> + self.0.active_discharge_off = off;
> + self.0.active_discharge_reg = reg;
> + self.0.active_discharge_mask = mask;
> + self
> + }
> +
> + /// Setup the current selection register address, mask, and current table
> + pub const fn with_csel(mut self, reg: u32, mask: u32, table: &'static [u32]) -> Self {
> + self.0.csel_reg = reg;
> + self.0.csel_mask = mask;
> + self.0.curr_table = table.as_ptr();
> + self
> + }
> +
> + /// Voltages are a linear mapping
> + pub const fn with_linear_mapping(
> + mut self,
> + reg: u32,
> + mask: u32,
> + min_uv: u32,
> + uv_step: u32,
> + n_voltages: u32,
> + linear_min_sel: u32,
> + ) -> Self {
> + self.0.vsel_reg = reg;
> + self.0.vsel_mask = mask;
> + self.0.n_voltages = n_voltages;
> + self.0.min_uV = min_uv;
> + self.0.uV_step = uv_step;
> + self.0.linear_min_sel = linear_min_sel;
> + self
> + }
> +
> + /// Set the regulator owner
> + pub const fn with_owner(mut self, owner: &'static ThisModule) -> Self {
> + self.0.owner = owner.as_ptr();
> + self
> + }
> +
> + /// Set the name used to identify the regulator in the DT.
> + pub const fn with_of_match(mut self, of_match: &'static CStr) -> Self {
> + self.0.of_match = of_match.as_char_ptr();
> + self
> + }
> +}
> +
> +// SAFETY: `Desc` cannot be modified after its declaration and owns its data, hence it is safe
> +// to share references between threads.
> +unsafe impl Sync for Desc {}
> +
> +/// [`Device`]'s Config
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::regulator::driver::Config;
> +/// # use kernel::regulator::driver::{Desc, Device};
> +/// # use kernel::{device, sync::Arc};
> +///
> +/// struct DriverData(u32);
> +///
> +/// # fn probe(dev: &device::Device, desc: &'static Desc) -> Result {
> +/// let config = Config::<Arc<DriverData>>::new(dev, Arc::new(DriverData(128), GFP_KERNEL)?);
Why does this need reference counting?
> +/// let reg = Device::register(dev, desc, config)?;
> +/// # Ok(())
> +/// # }
I think this example is a bit misleading, the regulator device is immediately
unregistered after probe() returns.
Here you have to rely on the driver to keep `reg` alive until the device is
dropped. Instead you can use `Devres::new_foreign_owned`, like I do in [1].
[1] https://cgit.freedesktop.org/drm/drm-misc/tree/rust/kernel/drm/drv.rs?h=topic/rust-drm#n173
> +/// ```
> +///
> +/// # Invariants
> +///
> +/// `self.cfg` always hold valid data.
> +pub struct Config<T: ForeignOwnable + Send + Sync = ()> {
> + cfg: bindings::regulator_config,
> + data: T,
> +}
> +
> +impl<T: ForeignOwnable + Send + Sync> Config<T> {
> + /// Create a [`Device`] config.
> + pub fn new(dev: &device::Device, data: T) -> Self {
> + Self {
> + cfg: bindings::regulator_config {
> + dev: dev.as_raw(),
> + ..Default::default()
> + },
> + data,
> + }
> + }
> +}
> +
> +/// Regulator device
> +///
> +/// Abstraction for `struct regulator_dev`.
> +///
> +/// # Invariants
> +///
> +/// * `self.rdev` is valid and non-null.
> +/// * [`Self`] has owns `self.rdev` memory allocation.
> +/// * [`Self`] has owns memory of type `T` that can be retrieved through `rdev_get_drvdata`.
> +pub struct Device<T: ForeignOwnable + Send + Sync> {
> + rdev: NonNull<bindings::regulator_dev>,
> + _data_type: PhantomData<T>,
> +}
I think you should split this into a `regulator::Device` and a
`regulator::Registration` structure instead. For regulator this seems to works
as well, but it gets confusing if we do not stick to the same representations
between subsystems.
> +
> +impl<T: ForeignOwnable + Send + Sync> Device<T> {
> + /// # Safety
> + ///
> + /// `rdev` must be valid and non-null.
> + unsafe fn from_raw(rdev: *mut bindings::regulator_dev) -> ManuallyDrop<Self> {
> + ManuallyDrop::new(Self {
> + // SAFETY: The caller of `Self::from_raw` must garantee that `rdev` is non-null and
> + // valid..
> + rdev: unsafe { NonNull::new_unchecked(rdev) },
> + _data_type: PhantomData::<T>,
> + })
> + }
> +
> + /// register a Regulator driver
> + pub fn register(
> + dev: &device::Device,
> + desc: &'static Desc,
> + mut config: Config<T>,
> + ) -> Result<Self> {
> + config.cfg.driver_data = config.data.into_foreign() as _;
> +
> + // SAFETY: By the type invariants, we know that `dev.as_ref().as_raw()` is always
> + // valid and non-null, and the descriptor and config are guaranteed to be valid values,
> + // hence it is safe to perform the FFI call.
> + let rdev = from_err_ptr(unsafe {
> + bindings::regulator_register(dev.as_raw(), &desc.0, &config.cfg)
> + })?;
> +
> + Ok(Self {
> + rdev: NonNull::new(rdev).ok_or(EINVAL)?,
> + _data_type: PhantomData::<T>,
> + })
> + }
> +
> + /// List voltages when the regulator is using linear mapping
> + pub fn list_voltage_linear(&self, selector: u32) -> Result<i32> {
> + // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
> + // The C function is safe to call with any selector values.
> + let ret = unsafe { bindings::regulator_list_voltage_linear(self.rdev.as_ptr(), selector) };
> + if ret < 0 {
> + return Err(Error::from_errno(ret));
> + }
> + Ok(ret)
> + }
> +
> + /// Get regulator's name
> + pub fn get_name(&self) -> &'static CStr {
> + // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
> + // The C function is guaranteed to return a valid string.
> + unsafe { CStr::from_char_ptr(bindings::rdev_get_name(self.rdev.as_ptr())) }
> + }
> +
> + /// Get regulator's ID
> + pub fn get_id(&self) -> i32 {
> + // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
> + unsafe { bindings::rdev_get_id(self.rdev.as_ptr()) }
> + }
> +
> + /// Retrieve driver data associated to `self`
> + pub fn data(&self) -> T::Borrowed<'_> {
> + // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
> + unsafe { T::borrow(bindings::rdev_get_drvdata(self.rdev.as_ptr())) }
> + }
> +}
> +
> +impl<T: ForeignOwnable + Send + Sync> Drop for Device<T> {
> + fn drop(&mut self) {
> + // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
> + // so it is safe to perform the FFI call.
> + unsafe { bindings::regulator_unregister(self.rdev.as_ptr()) };
> +
> + // SAFETY: The type invariants garuantee that `self.rdev` is valid and non-null, and
> + // that `rdev_get_drvdata` is valid memory of type `T` stored there by calling
> + // `T::into_foreign`.
> + unsafe { T::from_foreign(bindings::rdev_get_drvdata(self.rdev.as_ptr())) };
> + }
> +}
> +
> +// SAFETY: `Device` has sole ownership of `self.rdev` and is never read outside of the C
> +// implementation. It is safe to use it from any thread.
> +unsafe impl<T: ForeignOwnable + Send + Sync> Send for Device<T> {}
> +
> +// SAFETY: It is OK to access `Device` through shared references from other threads because
> +// the C code is insuring proper synchronization of `self.rdev`.
> +unsafe impl<T: ForeignOwnable + Send + Sync> Sync for Device<T> {}
> +
> +/// [`Device`] type
> +pub enum Type {
> + /// Voltage regulator
> + Voltage,
> + /// Current regulator
> + Current,
> +}
> +
> +pub(crate) struct Adapter<T>(PhantomData<T>);
> +
> +impl<T: Driver> Adapter<T> {
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn list_voltage_callback(
> + rdev: *mut bindings::regulator_dev,
> + selector: core::ffi::c_uint,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| T::list_voltage(&mut rdev, selector))
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` and `selector` must be non-null and valid.
> + unsafe extern "C" fn set_voltage_callback(
> + rdev: *mut bindings::regulator_dev,
> + min_uv: core::ffi::c_int,
> + max_uv: core::ffi::c_int,
> + selector: *mut core::ffi::c_uint,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + match T::set_voltage(&mut rdev, min_uv, max_uv) {
> + Ok(v) => {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + unsafe { *selector = v as _ };
> + 0
> + }
> + Err(e) => e.to_errno(),
> + }
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn map_voltage_callback(
> + rdev: *mut bindings::regulator_dev,
> + min_uv: core::ffi::c_int,
> + max_uv: core::ffi::c_int,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| T::map_voltage(&mut rdev, min_uv, max_uv))
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn set_voltage_sel_callback(
> + rdev: *mut bindings::regulator_dev,
> + selector: core::ffi::c_uint,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + T::set_voltage_sel(&mut rdev, selector)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn get_voltage_callback(
> + rdev: *mut bindings::regulator_dev,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| T::get_voltage(&mut rdev))
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn get_voltage_sel_callback(
> + rdev: *mut bindings::regulator_dev,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| T::get_voltage_sel(&mut rdev))
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn set_current_limit_callback(
> + rdev: *mut bindings::regulator_dev,
> + min_ua: core::ffi::c_int,
> + max_ua: core::ffi::c_int,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + T::set_current_limit(&mut rdev, min_ua, max_ua)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn get_current_limit_callback(
> + rdev: *mut bindings::regulator_dev,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| T::get_current_limit(&mut rdev))
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn set_active_discharge_callback(
> + rdev: *mut bindings::regulator_dev,
> + enable: bool,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + T::set_active_discharge(&mut rdev, enable)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn enable_callback(rdev: *mut bindings::regulator_dev) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + T::enable(&mut rdev)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn disable_callback(rdev: *mut bindings::regulator_dev) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + T::disable(&mut rdev)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn is_enabled_callback(
> + rdev: *mut bindings::regulator_dev,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + T::is_enabled(&mut rdev)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn set_mode_callback(
> + rdev: *mut bindings::regulator_dev,
> + mode: core::ffi::c_uint,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + let mode = Mode::try_from(mode).unwrap_or(Mode::Invalid);
> + T::set_mode(&mut rdev, mode)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn get_mode_callback(
> + rdev: *mut bindings::regulator_dev,
> + ) -> core::ffi::c_uint {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + T::get_mode(&mut rdev) as _
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn get_status_callback(
> + rdev: *mut bindings::regulator_dev,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| Ok(T::get_status(&mut rdev)? as _))
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn set_suspend_voltage_callback(
> + rdev: *mut bindings::regulator_dev,
> + uv: core::ffi::c_int,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + T::set_suspend_voltage(&mut rdev, uv)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn set_suspend_enable_callback(
> + rdev: *mut bindings::regulator_dev,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + T::set_suspend_enable(&mut rdev)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn set_suspend_disable_callback(
> + rdev: *mut bindings::regulator_dev,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + T::set_suspend_disable(&mut rdev)?;
> + Ok(0)
> + })
> + }
> +
> + /// # Safety
> + ///
> + /// `rdev` must be non-null and valid.
> + unsafe extern "C" fn set_suspend_mode_callback(
> + rdev: *mut bindings::regulator_dev,
> + mode: core::ffi::c_uint,
> + ) -> core::ffi::c_int {
> + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> + let mut rdev = unsafe { Device::from_raw(rdev) };
> + from_result(|| {
> + let mode = Mode::try_from(mode).unwrap_or(Mode::Invalid);
> + T::set_suspend_mode(&mut rdev, mode)?;
> + Ok(0)
> + })
> + }
> +
> + const VTABLE: bindings::regulator_ops = bindings::regulator_ops {
> + list_voltage: if T::HAS_LIST_VOLTAGE {
> + Some(Adapter::<T>::list_voltage_callback)
> + } else {
> + None
> + },
> + set_voltage: if T::HAS_SET_VOLTAGE {
> + Some(Adapter::<T>::set_voltage_callback)
> + } else {
> + None
> + },
> + map_voltage: if T::HAS_MAP_VOLTAGE {
> + Some(Adapter::<T>::map_voltage_callback)
> + } else {
> + None
> + },
> + set_voltage_sel: if T::HAS_SET_VOLTAGE_SEL {
> + Some(Adapter::<T>::set_voltage_sel_callback)
> + } else {
> + None
> + },
> + get_voltage: if T::HAS_GET_VOLTAGE {
> + Some(Adapter::<T>::get_voltage_callback)
> + } else {
> + None
> + },
> + get_voltage_sel: if T::HAS_GET_VOLTAGE_SEL {
> + Some(Adapter::<T>::get_voltage_sel_callback)
> + } else {
> + None
> + },
> + set_current_limit: if T::HAS_SET_CURRENT_LIMIT {
> + Some(Adapter::<T>::set_current_limit_callback)
> + } else {
> + None
> + },
> + get_current_limit: if T::HAS_GET_CURRENT_LIMIT {
> + Some(Adapter::<T>::get_current_limit_callback)
> + } else {
> + None
> + },
> + set_active_discharge: if T::HAS_SET_ACTIVE_DISCHARGE {
> + Some(Adapter::<T>::set_active_discharge_callback)
> + } else {
> + None
> + },
> + enable: if T::HAS_ENABLE {
> + Some(Adapter::<T>::enable_callback)
> + } else {
> + None
> + },
> + disable: if T::HAS_DISABLE {
> + Some(Adapter::<T>::disable_callback)
> + } else {
> + None
> + },
> + is_enabled: if T::HAS_IS_ENABLED {
> + Some(Adapter::<T>::is_enabled_callback)
> + } else {
> + None
> + },
> + set_mode: if T::HAS_SET_MODE {
> + Some(Adapter::<T>::set_mode_callback)
> + } else {
> + None
> + },
> + get_mode: if T::HAS_GET_MODE {
> + Some(Adapter::<T>::get_mode_callback)
> + } else {
> + None
> + },
> + get_status: if T::HAS_GET_STATUS {
> + Some(Adapter::<T>::get_status_callback)
> + } else {
> + None
> + },
> + set_suspend_voltage: if T::HAS_SET_SUSPEND_VOLTAGE {
> + Some(Adapter::<T>::set_suspend_voltage_callback)
> + } else {
> + None
> + },
> + set_suspend_enable: if T::HAS_SET_SUSPEND_ENABLE {
> + Some(Adapter::<T>::set_suspend_enable_callback)
> + } else {
> + None
> + },
> + set_suspend_disable: if T::HAS_SET_SUSPEND_DISABLE {
> + Some(Adapter::<T>::set_suspend_disable_callback)
> + } else {
> + None
> + },
> + set_suspend_mode: if T::HAS_SET_SUSPEND_MODE {
> + Some(Adapter::<T>::set_suspend_mode_callback)
> + } else {
> + None
> + },
> + // SAFETY: The rest is zeroed out to initialize `struct regulator_ops`,
> + // sets `Option<&F>` to be `None`.
> + ..unsafe { core::mem::zeroed() }
> + };
> +
> + const fn build() -> &'static bindings::regulator_ops {
> + &Self::VTABLE
> + }
> +}
>
> --
> 2.45.2
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/9] rust: i2c: add basic I2C client abstraction
2024-12-18 23:36 ` [PATCH 1/9] rust: i2c: add basic I2C client abstraction Fabien Parent
@ 2024-12-19 13:03 ` Rob Herring
2024-12-19 15:33 ` Fabien Parent
0 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2024-12-19 13:03 UTC (permalink / raw)
To: Fabien Parent
Cc: Saravana Kannan, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang,
Mark Brown, Liam Girdwood, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, devicetree, rust-for-linux,
linux-kernel, linux-i2c, linux-arm-msm, vinod.koul, Fabien Parent,
Fiona Behrens
On Wed, Dec 18, 2024 at 03:36:31PM -0800, Fabien Parent wrote:
> From: Fiona Behrens <me@kloenk.dev>
>
> Implement an abstraction to write I2C device drivers. The abstraction
> is pretty basic and provides just the infrastructure to probe
> a device from I2C/OF device_id and abstract `i2c_client`.
> The client will be used by the Regmap abstraction to perform
> I/O on the I2C bus.
>
> Signed-off-by: Fiona Behrens <me@kloenk.dev>
> Co-developed-by: Fabien Parent <fabien.parent@linaro.org>
> Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> ---
> MAINTAINERS | 1 +
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/helpers.c | 1 +
> rust/helpers/i2c.c | 13 ++
> rust/kernel/i2c.rs | 288 ++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> 6 files changed, 306 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6b9e10551392c185b9314c9f94edeaf6e85af58f..961fe4ed39605bf489d1d9e473f47bccb692ff14 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10796,6 +10796,7 @@ F: include/linux/i2c-smbus.h
> F: include/linux/i2c.h
> F: include/uapi/linux/i2c-*.h
> F: include/uapi/linux/i2c.h
> +F: rust/kernel/i2c.rs
>
> I2C SUBSYSTEM HOST DRIVERS
> M: Andi Shyti <andi.shyti@kernel.org>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index e9fdceb568b8f94e602ee498323e5768a40a6cba..a882efb90bfc27960ef1fd5f2dc8cc40533a1c27 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -16,6 +16,7 @@
> #include <linux/file.h>
> #include <linux/firmware.h>
> #include <linux/fs.h>
> +#include <linux/i2c.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> #include <linux/mdio.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 0640b7e115be1553549312dcfdf842bcae3bde1b..630e903f516ee14a51f46ff0bcc68e8f9a64021a 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -15,6 +15,7 @@
> #include "device.c"
> #include "err.c"
> #include "fs.c"
> +#include "i2c.c"
> #include "io.c"
> #include "jump_label.c"
> #include "kunit.c"
> diff --git a/rust/helpers/i2c.c b/rust/helpers/i2c.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..8ffdc454e7597cc61909da5b3597057aeb5f7299
> --- /dev/null
> +++ b/rust/helpers/i2c.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/i2c.h>
> +
> +void *rust_helper_i2c_get_clientdata(const struct i2c_client *client)
> +{
> + return i2c_get_clientdata(client);
> +}
> +
> +void rust_helper_i2c_set_clientdata(struct i2c_client *client, void *data)
> +{
> + i2c_set_clientdata(client, data);
> +}
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..efa03335e5b59e72738380e94213976b2464c25b
> --- /dev/null
> +++ b/rust/kernel/i2c.rs
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions for the I2C bus.
> +//!
> +//! C header: [`include/linux/i2c.h`](srctree/include/linux/i2c.h)
> +
> +use crate::{
> + bindings, container_of,
> + device::Device,
> + device_id::{self, RawDeviceId},
> + driver,
> + error::{to_result, Result},
> + of,
> + prelude::*,
> + str::CStr,
> + types::{ARef, ForeignOwnable, Opaque},
> + ThisModule,
> +};
> +
> +/// Abstraction for `bindings::i2c_device_id`.
> +#[repr(transparent)]
> +#[derive(Clone, Copy)]
> +pub struct DeviceId(bindings::i2c_device_id);
> +
> +impl DeviceId {
> + /// Create a new device id from an I2C name.
> + pub const fn new(name: &CStr) -> Self {
> + let src = name.as_bytes_with_nul();
> + // TODO: Replace with `bindings::i2c_device_id::default()` once stabilized for `const`.
> + // SAFETY: FFI type is valid to be zero-initialized.
> + let mut i2c: bindings::i2c_device_id = unsafe { core::mem::zeroed() };
> +
> + let mut i = 0;
> + while i < src.len() {
> + i2c.name[i] = src[i] as _;
> + i += 1;
> + }
You can simplify this now that char maps to u8 (in rust next).
> +
> + Self(i2c)
> + }
> +}
> +
> +// SAFETY:
> +// * `DeviceId` is a `#[repr(transparent)` wrapper of `i2c_device_id` and does not add
> +// additional invariants, so it's safe to transmute to `RawType`.
> +// * `DRIVER_DATA_OFFSET` is the offset to the `data` field.
> +unsafe impl RawDeviceId for DeviceId {
> + type RawType = bindings::i2c_device_id;
> +
> + const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::i2c_device_id, driver_data);
> +
> + fn index(&self) -> usize {
> + self.0.driver_data as _
> + }
> +}
> +
> +/// I2C [`DeviceId`] table.
> +pub type IdTable<T> = &'static dyn device_id::IdTable<DeviceId, T>;
> +
> +/// An adapter for the registration of I2C drivers.
> +#[doc(hidden)]
> +pub struct Adapter<T: Driver + 'static>(T);
> +
> +impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> + type RegType = bindings::i2c_driver;
> +
> + fn register(
> + i2cdrv: &Opaque<Self::RegType>,
> + name: &'static CStr,
> + module: &'static ThisModule,
> + ) -> Result {
> + // SAFETY: It's safe to set the fields of `struct i2c_driver` on initialization.
> + unsafe {
> + (*i2cdrv.get()).driver.name = name.as_char_ptr();
> + (*i2cdrv.get()).probe = Some(Self::probe_callback);
> + (*i2cdrv.get()).remove = Some(Self::remove_callback);
> + if let Some(t) = T::I2C_ID_TABLE {
> + (*i2cdrv.get()).id_table = t.as_ptr();
> + }
> + if let Some(t) = T::OF_ID_TABLE {
> + (*i2cdrv.get()).driver.of_match_table = t.as_ptr();
> + }
> + }
> +
> + // SAFETY: `i2cdrv` is guaranteed to be a valid `RegType`.
> + to_result(unsafe { bindings::i2c_register_driver(module.0, i2cdrv.get()) })
> + }
> +
> + fn unregister(i2cdrv: &Opaque<Self::RegType>) {
> + // SAFETY: `i2cdrv` is guaranteed to be a valid `RegType`.
> + unsafe { bindings::i2c_del_driver(i2cdrv.get()) };
> + }
> +}
> +
> +impl<T: Driver> Adapter<T> {
> + /// Get the [`Self::IdInfo`] that matched during probe.
> + fn id_info(client: &mut Client) -> Option<&'static T::IdInfo> {
> + let id = <Self as driver::Adapter>::id_info(client.as_ref());
> + if id.is_some() {
> + return id;
> + }
> +
> + // SAFETY: `client` and `client.as_raw()` are guaranteed to be valid.
> + let id = unsafe { bindings::i2c_client_get_device_id(client.as_raw()) };
> + if !id.is_null() {
> + // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct i2c_device_id` and
> + // does not add additional invariants, so it's safe to transmute.
> + let id = unsafe { &*id.cast::<DeviceId>() };
> + return Some(T::I2C_ID_TABLE?.info(id.index()));
> + }
You aren't handling the DT based matching.
Rob
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/9] rust: i2c: add basic I2C client abstraction
2024-12-19 13:03 ` Rob Herring
@ 2024-12-19 15:33 ` Fabien Parent
0 siblings, 0 replies; 21+ messages in thread
From: Fabien Parent @ 2024-12-19 15:33 UTC (permalink / raw)
To: Rob Herring
Cc: Saravana Kannan, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang,
Mark Brown, Liam Girdwood, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, devicetree, rust-for-linux,
linux-kernel, linux-i2c, linux-arm-msm, vinod.koul, Fabien Parent,
Fiona Behrens
On Thu, Dec 19, 2024 at 5:03 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Dec 18, 2024 at 03:36:31PM -0800, Fabien Parent wrote:
> > From: Fiona Behrens <me@kloenk.dev>
> >
> > Implement an abstraction to write I2C device drivers. The abstraction
> > is pretty basic and provides just the infrastructure to probe
> > a device from I2C/OF device_id and abstract `i2c_client`.
> > The client will be used by the Regmap abstraction to perform
> > I/O on the I2C bus.
> >
> > Signed-off-by: Fiona Behrens <me@kloenk.dev>
> > Co-developed-by: Fabien Parent <fabien.parent@linaro.org>
> > Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> > ---
> > MAINTAINERS | 1 +
> > rust/bindings/bindings_helper.h | 1 +
> > rust/helpers/helpers.c | 1 +
> > rust/helpers/i2c.c | 13 ++
> > rust/kernel/i2c.rs | 288 ++++++++++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs | 2 +
> > 6 files changed, 306 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6b9e10551392c185b9314c9f94edeaf6e85af58f..961fe4ed39605bf489d1d9e473f47bccb692ff14 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10796,6 +10796,7 @@ F: include/linux/i2c-smbus.h
> > F: include/linux/i2c.h
> > F: include/uapi/linux/i2c-*.h
> > F: include/uapi/linux/i2c.h
> > +F: rust/kernel/i2c.rs
> >
> > I2C SUBSYSTEM HOST DRIVERS
> > M: Andi Shyti <andi.shyti@kernel.org>
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index e9fdceb568b8f94e602ee498323e5768a40a6cba..a882efb90bfc27960ef1fd5f2dc8cc40533a1c27 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -16,6 +16,7 @@
> > #include <linux/file.h>
> > #include <linux/firmware.h>
> > #include <linux/fs.h>
> > +#include <linux/i2c.h>
> > #include <linux/jiffies.h>
> > #include <linux/jump_label.h>
> > #include <linux/mdio.h>
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 0640b7e115be1553549312dcfdf842bcae3bde1b..630e903f516ee14a51f46ff0bcc68e8f9a64021a 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -15,6 +15,7 @@
> > #include "device.c"
> > #include "err.c"
> > #include "fs.c"
> > +#include "i2c.c"
> > #include "io.c"
> > #include "jump_label.c"
> > #include "kunit.c"
> > diff --git a/rust/helpers/i2c.c b/rust/helpers/i2c.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..8ffdc454e7597cc61909da5b3597057aeb5f7299
> > --- /dev/null
> > +++ b/rust/helpers/i2c.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/i2c.h>
> > +
> > +void *rust_helper_i2c_get_clientdata(const struct i2c_client *client)
> > +{
> > + return i2c_get_clientdata(client);
> > +}
> > +
> > +void rust_helper_i2c_set_clientdata(struct i2c_client *client, void *data)
> > +{
> > + i2c_set_clientdata(client, data);
> > +}
> > diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..efa03335e5b59e72738380e94213976b2464c25b
> > --- /dev/null
> > +++ b/rust/kernel/i2c.rs
> > @@ -0,0 +1,288 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Abstractions for the I2C bus.
> > +//!
> > +//! C header: [`include/linux/i2c.h`](srctree/include/linux/i2c.h)
> > +
> > +use crate::{
> > + bindings, container_of,
> > + device::Device,
> > + device_id::{self, RawDeviceId},
> > + driver,
> > + error::{to_result, Result},
> > + of,
> > + prelude::*,
> > + str::CStr,
> > + types::{ARef, ForeignOwnable, Opaque},
> > + ThisModule,
> > +};
> > +
> > +/// Abstraction for `bindings::i2c_device_id`.
> > +#[repr(transparent)]
> > +#[derive(Clone, Copy)]
> > +pub struct DeviceId(bindings::i2c_device_id);
> > +
> > +impl DeviceId {
> > + /// Create a new device id from an I2C name.
> > + pub const fn new(name: &CStr) -> Self {
> > + let src = name.as_bytes_with_nul();
> > + // TODO: Replace with `bindings::i2c_device_id::default()` once stabilized for `const`.
> > + // SAFETY: FFI type is valid to be zero-initialized.
> > + let mut i2c: bindings::i2c_device_id = unsafe { core::mem::zeroed() };
> > +
> > + let mut i = 0;
> > + while i < src.len() {
> > + i2c.name[i] = src[i] as _;
> > + i += 1;
> > + }
>
> You can simplify this now that char maps to u8 (in rust next).
>
> > +
> > + Self(i2c)
> > + }
> > +}
> > +
> > +// SAFETY:
> > +// * `DeviceId` is a `#[repr(transparent)` wrapper of `i2c_device_id` and does not add
> > +// additional invariants, so it's safe to transmute to `RawType`.
> > +// * `DRIVER_DATA_OFFSET` is the offset to the `data` field.
> > +unsafe impl RawDeviceId for DeviceId {
> > + type RawType = bindings::i2c_device_id;
> > +
> > + const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::i2c_device_id, driver_data);
> > +
> > + fn index(&self) -> usize {
> > + self.0.driver_data as _
> > + }
> > +}
> > +
> > +/// I2C [`DeviceId`] table.
> > +pub type IdTable<T> = &'static dyn device_id::IdTable<DeviceId, T>;
> > +
> > +/// An adapter for the registration of I2C drivers.
> > +#[doc(hidden)]
> > +pub struct Adapter<T: Driver + 'static>(T);
> > +
> > +impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> > + type RegType = bindings::i2c_driver;
> > +
> > + fn register(
> > + i2cdrv: &Opaque<Self::RegType>,
> > + name: &'static CStr,
> > + module: &'static ThisModule,
> > + ) -> Result {
> > + // SAFETY: It's safe to set the fields of `struct i2c_driver` on initialization.
> > + unsafe {
> > + (*i2cdrv.get()).driver.name = name.as_char_ptr();
> > + (*i2cdrv.get()).probe = Some(Self::probe_callback);
> > + (*i2cdrv.get()).remove = Some(Self::remove_callback);
> > + if let Some(t) = T::I2C_ID_TABLE {
> > + (*i2cdrv.get()).id_table = t.as_ptr();
> > + }
> > + if let Some(t) = T::OF_ID_TABLE {
> > + (*i2cdrv.get()).driver.of_match_table = t.as_ptr();
> > + }
> > + }
> > +
> > + // SAFETY: `i2cdrv` is guaranteed to be a valid `RegType`.
> > + to_result(unsafe { bindings::i2c_register_driver(module.0, i2cdrv.get()) })
> > + }
> > +
> > + fn unregister(i2cdrv: &Opaque<Self::RegType>) {
> > + // SAFETY: `i2cdrv` is guaranteed to be a valid `RegType`.
> > + unsafe { bindings::i2c_del_driver(i2cdrv.get()) };
> > + }
> > +}
> > +
> > +impl<T: Driver> Adapter<T> {
> > + /// Get the [`Self::IdInfo`] that matched during probe.
> > + fn id_info(client: &mut Client) -> Option<&'static T::IdInfo> {
> > + let id = <Self as driver::Adapter>::id_info(client.as_ref());
> > + if id.is_some() {
> > + return id;
> > + }
> > +
> > + // SAFETY: `client` and `client.as_raw()` are guaranteed to be valid.
> > + let id = unsafe { bindings::i2c_client_get_device_id(client.as_raw()) };
> > + if !id.is_null() {
> > + // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct i2c_device_id` and
> > + // does not add additional invariants, so it's safe to transmute.
> > + let id = unsafe { &*id.cast::<DeviceId>() };
> > + return Some(T::I2C_ID_TABLE?.info(id.index()));
> > + }
>
> You aren't handling the DT based matching.
It is handled with the first line of this function:
+ fn id_info(client: &mut Client) -> Option<&'static T::IdInfo> {
+ let id = <Self as driver::Adapter>::id_info(client.as_ref());
This Adapter driver::Adapter is implemented by the I2C bus:
+impl<T: Driver + 'static> driver::Adapter for Adapter<T> {
+ type IdInfo = T::IdInfo;
+
+ fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> {
+ T::OF_ID_TABLE
+ }
+}
This function is first using the adapter to retrieve the IdInfo, and
if nothing is returned then we perform our own match based on
i2c::IdInfo.
>
> Rob
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/9] rust: regulator: add Regulator Driver abstraction
2024-12-19 10:26 ` Danilo Krummrich
@ 2024-12-19 16:00 ` Fabien Parent
2024-12-19 18:58 ` Mark Brown
0 siblings, 1 reply; 21+ messages in thread
From: Fabien Parent @ 2024-12-19 16:00 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
On Thu, Dec 19, 2024 at 2:26 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Dec 18, 2024 at 03:36:35PM -0800, Fabien Parent wrote:
> > From: Fabien Parent <fabien.parent@linaro.org>
> >
> > This commit adds a Rust abstraction to write Regulator drivers. Only
> > the features used by the NCV6336 driver were added to this abstraction.
> >
> > Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> > ---
> > MAINTAINERS | 1 +
> > rust/bindings/bindings_helper.h | 1 +
> > rust/kernel/regulator.rs | 4 +-
> > rust/kernel/regulator/driver.rs | 850 ++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 855 insertions(+), 1 deletion(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 90c231f0aa7381aa8d206fb94c5d1f013dfcae41..87da43251bf0f20d2b5831345778ead592c407dc 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -25160,6 +25160,7 @@ F: drivers/regulator/
> > F: include/dt-bindings/regulator/
> > F: include/linux/regulator/
> > F: rust/kernel/regulator.rs
> > +F: rust/kernel/regulator/
> > K: regulator_get_optional
> >
> > VOLTAGE AND CURRENT REGULATOR IRQ HELPERS
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index b18d772bc3a0e78d749cc9e5ae81a4237a57f8c5..124129daea73c143c919d05814fc02bb4460ddfd 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -30,6 +30,7 @@
> > #include <linux/refcount.h>
> > #include <linux/regmap.h>
> > #include <linux/regulator/consumer.h>
> > +#include <linux/regulator/driver.h>
> > #include <linux/sched.h>
> > #include <linux/security.h>
> > #include <linux/slab.h>
> > diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> > index d695ac955193efcfda62770784a92d70d606b93d..bd8202fe5702b944201e76553b9496e1d42cb429 100644
> > --- a/rust/kernel/regulator.rs
> > +++ b/rust/kernel/regulator.rs
> > @@ -2,12 +2,14 @@
> >
> > //! SoC Regulators
>
> Why "SoC", could be a regulator for anything else too, right?
I used "SoC" to match the C headers "include/linux/regulator/driver.h"
and "include/linux/regulator/consumer.h". But as you
mention this, I just noticed that "SoC" is nowhere to be found in
Documentation/driver-api/regulator.rst, so maybe I should
indeed remove it.
>
> >
> > +pub mod driver;
> > +
> > use crate::{
> > bindings,
> > error::{code::*, Error, Result},
> > };
> >
> > -/// Regulators operating modes
> > +/// [`driver::Device`] operating modes
> > #[derive(Copy, Clone)]
> > #[repr(u32)]
> > pub enum Mode {
> > diff --git a/rust/kernel/regulator/driver.rs b/rust/kernel/regulator/driver.rs
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..8079ea28fd5bf7b6871a0b1d2cea7a6fffcb43ca
> > --- /dev/null
> > +++ b/rust/kernel/regulator/driver.rs
> > @@ -0,0 +1,850 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! SoC Device Driver Interface
>
> Should rather be "Regulator Device Driver Interface".
>
> > +//!
> > +//! C header: [`include/linux/regulator/driver.h`](srctree/include/linux/regulator/driver.h)
> > +//!
> > +//! # Examples
> > +//!
> > +//! ```
> > +//! use kernel::regulator::driver::{Config, Desc, Device, Driver, Type};
> > +//!
> > +//! static DESC: Desc =
> > +//! Desc::new::<MyDeviceDriver>(kernel::c_str!("my-regulator-driver"), Type::Voltage);
> > +//!
> > +//! struct MyDeviceDriver;
> > +//!
> > +//! #[vtable]
> > +//! impl Driver for MyDeviceDriver {
>
> I usually prefer to keep the module prefix for the `Driver` traits, i.e.
> `impl regulator::Driver for ...`, this makes things a bit more obvious.
>
> > +//! type Data = ();
> > +//!
> > +//! // Implement supported `Driver`'s operations here.
> > +//!
> > +//! // Example:
> > +//! fn is_enabled(reg: &mut Device<Self::Data>) -> Result<bool> {
> > +//! Ok(true)
> > +//! }
> > +//! }
> > +//!
> > +//! impl MyDeviceDriver {
> > +//! fn probe(dev: &mut kernel::device::Device) {
> > +//! let _ = Device::register(dev, &DESC, Config::<<Self as Driver>::Data>::new(dev, ()));
>
> Like below, this is confusing, the device is immediately unregistered again.
>
> > +//! }
> > +//! }
> > +//! ```
> > +
> > +use crate::{
> > + device,
> > + error::{code::*, from_err_ptr, from_result, Error, Result},
> > + macros::vtable,
> > + regulator::Mode,
> > + str::CStr,
> > + types::ForeignOwnable,
> > + ThisModule,
> > +};
> > +use core::{marker::PhantomData, mem::ManuallyDrop, ptr::NonNull};
> > +
> > +/// [`Device`]'s status
> > +#[derive(Eq, PartialEq)]
> > +pub enum Status {
> > + /// Device is off
> > + Off,
> > + /// Device is on
> > + On,
> > + /// Device is in an error state
> > + Error,
> > + /// Device is on and in Fast mode
> > + Fast,
> > + /// Device is on and in Normal mode
> > + Normal,
> > + /// Device is on and in Idle mode
> > + Idle,
> > + /// Device is on and in Standby mode
> > + Standby,
> > + /// Device is enabled but not regulating
> > + Bypass,
> > + /// Device is any other status
> > + Undefined,
> > +}
> > +
> > +impl TryFrom<core::ffi::c_uint> for Status {
> > + type Error = Error;
> > +
> > + fn try_from(status: core::ffi::c_uint) -> Result<Self> {
> > + match status {
> > + bindings::regulator_status_REGULATOR_STATUS_OFF => Ok(Self::Off),
> > + bindings::regulator_status_REGULATOR_STATUS_ON => Ok(Self::On),
> > + bindings::regulator_status_REGULATOR_STATUS_ERROR => Ok(Self::Error),
> > + bindings::regulator_status_REGULATOR_STATUS_FAST => Ok(Self::Fast),
> > + bindings::regulator_status_REGULATOR_STATUS_NORMAL => Ok(Self::Normal),
> > + bindings::regulator_status_REGULATOR_STATUS_IDLE => Ok(Self::Idle),
> > + bindings::regulator_status_REGULATOR_STATUS_STANDBY => Ok(Self::Standby),
> > + bindings::regulator_status_REGULATOR_STATUS_BYPASS => Ok(Self::Bypass),
> > + bindings::regulator_status_REGULATOR_STATUS_UNDEFINED => Ok(Self::Undefined),
> > + _ => Err(EINVAL),
> > + }
> > + }
> > +}
> > +
> > +impl From<Mode> for Status {
> > + fn from(mode: Mode) -> Self {
> > + // SAFETY: `regulator_mode_to_status` is a `pure function` that is only doing integer
> > + // to integer conversion, hence this function call is safe.
> > + let status = unsafe { bindings::regulator_mode_to_status(mode as _) };
> > +
> > + if status < 0 {
> > + Self::Undefined
> > + } else {
> > + Self::try_from(status as core::ffi::c_uint).unwrap_or(Self::Undefined)
> > + }
> > + }
> > +}
> > +
> > +/// [`Device`]'s operations
> > +#[vtable]
> > +pub trait Driver {
> > + /// User data that will be accessible to all operations
> > + type Data: ForeignOwnable + Send + Sync;
> > +
> > + /// Return one of the supported voltages, in microvolt; zero if the selector indicates a
> > + /// voltage that is unusable by the system; or negative errno. Selectors range from zero to one
> > + /// less than the number of voltages supported by the system.
> > + fn list_voltage(_rdev: &mut Device<Self::Data>, _selector: u32) -> Result<i32> {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Set the voltage for the regulator within the range specified. The driver should select the
> > + /// voltage closest to `min_uv`.
> > + fn set_voltage(_rdev: &mut Device<Self::Data>, _min_uv: i32, _max_uv: i32) -> Result<i32> {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Set the voltage for the regulator using the specified selector.
> > + fn set_voltage_sel(_rdev: &mut Device<Self::Data>, _selector: u32) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Convert a voltage into a selector.
> > + fn map_voltage(_rdev: &mut Device<Self::Data>, _min_uv: i32, _max_uv: i32) -> Result<i32> {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Get the currently configured voltage for the regulator; Returns
> > + /// [`ENOTRECOVERABLE`] if the regulator can't be read at bootup and hasn't been
> > + /// set yet.
> > + fn get_voltage(_rdev: &mut Device<Self::Data>) -> Result<i32> {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Get the currently configured voltage selector for the regulator; Returns
> > + /// [`ENOTRECOVERABLE`] if the regulator can't be read at bootup and hasn't been
> > + /// set yet.
> > + fn get_voltage_sel(_rdev: &mut Device<Self::Data>) -> Result<i32> {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Configure a limit for a current-limited regulator.
> > + ///
> > + /// The driver should select the current closest to `max_ua`.
> > + fn set_current_limit(_rdev: &mut Device<Self::Data>, _min_ua: i32, _max_ua: i32) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Get the configured limit for a current-limited regulator.
> > + fn get_current_limit(_rdev: &mut Device<Self::Data>) -> Result<i32> {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Enable or disable the active discharge of the regulator.
> > + fn set_active_discharge(_rdev: &mut Device<Self::Data>, _enable: bool) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Configure the regulator as enabled.
> > + fn enable(_rdev: &mut Device<Self::Data>) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Configure the regulator as disabled.
> > + fn disable(_rdev: &mut Device<Self::Data>) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Returns enablement state of the regulator.
> > + fn is_enabled(_rdev: &mut Device<Self::Data>) -> Result<bool> {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Set the configured operating [`Mode`] for the regulator.
> > + fn set_mode(_rdev: &mut Device<Self::Data>, _mode: Mode) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Get the configured operating [`Mode`] for the regulator.
> > + fn get_mode(_rdev: &mut Device<Self::Data>) -> Mode {
> > + Mode::Invalid
> > + }
> > +
> > + /// Report the regulator [`Status`].
> > + fn get_status(_rdev: &mut Device<Self::Data>) -> Result<Status> {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Set the voltage for the regaultor when the system is suspended.
> > + fn set_suspend_voltage(_rdev: &mut Device<Self::Data>, _uv: i32) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Mark the regulator as enabled when the system is suspended.
> > + fn set_suspend_enable(_rdev: &mut Device<Self::Data>) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Mark the regulator as disabled when the system is suspended.
> > + fn set_suspend_disable(_rdev: &mut Device<Self::Data>) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +
> > + /// Set the operating mode for the regulator when the system is suspended.
> > + fn set_suspend_mode(_rdev: &mut Device<Self::Data>, _mode: Mode) -> Result {
> > + Err(ENOTSUPP)
> > + }
> > +}
> > +
> > +/// [`Device`]'s descriptor
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// use kernel::{
> > +/// c_str,
> > +/// device,
> > +/// regulator::driver::{Config, Desc, Device, Driver, Type},
> > +/// types::ForeignOwnable,
> > +/// };
> > +///
> > +/// struct MyDeviceDriver;
> > +///
> > +/// #[vtable]
> > +/// impl Driver for MyDeviceDriver {
> > +/// type Data = ();
> > +/// }
>
> This seems incomplete.
>
> > +///
> > +/// static BUCK_DESC: Desc = Desc::new::<MyDeviceDriver>(c_str!("my_driver"), Type::Voltage)
> > +/// .with_of_match(c_str!("buck"))
> > +/// .with_enable(0x24, 0x1, 0x1, 0);
> > +///
> > +/// fn example(dev: &mut device::Device, mut config: Config<<MyDeviceDriver as Driver>::Data>) {
> > +/// let _ = Device::register(dev, &BUCK_DESC, config);
> > +/// }
>
> `example`? This should be within a driver trait with `probe` instead.
>
> > +/// ```
> > +///
> > +/// # Invariants
> > +///
> > +/// `self.0` has always valid data.
> > +pub struct Desc(bindings::regulator_desc);
>
> I think this needs `#[repr(transparent)]`.
>
> > +impl Desc {
> > + /// Create a new [`Device`] descriptor
> > + pub const fn new<T: Driver>(name: &'static CStr, reg_type: Type) -> Self {
> > + // SAFETY: `bindings::regulator_desc" is safe to initialize with 0s.
> > + let mut desc: bindings::regulator_desc = unsafe { core::mem::zeroed() };
> > + desc.name = name.as_char_ptr();
> > + desc.type_ = match reg_type {
> > + Type::Voltage => bindings::regulator_type_REGULATOR_VOLTAGE,
> > + Type::Current => bindings::regulator_type_REGULATOR_CURRENT,
> > + };
> > + desc.ops = Adapter::<T>::build();
> > + Self(desc)
> > + }
> > +
> > + /// Setup the register address, mask, and {en,dis}able values
> > + pub const fn with_enable(mut self, reg: u32, mask: u32, en_val: u32, dis_val: u32) -> Self {
> > + self.0.enable_reg = reg;
> > + self.0.enable_mask = mask;
> > + self.0.enable_val = en_val;
> > + self.0.disable_val = dis_val;
> > + self
> > + }
> > +
> > + /// Setup the register address, mask, and {en,dis}able values. {En,Dis}able values are
> > + /// inverted, i.e. `dis_val` will be use to enable the regulator while `en_val` will be used
> > + /// to disable the regulator.
> > + pub const fn with_inverted_enable(
> > + mut self,
> > + reg: u32,
> > + mask: u32,
> > + en_val: u32,
> > + dis_val: u32,
> > + ) -> Self {
> > + self.0.enable_is_inverted = true;
> > + self.with_enable(reg, mask, en_val, dis_val)
> > + }
> > +
> > + /// Setup the active discharge regiter address, mask, on/off values.
> > + pub const fn with_active_discharge(mut self, reg: u32, mask: u32, on: u32, off: u32) -> Self {
> > + self.0.active_discharge_on = on;
> > + self.0.active_discharge_off = off;
> > + self.0.active_discharge_reg = reg;
> > + self.0.active_discharge_mask = mask;
> > + self
> > + }
> > +
> > + /// Setup the current selection register address, mask, and current table
> > + pub const fn with_csel(mut self, reg: u32, mask: u32, table: &'static [u32]) -> Self {
> > + self.0.csel_reg = reg;
> > + self.0.csel_mask = mask;
> > + self.0.curr_table = table.as_ptr();
> > + self
> > + }
> > +
> > + /// Voltages are a linear mapping
> > + pub const fn with_linear_mapping(
> > + mut self,
> > + reg: u32,
> > + mask: u32,
> > + min_uv: u32,
> > + uv_step: u32,
> > + n_voltages: u32,
> > + linear_min_sel: u32,
> > + ) -> Self {
> > + self.0.vsel_reg = reg;
> > + self.0.vsel_mask = mask;
> > + self.0.n_voltages = n_voltages;
> > + self.0.min_uV = min_uv;
> > + self.0.uV_step = uv_step;
> > + self.0.linear_min_sel = linear_min_sel;
> > + self
> > + }
> > +
> > + /// Set the regulator owner
> > + pub const fn with_owner(mut self, owner: &'static ThisModule) -> Self {
> > + self.0.owner = owner.as_ptr();
> > + self
> > + }
> > +
> > + /// Set the name used to identify the regulator in the DT.
> > + pub const fn with_of_match(mut self, of_match: &'static CStr) -> Self {
> > + self.0.of_match = of_match.as_char_ptr();
> > + self
> > + }
> > +}
> > +
> > +// SAFETY: `Desc` cannot be modified after its declaration and owns its data, hence it is safe
> > +// to share references between threads.
> > +unsafe impl Sync for Desc {}
> > +
> > +/// [`Device`]'s Config
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// use kernel::regulator::driver::Config;
> > +/// # use kernel::regulator::driver::{Desc, Device};
> > +/// # use kernel::{device, sync::Arc};
> > +///
> > +/// struct DriverData(u32);
> > +///
> > +/// # fn probe(dev: &device::Device, desc: &'static Desc) -> Result {
> > +/// let config = Config::<Arc<DriverData>>::new(dev, Arc::new(DriverData(128), GFP_KERNEL)?);
>
> Why does this need reference counting?
>
> > +/// let reg = Device::register(dev, desc, config)?;
> > +/// # Ok(())
> > +/// # }
>
> I think this example is a bit misleading, the regulator device is immediately
> unregistered after probe() returns.
>
> Here you have to rely on the driver to keep `reg` alive until the device is
> dropped. Instead you can use `Devres::new_foreign_owned`, like I do in [1].
>
> [1] https://cgit.freedesktop.org/drm/drm-misc/tree/rust/kernel/drm/drv.rs?h=topic/rust-drm#n173
>
> > +/// ```
> > +///
> > +/// # Invariants
> > +///
> > +/// `self.cfg` always hold valid data.
> > +pub struct Config<T: ForeignOwnable + Send + Sync = ()> {
> > + cfg: bindings::regulator_config,
> > + data: T,
> > +}
> > +
> > +impl<T: ForeignOwnable + Send + Sync> Config<T> {
> > + /// Create a [`Device`] config.
> > + pub fn new(dev: &device::Device, data: T) -> Self {
> > + Self {
> > + cfg: bindings::regulator_config {
> > + dev: dev.as_raw(),
> > + ..Default::default()
> > + },
> > + data,
> > + }
> > + }
> > +}
> > +
> > +/// Regulator device
> > +///
> > +/// Abstraction for `struct regulator_dev`.
> > +///
> > +/// # Invariants
> > +///
> > +/// * `self.rdev` is valid and non-null.
> > +/// * [`Self`] has owns `self.rdev` memory allocation.
> > +/// * [`Self`] has owns memory of type `T` that can be retrieved through `rdev_get_drvdata`.
> > +pub struct Device<T: ForeignOwnable + Send + Sync> {
> > + rdev: NonNull<bindings::regulator_dev>,
> > + _data_type: PhantomData<T>,
> > +}
>
> I think you should split this into a `regulator::Device` and a
> `regulator::Registration` structure instead. For regulator this seems to works
> as well, but it gets confusing if we do not stick to the same representations
> between subsystems.
>
> > +
> > +impl<T: ForeignOwnable + Send + Sync> Device<T> {
> > + /// # Safety
> > + ///
> > + /// `rdev` must be valid and non-null.
> > + unsafe fn from_raw(rdev: *mut bindings::regulator_dev) -> ManuallyDrop<Self> {
> > + ManuallyDrop::new(Self {
> > + // SAFETY: The caller of `Self::from_raw` must garantee that `rdev` is non-null and
> > + // valid..
> > + rdev: unsafe { NonNull::new_unchecked(rdev) },
> > + _data_type: PhantomData::<T>,
> > + })
> > + }
> > +
> > + /// register a Regulator driver
> > + pub fn register(
> > + dev: &device::Device,
> > + desc: &'static Desc,
> > + mut config: Config<T>,
> > + ) -> Result<Self> {
> > + config.cfg.driver_data = config.data.into_foreign() as _;
> > +
> > + // SAFETY: By the type invariants, we know that `dev.as_ref().as_raw()` is always
> > + // valid and non-null, and the descriptor and config are guaranteed to be valid values,
> > + // hence it is safe to perform the FFI call.
> > + let rdev = from_err_ptr(unsafe {
> > + bindings::regulator_register(dev.as_raw(), &desc.0, &config.cfg)
> > + })?;
> > +
> > + Ok(Self {
> > + rdev: NonNull::new(rdev).ok_or(EINVAL)?,
> > + _data_type: PhantomData::<T>,
> > + })
> > + }
> > +
> > + /// List voltages when the regulator is using linear mapping
> > + pub fn list_voltage_linear(&self, selector: u32) -> Result<i32> {
> > + // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
> > + // The C function is safe to call with any selector values.
> > + let ret = unsafe { bindings::regulator_list_voltage_linear(self.rdev.as_ptr(), selector) };
> > + if ret < 0 {
> > + return Err(Error::from_errno(ret));
> > + }
> > + Ok(ret)
> > + }
> > +
> > + /// Get regulator's name
> > + pub fn get_name(&self) -> &'static CStr {
> > + // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
> > + // The C function is guaranteed to return a valid string.
> > + unsafe { CStr::from_char_ptr(bindings::rdev_get_name(self.rdev.as_ptr())) }
> > + }
> > +
> > + /// Get regulator's ID
> > + pub fn get_id(&self) -> i32 {
> > + // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
> > + unsafe { bindings::rdev_get_id(self.rdev.as_ptr()) }
> > + }
> > +
> > + /// Retrieve driver data associated to `self`
> > + pub fn data(&self) -> T::Borrowed<'_> {
> > + // SAFETY: By the type invariants, we know that `self.rdev` is always valid and non-null.
> > + unsafe { T::borrow(bindings::rdev_get_drvdata(self.rdev.as_ptr())) }
> > + }
> > +}
> > +
> > +impl<T: ForeignOwnable + Send + Sync> Drop for Device<T> {
> > + fn drop(&mut self) {
> > + // SAFETY: The type invariants guarantee that `self.rdev` is valid and non-null,
> > + // so it is safe to perform the FFI call.
> > + unsafe { bindings::regulator_unregister(self.rdev.as_ptr()) };
> > +
> > + // SAFETY: The type invariants garuantee that `self.rdev` is valid and non-null, and
> > + // that `rdev_get_drvdata` is valid memory of type `T` stored there by calling
> > + // `T::into_foreign`.
> > + unsafe { T::from_foreign(bindings::rdev_get_drvdata(self.rdev.as_ptr())) };
> > + }
> > +}
> > +
> > +// SAFETY: `Device` has sole ownership of `self.rdev` and is never read outside of the C
> > +// implementation. It is safe to use it from any thread.
> > +unsafe impl<T: ForeignOwnable + Send + Sync> Send for Device<T> {}
> > +
> > +// SAFETY: It is OK to access `Device` through shared references from other threads because
> > +// the C code is insuring proper synchronization of `self.rdev`.
> > +unsafe impl<T: ForeignOwnable + Send + Sync> Sync for Device<T> {}
> > +
> > +/// [`Device`] type
> > +pub enum Type {
> > + /// Voltage regulator
> > + Voltage,
> > + /// Current regulator
> > + Current,
> > +}
> > +
> > +pub(crate) struct Adapter<T>(PhantomData<T>);
> > +
> > +impl<T: Driver> Adapter<T> {
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn list_voltage_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + selector: core::ffi::c_uint,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| T::list_voltage(&mut rdev, selector))
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` and `selector` must be non-null and valid.
> > + unsafe extern "C" fn set_voltage_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + min_uv: core::ffi::c_int,
> > + max_uv: core::ffi::c_int,
> > + selector: *mut core::ffi::c_uint,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + match T::set_voltage(&mut rdev, min_uv, max_uv) {
> > + Ok(v) => {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + unsafe { *selector = v as _ };
> > + 0
> > + }
> > + Err(e) => e.to_errno(),
> > + }
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn map_voltage_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + min_uv: core::ffi::c_int,
> > + max_uv: core::ffi::c_int,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| T::map_voltage(&mut rdev, min_uv, max_uv))
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn set_voltage_sel_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + selector: core::ffi::c_uint,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + T::set_voltage_sel(&mut rdev, selector)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn get_voltage_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| T::get_voltage(&mut rdev))
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn get_voltage_sel_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| T::get_voltage_sel(&mut rdev))
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn set_current_limit_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + min_ua: core::ffi::c_int,
> > + max_ua: core::ffi::c_int,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + T::set_current_limit(&mut rdev, min_ua, max_ua)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn get_current_limit_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| T::get_current_limit(&mut rdev))
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn set_active_discharge_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + enable: bool,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + T::set_active_discharge(&mut rdev, enable)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn enable_callback(rdev: *mut bindings::regulator_dev) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + T::enable(&mut rdev)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn disable_callback(rdev: *mut bindings::regulator_dev) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + T::disable(&mut rdev)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn is_enabled_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + T::is_enabled(&mut rdev)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn set_mode_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + mode: core::ffi::c_uint,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + let mode = Mode::try_from(mode).unwrap_or(Mode::Invalid);
> > + T::set_mode(&mut rdev, mode)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn get_mode_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + ) -> core::ffi::c_uint {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + T::get_mode(&mut rdev) as _
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn get_status_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| Ok(T::get_status(&mut rdev)? as _))
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn set_suspend_voltage_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + uv: core::ffi::c_int,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + T::set_suspend_voltage(&mut rdev, uv)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn set_suspend_enable_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + T::set_suspend_enable(&mut rdev)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn set_suspend_disable_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + T::set_suspend_disable(&mut rdev)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + /// # Safety
> > + ///
> > + /// `rdev` must be non-null and valid.
> > + unsafe extern "C" fn set_suspend_mode_callback(
> > + rdev: *mut bindings::regulator_dev,
> > + mode: core::ffi::c_uint,
> > + ) -> core::ffi::c_int {
> > + // SAFETY: Per this function safety requirements, `rdev` is non-null and valid.
> > + let mut rdev = unsafe { Device::from_raw(rdev) };
> > + from_result(|| {
> > + let mode = Mode::try_from(mode).unwrap_or(Mode::Invalid);
> > + T::set_suspend_mode(&mut rdev, mode)?;
> > + Ok(0)
> > + })
> > + }
> > +
> > + const VTABLE: bindings::regulator_ops = bindings::regulator_ops {
> > + list_voltage: if T::HAS_LIST_VOLTAGE {
> > + Some(Adapter::<T>::list_voltage_callback)
> > + } else {
> > + None
> > + },
> > + set_voltage: if T::HAS_SET_VOLTAGE {
> > + Some(Adapter::<T>::set_voltage_callback)
> > + } else {
> > + None
> > + },
> > + map_voltage: if T::HAS_MAP_VOLTAGE {
> > + Some(Adapter::<T>::map_voltage_callback)
> > + } else {
> > + None
> > + },
> > + set_voltage_sel: if T::HAS_SET_VOLTAGE_SEL {
> > + Some(Adapter::<T>::set_voltage_sel_callback)
> > + } else {
> > + None
> > + },
> > + get_voltage: if T::HAS_GET_VOLTAGE {
> > + Some(Adapter::<T>::get_voltage_callback)
> > + } else {
> > + None
> > + },
> > + get_voltage_sel: if T::HAS_GET_VOLTAGE_SEL {
> > + Some(Adapter::<T>::get_voltage_sel_callback)
> > + } else {
> > + None
> > + },
> > + set_current_limit: if T::HAS_SET_CURRENT_LIMIT {
> > + Some(Adapter::<T>::set_current_limit_callback)
> > + } else {
> > + None
> > + },
> > + get_current_limit: if T::HAS_GET_CURRENT_LIMIT {
> > + Some(Adapter::<T>::get_current_limit_callback)
> > + } else {
> > + None
> > + },
> > + set_active_discharge: if T::HAS_SET_ACTIVE_DISCHARGE {
> > + Some(Adapter::<T>::set_active_discharge_callback)
> > + } else {
> > + None
> > + },
> > + enable: if T::HAS_ENABLE {
> > + Some(Adapter::<T>::enable_callback)
> > + } else {
> > + None
> > + },
> > + disable: if T::HAS_DISABLE {
> > + Some(Adapter::<T>::disable_callback)
> > + } else {
> > + None
> > + },
> > + is_enabled: if T::HAS_IS_ENABLED {
> > + Some(Adapter::<T>::is_enabled_callback)
> > + } else {
> > + None
> > + },
> > + set_mode: if T::HAS_SET_MODE {
> > + Some(Adapter::<T>::set_mode_callback)
> > + } else {
> > + None
> > + },
> > + get_mode: if T::HAS_GET_MODE {
> > + Some(Adapter::<T>::get_mode_callback)
> > + } else {
> > + None
> > + },
> > + get_status: if T::HAS_GET_STATUS {
> > + Some(Adapter::<T>::get_status_callback)
> > + } else {
> > + None
> > + },
> > + set_suspend_voltage: if T::HAS_SET_SUSPEND_VOLTAGE {
> > + Some(Adapter::<T>::set_suspend_voltage_callback)
> > + } else {
> > + None
> > + },
> > + set_suspend_enable: if T::HAS_SET_SUSPEND_ENABLE {
> > + Some(Adapter::<T>::set_suspend_enable_callback)
> > + } else {
> > + None
> > + },
> > + set_suspend_disable: if T::HAS_SET_SUSPEND_DISABLE {
> > + Some(Adapter::<T>::set_suspend_disable_callback)
> > + } else {
> > + None
> > + },
> > + set_suspend_mode: if T::HAS_SET_SUSPEND_MODE {
> > + Some(Adapter::<T>::set_suspend_mode_callback)
> > + } else {
> > + None
> > + },
> > + // SAFETY: The rest is zeroed out to initialize `struct regulator_ops`,
> > + // sets `Option<&F>` to be `None`.
> > + ..unsafe { core::mem::zeroed() }
> > + };
> > +
> > + const fn build() -> &'static bindings::regulator_ops {
> > + &Self::VTABLE
> > + }
> > +}
> >
> > --
> > 2.45.2
> >
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/9] dt-bindings: regulator: add binding for ncv6336 regulator
2024-12-19 9:28 ` Krzysztof Kozlowski
@ 2024-12-19 16:13 ` Fabien Parent
2024-12-21 20:20 ` Krzysztof Kozlowski
0 siblings, 1 reply; 21+ messages in thread
From: Fabien Parent @ 2024-12-19 16:13 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
On Thu, Dec 19, 2024 at 1:28 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Wed, Dec 18, 2024 at 03:36:37PM -0800, Fabien Parent wrote:
> > From: Fabien Parent <fabien.parent@linaro.org>
> >
> > Add binding documentation for the Onsemi NCV6336 regulator.
> >
> > Signed-off-by: Fabien Parent <fabien.parent@linaro.org>
> > ---
> > .../bindings/regulator/onnn,ncv6336.yaml | 54 ++++++++++++++++++++++
> > 1 file changed, 54 insertions(+)
> >
>
> subject: regulator first, then dt-bindings.
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with 'git log --oneline -- DIRECTORY_OR_FILE' on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>
>
> > diff --git a/Documentation/devicetree/bindings/regulator/onnn,ncv6336.yaml b/Documentation/devicetree/bindings/regulator/onnn,ncv6336.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..c69d126cab33668febe18e77bb34bd4bef52c993
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/onnn,ncv6336.yaml
> > @@ -0,0 +1,54 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/regulator/onnn,ncv6336.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Onsemi NCV6336 Buck converter
> > +
> > +maintainers:
> > + - Fabien Parent <fabien.parent@linaro.org>
> > +
> > +description: |
>
> Do not need '|' unless you need to preserve formatting.
>
> > + The NCV6336 is an I2C programmable BUCK (step-down) converter.
> > + It is designed for mobile power applications.
> > +
> > +properties:
> > + $nodename:
> > + pattern: "regulator@[0-9a-f]{2}"
>
> Drop nodename, not really needed in device schema.
>
> > +
> > + compatible:
> > + const: onnn,ncv6336
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + buck:
> > + description: buck regulator description
>
> Why do you need "buck" node? Just merge the properties into this device
> node.
I decided to move the properties into a "buck" node to make the
upstream process of the driver
a little bit simpler. The driver is written in Rust, and if I want to
move the properties to the device
node I will need to provide a Rust abstraction for "struct
device_node". I decided to avoid this
to keep the patch series simpler by having one less abstraction to review.
If you think that's a problem, let me know and I will implement it the
way you are suggesting for v2.
>
> > + type: object
> > + $ref: regulator.yaml#
> > + unevaluatedProperties: false
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - buck
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + regulator@1c {
> > + compatible = "onnn,ncv6336";
> > + reg = <0x1c>;
> > +
> > + buck {
> > + regulator-name = "ncv6336,buck";
> > + };
> > + };
>
> Messed indentation.
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/9] rust: regulator: add Regulator Driver abstraction
2024-12-19 16:00 ` Fabien Parent
@ 2024-12-19 18:58 ` Mark Brown
0 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2024-12-19 18:58 UTC (permalink / raw)
To: Fabien Parent
Cc: Danilo Krummrich, Rob Herring, Saravana Kannan, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Greg Kroah-Hartman, Rafael J. Wysocki, Wolfram Sang,
Liam Girdwood, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, devicetree, rust-for-linux, linux-kernel,
linux-i2c, linux-arm-msm, vinod.koul, Fabien Parent
[-- Attachment #1: Type: text/plain, Size: 657 bytes --]
On Thu, Dec 19, 2024 at 08:00:20AM -0800, Fabien Parent wrote:
> On Thu, Dec 19, 2024 at 2:26 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, Dec 18, 2024 at 03:36:35PM -0800, Fabien Parent wrote:
> > > From: Fabien Parent <fabien.parent@linaro.org>
> > >
> > > This commit adds a Rust abstraction to write Regulator drivers. Only
> > > the features used by the NCV6336 driver were added to this abstraction.
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/9] rust: add abstraction for regmap
2024-12-18 23:36 ` [PATCH 2/9] rust: add abstraction for regmap Fabien Parent
@ 2024-12-20 13:16 ` Mark Brown
0 siblings, 0 replies; 21+ messages in thread
From: Mark Brown @ 2024-12-20 13:16 UTC (permalink / raw)
To: Fabien Parent
Cc: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]
On Wed, Dec 18, 2024 at 03:36:32PM -0800, Fabien Parent wrote:
> The abstraction is bringing only a small subset of all the features
> provided by regmap by only supporting the most vital field from
> `struct regmap_config`.
I'm not so sure about that...
> +int rust_helper_regmap_field_write(struct regmap_field *field, unsigned int val)
> +{
> + return regmap_field_write(field, val);
> +}
...this is wrapping the field API. That's not very widely used at all,
and all the usages are in leaf drivers rather than frameworks. Given
this and that the code is basically simple syntactic sugar rather than
any substantial logic I would suggest not wrapping this for Rust but
instead writing native Rust abstractions that do the same thing. That
seems likely to be less work and give something that is more pleasant to
use in the Rust environment.
Indeed, it looks like the actual core regmap APIs aren't wrapped at all,
only the field ones?
> +//! ```ignore
> +//! regmap::define_regmap_field_descs!(FIELD_DESCS, {
> +//! (pid, 0x3, READ, { value => raw([7:0], ro) }),
> +//! (limconf, 0x16, RW, {
> +//! rearm => bit(0, rw),
> +//! rststatus => bit(1, rw),
> +//! tpwth => enum([5:4], rw, {
> +//! Temp83C = 0x0,
> +//! Temp94C = 0x1,
> +//! Temp105C = 0x2,
> +//! Temp116C = 0x3,
> +//! }),
> +//! })
> +//! });
You might want to include some explanation as to what this does because
it's quite unclear.
> +//! fn probe(client: &mut i2c::Client) -> Result {
> +//! let config = regmap::Config::<AccessOps>::new(8, 8)
> +//! .with_max_register(0x16)
> +//! .with_cache_type(regmap::CacheType::RbTree);
New code should use maple tree unless it's got a good reason.
> + /// Use RbTree caching
> + RbTree = bindings::regcache_type_REGCACHE_RBTREE,
> + /// Use Flat caching
> + Flat = bindings::regcache_type_REGCACHE_FLAT,
> + /// Use Maple caching
> + Maple = bindings::regcache_type_REGCACHE_MAPLE,
Maple Tree.
> +impl Regmap {
> + #[cfg(CONFIG_REGMAP_I2C = "y")]
Built in only?
> +/// `raw` should be used when bits cannot be represented by any other field types. It provides
> +/// raw access to the register bits.
> +///
> +/// # Syntax
> +///
> +/// `raw(bits_range, access)`
Given how many register fields are just a number I'm not sure calling
them "raw" fields really conveys the right message, it sounds like this
is bypassing things that should be there rather than a perfectly normal
thing to do.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 8/9] regulator: add driver for ncv6336 regulator
2024-12-18 23:36 ` [PATCH 8/9] regulator: add driver " Fabien Parent
2024-12-19 10:19 ` Dirk Behme
@ 2024-12-20 14:50 ` Mark Brown
1 sibling, 0 replies; 21+ messages in thread
From: Mark Brown @ 2024-12-20 14:50 UTC (permalink / raw)
To: Fabien Parent
Cc: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]
On Wed, Dec 18, 2024 at 03:36:38PM -0800, Fabien Parent wrote:
> +regmap::define_regmap_field_descs!(FIELD_DESCS, {
> + (pid, 0x3, READ, { value => raw([7:0], ro) }),
Looking at this it appears that the whole thing with only exposing
regmap fields is actually a deliberate attempt to elide whole register
access. This doesn't seem like a good idea, it's quite common to want
to operate on a register as a whole. For example there's the very
common case where we're updating multiple fields at once, we don't want
to do a separate read/modify/write cycle for each field.
As I mentioned in review of the regmap patch I'd expect that fields
would be layered on the underlying register access interface, and I'd
expect that we'd always be able to work with the registers.
> +static NCV6336_DESC: Desc = Desc::new::<Ncv6336>(c_str!("ncv6336"), Type::Voltage)
> + .with_owner(&THIS_MODULE)
> + .with_of_match(c_str!("buck"))
> + .with_active_discharge(
> + pgood::addr(),
> + pgood::dischg::mask(),
> + pgood::dischg::mask(),
> + 0,
> + )
I'm not sure these sequences of unnamed arguments are great for
legibility.
> +#[vtable]
> +impl Driver for Ncv6336 {
> + type Data = Arc<Mutex<Ncv6336RegulatorData>>;
> +
> + fn list_voltage(reg: &mut Device<Self::Data>, selector: u32) -> Result<i32> {
> + reg.list_voltage_linear(selector)
> + }
> +
> + fn enable(reg: &mut Device<Self::Data>) -> Result {
> + reg.enable_regmap()
> + }
> +
> + fn disable(reg: &mut Device<Self::Data>) -> Result {
> + reg.disable_regmap()
> + }
> +
> + fn is_enabled(reg: &mut Device<Self::Data>) -> Result<bool> {
> + reg.is_enabled_regmap()
> + }
I can't help but feel that this isn't a great way to deal with the ops
table - AIUI we're mapping the C functions to rust, then bouncing those
wrapped functions back to the C API. All of this with no abstraction of
the whole fact that this is a data driven way of specifying the regmap.
It feels like we're loosing a lot of the point of having these data
driven helpers, for most devices where we're data driven it looks like
it would be actively worse to write in rust.
As with the regmap fields I'd really expect to see this handled in a
much more directly rust native manner for drivers, in this case I'd
expect that to wind up with us reusing the C ops but handled by the
support code rather than directly by each driver. Drivers doing
complicated things might want to peer in and work with the ops (eg, some
devices need a special write sequence but the read side uses helpers)
but it shouldn't be the default.
> + fn get_status(reg: &mut Device<Self::Data>) -> Result<Status> {
> + if !Self::is_enabled(reg)? {
> + return Ok(Status::Off);
> + }
> +
> + Ok(Self::get_mode(reg).into())
> + }
This is buggy, it's just returning the values configured by the driver
not status from the hardware. If the hardware doesn't report status
don't implement this operation.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/9] dt-bindings: regulator: add binding for ncv6336 regulator
2024-12-19 16:13 ` Fabien Parent
@ 2024-12-21 20:20 ` Krzysztof Kozlowski
0 siblings, 0 replies; 21+ messages in thread
From: Krzysztof Kozlowski @ 2024-12-21 20:20 UTC (permalink / raw)
To: Fabien Parent
Cc: Rob Herring, Saravana Kannan, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Greg Kroah-Hartman,
Rafael J. Wysocki, Wolfram Sang, Mark Brown, Liam Girdwood,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
devicetree, rust-for-linux, linux-kernel, linux-i2c,
linux-arm-msm, vinod.koul, Fabien Parent
On 19/12/2024 17:13, Fabien Parent wrote:
>>
>>> +
>>> + compatible:
>>> + const: onnn,ncv6336
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + buck:
>>> + description: buck regulator description
>>
>> Why do you need "buck" node? Just merge the properties into this device
>> node.
>
> I decided to move the properties into a "buck" node to make the
> upstream process of the driver
> a little bit simpler. The driver is written in Rust, and if I want to
> move the properties to the device
> node I will need to provide a Rust abstraction for "struct
> device_node". I decided to avoid this
buck is already a device node, so I don't quite get how this design
avoids such abstraction, but anyway driver design choices like this do
not shape DT.
> to keep the patch series simpler by having one less abstraction to review.
> If you think that's a problem, let me know and I will implement it the
> way you are suggesting for v2.
>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2024-12-21 20:21 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-18 23:36 [PATCH 0/9] Regulator driver with I2C/Regmap Rust abstractions Fabien Parent
2024-12-18 23:36 ` [PATCH 1/9] rust: i2c: add basic I2C client abstraction Fabien Parent
2024-12-19 13:03 ` Rob Herring
2024-12-19 15:33 ` Fabien Parent
2024-12-18 23:36 ` [PATCH 2/9] rust: add abstraction for regmap Fabien Parent
2024-12-20 13:16 ` Mark Brown
2024-12-18 23:36 ` [PATCH 3/9] rust: error: add declaration for ENOTRECOVERABLE error Fabien Parent
2024-12-18 23:36 ` [PATCH 4/9] rust: regulator: add abstraction for Regulator's modes Fabien Parent
2024-12-18 23:36 ` [PATCH 5/9] rust: regulator: add Regulator Driver abstraction Fabien Parent
2024-12-19 10:26 ` Danilo Krummrich
2024-12-19 16:00 ` Fabien Parent
2024-12-19 18:58 ` Mark Brown
2024-12-18 23:36 ` [PATCH 6/9] rust: regulator: add support for regmap Fabien Parent
2024-12-18 23:36 ` [PATCH 7/9] dt-bindings: regulator: add binding for ncv6336 regulator Fabien Parent
2024-12-19 9:28 ` Krzysztof Kozlowski
2024-12-19 16:13 ` Fabien Parent
2024-12-21 20:20 ` Krzysztof Kozlowski
2024-12-18 23:36 ` [PATCH 8/9] regulator: add driver " Fabien Parent
2024-12-19 10:19 ` Dirk Behme
2024-12-20 14:50 ` Mark Brown
2024-12-18 23:36 ` [PATCH 9/9] arm64: dts: qcom: apq8039-t2: add node " Fabien Parent
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).