Re: [PATCH v3 09/10] OMAP4: DSS2: HDMI: Function pointer approach to call

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Tue, Sep 6, 2011 at 6:15 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote:
> On Mon, 2011-09-05 at 23:03 +0530, K, Mythri P wrote:
>> Hi,
>>
>> On Mon, Sep 5, 2011 at 4:31 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote:
>> > On Fri, 2011-09-02 at 16:17 +0530, mythripk@xxxxxx wrote:
>> >> From: Mythri P K <mythripk@xxxxxx>
>> >>
>> >> To make the current hdmi DSS driver compatible with future OMAP having
>> >> different IP blocks( A combination of different core, PHY, PLL blocks),
>> >> Add HDMI hdmi functions as a function pointer in dss_features to abstract
>> >> hdmi dss driver IP agnostic, hdmi dss driver  which will now access
>> >> generic functions irrespective of underlying IP.
>> >>
>
> <snip>
>
>> >> diff --git a/include/video/omapdss.h b/include/video/omapdss.h
>> >> index 9398dd3..c8891d1 100644
>> >> --- a/include/video/omapdss.h
>> >> +++ b/include/video/omapdss.h
>> >> @@ -21,6 +21,9 @@
>> >>  #include <linux/list.h>
>> >>  #include <linux/kobject.h>
>> >>  #include <linux/device.h>
>> >> +#if defined(CONFIG_OMAP4_DSS_HDMI)
>> >> +#include <video/ti_hdmi.h>
>> >> +#endif
>> >>
>> >>  #define DISPC_IRQ_FRAMEDONE          (1 << 0)
>> >>  #define DISPC_IRQ_VSYNC                      (1 << 1)
>> >> @@ -555,6 +558,26 @@ struct omap_dss_driver {
>> >>       u32 (*get_wss)(struct omap_dss_device *dssdev);
>> >>  };
>> >>
>> >> +#if defined(CONFIG_OMAP4_DSS_HDMI)
>> >> +struct omap_hdmi_ip_ops {
>> >> +
>> >> +     void (*video_configure)(struct hdmi_ip_data *ip_data);
>> >> +
>> >> +     int (*phy_enable)(struct hdmi_ip_data *ip_data);
>> >> +
>> >> +     void (*phy_disable)(struct hdmi_ip_data *ip_data);
>> >> +
>> >> +     int (*read_edid)(struct hdmi_ip_data *ip_data,
>> >> +                     u8 *pedid, u16 max_length);
>> >> +
>> >> +     int (*pll_enable)(struct hdmi_ip_data *ip_data);
>> >> +
>> >> +     void (*pll_disable)(struct hdmi_ip_data *ip_data);
>> >> +
>> >> +     void (*video_enable)(struct hdmi_ip_data *ip_data, bool start);
>> >> +};
>> >> +#endif
>> >> +
>> >
>> > Hmm, I don't think omapdss.h is the right place for this struct.
>> >
>> > You've made it omap specific, but similar struct will be needed by other
>> > platforms also, right? So would it better be in ti_hdmi.h, and perhaps
>> > called ti_hdmi_ip_ops?
>> >
>> > And actually, ti_hdmi.h contains struct hdmi_ip_data, which contains
>> > pointer to struct omap_hdmi_ip_ops, so it's clear something is wrong
>> > there.
>> >
>> > So either the omap_hdmi_ip_ops should be omapdss internal struct, and it
>> > shouldn't be in struct hdmi_ip_data, or the ops struct should be generic
>> > one in ti_hdmi.h.
>> >
>> > Did you consider how the code would look if the function pointers were
>> > just included into struct hdmi_ip_data, without any ops struct at all?
>> >
>> I guess moving it to ti_hdmi.h is a good option.. but i would think
>> wrapping it in a struct
>> would look cleaner ?
>
> You didn't make this change for the v4?
>
copy error , I resent the patch. Incorporated the comment.

Thanks and regards,
Mythri.
>  Tomi
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux