On Fri, Nov 22, 2019 at 03:55:32PM +0200, Ville Syrjälä wrote:
On Thu, Nov 21, 2019 at 10:54:29AM -0800, Lucas De Marchi wrote:On Thu, Nov 21, 2019 at 03:09:03PM +0200, Jani Nikula wrote: >On Wed, 20 Nov 2019, Lucas De Marchi <lucas.demarchi@xxxxxxxxx> wrote: >> The unaligned ioread32() will make us read byte by byte looking for the >> vbt. We could just as well have done a ioread8() + a shift and avoid the >> extra confusion on how we are looking for "$VBT". >> >> However when using ACPI it's guaranteed the VBT is 4-byte aligned >> per spec, so we can probably assume it here as well. >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/display/intel_bios.c | 19 ++++++------------- >> 1 file changed, 6 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c >> index aa9b182efee5..6bf57b1ad056 100644 >> --- a/drivers/gpu/drm/i915/display/intel_bios.c >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c >> @@ -1902,27 +1902,20 @@ static struct vbt_header *oprom_get_vbt(struct drm_i915_private *dev_priv) >> void __iomem *p = NULL, *oprom; >> struct vbt_header *vbt; >> u16 vbt_size; >> - size_t i, size; >> + size_t size; >> >> oprom = pci_map_rom(pdev, &size); >> if (!oprom) >> return NULL; >> >> /* Scour memory looking for the VBT signature. */ >> - for (i = 0; i + 4 < size; i++) { >> - if (ioread32(oprom + i) != *((const u32 *)"$VBT")) >> - continue; >> - >> - p = oprom + i; >> - size -= i; >> - break; >> - } >> - >> - if (!p) >> - goto err_unmap_oprom; >> + for (p = oprom; size >= 4; p += 4, size -= 4) >> + if (ioread32(p) == *((const u32 *)"$VBT")) >> + break; > >I think the original is easier to read. You only really need to change >"i++" to "i += 4" and be done with it. I really liked this version much more... shorter and with only one control variable rather than keeping the control in 3 different places (i, size and p).I think I'm with Jani here. Generally not a huge fan of pointer arithmetic, and having just one variable modified by the loop is more customary so usually doesn't require me to read more than
we were previously modifying 3: i, p and size. And additionally using oprom.... versus we only care about p inside the loop, which points to whatever we just read... and we we keep updates on size to control the stop condition.
once. This thing I had to read a few times to make sure I understood what it's doing.
Ok, 2 against 1. I will respin this. Lucas De Marchi
-- Ville Syrjälä Intel
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
