diff mbox series

[8/9] regulator: add driver for ncv6336 regulator

Message ID 20241218-ncv6336-v1-8-b8d973747f7a@gmail.com (mailing list archive)
State New
Headers show
Series Regulator driver with I2C/Regmap Rust abstractions | expand

Commit Message

Fabien Parent Dec. 18, 2024, 11:36 p.m. UTC
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(+)

Comments

Dirk Behme Dec. 19, 2024, 10:19 a.m. UTC | #1
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(&regmap, &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),
> +        }
> +    }
> +}
>
Mark Brown Dec. 20, 2024, 2:50 p.m. UTC | #2
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.
diff mbox series

Patch

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(&regmap, &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),
+        }
+    }
+}