[PATCH 0/7] drm/exynos: move hdmiphy related code to hdmiphy driver
Inki Dae
inki.dae at samsung.com
Sun Sep 1 22:08:18 PDT 2013
Hi Rahul,
> -----Original Message-----
> From: linux-samsung-soc-owner at vger.kernel.org [mailto:linux-samsung-soc-
> owner at 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 at 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.dae at 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.
>
Can you re-check it? I guess platform_driver_register function would be
failed. Actually, I had faced with same issue at hdmi initial work. And then
only thing we have to discuss is different buses issue on single hw device
if the above case really works fine.
For this, my opinion is to separate the hdmiphy driver into i2c based and
platform device based drivers; they include same common header file
containing phy configuration data. And it makes hdmi driver call
exynos_hdmiphy_driver_register function in i2c based hdmiphy or platform
device based hdmiphy drivers according to hdmi driver data.
However, we may need to re-design it if the above case is failed.
Thanks,
Inki Dae
> regards,
> Rahul Sharma.
>
> > return ret;
> > }
> >
> > Thanks,
> > Inki Dae
> >
> >> -----Original Message-----
> >> From: Rahul Sharma [mailto:rahul.sharma at samsung.com]
> >> Sent: Friday, August 30, 2013 3:59 PM
> >> To: linux-samsung-soc at vger.kernel.org; dri-devel at lists.freedesktop.org
> >> Cc: kgene.kim at samsung.com; sw0312.kim at samsung.com;
inki.dae at samsung.com;
> >> seanpaul at chromium.org; l.stach at pengutronix.de; tomasz.figa at gmail.com;
> >> s.nawrocki at samsung.com; joshi at samsung.com; r.sh.open at 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-
> >> soc at 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
> >>
> >> .../devicetree/bindings/video/exynos_hdmi.txt | 2 +
> >> .../devicetree/bindings/video/exynos_hdmiphy.txt | 6 +
> >> arch/arm/boot/dts/exynos5250-smdk5250.dts | 9 +-
> >> arch/arm/boot/dts/exynos5420.dtsi | 12 +
> >> drivers/gpu/drm/exynos/exynos_ddc.c | 5 +
> >> drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 13 +
> >> drivers/gpu/drm/exynos/exynos_hdmi.c | 353 ++--------
> >> drivers/gpu/drm/exynos/exynos_hdmiphy.c | 738
> >> +++++++++++++++++++-
> >> drivers/gpu/drm/exynos/regs-hdmiphy.h | 35 +
> >> 9 files changed, 868 insertions(+), 305 deletions(-)
> >> create mode 100644 drivers/gpu/drm/exynos/regs-hdmiphy.h
> >>
> >> --
> >> 1.7.10.4
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-
> soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the dri-devel
mailing list