Re: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset

[Paolo Bonzini]

On 1/22/25 15:34, Zhao Liu wrote:
> On Fri, Jan 17, 2025 at 10:26:50AM +0100, Paolo Bonzini wrote:
> > Date: Fri, 17 Jan 2025 10:26:50 +0100
> > From: Paolo Bonzini <pbonzini@redhat.com>
> > Subject: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset
> > X-Mailer: git-send-email 2.47.1
> >
> > As an added bonus, this also makes the new function return u32 instead
> > of u64, thus factoring some casts into a single place.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   rust/hw/char/pl011/src/device.rs | 114 +++++++++++++++++--------------
> >   1 file changed, 63 insertions(+), 51 deletions(-)
>
> [snip]
>
> > -    pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> 
> > std::ops::ControlFlow<u64, u64> {
> > +    fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> {
> >           use RegisterOffset::*;
>
> Can we move this "use" to the start of the file?

I don't think it's a good idea to make the register names visible 
globally...  "use Enum::*" before a match statement is relatively 
common.  For example: https://doc.rust-lang.org/src/std/io/error.rs.html#436

> > +        std::ops::ControlFlow::Break(match offset {
>
> std::ops can be omitted now.

Done, add added a patch to get rid of ControlFlow completely.

> > -            Ok(RSR) => {
> > -                self.receive_status_error_clear.reset();
> > +            RSR => {
> > +                self.receive_status_error_clear = 0.into();
>
> Emm, why do we use 0.into() instead of reset() here? It looks they're
> same.

Fixed.

> > +    pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, 
> > u64> {
>
> Maybe pub(crate)? But both are fine for me :-)

The struct is not public outside the crate, so it doesn't make a 
difference, does it?

> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Thanks, I'll post a quick v2 anyway once you've finished reviewing.

Paolo