From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: mythripk@ti.com
Cc: linux-omap@vger.kernel.org
Subject: Re: [PATCH v2 08/10] OMAP4: DSS2: HDMI: Function pointer approach to call
Date: Thu, 01 Sep 2011 11:55:41 +0300 [thread overview]
Message-ID: <1314867341.2169.18.camel@lappyti> (raw)
In-Reply-To: <1314598500-24005-9-git-send-email-mythripk@ti.com>
On Mon, 2011-08-29 at 11:44 +0530, mythripk@ti.com wrote:
> From: Mythri P K <mythripk@ti.com>
>
> To make the current hdmi DSS driver compatible with future OMAP with different
> IP blocks , add HDMI as a feature in dss_features and abstract the internal
> function in hdmi dss driver.
No space before comma.
The description could use some improvement, HDMI is not "a feature in
dss_features", and I don't understand what the rest of the sentence
means.
> Signed-off-by: Mythri P K <mythripk@ti.com>
> ---
> drivers/video/omap2/dss/dss_features.c | 24 +++++++++++++++++++++++-
> drivers/video/omap2/dss/dss_features.h | 1 +
> drivers/video/omap2/dss/hdmi.c | 22 +++++++++++-----------
> include/video/omapdss.h | 19 +++++++++++++++++++
> include/video/omaphdmi.h | 1 +
> 5 files changed, 55 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
> index b63c5f8..edf2929 100644
> --- a/drivers/video/omap2/dss/dss_features.c
> +++ b/drivers/video/omap2/dss/dss_features.c
> @@ -429,6 +429,26 @@ static const struct omap_dss_features omap4_dss_features = {
> .burst_size_unit = 16,
> };
>
> +const struct omap_hdmi_ip_driver *omap_hdmi_functions;
Should be static, but considering you never use this variable, except to
assign it once, it should be removed.
> +
> +/* HDMI OMAP4 Functions*/
> +const struct omap_hdmi_ip_driver omap4_hdmi_functions = {
Should be static.
> +
> + .video_configure = hdmi_basic_configure,
> + .phy_enable = hdmi_phy_enable,
> + .phy_disable = hdmi_phy_disable,
> + .read_edid = read_edid,
> + .pll_enable = hdmi_pll_enable,
> + .pll_disable = hdmi_pll_disable,
> + .video_enable = hdmi_wp_video_start,
> +};
Check that this compiles if HDMI is disabled.
> +
> +void dss_hdmi_features_init(struct hdmi_ip_data *ip_data)
Perhaps something like dss_init_hdmi_ip_data() would be better?
> +{
> + if (cpu_is_omap44xx())
> + ip_data->hdmi_ops = &omap4_hdmi_functions;
> +}
> +
> /* Functions returning values related to a DSS feature */
> int dss_feat_get_num_mgrs(void)
> {
> @@ -512,8 +532,10 @@ void dss_features_init(void)
> omap_current_dss_features = &omap3430_dss_features;
> else if (omap_rev() == OMAP4430_REV_ES1_0)
> omap_current_dss_features = &omap4430_es1_0_dss_features;
> - else if (cpu_is_omap44xx())
> + else if (cpu_is_omap44xx()) {
> omap_current_dss_features = &omap4_dss_features;
> + omap_hdmi_functions = &omap4_hdmi_functions;
No need for this code.
> + }
> else
> DSSWARN("Unsupported OMAP version");
> }
> diff --git a/drivers/video/omap2/dss/dss_features.h b/drivers/video/omap2/dss/dss_features.h
> index 4271e96..ca64b21 100644
> --- a/drivers/video/omap2/dss/dss_features.h
> +++ b/drivers/video/omap2/dss/dss_features.h
> @@ -99,4 +99,5 @@ u32 dss_feat_get_burst_size_unit(void); /* in bytes */
> bool dss_has_feature(enum dss_feat_id id);
> void dss_feat_get_reg_field(enum dss_feat_reg_field id, u8 *start, u8 *end);
> void dss_features_init(void);
> +void dss_hdmi_features_init(struct hdmi_ip_data *ip_data);
> #endif
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index fb7d66f..c508cf6 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -184,7 +184,7 @@ static void hdmi_runtime_put(void)
> int hdmi_init_display(struct omap_dss_device *dssdev)
> {
> DSSDBG("init_display\n");
> -
> + dss_hdmi_features_init(&hdmi.hdmi_data);
> return 0;
> }
>
> @@ -364,8 +364,8 @@ static void hdmi_read_edid(struct omap_video_timings *dp)
> memset(hdmi.edid, 0, HDMI_EDID_MAX_LENGTH);
>
> if (!hdmi.edid_set)
> - ret = read_edid(&hdmi.hdmi_data, hdmi.edid,
> - HDMI_EDID_MAX_LENGTH);
> + ret = hdmi.hdmi_data.hdmi_ops->read_edid(&hdmi.hdmi_data,
> + hdmi.edid, HDMI_EDID_MAX_LENGTH);
hdmi_ops could be just "ops", there's no possibility to confuse it with
some other ops.
> if (!ret) {
> if (!memcmp(hdmi.edid, edid_header, sizeof(edid_header))) {
> /* search for timings of default resolution */
> @@ -475,16 +475,16 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
>
> hdmi_compute_pll(dssdev, phy, &hdmi.hdmi_data.pll_data);
>
> - hdmi_wp_video_start(&hdmi.hdmi_data, 0);
> + hdmi.hdmi_data.hdmi_ops->video_enable(&hdmi.hdmi_data, 0);
>
> /* config the PLL and PHY hdmi_set_pll_pwrfirst */
> - r = hdmi_pll_enable(&hdmi.hdmi_data);
> + r = hdmi.hdmi_data.hdmi_ops->pll_enable(&hdmi.hdmi_data);
> if (r) {
> DSSDBG("Failed to lock PLL\n");
> goto err;
> }
>
> - r = hdmi_phy_enable(&hdmi.hdmi_data);
> + r = hdmi.hdmi_data.hdmi_ops->phy_enable(&hdmi.hdmi_data);
> if (r) {
> DSSDBG("Failed to start PHY\n");
> goto err;
> @@ -492,7 +492,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
>
> hdmi.hdmi_data.cfg.cm.mode = hdmi.mode;
> hdmi.hdmi_data.cfg.cm.code = hdmi.code;
> - hdmi_basic_configure(&hdmi.hdmi_data);
> + hdmi.hdmi_data.hdmi_ops->video_configure(&hdmi.hdmi_data);
>
> /* Make selection of HDMI in DSS */
> dss_select_hdmi_venc_clk_source(DSS_HDMI_M_PCLK);
> @@ -514,7 +514,7 @@ static int hdmi_power_on(struct omap_dss_device *dssdev)
>
> dispc_mgr_enable(OMAP_DSS_CHANNEL_DIGIT, 1);
>
> - hdmi_wp_video_start(&hdmi.hdmi_data, 1);
> + hdmi.hdmi_data.hdmi_ops->video_enable(&hdmi.hdmi_data, 1);
>
> return 0;
> err:
> @@ -526,9 +526,9 @@ static void hdmi_power_off(struct omap_dss_device *dssdev)
> {
> dispc_mgr_enable(OMAP_DSS_CHANNEL_DIGIT, 0);
>
> - hdmi_wp_video_start(&hdmi.hdmi_data, 0);
> - hdmi_phy_disable(&hdmi.hdmi_data);
> - hdmi_pll_disable(&hdmi.hdmi_data);
> + hdmi.hdmi_data.hdmi_ops->video_enable(&hdmi.hdmi_data, 0);
> + hdmi.hdmi_data.hdmi_ops->phy_disable(&hdmi.hdmi_data);
> + hdmi.hdmi_data.hdmi_ops->pll_disable(&hdmi.hdmi_data);
> hdmi_runtime_put();
>
> hdmi.edid_set = 0;
> diff --git a/include/video/omapdss.h b/include/video/omapdss.h
> index 9398dd3..bf2aeba 100644
> --- a/include/video/omapdss.h
> +++ b/include/video/omapdss.h
> @@ -21,6 +21,7 @@
> #include <linux/list.h>
> #include <linux/kobject.h>
> #include <linux/device.h>
> +#include <video/omaphdmi.h>
Why do you add this?
>
> #define DISPC_IRQ_FRAMEDONE (1 << 0)
> #define DISPC_IRQ_VSYNC (1 << 1)
> @@ -555,6 +556,24 @@ struct omap_dss_driver {
> u32 (*get_wss)(struct omap_dss_device *dssdev);
> };
>
> +struct omap_hdmi_ip_driver {
The naming is somewhat confusing. Why not just name it 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);
> +};
> +
> int omap_dss_register_driver(struct omap_dss_driver *);
> void omap_dss_unregister_driver(struct omap_dss_driver *);
>
> diff --git a/include/video/omaphdmi.h b/include/video/omaphdmi.h
> index 88b1ccb..a740237 100644
> --- a/include/video/omaphdmi.h
> +++ b/include/video/omaphdmi.h
> @@ -80,6 +80,7 @@ struct hdmi_ip_data {
> unsigned long core_av_offset;
> unsigned long pll_offset;
> unsigned long phy_offset;
> + const struct omap_hdmi_ip_driver *hdmi_ops;
> struct hdmi_config cfg;
> struct hdmi_pll_info pll_data;
> };
next prev parent reply other threads:[~2011-09-01 8:55 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-08-29 6:14 [PATCH v2 00/10]HDMI: Split hdmi.c in DSS to seperate OMAP dependent mythripk
2011-08-29 6:14 ` [PATCH v2 01/10] OMAP4: DSS: HDMI: HDMI clean up to pass base_address mythripk
2011-08-29 6:14 ` [PATCH v2 02/10] OMAP4: DSS: HDMI: Move pll and video configuration mythripk
2011-08-29 6:14 ` [PATCH v2 03/10] OMAP4: DSS: HDMI: Use specific HDMI timings structure mythripk
2011-08-29 6:14 ` [PATCH v2 04/10] OMAP4: DSS: HDMI: Move the common header file mythripk
2011-08-29 6:14 ` [PATCH v2 05/10] OMAP4 : DSS : HDMI : Move the EDID portion from HDMI mythripk
2011-08-29 6:14 ` [PATCH v2 06/10] OMAP4: DSS2: HDMI: Split the HDMI driver to DSS and IP mythripk
2011-08-29 6:14 ` [PATCH v2 07/10] OMAP4: DSS2: HDMI: Provide a wrapper API to configure mythripk
2011-08-29 6:14 ` [PATCH v2 08/10] OMAP4: DSS2: HDMI: Function pointer approach to call mythripk
2011-08-29 6:14 ` [PATCH v2 09/10] MAP4: DSS: HDMI: Rename the functions in HDMI IP mythripk
2011-08-29 6:15 ` [PATCH v2 10/10] OMAP4: DSS: Rename hdmi_omap4_panel.c to hdmi_panel.c mythripk
2011-09-01 8:55 ` Tomi Valkeinen [this message]
2011-09-01 8:38 ` [PATCH v2 07/10] OMAP4: DSS2: HDMI: Provide a wrapper API to configure Tomi Valkeinen
2011-09-01 9:00 ` [PATCH v2 04/10] OMAP4: DSS: HDMI: Move the common header file Tomi Valkeinen
2011-09-02 5:15 ` K, Mythri P
2011-09-02 5:24 ` Tomi Valkeinen
2011-09-02 5:27 ` K, Mythri P
2011-09-01 8:14 ` [PATCH v2 03/10] OMAP4: DSS: HDMI: Use specific HDMI timings structure Tomi Valkeinen
2011-09-02 5:09 ` K, Mythri P
2011-09-01 8:27 ` [PATCH v2 02/10] OMAP4: DSS: HDMI: Move pll and video configuration Tomi Valkeinen
2011-09-02 5:11 ` K, Mythri P
2011-09-02 5:13 ` Tomi Valkeinen
2011-09-02 5:22 ` K, Mythri P
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1314867341.2169.18.camel@lappyti \
--to=tomi.valkeinen@ti.com \
--cc=linux-omap@vger.kernel.org \
--cc=mythripk@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox