W dniu 14.09.2021 o 20:59, Jani Nikula pisze:
On Tue, 14 Sep 2021, Doug Anderson dianders@chromium.org wrote:
Hi,
On Tue, Sep 14, 2021 at 11:16 AM Jani Nikula jani.nikula@linux.intel.com wrote:
On Thu, 09 Sep 2021, Douglas Anderson dianders@chromium.org wrote:
In the patch ("drm/edid: Allow the querying/working with the panel ID from the EDID") we introduced a different way of working with the panel ID stored in the EDID. Let's use this new way for the quirks code.
Advantages of the new style:
- Smaller data structure size. Saves 4 bytes per panel.
- Iterate through quirks structure with just "==" instead of strncmp()
- In-kernel storage is more similar to what's stored in the EDID itself making it easier to grok that they are referring to the same value.
The quirk table itself is arguably a bit less readable in the new style but not a ton less and it feels like the above advantages make up for it.
I suppose you could pass vendor as a string to EDID_QUIRK() to retain better readability?
I would love to, but I couldn't figure out how to do this and have it compile! Notably I need the compiler to be able to do math at compile time to compute the final u32 to store in the init data. I don't think the compiler can dereference strings (even constant strings) and do math on the result at compile time.
Ah, right.
What about:
+#define drm_edid_encode_panel_id(vend, product_id) \ + ((((u32)((vend)[0]) - '@') & 0x1f) << 26 | \ + (((u32)((vend)[1]) - '@') & 0x1f) << 21 | \ + (((u32)((vend)[2]) - '@') & 0x1f) << 16 | \ + ((product_id) & 0xffff))
Regards
Andrzej
I _think_ you could make it work with four-character codes (only specifying 3 characters), AKA single-quoting something like 'AUO' but I think four-character codes are not dealt with in a standard enough way between compilers so they're not allowed in Linux.
If you like it better, I could do something like this:
#define ACR_CODE 'A', 'C', 'R' #define AUO_CODE 'A', 'U', 'O' ... ...
...then I could refer to the #defines...
Nah.
Just bikeshedding, really. ;)
I'll take this comment (without any formal tags) as:
- You've seen this patch (and the previous ones) and wouldn't object
to it merging.
- You're not planning on any deeper review / testing, so I shouldn't
wait for more stuff from you before merging. Please yell if this is not the case. I'm happy to wait but I don't want to wait if no further review is planned.
I have now reviewed the ones where my review is relevant, and certainly don't expect me to comment on the rest. ;)
BR, Jani.
In general I'm still planning to give this series at least another week for comments before merging. ${SUBJECT} patch also is the only one lacking any type of Review / Ack tags so I'll see if I can find someone to give it something before merging, too.
-Doug