On Tue Oct 28, 2025 at 2:37 AM JST, Joel Fernandes wrote: > Hello Alex, > > On Mon, Oct 27, 2025 at 09:54:43PM +0900, Alexandre Courbot wrote: >> Since `BiosImageType` is `repr(u8)`, if can safely be converted into a >> `u8` but this is not obvious when doing this in the code. >> >> Instead, implement `From<BiosImageType>` for `u8` so the cast can be >> done in a single place, with a justifying comment. >> >> Acked-by: Danilo Krummrich <[email protected]> >> Signed-off-by: Alexandre Courbot <[email protected]> >> --- >> drivers/gpu/nova-core/vbios.rs | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs >> index dbe0d6e4a015..a521c0a4df0f 100644 >> --- a/drivers/gpu/nova-core/vbios.rs >> +++ b/drivers/gpu/nova-core/vbios.rs >> @@ -50,6 +50,13 @@ fn try_from(code: u8) -> Result<Self> { >> } >> } >> >> +impl From<BiosImageType> for u8 { >> + fn from(value: BiosImageType) -> Self { >> + // `BiosImageType` is `repr(u8)` and thus convertible without loss. >> + value as u8 >> + } >> +} >> + >> // PMU lookup table entry types. Used to locate PMU table entries >> // in the Fwsec image, corresponding to falcon ucodes. >> #[expect(dead_code)] >> @@ -711,7 +718,7 @@ fn image_type(&self) -> Result<BiosImageType> { >> fn is_last(&self) -> bool { >> // For NBSI images (type == 0x70), return true as they're >> // considered the last image >> - if self.pcir.code_type == BiosImageType::Nbsi as u8 { >> + if self.pcir.code_type == BiosImageType::Nbsi.into() { > > I strongly prefer u8::from(BiosImageType::Nbsi) here so there is no loss of > readability of the type. Can we please use ::from()?
What benefit do we get from knowing the representing type of BiosImageType? We are only interested in knowing whether the two values are equal or not. But you have a point that `::from()` is generally easier to read, only I would apply it on the left value: if BiosImageType::try_from(self.pcir.code_type) == Ok(BiosImageType::Nbsi) or reusing the already existing code: if self.image_type() == Ok(BiosImageType::Nbsi) That reads even more naturally imho.
