On 22.06.2022 08:17, Alyssa Rosenzweig wrote:
>> + js_as_offset = slot * 0x80;
>
>JS_SLOT_STRIDE
Sorry about this blunder.
>> + slot = panfrost_job_get_slot(job);
>> + slot = slot ? slot : 0;
>
>`slot = slot ? slot : 0` is a no-op. Delete the line.
I think what I meant here was 'slot = (slot >= 0) ? slot : 0;' but for some
reason I blundered again. The point of this was ensuring the slot value wouldn't
end up wrapping about the maximum unsigned integer value when using it as an
array offset, in the off-chance that panfrost_job_get_slot() ever returned a
negative value.
In v4 I've instead rewritten this as a sanity check:
WARN_ON(slot < 0);
Although perhaps in the future panfrost_job_get_slot should return an unsigned
integer instead?
>> + if (!IS_ERR(page))
>> + *bomap++ = cpu_to_le64(page_to_phys(page));
>> + else {
>> + dev_err(pfdev->dev, "Panfrost Dump: wrong page\n");
>> + *bomap++ = ~cpu_to_le64(0);
>> + }
>> + }
>
>Nit: because you have { braces } around half the if, please add
>{ braces } around the other half for consistency.
I thought checkpatch.pl would complain about braces wrapping a single if
statement, but apparently it's fine with it in this case.
>---
>
>As a general note, I'd appreciate breaking out the panfrost_regs.h
>changes into a separate patch, as they are a logically separate clean
>up to make room for this patch. Thanks.
Done in v4.
Cheers,
Adrian
