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:
signature.asc
Description: PGP signature
