> -----Original Message----- > From: Rahul Sharma [mailto:r.sh.open@xxxxxxxxx] > Sent: Monday, September 02, 2013 6:06 PM > To: Inki Dae > Cc: Rahul Sharma; linux-samsung-soc; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > 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.dae@xxxxxxxxxxx> wrote: > > > > > >> -----Original Message----- > >> From: Rahul Sharma [mailto:r.sh.open@xxxxxxxxx] > >> Sent: Monday, September 02, 2013 3:28 PM > >> To: Inki Dae > >> Cc: Rahul Sharma; linux-samsung-soc; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > >> 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.dae@xxxxxxxxxxx> wrote: > >> > Hi Rahul, > >> > > >> >> -----Original Message----- > >> >> From: linux-samsung-soc-owner@xxxxxxxxxxxxxxx [mailto:linux-samsung- > >> soc- > >> >> owner@xxxxxxxxxxxxxxx] On Behalf Of Rahul Sharma > >> >> Sent: Friday, August 30, 2013 7:06 PM > >> >> To: Inki Dae > >> >> Cc: Rahul Sharma; linux-samsung-soc; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > >> >> 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@xxxxxxxxxxx> 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. > >> > > >> Mr. dae, > >> > >> I have re-confirmed. It is working fine. No failure during registering > >> platform > >> device. Probe hits immediately after registration. I tried 8~9 times. > >> No failure. > >> see logs: > >> > >> # dmesg | grep -i RSH > >> [ 0.895000] [RSH][hdmi_probe][1719] Starting phy registeration > >> [ 0.900000] [RSH][hdmiphy_platform_device_probe Enter][644] > >> [ 0.905000] [RSH][hdmiphy_platform_device_probe Exit Success][683] > >> [ 0.910000] [RSH][exynos_hdmiphy_driver_register][768] Phy > >> registeration Success. > >> [ 0.915000] [RSH][hdmi_probe][1729] Phy registeration completed. > >> > > > > Great. I will also re-check it. > > Works fine. :) It seems that some patch to avoid duplicate probe was merged to mainline. For this, someone had posed it like below but this patch hasn't been merged to mainline. https://lists.ozlabs.org/pipermail/devicetree-discuss/2012-July/017011.html So it seems that mainline has other solution to avoid that issue. Is there anyone to give me more detail for this? > >> > 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. > >> > > >> > >> I am fine with it. We can register hdmiphy-i2c or hdmiphy-platform > >> driver based on the flag from hdmi driver. But we can still keep both > >> the drivers in exynos_hdmiphy.c. file and let them share most of the > >> other callbacks. > >> > > > > Is there any mainline driver that keeps two bus drivers; i2c and > platform > > device, in one file? I'm not sure that this is a good way. So it seems > good > > I haven't come across any such mainline driver. I always have this doubt. > If it is not fitting properly, I will implement 2 different drivers in 2 > files > (exynos_hdmiphy_i2c.c and exynos_hdmiphy_platform.c). > Let's do it if there is no other opinion. :) Thanks, Inki Dae > Regards, > Rahul Sharma. > > > to keep two hdmiphy drivers: One is based on i2c bus, and other is based > on > > platform device. Anyway, we should keep different type's drivers because > > Exynos SoC has different type's hdmiphy IP; based on i2c or memory > mapped > > IO. > > > > Thanks, > > Inki Dae > > > >> regards, > >> Rahul Sharma. > >> > >> > 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@xxxxxxxxxxx] > >> >> >> Sent: Friday, August 30, 2013 3:59 PM > >> >> >> To: linux-samsung-soc@xxxxxxxxxxxxxxx; dri- > >> devel@xxxxxxxxxxxxxxxxxxxxx > >> >> >> Cc: kgene.kim@xxxxxxxxxxx; sw0312.kim@xxxxxxxxxxx; > >> > inki.dae@xxxxxxxxxxx; > >> >> >> seanpaul@xxxxxxxxxxxx; l.stach@xxxxxxxxxxxxxx; > > tomasz.figa@xxxxxxxxx; > >> >> >> s.nawrocki@xxxxxxxxxxx; joshi@xxxxxxxxxxx; r.sh.open@xxxxxxxxx; > >> 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@xxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxxx > >> >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
