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