>>> 
>>> Is there any reason why you replace the UPPERCASE register names with
>>> CamelCase ones?
>>> 
>>> I was under the impression that we want to use UPPERCASE for register
>>> names. Like in nova
>>> 
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs
>> Not really. UPPERCASE for non-const items will trigger the linter. The Nova
>> people chose to #[allow] this to align with OpenRM and, IIRC from the LPC
>> discussions, their registers are automatically generated from some internal
>> docs.
>> We have only a few, we can simply convert them to CamelCase.
> 
> 
> I'm under the impression that we define the "future RFL register!() style 
> standard" here.
> 
> So we want to make the CamelCase the default? And nova is the exception?

I’m not sure I would say this. It’s just that you would hit this lint
[0]. If UPPER_CASE was the “default", we would have to have the #[allow] on
every driver. 

> 
> I'm fine with that. Just want to make sure we talked about it :)
> 
> 
> ....
>>>> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
>>>> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
>>>> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>>>> 
>>>> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
>>>> +register!(McuControl @ 0x700, "Controls the execution state of the MCU 
>>>> subsystem" {
>>>> +    1:0     req as u32, "Request state change";
>>>> +});
>>> 
>>> 
>>> Any reason why req is a u32 and not a u8? Same for some other places.
>> I tend to default to u32/i32 in general, as that’s usually the native 
>> machine integer type.
>> All we get from smaller types is a spam of `into()`, `from()` and their 
>> `try_`
>> equivalents. When stored in a struct, they usually do not save space due to
>> padding that is usually inserted to fix the alignment for the type. IMHO not
>> worth it unless it really matters. Correct me if I'm wrong, but it doesn't 
>> seem
>> to be the case here.
> 
> 
> Wouldn't using u8 prevent any accidental access to 31:8 ?

The macro is doing the appropriate masking according to the bit ranges you pass
in (i.e.: 31:8), not according to the type (u8 or u32), i.e.:

        const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = 
$lo..=$hi; <------
        const [<$field:upper _MASK>]: $storage = {
            // Generate mask for shifting
            match ::core::mem::size_of::<$storage>() {
                1 => ::kernel::bits::genmask_u8($lo..=$hi) as $storage,
                2 => ::kernel::bits::genmask_u16($lo..=$hi) as $storage,
                4 => ::kernel::bits::genmask_u32($lo..=$hi) as $storage,
                8 => ::kernel::bits::genmask_u64($lo..=$hi) as $storage,
                _ => ::kernel::build_error!("Unsupported storage type size")
            }
        };
        const [<$field:upper _SHIFT>]: u32 = $lo;
        );

        $(
        #[doc="Returns the value of this field:"]
        #[doc=$comment]
        )?
        #[inline(always)]
        $vis fn $field(self) -> $res_type {
            ::kernel::macros::paste!(
            const MASK: $storage = $name::[<$field:upper _MASK>];
            const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
            );
            let field = ((self.0 & MASK) >> SHIFT);

            $process(field)
        }


> 
> 
> Best regards
> 
> Dirk


[0]: 
https://doc.rust-lang.org/stable/nightly-rustc/rustc_lint/nonstandard_style/static.NON_CAMEL_CASE_TYPES.html

Reply via email to