Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy driver
On 2 September 2013 10:38, Inki Dae inki@samsung.com wrote:
Hi Rahul,
-Original Message-
From: linux-samsung-soc-ow...@vger.kernel.org [mailto:linux-samsung-soc-
ow...@vger.kernel.org] On Behalf Of Rahul Sharma
Sent: Friday, August 30, 2013 7:06 PM
To: Inki Dae
Cc: Rahul Sharma; linux-samsung-soc; dri-devel@lists.freedesktop.org;
Kukjin Kim; sw0312.kim; Sean Paul; Lucas Stach; Tomasz Figa; Sylwester
Nawrocki; sunil joshi
Subject: Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy
driver
Thanks Mr. Dae,
I have some points for discussion.
On 30 August 2013 14:03, Inki Dae inki@samsung.com wrote:
Hi Rahul.
Thanks for your patch set.
I had just quick review to all patch series. And I think we could fully
hide
hdmiphy interfaces,
exynos_hdmiphy_enable/disable/check_mode/set_mode/conf_apply, from hdmi
driver.
That may be prototyped like below,
at exynos_hdmi.h
/* Define hdmiphy callbacks. */
struct exynos_hdmiphy_ops {
void (*enable)(struct device *dev);
void (*disable)(struct device *dev);
int (*check_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*set_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*apply)(struct device *dev);
};
Above looks fine to me. I will hide the ops as you suggested.
at exynos_hdmi.c
/*
* Add a new structure for hdmi driver data.
* type could be HDMI_TYPE13 or HDMI_TYPE14.
* i2c_hdmiphy could be true or false: true means that current hdmi
device
uses i2c
* based hdmiphy device, otherwise platform device based one.
*/
struct hdmi_drv_data {
unsigned int type;
unsigned int i2c_hdmiphy;
};
...
/* Add new members to hdmi context. */
struct hdmi_context {
...
struct hdmi_drv_data *drv_data;
struct hdmiphy_ops *ops;
...
};
/* Add hdmi device data according Exynos SoC. */
static struct hdmi_drv_data exynos4212_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = true
};
static struct hdmi_drv_data exynos5420_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = false
};
static struct of_device_id hdmi_match_types[] = {
{
.compatible = samsung,exynos4212-hdmi,
.data = (void *)exynos4212_hdmi_drv_data,
}, {
...
.compatible = samsung,exynos5420-hdmi,
.data = (void *)exynos5420_hdmi_drv_data,
}, {
}
};
/* the below example function shows how hdmiphy interfaces can be hided
from
hdmi driver. */
static void hdmi_mode_set(...)
{
...
hdata-ops-set_mode(hdata-hdmiphy_dev, mode);
This looks fine.
}
static int hdmi_get_phy_device(struct hdmi_context *hdata)
{
struct hdmi_drv_data *data = hdata-drv_data;
...
/* Register hdmiphy driver according to i2c_hdmiphy value. */
ret = exynos_hdmiphy_driver_register(data-i2c_hdmiphy);
...
/* Get hdmiphy driver ops according to i2c_hdmiphy value. */
hdata-ops = exynos_hdmiphy_get_ops(data-i2c_hdmiphy);
...
}
at exynos_hdmiphy.c
/* Define hdmiphy ops respectively. */
struct exynos_hdmiphy_ops hdmiphy_i2c_ops = {
.enable = exynos_hdmiphy_i2c_enable,
.disable = exynos_hdmiphy_i2c_disable,
...
};
struct exynos_hdmiphy_ops hdmiphy_platdev_ops = {
.enable = exynos_hdmiphy_platdev_enable,
.disable = exynos_hdmiphy_platdev_disable,
...
};
Actually, Ops for Hdmiphy I2c and platform devices are exactly
same. We don't need 2 sets. Only difference is in static function to
write configuration values to phy. Based on i2c_verify_client(hdata-dev),
we use i2c_master_send or writeb.
struct exynos_hdmiphy_ops *exynos_hdmiphy_get_ops(unsigned int
i2c_hdmiphy)
{
/* Return hdmiphy ops appropriately according to i2c_hdmiphy. */
if (i2c_hdmiphy)
return hdmiphy_i2c_ops;
return hdmiphy_platdev_ops;
}
We don't need i2c_hdmiphy flag from the hdmi driver. After probe,
this information is available in hdmiphy driver itself.
int exynos_hdmiphy_driver_register(unsigned int i2c_hdmiphy)
{
...
/* Register hdmiphy driver appropriately according to
i2c_hdmiphy.
*/
if (i2c_hdmiphy) {
ret = i2c_add_driver(hdmiphy_i2c_driver);
...
} else {
ret =
platform_driver_register(hdmiphy_platform_driver);
...
}
Here i2c_hdmiphy flag helps in avoiding registering both i2c
and platform drivers for phy. But is it a concern that we should
not register 2 drivers on different buses for single hw device. I am
still not clear
RE: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy driver
-Original Message-
From: Rahul Sharma [mailto:r.sh.o...@gmail.com]
Sent: Monday, September 02, 2013 3:28 PM
To: Inki Dae
Cc: Rahul Sharma; linux-samsung-soc; dri-devel@lists.freedesktop.org;
Kukjin Kim; sw0312.kim; Sean Paul; Lucas Stach; Tomasz Figa; Sylwester
Nawrocki; sunil joshi
Subject: Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy
driver
On 2 September 2013 10:38, Inki Dae inki@samsung.com wrote:
Hi Rahul,
-Original Message-
From: linux-samsung-soc-ow...@vger.kernel.org [mailto:linux-samsung-
soc-
ow...@vger.kernel.org] On Behalf Of Rahul Sharma
Sent: Friday, August 30, 2013 7:06 PM
To: Inki Dae
Cc: Rahul Sharma; linux-samsung-soc; dri-devel@lists.freedesktop.org;
Kukjin Kim; sw0312.kim; Sean Paul; Lucas Stach; Tomasz Figa; Sylwester
Nawrocki; sunil joshi
Subject: Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to
hdmiphy
driver
Thanks Mr. Dae,
I have some points for discussion.
On 30 August 2013 14:03, Inki Dae inki@samsung.com wrote:
Hi Rahul.
Thanks for your patch set.
I had just quick review to all patch series. And I think we could
fully
hide
hdmiphy interfaces,
exynos_hdmiphy_enable/disable/check_mode/set_mode/conf_apply, from
hdmi
driver.
That may be prototyped like below,
at exynos_hdmi.h
/* Define hdmiphy callbacks. */
struct exynos_hdmiphy_ops {
void (*enable)(struct device *dev);
void (*disable)(struct device *dev);
int (*check_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*set_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*apply)(struct device *dev);
};
Above looks fine to me. I will hide the ops as you suggested.
at exynos_hdmi.c
/*
* Add a new structure for hdmi driver data.
* type could be HDMI_TYPE13 or HDMI_TYPE14.
* i2c_hdmiphy could be true or false: true means that current hdmi
device
uses i2c
* based hdmiphy device, otherwise platform device based one.
*/
struct hdmi_drv_data {
unsigned int type;
unsigned int i2c_hdmiphy;
};
...
/* Add new members to hdmi context. */
struct hdmi_context {
...
struct hdmi_drv_data *drv_data;
struct hdmiphy_ops *ops;
...
};
/* Add hdmi device data according Exynos SoC. */
static struct hdmi_drv_data exynos4212_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = true
};
static struct hdmi_drv_data exynos5420_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = false
};
static struct of_device_id hdmi_match_types[] = {
{
.compatible = samsung,exynos4212-hdmi,
.data = (void *)exynos4212_hdmi_drv_data,
}, {
...
.compatible = samsung,exynos5420-hdmi,
.data = (void *)exynos5420_hdmi_drv_data,
}, {
}
};
/* the below example function shows how hdmiphy interfaces can be
hided
from
hdmi driver. */
static void hdmi_mode_set(...)
{
...
hdata-ops-set_mode(hdata-hdmiphy_dev, mode);
This looks fine.
}
static int hdmi_get_phy_device(struct hdmi_context *hdata)
{
struct hdmi_drv_data *data = hdata-drv_data;
...
/* Register hdmiphy driver according to i2c_hdmiphy value. */
ret = exynos_hdmiphy_driver_register(data-i2c_hdmiphy);
...
/* Get hdmiphy driver ops according to i2c_hdmiphy value. */
hdata-ops = exynos_hdmiphy_get_ops(data-i2c_hdmiphy);
...
}
at exynos_hdmiphy.c
/* Define hdmiphy ops respectively. */
struct exynos_hdmiphy_ops hdmiphy_i2c_ops = {
.enable = exynos_hdmiphy_i2c_enable,
.disable = exynos_hdmiphy_i2c_disable,
...
};
struct exynos_hdmiphy_ops hdmiphy_platdev_ops = {
.enable = exynos_hdmiphy_platdev_enable,
.disable = exynos_hdmiphy_platdev_disable,
...
};
Actually, Ops for Hdmiphy I2c and platform devices are exactly
same. We don't need 2 sets. Only difference is in static function to
write configuration values to phy. Based on i2c_verify_client(hdata-
dev),
we use i2c_master_send or writeb.
struct exynos_hdmiphy_ops *exynos_hdmiphy_get_ops(unsigned int
i2c_hdmiphy)
{
/* Return hdmiphy ops appropriately according to i2c_hdmiphy.
*/
if (i2c_hdmiphy)
return hdmiphy_i2c_ops;
return hdmiphy_platdev_ops;
}
We don't need i2c_hdmiphy flag from the hdmi driver. After probe,
this information is available in hdmiphy driver itself.
int exynos_hdmiphy_driver_register(unsigned int i2c_hdmiphy
Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy driver
On 2 September 2013 12:52, Inki Dae inki@samsung.com wrote:
-Original Message-
From: Rahul Sharma [mailto:r.sh.o...@gmail.com]
Sent: Monday, September 02, 2013 3:28 PM
To: Inki Dae
Cc: Rahul Sharma; linux-samsung-soc; dri-devel@lists.freedesktop.org;
Kukjin Kim; sw0312.kim; Sean Paul; Lucas Stach; Tomasz Figa; Sylwester
Nawrocki; sunil joshi
Subject: Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy
driver
On 2 September 2013 10:38, Inki Dae inki@samsung.com wrote:
Hi Rahul,
-Original Message-
From: linux-samsung-soc-ow...@vger.kernel.org [mailto:linux-samsung-
soc-
ow...@vger.kernel.org] On Behalf Of Rahul Sharma
Sent: Friday, August 30, 2013 7:06 PM
To: Inki Dae
Cc: Rahul Sharma; linux-samsung-soc; dri-devel@lists.freedesktop.org;
Kukjin Kim; sw0312.kim; Sean Paul; Lucas Stach; Tomasz Figa; Sylwester
Nawrocki; sunil joshi
Subject: Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to
hdmiphy
driver
Thanks Mr. Dae,
I have some points for discussion.
On 30 August 2013 14:03, Inki Dae inki@samsung.com wrote:
Hi Rahul.
Thanks for your patch set.
I had just quick review to all patch series. And I think we could
fully
hide
hdmiphy interfaces,
exynos_hdmiphy_enable/disable/check_mode/set_mode/conf_apply, from
hdmi
driver.
That may be prototyped like below,
at exynos_hdmi.h
/* Define hdmiphy callbacks. */
struct exynos_hdmiphy_ops {
void (*enable)(struct device *dev);
void (*disable)(struct device *dev);
int (*check_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*set_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*apply)(struct device *dev);
};
Above looks fine to me. I will hide the ops as you suggested.
at exynos_hdmi.c
/*
* Add a new structure for hdmi driver data.
* type could be HDMI_TYPE13 or HDMI_TYPE14.
* i2c_hdmiphy could be true or false: true means that current hdmi
device
uses i2c
* based hdmiphy device, otherwise platform device based one.
*/
struct hdmi_drv_data {
unsigned int type;
unsigned int i2c_hdmiphy;
};
...
/* Add new members to hdmi context. */
struct hdmi_context {
...
struct hdmi_drv_data *drv_data;
struct hdmiphy_ops *ops;
...
};
/* Add hdmi device data according Exynos SoC. */
static struct hdmi_drv_data exynos4212_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = true
};
static struct hdmi_drv_data exynos5420_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = false
};
static struct of_device_id hdmi_match_types[] = {
{
.compatible = samsung,exynos4212-hdmi,
.data = (void *)exynos4212_hdmi_drv_data,
}, {
...
.compatible = samsung,exynos5420-hdmi,
.data = (void *)exynos5420_hdmi_drv_data,
}, {
}
};
/* the below example function shows how hdmiphy interfaces can be
hided
from
hdmi driver. */
static void hdmi_mode_set(...)
{
...
hdata-ops-set_mode(hdata-hdmiphy_dev, mode);
This looks fine.
}
static int hdmi_get_phy_device(struct hdmi_context *hdata)
{
struct hdmi_drv_data *data = hdata-drv_data;
...
/* Register hdmiphy driver according to i2c_hdmiphy value. */
ret = exynos_hdmiphy_driver_register(data-i2c_hdmiphy);
...
/* Get hdmiphy driver ops according to i2c_hdmiphy value. */
hdata-ops = exynos_hdmiphy_get_ops(data-i2c_hdmiphy);
...
}
at exynos_hdmiphy.c
/* Define hdmiphy ops respectively. */
struct exynos_hdmiphy_ops hdmiphy_i2c_ops = {
.enable = exynos_hdmiphy_i2c_enable,
.disable = exynos_hdmiphy_i2c_disable,
...
};
struct exynos_hdmiphy_ops hdmiphy_platdev_ops = {
.enable = exynos_hdmiphy_platdev_enable,
.disable = exynos_hdmiphy_platdev_disable,
...
};
Actually, Ops for Hdmiphy I2c and platform devices are exactly
same. We don't need 2 sets. Only difference is in static function to
write configuration values to phy. Based on i2c_verify_client(hdata-
dev),
we use i2c_master_send or writeb.
struct exynos_hdmiphy_ops *exynos_hdmiphy_get_ops(unsigned int
i2c_hdmiphy)
{
/* Return hdmiphy ops appropriately according to i2c_hdmiphy.
*/
if (i2c_hdmiphy)
return hdmiphy_i2c_ops;
return hdmiphy_platdev_ops;
}
We don't need i2c_hdmiphy flag from the hdmi driver. After probe,
this information is available in hdmiphy driver itself.
int
RE: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy driver
-Original Message-
From: Rahul Sharma [mailto:r.sh.o...@gmail.com]
Sent: Monday, September 02, 2013 6:06 PM
To: Inki Dae
Cc: Rahul Sharma; linux-samsung-soc; dri-devel@lists.freedesktop.org;
Kukjin Kim; sw0312.kim; Sean Paul; Lucas Stach; Tomasz Figa; Sylwester
Nawrocki; sunil joshi
Subject: Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy
driver
On 2 September 2013 12:52, Inki Dae inki@samsung.com wrote:
-Original Message-
From: Rahul Sharma [mailto:r.sh.o...@gmail.com]
Sent: Monday, September 02, 2013 3:28 PM
To: Inki Dae
Cc: Rahul Sharma; linux-samsung-soc; dri-devel@lists.freedesktop.org;
Kukjin Kim; sw0312.kim; Sean Paul; Lucas Stach; Tomasz Figa; Sylwester
Nawrocki; sunil joshi
Subject: Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to
hdmiphy
driver
On 2 September 2013 10:38, Inki Dae inki@samsung.com wrote:
Hi Rahul,
-Original Message-
From: linux-samsung-soc-ow...@vger.kernel.org [mailto:linux-samsung-
soc-
ow...@vger.kernel.org] On Behalf Of Rahul Sharma
Sent: Friday, August 30, 2013 7:06 PM
To: Inki Dae
Cc: Rahul Sharma; linux-samsung-soc;
dri-devel@lists.freedesktop.org;
Kukjin Kim; sw0312.kim; Sean Paul; Lucas Stach; Tomasz Figa;
Sylwester
Nawrocki; sunil joshi
Subject: Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to
hdmiphy
driver
Thanks Mr. Dae,
I have some points for discussion.
On 30 August 2013 14:03, Inki Dae inki@samsung.com wrote:
Hi Rahul.
Thanks for your patch set.
I had just quick review to all patch series. And I think we could
fully
hide
hdmiphy interfaces,
exynos_hdmiphy_enable/disable/check_mode/set_mode/conf_apply, from
hdmi
driver.
That may be prototyped like below,
at exynos_hdmi.h
/* Define hdmiphy callbacks. */
struct exynos_hdmiphy_ops {
void (*enable)(struct device *dev);
void (*disable)(struct device *dev);
int (*check_mode)(struct device *dev, struct
drm_display_mode
*mode);
int (*set_mode)(struct device *dev, struct
drm_display_mode
*mode);
int (*apply)(struct device *dev);
};
Above looks fine to me. I will hide the ops as you suggested.
at exynos_hdmi.c
/*
* Add a new structure for hdmi driver data.
* type could be HDMI_TYPE13 or HDMI_TYPE14.
* i2c_hdmiphy could be true or false: true means that current
hdmi
device
uses i2c
* based hdmiphy device, otherwise platform device based one.
*/
struct hdmi_drv_data {
unsigned int type;
unsigned int i2c_hdmiphy;
};
...
/* Add new members to hdmi context. */
struct hdmi_context {
...
struct hdmi_drv_data *drv_data;
struct hdmiphy_ops *ops;
...
};
/* Add hdmi device data according Exynos SoC. */
static struct hdmi_drv_data exynos4212_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = true
};
static struct hdmi_drv_data exynos5420_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = false
};
static struct of_device_id hdmi_match_types[] = {
{
.compatible = samsung,exynos4212-hdmi,
.data = (void
*)exynos4212_hdmi_drv_data,
}, {
...
.compatible = samsung,exynos5420-hdmi,
.data = (void
*)exynos5420_hdmi_drv_data,
}, {
}
};
/* the below example function shows how hdmiphy interfaces can be
hided
from
hdmi driver. */
static void hdmi_mode_set(...)
{
...
hdata-ops-set_mode(hdata-hdmiphy_dev, mode);
This looks fine.
}
static int hdmi_get_phy_device(struct hdmi_context *hdata)
{
struct hdmi_drv_data *data = hdata-drv_data;
...
/* Register hdmiphy driver according to i2c_hdmiphy value.
*/
ret = exynos_hdmiphy_driver_register(data-i2c_hdmiphy);
...
/* Get hdmiphy driver ops according to i2c_hdmiphy value.
*/
hdata-ops = exynos_hdmiphy_get_ops(data-i2c_hdmiphy);
...
}
at exynos_hdmiphy.c
/* Define hdmiphy ops respectively. */
struct exynos_hdmiphy_ops hdmiphy_i2c_ops = {
.enable = exynos_hdmiphy_i2c_enable,
.disable = exynos_hdmiphy_i2c_disable,
...
};
struct exynos_hdmiphy_ops hdmiphy_platdev_ops = {
.enable = exynos_hdmiphy_platdev_enable,
.disable = exynos_hdmiphy_platdev_disable,
...
};
Actually, Ops for Hdmiphy I2c and platform devices are exactly
same. We don't need 2 sets. Only difference is in static function
RE: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy driver
Hi Rahul,
-Original Message-
From: linux-samsung-soc-ow...@vger.kernel.org [mailto:linux-samsung-soc-
ow...@vger.kernel.org] On Behalf Of Rahul Sharma
Sent: Friday, August 30, 2013 7:06 PM
To: Inki Dae
Cc: Rahul Sharma; linux-samsung-soc; dri-devel@lists.freedesktop.org;
Kukjin Kim; sw0312.kim; Sean Paul; Lucas Stach; Tomasz Figa; Sylwester
Nawrocki; sunil joshi
Subject: Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy
driver
Thanks Mr. Dae,
I have some points for discussion.
On 30 August 2013 14:03, Inki Dae inki@samsung.com wrote:
Hi Rahul.
Thanks for your patch set.
I had just quick review to all patch series. And I think we could fully
hide
hdmiphy interfaces,
exynos_hdmiphy_enable/disable/check_mode/set_mode/conf_apply, from hdmi
driver.
That may be prototyped like below,
at exynos_hdmi.h
/* Define hdmiphy callbacks. */
struct exynos_hdmiphy_ops {
void (*enable)(struct device *dev);
void (*disable)(struct device *dev);
int (*check_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*set_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*apply)(struct device *dev);
};
Above looks fine to me. I will hide the ops as you suggested.
at exynos_hdmi.c
/*
* Add a new structure for hdmi driver data.
* type could be HDMI_TYPE13 or HDMI_TYPE14.
* i2c_hdmiphy could be true or false: true means that current hdmi
device
uses i2c
* based hdmiphy device, otherwise platform device based one.
*/
struct hdmi_drv_data {
unsigned int type;
unsigned int i2c_hdmiphy;
};
...
/* Add new members to hdmi context. */
struct hdmi_context {
...
struct hdmi_drv_data *drv_data;
struct hdmiphy_ops *ops;
...
};
/* Add hdmi device data according Exynos SoC. */
static struct hdmi_drv_data exynos4212_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = true
};
static struct hdmi_drv_data exynos5420_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = false
};
static struct of_device_id hdmi_match_types[] = {
{
.compatible = samsung,exynos4212-hdmi,
.data = (void *)exynos4212_hdmi_drv_data,
}, {
...
.compatible = samsung,exynos5420-hdmi,
.data = (void *)exynos5420_hdmi_drv_data,
}, {
}
};
/* the below example function shows how hdmiphy interfaces can be hided
from
hdmi driver. */
static void hdmi_mode_set(...)
{
...
hdata-ops-set_mode(hdata-hdmiphy_dev, mode);
This looks fine.
}
static int hdmi_get_phy_device(struct hdmi_context *hdata)
{
struct hdmi_drv_data *data = hdata-drv_data;
...
/* Register hdmiphy driver according to i2c_hdmiphy value. */
ret = exynos_hdmiphy_driver_register(data-i2c_hdmiphy);
...
/* Get hdmiphy driver ops according to i2c_hdmiphy value. */
hdata-ops = exynos_hdmiphy_get_ops(data-i2c_hdmiphy);
...
}
at exynos_hdmiphy.c
/* Define hdmiphy ops respectively. */
struct exynos_hdmiphy_ops hdmiphy_i2c_ops = {
.enable = exynos_hdmiphy_i2c_enable,
.disable = exynos_hdmiphy_i2c_disable,
...
};
struct exynos_hdmiphy_ops hdmiphy_platdev_ops = {
.enable = exynos_hdmiphy_platdev_enable,
.disable = exynos_hdmiphy_platdev_disable,
...
};
Actually, Ops for Hdmiphy I2c and platform devices are exactly
same. We don't need 2 sets. Only difference is in static function to
write configuration values to phy. Based on i2c_verify_client(hdata-dev),
we use i2c_master_send or writeb.
struct exynos_hdmiphy_ops *exynos_hdmiphy_get_ops(unsigned int
i2c_hdmiphy)
{
/* Return hdmiphy ops appropriately according to i2c_hdmiphy. */
if (i2c_hdmiphy)
return hdmiphy_i2c_ops;
return hdmiphy_platdev_ops;
}
We don't need i2c_hdmiphy flag from the hdmi driver. After probe,
this information is available in hdmiphy driver itself.
int exynos_hdmiphy_driver_register(unsigned int i2c_hdmiphy)
{
...
/* Register hdmiphy driver appropriately according to
i2c_hdmiphy.
*/
if (i2c_hdmiphy) {
ret = i2c_add_driver(hdmiphy_i2c_driver);
...
} else {
ret =
platform_driver_register(hdmiphy_platform_driver);
...
}
Here i2c_hdmiphy flag helps in avoiding registering both i2c
and platform drivers for phy. But is it a concern that we should
not register 2 drivers on different buses for single hw device. I am
still not clear on this.
Otherwise we do not need to maintain i2c_hdmiphy flag
RE: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy driver
Hi Rahul.
Thanks for your patch set.
I had just quick review to all patch series. And I think we could fully hide
hdmiphy interfaces,
exynos_hdmiphy_enable/disable/check_mode/set_mode/conf_apply, from hdmi
driver.
That may be prototyped like below,
at exynos_hdmi.h
/* Define hdmiphy callbacks. */
struct exynos_hdmiphy_ops {
void (*enable)(struct device *dev);
void (*disable)(struct device *dev);
int (*check_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*set_mode)(struct device *dev, struct drm_display_mode *mode);
int (*apply)(struct device *dev);
};
at exynos_hdmi.c
/*
* Add a new structure for hdmi driver data.
* type could be HDMI_TYPE13 or HDMI_TYPE14.
* i2c_hdmiphy could be true or false: true means that current hdmi device
uses i2c
* based hdmiphy device, otherwise platform device based one.
*/
struct hdmi_drv_data {
unsigned int type;
unsigned int i2c_hdmiphy;
};
...
/* Add new members to hdmi context. */
struct hdmi_context {
...
struct hdmi_drv_data *drv_data;
struct hdmiphy_ops *ops;
...
};
/* Add hdmi device data according Exynos SoC. */
static struct hdmi_drv_data exynos4212_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = true
};
static struct hdmi_drv_data exynos5420_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = false
};
static struct of_device_id hdmi_match_types[] = {
{
.compatible = samsung,exynos4212-hdmi,
.data = (void *)exynos4212_hdmi_drv_data,
}, {
...
.compatible = samsung,exynos5420-hdmi,
.data = (void *)exynos5420_hdmi_drv_data,
}, {
}
};
/* the below example function shows how hdmiphy interfaces can be hided from
hdmi driver. */
static void hdmi_mode_set(...)
{
...
hdata-ops-set_mode(hdata-hdmiphy_dev, mode);
}
static int hdmi_get_phy_device(struct hdmi_context *hdata)
{
struct hdmi_drv_data *data = hdata-drv_data;
...
/* Register hdmiphy driver according to i2c_hdmiphy value. */
ret = exynos_hdmiphy_driver_register(data-i2c_hdmiphy);
...
/* Get hdmiphy driver ops according to i2c_hdmiphy value. */
hdata-ops = exynos_hdmiphy_get_ops(data-i2c_hdmiphy);
...
}
at exynos_hdmiphy.c
/* Define hdmiphy ops respectively. */
struct exynos_hdmiphy_ops hdmiphy_i2c_ops = {
.enable = exynos_hdmiphy_i2c_enable,
.disable = exynos_hdmiphy_i2c_disable,
...
};
struct exynos_hdmiphy_ops hdmiphy_platdev_ops = {
.enable = exynos_hdmiphy_platdev_enable,
.disable = exynos_hdmiphy_platdev_disable,
...
};
struct exynos_hdmiphy_ops *exynos_hdmiphy_get_ops(unsigned int i2c_hdmiphy)
{
/* Return hdmiphy ops appropriately according to i2c_hdmiphy. */
if (i2c_hdmiphy)
return hdmiphy_i2c_ops;
return hdmiphy_platdev_ops;
}
int exynos_hdmiphy_driver_register(unsigned int i2c_hdmiphy)
{
...
/* Register hdmiphy driver appropriately according to i2c_hdmiphy.
*/
if (i2c_hdmiphy) {
ret = i2c_add_driver(hdmiphy_i2c_driver);
...
} else {
ret = platform_driver_register(hdmiphy_platform_driver);
...
}
return ret;
}
Thanks,
Inki Dae
-Original Message-
From: Rahul Sharma [mailto:rahul.sha...@samsung.com]
Sent: Friday, August 30, 2013 3:59 PM
To: linux-samsung-...@vger.kernel.org; dri-devel@lists.freedesktop.org
Cc: kgene@samsung.com; sw0312@samsung.com; inki@samsung.com;
seanp...@chromium.org; l.st...@pengutronix.de; tomasz.f...@gmail.com;
s.nawro...@samsung.com; jo...@samsung.com; r.sh.o...@gmail.com; Rahul
Sharma
Subject: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy
driver
Currently, exynos hdmiphy operations and configs are kept
inside the hdmi driver. Hdmiphy related code is tightly
coupled with hdmi IP driver.
This series also removes hdmiphy dummy clock for hdmiphy
and replace it with Phy PMU Control from the hdmiphy driver.
At the end, support for exynos5420 hdmiphy is added to the
hdmiphy driver which is a platform device.
Drm related paches are based on exynos-drm-next branch at
git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
Arch patches are dependent on
http://www.mail-archive.com/linux-samsung-
s...@vger.kernel.org/msg22195.html
Rahul Sharma (7):
drm/exynos: move hdmiphy related code to hdmiphy driver
drm/exynos: remove dummy hdmiphy clock
drm/exynos: add hdmiphy pmu bit control in hdmiphy driver
drm/exynos: add support for exynos5420 hdmiphy
exynos/drm: fix ddc i2c device probe failure
ARM: dts: update hdmiphy dt node for exynos5250
ARM: dts: update hdmiphy dt node for exynos5420
RE: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy driver
One more thing, you would need to check if other driver can be probed in
probe context. With your patch, exynos_hdmiphy_driver_register() is called
in hdmi_probe() via hdmi_get_phy_device(), and then
platform_driver_reigster() is called via the
exynos_hdmiphy_driver_register(). I remember that was failed.
Thanks,
Inki Dae
-Original Message-
From: dri-devel-bounces+inki.dae=samsung@lists.freedesktop.org
[mailto:dri-devel-bounces+inki.dae=samsung@lists.freedesktop.org] On
Behalf Of Inki Dae
Sent: Friday, August 30, 2013 5:33 PM
To: 'Rahul Sharma'; linux-samsung-...@vger.kernel.org; dri-
de...@lists.freedesktop.org
Cc: kgene@samsung.com; sw0312@samsung.com; jo...@samsung.com;
s.nawro...@samsung.com
Subject: RE: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy
driver
Hi Rahul.
Thanks for your patch set.
I had just quick review to all patch series. And I think we could fully
hide
hdmiphy interfaces,
exynos_hdmiphy_enable/disable/check_mode/set_mode/conf_apply, from hdmi
driver.
That may be prototyped like below,
at exynos_hdmi.h
/* Define hdmiphy callbacks. */
struct exynos_hdmiphy_ops {
void (*enable)(struct device *dev);
void (*disable)(struct device *dev);
int (*check_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*set_mode)(struct device *dev, struct drm_display_mode *mode);
int (*apply)(struct device *dev);
};
at exynos_hdmi.c
/*
* Add a new structure for hdmi driver data.
* type could be HDMI_TYPE13 or HDMI_TYPE14.
* i2c_hdmiphy could be true or false: true means that current hdmi
device
uses i2c
* based hdmiphy device, otherwise platform device based one.
*/
struct hdmi_drv_data {
unsigned int type;
unsigned int i2c_hdmiphy;
};
...
/* Add new members to hdmi context. */
struct hdmi_context {
...
struct hdmi_drv_data *drv_data;
struct hdmiphy_ops *ops;
...
};
/* Add hdmi device data according Exynos SoC. */
static struct hdmi_drv_data exynos4212_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = true
};
static struct hdmi_drv_data exynos5420_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = false
};
static struct of_device_id hdmi_match_types[] = {
{
.compatible = samsung,exynos4212-hdmi,
.data = (void *)exynos4212_hdmi_drv_data,
}, {
...
.compatible = samsung,exynos5420-hdmi,
.data = (void *)exynos5420_hdmi_drv_data,
}, {
}
};
/* the below example function shows how hdmiphy interfaces can be hided
from
hdmi driver. */
static void hdmi_mode_set(...)
{
...
hdata-ops-set_mode(hdata-hdmiphy_dev, mode);
}
static int hdmi_get_phy_device(struct hdmi_context *hdata)
{
struct hdmi_drv_data *data = hdata-drv_data;
...
/* Register hdmiphy driver according to i2c_hdmiphy value. */
ret = exynos_hdmiphy_driver_register(data-i2c_hdmiphy);
...
/* Get hdmiphy driver ops according to i2c_hdmiphy value. */
hdata-ops = exynos_hdmiphy_get_ops(data-i2c_hdmiphy);
...
}
at exynos_hdmiphy.c
/* Define hdmiphy ops respectively. */
struct exynos_hdmiphy_ops hdmiphy_i2c_ops = {
.enable = exynos_hdmiphy_i2c_enable,
.disable = exynos_hdmiphy_i2c_disable,
...
};
struct exynos_hdmiphy_ops hdmiphy_platdev_ops = {
.enable = exynos_hdmiphy_platdev_enable,
.disable = exynos_hdmiphy_platdev_disable,
...
};
struct exynos_hdmiphy_ops *exynos_hdmiphy_get_ops(unsigned int
i2c_hdmiphy)
{
/* Return hdmiphy ops appropriately according to i2c_hdmiphy. */
if (i2c_hdmiphy)
return hdmiphy_i2c_ops;
return hdmiphy_platdev_ops;
}
int exynos_hdmiphy_driver_register(unsigned int i2c_hdmiphy)
{
...
/* Register hdmiphy driver appropriately according to i2c_hdmiphy.
*/
if (i2c_hdmiphy) {
ret = i2c_add_driver(hdmiphy_i2c_driver);
...
} else {
ret = platform_driver_register(hdmiphy_platform_driver);
...
}
return ret;
}
Thanks,
Inki Dae
-Original Message-
From: Rahul Sharma [mailto:rahul.sha...@samsung.com]
Sent: Friday, August 30, 2013 3:59 PM
To: linux-samsung-...@vger.kernel.org; dri-devel@lists.freedesktop.org
Cc: kgene@samsung.com; sw0312@samsung.com; inki@samsung.com;
seanp...@chromium.org; l.st...@pengutronix.de; tomasz.f...@gmail.com;
s.nawro...@samsung.com; jo...@samsung.com; r.sh.o...@gmail.com; Rahul
Sharma
Subject: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy
driver
Currently, exynos hdmiphy operations and configs are kept
inside the hdmi driver. Hdmiphy related code is tightly
coupled with hdmi IP driver.
This series also
Re: [PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy driver
Thanks Mr. Dae,
I have some points for discussion.
On 30 August 2013 14:03, Inki Dae inki@samsung.com wrote:
Hi Rahul.
Thanks for your patch set.
I had just quick review to all patch series. And I think we could fully hide
hdmiphy interfaces,
exynos_hdmiphy_enable/disable/check_mode/set_mode/conf_apply, from hdmi
driver.
That may be prototyped like below,
at exynos_hdmi.h
/* Define hdmiphy callbacks. */
struct exynos_hdmiphy_ops {
void (*enable)(struct device *dev);
void (*disable)(struct device *dev);
int (*check_mode)(struct device *dev, struct drm_display_mode
*mode);
int (*set_mode)(struct device *dev, struct drm_display_mode *mode);
int (*apply)(struct device *dev);
};
Above looks fine to me. I will hide the ops as you suggested.
at exynos_hdmi.c
/*
* Add a new structure for hdmi driver data.
* type could be HDMI_TYPE13 or HDMI_TYPE14.
* i2c_hdmiphy could be true or false: true means that current hdmi device
uses i2c
* based hdmiphy device, otherwise platform device based one.
*/
struct hdmi_drv_data {
unsigned int type;
unsigned int i2c_hdmiphy;
};
...
/* Add new members to hdmi context. */
struct hdmi_context {
...
struct hdmi_drv_data *drv_data;
struct hdmiphy_ops *ops;
...
};
/* Add hdmi device data according Exynos SoC. */
static struct hdmi_drv_data exynos4212_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = true
};
static struct hdmi_drv_data exynos5420_hdmi_drv_data = {
.type = HDMI_TYPE14,
.i2c_hdmiphy = false
};
static struct of_device_id hdmi_match_types[] = {
{
.compatible = samsung,exynos4212-hdmi,
.data = (void *)exynos4212_hdmi_drv_data,
}, {
...
.compatible = samsung,exynos5420-hdmi,
.data = (void *)exynos5420_hdmi_drv_data,
}, {
}
};
/* the below example function shows how hdmiphy interfaces can be hided from
hdmi driver. */
static void hdmi_mode_set(...)
{
...
hdata-ops-set_mode(hdata-hdmiphy_dev, mode);
This looks fine.
}
static int hdmi_get_phy_device(struct hdmi_context *hdata)
{
struct hdmi_drv_data *data = hdata-drv_data;
...
/* Register hdmiphy driver according to i2c_hdmiphy value. */
ret = exynos_hdmiphy_driver_register(data-i2c_hdmiphy);
...
/* Get hdmiphy driver ops according to i2c_hdmiphy value. */
hdata-ops = exynos_hdmiphy_get_ops(data-i2c_hdmiphy);
...
}
at exynos_hdmiphy.c
/* Define hdmiphy ops respectively. */
struct exynos_hdmiphy_ops hdmiphy_i2c_ops = {
.enable = exynos_hdmiphy_i2c_enable,
.disable = exynos_hdmiphy_i2c_disable,
...
};
struct exynos_hdmiphy_ops hdmiphy_platdev_ops = {
.enable = exynos_hdmiphy_platdev_enable,
.disable = exynos_hdmiphy_platdev_disable,
...
};
Actually, Ops for Hdmiphy I2c and platform devices are exactly
same. We don't need 2 sets. Only difference is in static function to
write configuration values to phy. Based on i2c_verify_client(hdata-dev),
we use i2c_master_send or writeb.
struct exynos_hdmiphy_ops *exynos_hdmiphy_get_ops(unsigned int i2c_hdmiphy)
{
/* Return hdmiphy ops appropriately according to i2c_hdmiphy. */
if (i2c_hdmiphy)
return hdmiphy_i2c_ops;
return hdmiphy_platdev_ops;
}
We don't need i2c_hdmiphy flag from the hdmi driver. After probe,
this information is available in hdmiphy driver itself.
int exynos_hdmiphy_driver_register(unsigned int i2c_hdmiphy)
{
...
/* Register hdmiphy driver appropriately according to i2c_hdmiphy.
*/
if (i2c_hdmiphy) {
ret = i2c_add_driver(hdmiphy_i2c_driver);
...
} else {
ret = platform_driver_register(hdmiphy_platform_driver);
...
}
Here i2c_hdmiphy flag helps in avoiding registering both i2c
and platform drivers for phy. But is it a concern that we should
not register 2 drivers on different buses for single hw device. I am
still not clear on this.
Otherwise we do not need to maintain i2c_hdmiphy flag.
Secondly, we always have registration of i2c driver for ddc and
hdmiphy driver in hdmi probe. It works. I have also tested above
series for 5420 and 5250 smdk board. There are no issues.
regards,
Rahul Sharma.
return ret;
}
Thanks,
Inki Dae
-Original Message-
From: Rahul Sharma [mailto:rahul.sha...@samsung.com]
Sent: Friday, August 30, 2013 3:59 PM
To: linux-samsung-...@vger.kernel.org; dri-devel@lists.freedesktop.org
Cc: kgene@samsung.com; sw0312@samsung.com; inki@samsung.com;
seanp...@chromium.org; l.st...@pengutronix.de; tomasz.f...@gmail.com;
s.nawro...@samsung.com;
