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@xxxxxxxxxx> > > This commit adds support for the Onsemi NCV6336 buck down converter. > > Signed-off-by: Fabien Parent <fabien.parent@xxxxxxxxxx> > --- > 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@xxxxxxxxxx>", > + 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), > + } > + } > +} >
