diff mbox series

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

Message ID 20241218-ncv6336-v1-8-b8d973747f7a@gmail.com
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

Mark Brown Dec. 20, 2024, 2:50 p.m. UTC | #1
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),
+        }
+    }
+}