Hi Lyude,
On Sat Nov 15, 2025 at 6:55 AM JST, Lyude Paul wrote:
> We're very close! I see a few things we still need to fix though
>
> On Fri, 2025-11-14 at 14:55 -0500, Joel Fernandes wrote:
>> ...
>> -impl GspSeqCmdRunner for GspSeqCmd {
>> - fn run(&self, _seq: &GspSequencer<'_>) -> Result {
>> +impl GspSeqCmdRunner for fw::RegWritePayload {
>> + fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> + let addr = self.addr() as usize;
>> + let val = self.val();
>> + let _ = sequencer.bar.try_write32(val, addr);
>
> We're still not handling the possible error from try_write32() here
> Also - addr/val seem a bit superfluous
I wonder whether this could be on purpose? As in, the register sequence
might try to write to non-existing registers on some models but this is
not necessarily a fatal error. Still, I can complete initialization even
when handling the error, so let's err on the side or correctness unless
Joel signals that this is indeed what we want (in which case a comment
would ensure there cannot be any confusion as to the purpose).
For now, changed this to:
let addr = usize::from_safe_cast(self.addr());
sequencer.bar.try_write32(self.val(), addr)
>
>> Ok(())
>> }
>> }
>>
>> +impl GspSeqCmdRunner for fw::RegModifyPayload {
>> + fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> + let addr = self.addr() as usize;
>> + if let Ok(temp) = sequencer.bar.try_read32(addr) {
>> + let _ = sequencer
>> + .bar
>> + .try_write32((temp & !self.mask()) | self.val(), addr);
>
> Same here
This is now:
let addr = usize::from_safe_cast(self.addr());
sequencer.bar.try_read32(addr).and_then(|val| {
sequencer
.bar
.try_write32((val & !self.mask()) | self.val(), addr)
})
>
>> + }
>> + Ok(())
>> + }
>> +}
>> +
>> +impl GspSeqCmdRunner for fw::RegPollPayload {
>> + fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> + let addr = self.addr() as usize;
>> +
>> + // Default timeout to 4 seconds.
>> + let timeout_us = if self.timeout() == 0 {
>> + 4_000_000
>> + } else {
>> + i64::from(self.timeout())
>> + };
>> +
>> + // First read.
>> + sequencer.bar.try_read32(addr)?;
>> +
>> + // Poll the requested register with requested timeout.
>> + read_poll_timeout(
>> + || sequencer.bar.try_read32(addr),
>> + |current| (current & self.mask()) == self.val(),
>> + Delta::ZERO,
>> + Delta::from_micros(timeout_us),
>> + )
>> + .map(|_| ())
>> + }
>> +}
>> +
>> +impl GspSeqCmdRunner for fw::RegStorePayload {
>> + fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> + let addr = self.addr() as usize;
>> + let _index = self.index();
>> +
>> + let _val = sequencer.bar.try_read32(addr)?;
>> +
>> + Ok(())
>
> These variables still seem superfluous - we don't use _index at all.
>
> This function should just be rewritten as:
>
> fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
> sequencer.bar.try_read32(self.addr() as usize)?;
Actually I thought this is also a good opportunity to replace the `as`
with the safer `FromSafeCast` interface we introduced recently precisely
for that (you probably have already noticed from the previous fixes :)).
Only this would look a bit heavy if done inline, so for readability I'll
keep the temporary variable and change this to:
let addr = usize::from_safe_cast(self.addr());
sequencer.bar.try_read32(addr).map(|_| ())
This makes me wonder about the `index` member of this op - it is not
used at all! Joel, do you know what it is for?
Thanks for spotting these!
