On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote:
Hi
2012/12/6 Paulo Zanoni przanoni@gmail.com:
Hi
2012/12/5 Thierry Reding thierry.reding@avionic-design.de:
Use the generic HDMI infoframe helpers to get rid of the duplicate implementation in the i915 driver.
A few comments:
- I've compiled your patches and now "intel_infoframes -d" tells my my
infoframes are full of zeroes. So there's a bug somewhere... I have to say that the i915 infoframe registers are quite complicated and it took me a long time to fix a lot of its problems, so please don't break it and read below for a suggestion :)
- Why do we need that kconfig stuff? Why not just put all the code
inside drivers/gpu/drm?
- I really like having "common code between drivers merged", but I
really don't see how the i915 driver is benefiting from this change. We're just basically complicating intel_hdmi.c to use a new struct that doesn't fit our needs due to the lack of the ECC byte. My idea: Instead of calling hdmi_avi_infoframe_pack, why don't we just implement intel_hdmi_avi_infoframe_pack that converts a "struct hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that has the cool ECC value in the right place? This would probably drastically reduce your chances of introducing bugs in our code :)
Just to be a little bit more clear about this paragraph since Daniel asked me a few questions:
For the changes inside intel_hdmi.c, I'd really like your patch to not touch any of the "write_infoframe" or "set_infoframes" callbacks. I think you can do everything by just patching intel_set_infoframe, intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one of those functions you could call something like "intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct dip_infoframe)" and then proceed. This way you don 't need to touch the code that actually writes stuff to the hardware.
I think I already explained in my previous mail how IMO this completely defeats the purpose of this patch series. Furthermore the functions that write the infoframes to the hardware themselves could use some refactoring of their own. There is a lot of duplication there.
Thierry