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