Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1216602 > unrolled thread
| Started by | Yakir Yang <ykk@rock-chips.com> |
|---|---|
| First post | 2015-09-01 07:50 +0200 |
| Last post | 2015-09-07 04:30 +0200 |
| Articles | 11 on this page of 71 — 8 participants |
Back to article view | Back to linux.kernel
[PATCH v4 0/16] Add Analogix Core Display Port Driver Yakir Yang <ykk@rock-chips.com> - 2015-09-01 07:50 +0200
[PATCH v4 01/16] drm: exynos/dp: fix code style Yakir Yang <ykk@rock-chips.com> - 2015-09-01 07:50 +0200
Re: [PATCH v4 01/16] drm: exynos/dp: fix code style Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-03 02:30 +0200
Re: [PATCH v4 01/16] drm: exynos/dp: fix code style Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-03 07:10 +0200
Re: [PATCH v4 01/16] drm: exynos/dp: fix code style Yakir Yang <ykk@rock-chips.com> - 2015-09-03 07:40 +0200
Re: [PATCH v4 01/16] drm: exynos/dp: fix code style Joe Perches <joe@perches.com> - 2015-09-03 08:00 +0200
Re: [PATCH v4 01/16] drm: exynos/dp: fix code style Yakir Yang <ykk@rock-chips.com> - 2015-09-06 03:40 +0200
Re: [PATCH v4 01/16] drm: exynos/dp: fix code style Yakir Yang <ykk@rock-chips.com> - 2015-09-03 07:10 +0200
[PATCH v4 04/16] drm: bridge: analogix/dp: remove duplicate configuration of link rate and link count Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:00 +0200
[PATCH v4 05/16] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & colorimetry Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:00 +0200
Re: [PATCH v4 05/16] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & colorimetry Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-03 10:10 +0200
Re: [PATCH v4 05/16] drm: bridge: analogix/dp: dynamic parse sync_pol & interlace & colorimetry Yakir Yang <ykk@rock-chips.com> - 2015-09-06 04:10 +0200
[PATCH v4 11/16] drm: bridge: analogix/dp: add platform device type support Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:10 +0200
Re: [PATCH v4 11/16] drm: bridge: analogix/dp: add platform device type support Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-04 02:40 +0200
Re: [PATCH v4 11/16] drm: bridge: analogix/dp: add platform device type support Yakir Yang <ykk@rock-chips.com> - 2015-09-06 06:10 +0200
Re: [PATCH v4 11/16] drm: bridge: analogix/dp: add platform device type support Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-07 02:00 +0200
Re: [PATCH v4 11/16] drm: bridge: analogix/dp: add platform device type support Yakir Yang <ykk@rock-chips.com> - 2015-09-07 03:50 +0200
[PATCH v4 08/16] drm: rockchip/dp: add rockchip platform dp driver Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:10 +0200
Re: [PATCH v4 08/16] drm: rockchip/dp: add rockchip platform dp driver Heiko Stuebner <heiko@sntech.de> - 2015-09-01 16:30 +0200
Re: [PATCH v4 08/16] drm: rockchip/dp: add rockchip platform dp driver Yakir Yang <ykk@rock-chips.com> - 2015-09-01 16:50 +0200
Re: [PATCH v4 08/16] drm: rockchip/dp: add rockchip platform dp driver Heiko Stuebner <heiko@sntech.de> - 2015-09-01 23:10 +0200
Re: [PATCH v4 08/16] drm: rockchip/dp: add rockchip platform dp driver Yakir Yang <ykk@rock-chips.com> - 2015-09-02 04:00 +0200
[PATCH v4 06/16] Documentation: drm/bridge: add document for analogix_dp Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:10 +0200
[PATCH v4 09/16] drm: rockchip: add bpc and color mode setting Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:10 +0200
Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting Heiko Stuebner <heiko@sntech.de> - 2015-09-01 23:10 +0200
Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting Yakir Yang <ykk@rock-chips.com> - 2015-09-02 04:10 +0200
Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting Thierry Reding <treding@nvidia.com> - 2015-09-02 10:40 +0200
Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting Yakir Yang <ykk@rock-chips.com> - 2015-09-02 12:10 +0200
Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting Thierry Reding <treding@nvidia.com> - 2015-09-03 10:40 +0200
Re: [PATCH v4 09/16] drm: rockchip: add bpc and color mode setting Yakir Yang <ykk@rock-chips.com> - 2015-09-06 04:10 +0200
[PATCH v4 10/16] phy: Add driver for rockchip Display Port PHY Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:10 +0200
Re: [PATCH v4 10/16] phy: Add driver for rockchip Display Port PHY Heiko Stuebner <heiko@sntech.de> - 2015-09-01 19:00 +0200
Re: [PATCH v4 10/16] phy: Add driver for rockchip Display Port PHY Heiko Stuebner <heiko@sntech.de> - 2015-09-01 23:00 +0200
Re: [PATCH v4 10/16] phy: Add driver for rockchip Display Port PHY Yakir Yang <ykk@rock-chips.com> - 2015-09-02 03:50 +0200
Re: [PATCH v4 10/16] phy: Add driver for rockchip Display Port PHY Yakir Yang <ykk@rock-chips.com> - 2015-09-02 03:10 +0200
Re: [PATCH v4 10/16] phy: Add driver for rockchip Display Port PHY Rob Herring <robherring2@gmail.com> - 2015-09-02 15:30 +0200
Re: [PATCH v4 10/16] phy: Add driver for rockchip Display Port PHY Yakir Yang <ykk@rock-chips.com> - 2015-09-03 05:30 +0200
Re: [PATCH v4 10/16] phy: Add driver for rockchip Display Port PHY Heiko Stuebner <heiko@sntech.de> - 2015-09-03 16:00 +0200
[PATCH v4 07/16] ARM: dts: exynos/dp: remove some properties that deprecated by analogix_dp driver Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:10 +0200
Re: [PATCH v4 07/16] ARM: dts: exynos/dp: remove some properties that deprecated by analogix_dp driver Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-03 02:10 +0200
[PATCH v4 15/16] drm: bridge: analogix/dp: move hpd detect to connector detect function Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:20 +0200
[PATCH v4 12/16] drm: bridge: analogix/dp: add some rk3288 special registers setting Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:20 +0200
[PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:20 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Rob Herring <robherring2@gmail.com> - 2015-09-02 22:20 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Yakir Yang <ykk@rock-chips.com> - 2015-09-03 06:30 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Thierry Reding <treding@nvidia.com> - 2015-09-03 11:10 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-09-04 12:30 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Thierry Reding <treding@nvidia.com> - 2015-09-07 10:30 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Rob Herring <robherring2@gmail.com> - 2015-09-04 23:50 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Yakir Yang <ykk@rock-chips.com> - 2015-09-06 10:30 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Thierry Reding <treding@nvidia.com> - 2015-09-07 10:40 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Thierry Reding <treding@nvidia.com> - 2015-09-03 10:50 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Rob Herring <robherring2@gmail.com> - 2015-09-04 00:00 +0200
Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed Thierry Reding <treding@nvidia.com> - 2015-09-04 12:10 +0200
[PATCH v4 13/16] drm: bridge: analogix/dp: add max link rate and lane count limit for RK3288 Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:20 +0200
[PATCH v4 16/16] drm: bridge: analogix/dp: add edid modes parse in get_modes method Yakir Yang <ykk@rock-chips.com> - 2015-09-01 08:30 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Heiko Stuebner <heiko@sntech.de> - 2015-09-01 22:50 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Yakir Yang <ykk@rock-chips.com> - 2015-09-02 03:50 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Rob Herring <robherring2@gmail.com> - 2015-09-04 23:10 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Heiko Stuebner <heiko@sntech.de> - 2015-09-04 23:40 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Thierry Reding <treding@nvidia.com> - 2015-09-07 10:20 +0200
Re: [PATCH v4 0/16] Add Analogix Core Display Port Driver Heiko Stuebner <heiko@sntech.de> - 2015-09-02 00:20 +0200
Re: [PATCH v4 0/16] Add Analogix Core Display Port Driver Yakir Yang <ykk@rock-chips.com> - 2015-09-02 04:20 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Emil Velikov <emil.l.velikov@gmail.com> - 2015-09-02 17:00 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Yakir Yang <ykk@rock-chips.com> - 2015-09-03 06:00 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-03 03:00 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Yakir Yang <ykk@rock-chips.com> - 2015-09-03 07:40 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-04 02:50 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Yakir Yang <ykk@rock-chips.com> - 2015-09-06 10:00 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-07 02:30 +0200
Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir Yakir Yang <ykk@rock-chips.com> - 2015-09-07 04:30 +0200
Page 4 of 4 — ← Prev page 1 2 3 [4]
| From | Thierry Reding <treding@nvidia.com> |
|---|---|
| Date | 2015-09-07 10:20 +0200 |
| Subject | Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir |
| Message-ID | <q5ZOG-8nj-17@gated-at.bofh.it> |
| In reply to | #1219332 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, Sep 04, 2015 at 11:29:30PM +0200, Heiko Stuebner wrote:
> Am Freitag, 4. September 2015, 16:06:02 schrieb Rob Herring:
> > On Tue, Sep 1, 2015 at 3:46 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > > Am Dienstag, 1. September 2015, 13:49:58 schrieb Yakir Yang:
> > >> Split the dp core driver from exynos directory to bridge
> > >> directory, and rename the core driver to analogix_dp_*,
> > >> leave the platform code to analogix_dp-exynos.
> > >>
> > >> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> > >
> > > [...]
> > >
> > >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c similarity index 50%
> > >> rename from drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> rename to drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > >> index bed0252..7d62f22 100644
> > >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> > >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > >
> > > [...]
> > >
> > >> connector->polled = DRM_CONNECTOR_POLL_HPD;
> > >>
> > >> ret = drm_connector_init(dp->drm_dev, connector,
> > >>
> > >> - &exynos_dp_connector_funcs,
> > >> + &analogix_dp_connector_funcs,
> > >>
> > >> DRM_MODE_CONNECTOR_eDP);
> > >>
> > >> if (ret) {
> > >>
> > >> DRM_ERROR("Failed to initialize connector with drm\n");
> > >> return ret;
> > >>
> > >> }
> > >>
> > >> - drm_connector_helper_add(connector,
> > >> &exynos_dp_connector_helper_funcs); +
> > >> drm_connector_helper_add(connector,
> > >> + &analogix_dp_connector_helper_funcs);
> > >>
> > >> drm_connector_register(connector);
> > >
> > > this should only run on exynos, as we're doing all our connector
> > > registration in the core driver after all components are bound, so I
> > > guess something like>
> > > the following is needed:
> > > if (dp->plat_data && dp->plat_data->dev_type == EXYNOS_DP)
> > >
> > > drm_connector_register(connector);
> >
> > Yuck!
> >
> > Surely there is a better way. From what I've seen of DRM, I have no
> > doubt this is needed, but really a better solution is needed. Surely
> > there can be a more generic way for the driver to determine if it
> > should handle the connector or not. This seems like a common problem
> > including one I have seen. What I'm working on has onchip DSI encoder
> > -> ADV7533 -> HDMI. The DSI encoder can also have a direct attached
> > panel. So I have to check for a bridge in the encoder driver and only
> > register the connector for the panel if a bridge is not attached.
>
> I'm also only a part-time drm meddler, so things may be inaccurate.
>
> This conditional is not meant to prevent the registration of the dp connector,
> only delay it for non-exynos drms. The connector registration does publish it
> to userspace, so like i.MX we're doing that for all connectors at the same
> time after all components are bound - to also make x11 happy. Exynos on the
> other hand seems to register its connectors individually and I'm not sure if
> they would be willing to change that.
>
> see d3007dabeff4 ("drm/rockchip: register all connectors after bind") or
> simply rockchip_drm_drv.c line 178.
>
> and e355e7dd607b ("imx-drm: delay publishing sysfs connector entries")
There really shouldn't be a reason for both drivers to behave
differently in this case. Gustavo has done a lot of work on cleaning up
the Exynos driver lately, so perhaps you should sync up with him to see
if this is something that he can integrate with his changes.
If you can't reach an agreement and Exynos must keep the exact behaviour
as it has now, then perhaps a better option would be to have the
Analogix core driver not register the connector but rather leave that up
to users of the helpers. Then you don't have to clutter the core driver
with this type of platform-specific data.
Thierry
[toc] | [prev] | [next] | [standalone]
| From | Heiko Stuebner <heiko@sntech.de> |
|---|---|
| Date | 2015-09-02 00:20 +0200 |
| Message-ID | <q424h-2Oo-1@gated-at.bofh.it> |
| In reply to | #1216602 |
Hi Yakir, Am Dienstag, 1. September 2015, 13:46:11 schrieb Yakir Yang: > The Samsung Exynos eDP controller and Rockchip RK3288 eDP controller > share the same IP, so a lot of parts can be re-used. I split the common > code into bridge directory, then rk3288 and exynos only need to keep > some platform code. Cause I can't find the exact IP name of exynos dp > controller, so I decide to name dp core driver with "analogix" which I > find in rk3288 eDP TRM ;) > > Beyond that, there are three light registers setting differents bewteen > exynos and rk3288. > 1. RK3288 have five special pll resigters which not indicata in exynos > dp controller. > 2. The address of DP_PHY_PD(dp phy power manager register) are different > between rk3288 and exynos. > 3. Rk3288 and exynos have different setting with AUX_HW_RETRY_CTL(dp debug > register). > > I have verified this series on two kinds of rockchip platform board, one > is rk3288 sdk board which connect with a 2K display port monitor, the other > is google jerry chromebook which connect with a eDP screen "cnm,n116bgeea2", > both of them works rightlly. it looks like during the rebase something did go wrong and I found some issues I mentioned in the replies to individual patches. I did prepare a branch based on mainline [0] with both the old and the new edp driver - rk3288_veyron_defconfig build both drivers into the image. While the old driver still works, I wasn't able to make the new one work yet ... the drm core does find the connector, but not that anything is connected to it. I'll try to dig deeper tomorrow, but maybe you'll see anything interesting before then. Heiko [0] https://github.com/mmind/linux-rockchip/tree/tmp/edp-with-veyron -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Yakir Yang <ykk@rock-chips.com> |
|---|---|
| Date | 2015-09-02 04:20 +0200 |
| Message-ID | <q45Ox-8fT-3@gated-at.bofh.it> |
| In reply to | #1217116 |
Hi Heiko, 在 09/02/2015 05:47 AM, Heiko Stuebner 写道: > Hi Yakir, > > Am Dienstag, 1. September 2015, 13:46:11 schrieb Yakir Yang: >> The Samsung Exynos eDP controller and Rockchip RK3288 eDP controller >> share the same IP, so a lot of parts can be re-used. I split the common >> code into bridge directory, then rk3288 and exynos only need to keep >> some platform code. Cause I can't find the exact IP name of exynos dp >> controller, so I decide to name dp core driver with "analogix" which I >> find in rk3288 eDP TRM ;) >> >> Beyond that, there are three light registers setting differents bewteen >> exynos and rk3288. >> 1. RK3288 have five special pll resigters which not indicata in exynos >> dp controller. >> 2. The address of DP_PHY_PD(dp phy power manager register) are different >> between rk3288 and exynos. >> 3. Rk3288 and exynos have different setting with AUX_HW_RETRY_CTL(dp debug >> register). >> >> I have verified this series on two kinds of rockchip platform board, one >> is rk3288 sdk board which connect with a 2K display port monitor, the other >> is google jerry chromebook which connect with a eDP screen "cnm,n116bgeea2", >> both of them works rightlly. > it looks like during the rebase something did go wrong and I found some issues > I mentioned in the replies to individual patches. > > I did prepare a branch based on mainline [0] with both the old and the new edp > driver - rk3288_veyron_defconfig build both drivers into the image. > > While the old driver still works, I wasn't able to make the new one work yet > ... the drm core does find the connector, but not that anything is connected > to it. I'll try to dig deeper tomorrow, but maybe you'll see anything > interesting before then. Many thanks for your comment and debug, I would rebase on your "edp-with-veyron" branch and fix the broken, make sure v6 would work rightly at least in your side and my side. - Yakir > > Heiko > > [0] https://github.com/mmind/linux-rockchip/tree/tmp/edp-with-veyron > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Emil Velikov <emil.l.velikov@gmail.com> |
|---|---|
| Date | 2015-09-02 17:00 +0200 |
| Subject | Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir |
| Message-ID | <q4hG2-8ct-17@gated-at.bofh.it> |
| In reply to | #1216602 |
[Dropping the CC list] Hi Yakir Yang, On 1 September 2015 at 06:49, Yakir Yang <ykk@rock-chips.com> wrote: > Split the dp core driver from exynos directory to bridge > directory, and rename the core driver to analogix_dp_*, > leave the platform code to analogix_dp-exynos. > > Signed-off-by: Yakir Yang <ykk@rock-chips.com> > --- > Changes in v4: > - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT propery. > - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to "exynos_dp.c" > - Take Archit suggest, create a separate folder for analogix code in bridge/ > "Take X suggest", is grammatically incorrect. You should use "suggestion(s)" or alternatively use the following approach. - Create a separate folder for analogix code in bridge/ (Archit) Cheers, Emil P.S. Why do you resend the whole series (some 10+ patches) when only a few patches have been changed ? Are all the patches changed whist missing that information (vX: rebase on top of A) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Yakir Yang <ykk@rock-chips.com> |
|---|---|
| Date | 2015-09-03 06:00 +0200 |
| Subject | Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir |
| Message-ID | <q4tQR-lJ-5@gated-at.bofh.it> |
| In reply to | #1217685 |
Hi Emil, 在 09/02/2015 10:50 PM, Emil Velikov 写道: > [Dropping the CC list] Hmm...Don't understand what this means. If you can explain, that would be better, so I would not miss your suggest. :-) > Hi Yakir Yang, > > On 1 September 2015 at 06:49, Yakir Yang <ykk@rock-chips.com> wrote: >> Split the dp core driver from exynos directory to bridge >> directory, and rename the core driver to analogix_dp_*, >> leave the platform code to analogix_dp-exynos. >> >> Signed-off-by: Yakir Yang <ykk@rock-chips.com> >> --- >> Changes in v4: >> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT propery. >> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to "exynos_dp.c" >> - Take Archit suggest, create a separate folder for analogix code in bridge/ >> > "Take X suggest", is grammatically incorrect. You should use > "suggestion(s)" or alternatively use the following approach. > > - Create a separate folder for analogix code in bridge/ (Archit) Thanks for your correct. I prefer to take the last one ;) > Cheers, > Emil > > P.S. Why do you resend the whole series (some 10+ patches) when only a > few patches have been changed ? Are all the patches changed whist > missing that information (vX: rebase on top of A) Yes, those 10+ patches do base on the previous changes, specially when I rebase my series on the latest linux-next branch. Besides, I don't know that how to send parts of this series, and keep the unrelated patches no send. For example if the [v4 03/16] ~ [v4 10/16] need update to v5, and [v4 11/16] ~ [v4 16/16] keep in v4. So should I only send the [v5 03/16] ~ [v5 10/16], no need to send and update [v4 11/16] ~ [v4 16/16] to v5 ? If so I think it would be difficult for guys to comment, cause they can't see the whole series in one reply list . ;) Thanks, - Yakir > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2015-09-03 03:00 +0200 |
| Subject | Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir |
| Message-ID | <q4r2F-4Ip-13@gated-at.bofh.it> |
| In reply to | #1216602 |
On 01.09.2015 14:49, Yakir Yang wrote:
> Split the dp core driver from exynos directory to bridge
> directory, and rename the core driver to analogix_dp_*,
> leave the platform code to analogix_dp-exynos.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v4:
> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT propery.
> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to "exynos_dp.c"
> - Take Archit suggest, create a separate folder for analogix code in bridge/
>
> Changes in v3:
> - Take Thierry Reding suggest, move exynos's video_timing code
> to analogix_dp-exynos platform driver, add get_modes method
> to struct analogix_dp_plat_data.
> - Take Heiko suggest, rename some "samsung*" dts propery to "analogix*".
>
> Changes in v2:
> - Take Jingoo Han suggest, remove new copyright
> - Fix compiled failed dut to analogix_dp_device misspell
>
> drivers/gpu/drm/bridge/Kconfig | 2 +
> drivers/gpu/drm/bridge/Makefile | 1 +
> drivers/gpu/drm/bridge/analogix/Kconfig | 4 +
> drivers/gpu/drm/bridge/analogix/Makefile | 1 +
> .../analogix/analogix_dp_core.c} | 817 ++++++-------
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 283 +++++
> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 1265 ++++++++++++++++++++
> .../analogix/analogix_dp_reg.h} | 258 ++--
> drivers/gpu/drm/exynos/Kconfig | 3 +-
> drivers/gpu/drm/exynos/Makefile | 2 +-
> drivers/gpu/drm/exynos/exynos_dp.c | 306 +++++
> drivers/gpu/drm/exynos/exynos_dp_core.h | 282 -----
> drivers/gpu/drm/exynos/exynos_dp_reg.c | 1259 -------------------
> include/drm/bridge/analogix_dp.h | 24 +
> 14 files changed, 2357 insertions(+), 2150 deletions(-)
> create mode 100644 drivers/gpu/drm/bridge/analogix/Kconfig
> create mode 100644 drivers/gpu/drm/bridge/analogix/Makefile
> rename drivers/gpu/drm/{exynos/exynos_dp_core.c => bridge/analogix/analogix_dp_core.c} (50%)
> create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> rename drivers/gpu/drm/{exynos/exynos_dp_reg.h => bridge/analogix/analogix_dp_reg.h} (64%)
> create mode 100644 drivers/gpu/drm/exynos/exynos_dp.c
> delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_core.h
> delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_reg.c
> create mode 100644 include/drm/bridge/analogix_dp.h
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 2de52a5..7b5b77a 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -29,4 +29,6 @@ config DRM_PARADE_PS8622
> ---help---
> Parade eDP-LVDS bridge chip driver.
>
> +source "drivers/gpu/drm/bridge/analogix/Kconfig"
> +
> endmenu
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index e2eef1c..5366c6b 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -3,3 +3,4 @@ ccflags-y := -Iinclude/drm
> obj-$(CONFIG_DRM_DW_HDMI) += dw_hdmi.o
> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig b/drivers/gpu/drm/bridge/analogix/Kconfig
> new file mode 100644
> index 0000000..5ff6551
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
> @@ -0,0 +1,4 @@
> +config DRM_ANALOGIX_DP
> + tristate
> + depends on DRM
> + select DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile b/drivers/gpu/drm/bridge/analogix/Makefile
> new file mode 100644
> index 0000000..9107b86
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp_core.o analogix_dp_reg.o
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> similarity index 50%
> rename from drivers/gpu/drm/exynos/exynos_dp_core.c
> rename to drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index bed0252..7d62f22 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1,14 +1,14 @@
> /*
> - * Samsung SoC DP (Display Port) interface driver.
> - *
> - * Copyright (C) 2012 Samsung Electronics Co., Ltd.
> - * Author: Jingoo Han <jg1.han@samsung.com>
> - *
> - * This program is free software; you can redistribute it and/or modify it
> - * under the terms of the GNU General Public License as published by the
> - * Free Software Foundation; either version 2 of the License, or (at your
> - * option) any later version.
> - */
> +* Analogix DP (Display Port) core interface driver.
> +*
> +* Copyright (C) 2012 Samsung Electronics Co., Ltd.
> +* Author: Jingoo Han <jg1.han@samsung.com>
> +*
> +* This program is free software; you can redistribute it and/or modify it
> +* under the terms of the GNU General Public License as published by the
> +* Free Software Foundation; either version 2 of the License, or (at your
> +* option) any later version.
> +*/
>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> @@ -18,12 +18,9 @@
> #include <linux/interrupt.h>
> #include <linux/of.h>
> #include <linux/of_gpio.h>
> -#include <linux/of_graph.h>
> #include <linux/gpio.h>
> #include <linux/component.h>
> #include <linux/phy/phy.h>
> -#include <video/of_display_timing.h>
> -#include <video/of_videomode.h>
>
> #include <drm/drmP.h>
> #include <drm/drm_crtc.h>
> @@ -31,52 +28,42 @@
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_panel.h>
>
> -#include "exynos_dp_core.h"
> -#include "exynos_drm_crtc.h"
> +#include <drm/bridge/analogix_dp.h>
>
> -#define ctx_from_connector(c) container_of(c, struct exynos_dp_device, \
> - connector)
> +#include "analogix_dp_core.h"
>
> -static inline struct exynos_drm_crtc *dp_to_crtc(struct exynos_dp_device *dp)
> -{
> - return to_exynos_crtc(dp->encoder.crtc);
> -}
> -
> -static inline struct exynos_dp_device *encoder_to_dp(
> - struct drm_encoder *e)
> -{
> - return container_of(e, struct exynos_dp_device, encoder);
> -}
> +#define connector_to_dp(c) \
> + container_of(c, struct analogix_dp_device, connector)
>
> struct bridge_init {
> struct i2c_client *client;
> struct device_node *node;
> };
>
> -static void exynos_dp_init_dp(struct exynos_dp_device *dp)
> +static void analogix_dp_init_dp(struct analogix_dp_device *dp)
> {
> - exynos_dp_reset(dp);
> + analogix_dp_reset(dp);
>
> - exynos_dp_swreset(dp);
> + analogix_dp_swreset(dp);
>
> - exynos_dp_init_analog_param(dp);
> - exynos_dp_init_interrupt(dp);
> + analogix_dp_init_analog_param(dp);
> + analogix_dp_init_interrupt(dp);
>
> /* SW defined function Normal operation */
> - exynos_dp_enable_sw_function(dp);
> + analogix_dp_enable_sw_function(dp);
>
> - exynos_dp_config_interrupt(dp);
> - exynos_dp_init_analog_func(dp);
> + analogix_dp_config_interrupt(dp);
> + analogix_dp_init_analog_func(dp);
>
> - exynos_dp_init_hpd(dp);
> - exynos_dp_init_aux(dp);
> + analogix_dp_init_hpd(dp);
> + analogix_dp_init_aux(dp);
> }
>
> -static int exynos_dp_detect_hpd(struct exynos_dp_device *dp)
> +static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
> {
> int timeout_loop = 0;
>
> - while (exynos_dp_get_plug_in_status(dp) != 0) {
> + while (analogix_dp_get_plug_in_status(dp) != 0) {
> timeout_loop++;
> if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
> dev_err(dp->dev, "failed to get hpd plug status\n");
> @@ -88,7 +75,7 @@ static int exynos_dp_detect_hpd(struct exynos_dp_device *dp)
> return 0;
> }
>
> -static unsigned char exynos_dp_calc_edid_check_sum(unsigned char *edid_data)
> +static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
> {
> int i;
> unsigned char sum = 0;
> @@ -99,7 +86,7 @@ static unsigned char exynos_dp_calc_edid_check_sum(unsigned char *edid_data)
> return sum;
> }
>
> -static int exynos_dp_read_edid(struct exynos_dp_device *dp)
> +static int analogix_dp_read_edid(struct analogix_dp_device *dp)
> {
> unsigned char edid[EDID_BLOCK_LENGTH * 2];
> unsigned int extend_block = 0;
> @@ -114,9 +101,9 @@ static int exynos_dp_read_edid(struct exynos_dp_device *dp)
> */
>
> /* Read Extension Flag, Number of 128-byte EDID extension blocks */
> - retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
> - EDID_EXTENSION_FLAG,
> - &extend_block);
> + retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
> + EDID_EXTENSION_FLAG,
> + &extend_block);
Hi,
What was the point of patch 1 (checkpatch -f --subjective) if you
have to re-align the parameters again? It's meaningless.
I could understand that idea if this patch touched only first
line - name of the function. This would result in small
and effective diff, like:
- retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
+ retval = SAME_LEGNTH____byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
EDID_EXTENSION_FLAG,
But this is not the case. The reviewer have to look at 6 lines
of changes instead of two. You fixed the alignment just for two
patches and then you re-written it. It's meaningless.
Nope. Please drop all changes from patch 1 which:
(all conditions apply):
1. change alignment,
2. are immediately changed in next patches (e.g. like here),
and fix the alignment while renaming the function.
This is actually another point for not accepting commits where
the reason is "checkpatch told me".
> if (retval)
> return retval;
>
> @@ -124,7 +111,7 @@ static int exynos_dp_read_edid(struct exynos_dp_device *dp)
> dev_dbg(dp->dev, "EDID data includes a single extension!\n");
>
> /* Read EDID data */
> - retval = exynos_dp_read_bytes_from_i2c(
> + retval = analogix_dp_read_bytes_from_i2c(
> dp, I2C_EDID_DEVICE_ADDR,
> EDID_HEADER_PATTERN,
> EDID_BLOCK_LENGTH,
> @@ -133,14 +120,14 @@ static int exynos_dp_read_edid(struct exynos_dp_device *dp)
(...)
> @@ -871,218 +862,204 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
> return IRQ_HANDLED;
> }
>
> -static void exynos_dp_hotplug(struct work_struct *work)
> +static void analogix_dp_hotplug(struct work_struct *work)
> {
> - struct exynos_dp_device *dp;
> + struct analogix_dp_device *dp;
>
> - dp = container_of(work, struct exynos_dp_device, hotplug_work);
> + dp = container_of(work, struct analogix_dp_device, hotplug_work);
>
> if (dp->drm_dev)
> drm_helper_hpd_irq_event(dp->drm_dev);
> }
>
> -static void exynos_dp_commit(struct drm_encoder *encoder)
> +static void analogix_dp_commit(struct analogix_dp_device *dp)
> {
> - struct exynos_dp_device *dp = encoder_to_dp(encoder);
> int ret;
>
> /* Keep the panel disabled while we configure video */
> - if (dp->panel) {
> - if (drm_panel_disable(dp->panel))
> + if (dp->plat_data && dp->plat_data->panel) {
> + if (drm_panel_disable(dp->plat_data->panel))
> DRM_ERROR("failed to disable the panel\n");
> }
>
> - ret = exynos_dp_detect_hpd(dp);
> + ret = analogix_dp_detect_hpd(dp);
> if (ret) {
> /* Cable has been disconnected, we're done */
> return;
> }
>
> - ret = exynos_dp_handle_edid(dp);
> + ret = analogix_dp_handle_edid(dp);
> if (ret) {
> dev_err(dp->dev, "unable to handle edid\n");
> return;
> }
>
> - ret = exynos_dp_set_link_train(dp, dp->video_info->lane_count,
> - dp->video_info->link_rate);
> + ret = analogix_dp_set_link_train(dp, dp->video_info->lane_count,
> + dp->video_info->link_rate);
> if (ret) {
> dev_err(dp->dev, "unable to do link train\n");
> return;
> }
>
> - exynos_dp_enable_scramble(dp, 1);
> - exynos_dp_enable_rx_to_enhanced_mode(dp, 1);
> - exynos_dp_enable_enhanced_mode(dp, 1);
> + analogix_dp_enable_scramble(dp, 1);
> + analogix_dp_enable_rx_to_enhanced_mode(dp, 1);
> + analogix_dp_enable_enhanced_mode(dp, 1);
>
> - exynos_dp_set_lane_count(dp, dp->video_info->lane_count);
> - exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
> + analogix_dp_set_lane_count(dp, dp->video_info->lane_count);
> + analogix_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
>
> - exynos_dp_init_video(dp);
> - ret = exynos_dp_config_video(dp);
> + analogix_dp_init_video(dp);
> + ret = analogix_dp_config_video(dp);
> if (ret)
> dev_err(dp->dev, "unable to config video\n");
>
> /* Safe to enable the panel now */
> - if (dp->panel) {
> - if (drm_panel_enable(dp->panel))
> + if (dp->plat_data && dp->plat_data->panel) {
> + if (drm_panel_enable(dp->plat_data->panel))
> DRM_ERROR("failed to enable the panel\n");
> }
>
> /* Enable video */
> - exynos_dp_start_video(dp);
> + analogix_dp_start_video(dp);
> }
>
> static enum drm_connector_status
> -exynos_dp_detect(struct drm_connector *connector, bool force)
> +analogix_dp_detect(struct drm_connector *connector, bool force)
> {
> return connector_status_connected;
> }
>
> -static void exynos_dp_connector_destroy(struct drm_connector *connector)
> +static void analogix_dp_connector_destroy(struct drm_connector *connector)
> {
> drm_connector_unregister(connector);
> drm_connector_cleanup(connector);
> }
>
> -static struct drm_connector_funcs exynos_dp_connector_funcs = {
> +static struct drm_connector_funcs analogix_dp_connector_funcs = {
> .dpms = drm_atomic_helper_connector_dpms,
> .fill_modes = drm_helper_probe_single_connector_modes,
> - .detect = exynos_dp_detect,
> - .destroy = exynos_dp_connector_destroy,
> + .detect = analogix_dp_detect,
> + .destroy = analogix_dp_connector_destroy,
> .reset = drm_atomic_helper_connector_reset,
> .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };
>
> -static int exynos_dp_get_modes(struct drm_connector *connector)
> +static int analogix_dp_get_modes(struct drm_connector *connector)
> {
> - struct exynos_dp_device *dp = ctx_from_connector(connector);
> - struct drm_display_mode *mode;
> -
> - if (dp->panel)
> - return drm_panel_get_modes(dp->panel);
> + struct analogix_dp_device *dp = connector_to_dp(connector);
> + struct analogix_dp_plat_data *plat_data = dp->plat_data;
> + int num_modes = 0;
>
> - mode = drm_mode_create(connector->dev);
> - if (!mode) {
> - DRM_ERROR("failed to create a new display mode.\n");
> - return 0;
> - }
> -
> - drm_display_mode_from_videomode(&dp->priv.vm, mode);
> - mode->width_mm = dp->priv.width_mm;
> - mode->height_mm = dp->priv.height_mm;
> - connector->display_info.width_mm = mode->width_mm;
> - connector->display_info.height_mm = mode->height_mm;
> + if (plat_data && plat_data->panel)
> + num_modes += drm_panel_get_modes(plat_data->panel);
>
> - mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> - drm_mode_set_name(mode);
> - drm_mode_probed_add(connector, mode);
> + if (plat_data && plat_data->get_modes)
> + num_modes += plat_data->get_modes(plat_data, connector);
>
> - return 1;
> + return num_modes;
> }
>
> static struct drm_encoder *
> -exynos_dp_best_encoder(struct drm_connector *connector)
> +analogix_dp_best_encoder(struct drm_connector *connector)
> {
> - struct exynos_dp_device *dp = ctx_from_connector(connector);
> + struct analogix_dp_device *dp = connector_to_dp(connector);
>
> - return &dp->encoder;
> + return dp->encoder;
> }
>
> -static struct drm_connector_helper_funcs exynos_dp_connector_helper_funcs = {
> - .get_modes = exynos_dp_get_modes,
> - .best_encoder = exynos_dp_best_encoder,
> +static struct drm_connector_helper_funcs analogix_dp_connector_helper_funcs = {
> + .get_modes = analogix_dp_get_modes,
> + .best_encoder = analogix_dp_best_encoder,
> };
>
> -/* returns the number of bridges attached */
> -static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
> - struct drm_encoder *encoder)
> -{
> - int ret;
> -
> - encoder->bridge->next = dp->ptn_bridge;
> - dp->ptn_bridge->encoder = encoder;
> - ret = drm_bridge_attach(encoder->dev, dp->ptn_bridge);
> - if (ret) {
> - DRM_ERROR("Failed to attach bridge to drm\n");
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> -static int exynos_dp_bridge_attach(struct drm_bridge *bridge)
> +static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
> {
> - struct exynos_dp_device *dp = bridge->driver_private;
> - struct drm_encoder *encoder = &dp->encoder;
> + struct analogix_dp_device *dp = bridge->driver_private;
> + struct drm_encoder *encoder = dp->encoder;
> struct drm_connector *connector = &dp->connector;
> int ret;
>
> - /* Pre-empt DP connector creation if there's a bridge */
> - if (dp->ptn_bridge) {
> - ret = exynos_drm_attach_lcd_bridge(dp, encoder);
> - if (!ret)
> - return 0;
> + if (!bridge->encoder) {
> + DRM_ERROR("Parent encoder object not found");
> + return -ENODEV;
> }
>
> + encoder->bridge = bridge;
> +
> connector->polled = DRM_CONNECTOR_POLL_HPD;
>
> ret = drm_connector_init(dp->drm_dev, connector,
> - &exynos_dp_connector_funcs,
> + &analogix_dp_connector_funcs,
> DRM_MODE_CONNECTOR_eDP);
> if (ret) {
> DRM_ERROR("Failed to initialize connector with drm\n");
> return ret;
> }
>
> - drm_connector_helper_add(connector, &exynos_dp_connector_helper_funcs);
> + drm_connector_helper_add(connector,
> + &analogix_dp_connector_helper_funcs);
> drm_connector_register(connector);
> drm_mode_connector_attach_encoder(connector, encoder);
>
> - if (dp->panel)
> - ret = drm_panel_attach(dp->panel, &dp->connector);
> + if (dp->plat_data && dp->plat_data->panel) {
> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
> + if (ret) {
> + DRM_ERROR("Failed to attach panel\n");
> + return ret;
> + }
> + }
> +
> + /*
> + * This should be the end of attach function, caused
> + * we should ensure dp bridge could attach first.
> + */
> + if (dp->plat_data && dp->plat_data->attach) {
> + ret = dp->plat_data->attach(dp->plat_data, bridge);
> + if (ret) {
> + DRM_ERROR("Failed at platform attch func\n");
Two new error paths appeared here and above. Don't you have to
cleanup something? I don't know, just wondering...
(...)
> @@ -1345,22 +1286,9 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
> disable_irq(dp->irq);
>
> dp->drm_dev = drm_dev;
> + dp->encoder = encoder;
>
> - pipe = exynos_drm_crtc_get_pipe_from_type(drm_dev,
> - EXYNOS_DISPLAY_TYPE_LCD);
> - if (pipe < 0)
> - return pipe;
> -
> - encoder->possible_crtcs = 1 << pipe;
> -
> - DRM_DEBUG_KMS("possible_crtcs = 0x%x\n", encoder->possible_crtcs);
> -
> - drm_encoder_init(drm_dev, encoder, &exynos_dp_encoder_funcs,
> - DRM_MODE_ENCODER_TMDS);
> -
> - drm_encoder_helper_add(encoder, &exynos_dp_encoder_helper_funcs);
> -
> - ret = exynos_dp_create_connector(encoder);
> + ret = analogix_dp_create_bridge(drm_dev, dp);
> if (ret) {
> DRM_ERROR("failed to create connector ret = %d\n", ret);
> drm_encoder_cleanup(encoder);
> @@ -1369,103 +1297,36 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(analogix_dp_bind);
Ooo, a new export symbol. I suppose it is necessary for the split
but the commit message does not mention exporting
anything nor the reason for doing it. Could you extend the commit
message describing more tha changes here and rationale for it?
(...)
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> new file mode 100644
> index 0000000..b879d8c
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -0,0 +1,1265 @@
> +/*
> + * Analogix DP (Display port) core register interface driver.
> + *
> + * Copyright (C) 2012 Samsung Electronics Co., Ltd.
> + * Author: Jingoo Han <jg1.han@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +
> +#include "analogix_dp_core.h"
> +#include "analogix_dp_reg.h"
> +
> +#define COMMON_INT_MASK_1 0
> +#define COMMON_INT_MASK_2 0
> +#define COMMON_INT_MASK_3 0
> +#define COMMON_INT_MASK_4 (HOTPLUG_CHG | HPD_LOST | PLUG)
> +#define INT_STA_MASK INT_HPD
> +
> +void analogix_dp_enable_video_mute(struct analogix_dp_device *dp, bool enable)
> +{
> + u32 reg;
> +
> + if (enable) {
> + reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
> + reg |= HDCP_VIDEO_MUTE;
> + writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
> + } else {
> + reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
> + reg &= ~HDCP_VIDEO_MUTE;
> + writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
> + }
> +}
> +
> +void analogix_dp_stop_video(struct analogix_dp_device *dp)
> +{
> + u32 reg;
> +
> + reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
> + reg &= ~VIDEO_EN;
> + writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
> +}
> +
> +void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable)
> +{
> + u32 reg;
> +
> + if (enable)
> + reg = LANE3_MAP_LOGIC_LANE_0 | LANE2_MAP_LOGIC_LANE_1 |
> + LANE1_MAP_LOGIC_LANE_2 | LANE0_MAP_LOGIC_LANE_3;
> + else
> + reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 |
> + LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0;
Here and in few other places with new code: no alignment?
You spent some time fixing these in patch 1, so I would assume
that new code already comes after "checkpatch -f --subjective".
That's actually the only valid reason for using checkpatch - new code.
However my mail reader could corrupt the output so maybe this is aligned?
Code seems fine after looking at it but review was rather shallow.
I don't know the Exynos DP driver.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Yakir Yang <ykk@rock-chips.com> |
|---|---|
| Date | 2015-09-03 07:40 +0200 |
| Subject | Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir |
| Message-ID | <q4vpE-2GW-7@gated-at.bofh.it> |
| In reply to | #1217978 |
Hi Krzysztof,
在 09/03/2015 08:58 AM, Krzysztof Kozlowski 写道:
> On 01.09.2015 14:49, Yakir Yang wrote:
>> Split the dp core driver from exynos directory to bridge
>> directory, and rename the core driver to analogix_dp_*,
>> leave the platform code to analogix_dp-exynos.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Changes in v4:
>> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT propery.
>> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to "exynos_dp.c"
>> - Take Archit suggest, create a separate folder for analogix code in bridge/
>>
>> Changes in v3:
>> - Take Thierry Reding suggest, move exynos's video_timing code
>> to analogix_dp-exynos platform driver, add get_modes method
>> to struct analogix_dp_plat_data.
>> - Take Heiko suggest, rename some "samsung*" dts propery to "analogix*".
>>
>> Changes in v2:
>> - Take Jingoo Han suggest, remove new copyright
>> - Fix compiled failed dut to analogix_dp_device misspell
>>
>> drivers/gpu/drm/bridge/Kconfig | 2 +
>> drivers/gpu/drm/bridge/Makefile | 1 +
>> drivers/gpu/drm/bridge/analogix/Kconfig | 4 +
>> drivers/gpu/drm/bridge/analogix/Makefile | 1 +
>> .../analogix/analogix_dp_core.c} | 817 ++++++-------
>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 283 +++++
>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 1265 ++++++++++++++++++++
>> .../analogix/analogix_dp_reg.h} | 258 ++--
>> drivers/gpu/drm/exynos/Kconfig | 3 +-
>> drivers/gpu/drm/exynos/Makefile | 2 +-
>> drivers/gpu/drm/exynos/exynos_dp.c | 306 +++++
>> drivers/gpu/drm/exynos/exynos_dp_core.h | 282 -----
>> drivers/gpu/drm/exynos/exynos_dp_reg.c | 1259 -------------------
>> include/drm/bridge/analogix_dp.h | 24 +
>> 14 files changed, 2357 insertions(+), 2150 deletions(-)
>> create mode 100644 drivers/gpu/drm/bridge/analogix/Kconfig
>> create mode 100644 drivers/gpu/drm/bridge/analogix/Makefile
>> rename drivers/gpu/drm/{exynos/exynos_dp_core.c => bridge/analogix/analogix_dp_core.c} (50%)
>> create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> rename drivers/gpu/drm/{exynos/exynos_dp_reg.h => bridge/analogix/analogix_dp_reg.h} (64%)
>> create mode 100644 drivers/gpu/drm/exynos/exynos_dp.c
>> delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_core.h
>> delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_reg.c
>> create mode 100644 include/drm/bridge/analogix_dp.h
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index 2de52a5..7b5b77a 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -29,4 +29,6 @@ config DRM_PARADE_PS8622
>> ---help---
>> Parade eDP-LVDS bridge chip driver.
>>
>> +source "drivers/gpu/drm/bridge/analogix/Kconfig"
>> +
>> endmenu
>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
>> index e2eef1c..5366c6b 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -3,3 +3,4 @@ ccflags-y := -Iinclude/drm
>> obj-$(CONFIG_DRM_DW_HDMI) += dw_hdmi.o
>> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>> obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig b/drivers/gpu/drm/bridge/analogix/Kconfig
>> new file mode 100644
>> index 0000000..5ff6551
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
>> @@ -0,0 +1,4 @@
>> +config DRM_ANALOGIX_DP
>> + tristate
>> + depends on DRM
>> + select DRM_KMS_HELPER
>> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile b/drivers/gpu/drm/bridge/analogix/Makefile
>> new file mode 100644
>> index 0000000..9107b86
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp_core.o analogix_dp_reg.o
>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> similarity index 50%
>> rename from drivers/gpu/drm/exynos/exynos_dp_core.c
>> rename to drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index bed0252..7d62f22 100644
>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -1,14 +1,14 @@
>> /*
>> - * Samsung SoC DP (Display Port) interface driver.
>> - *
>> - * Copyright (C) 2012 Samsung Electronics Co., Ltd.
>> - * Author: Jingoo Han <jg1.han@samsung.com>
>> - *
>> - * This program is free software; you can redistribute it and/or modify it
>> - * under the terms of the GNU General Public License as published by the
>> - * Free Software Foundation; either version 2 of the License, or (at your
>> - * option) any later version.
>> - */
>> +* Analogix DP (Display Port) core interface driver.
>> +*
>> +* Copyright (C) 2012 Samsung Electronics Co., Ltd.
>> +* Author: Jingoo Han <jg1.han@samsung.com>
>> +*
>> +* This program is free software; you can redistribute it and/or modify it
>> +* under the terms of the GNU General Public License as published by the
>> +* Free Software Foundation; either version 2 of the License, or (at your
>> +* option) any later version.
>> +*/
>>
>> #include <linux/module.h>
>> #include <linux/platform_device.h>
>> @@ -18,12 +18,9 @@
>> #include <linux/interrupt.h>
>> #include <linux/of.h>
>> #include <linux/of_gpio.h>
>> -#include <linux/of_graph.h>
>> #include <linux/gpio.h>
>> #include <linux/component.h>
>> #include <linux/phy/phy.h>
>> -#include <video/of_display_timing.h>
>> -#include <video/of_videomode.h>
>>
>> #include <drm/drmP.h>
>> #include <drm/drm_crtc.h>
>> @@ -31,52 +28,42 @@
>> #include <drm/drm_atomic_helper.h>
>> #include <drm/drm_panel.h>
>>
>> -#include "exynos_dp_core.h"
>> -#include "exynos_drm_crtc.h"
>> +#include <drm/bridge/analogix_dp.h>
>>
>> -#define ctx_from_connector(c) container_of(c, struct exynos_dp_device, \
>> - connector)
>> +#include "analogix_dp_core.h"
>>
>> -static inline struct exynos_drm_crtc *dp_to_crtc(struct exynos_dp_device *dp)
>> -{
>> - return to_exynos_crtc(dp->encoder.crtc);
>> -}
>> -
>> -static inline struct exynos_dp_device *encoder_to_dp(
>> - struct drm_encoder *e)
>> -{
>> - return container_of(e, struct exynos_dp_device, encoder);
>> -}
>> +#define connector_to_dp(c) \
>> + container_of(c, struct analogix_dp_device, connector)
>>
>> struct bridge_init {
>> struct i2c_client *client;
>> struct device_node *node;
>> };
>>
>> -static void exynos_dp_init_dp(struct exynos_dp_device *dp)
>> +static void analogix_dp_init_dp(struct analogix_dp_device *dp)
>> {
>> - exynos_dp_reset(dp);
>> + analogix_dp_reset(dp);
>>
>> - exynos_dp_swreset(dp);
>> + analogix_dp_swreset(dp);
>>
>> - exynos_dp_init_analog_param(dp);
>> - exynos_dp_init_interrupt(dp);
>> + analogix_dp_init_analog_param(dp);
>> + analogix_dp_init_interrupt(dp);
>>
>> /* SW defined function Normal operation */
>> - exynos_dp_enable_sw_function(dp);
>> + analogix_dp_enable_sw_function(dp);
>>
>> - exynos_dp_config_interrupt(dp);
>> - exynos_dp_init_analog_func(dp);
>> + analogix_dp_config_interrupt(dp);
>> + analogix_dp_init_analog_func(dp);
>>
>> - exynos_dp_init_hpd(dp);
>> - exynos_dp_init_aux(dp);
>> + analogix_dp_init_hpd(dp);
>> + analogix_dp_init_aux(dp);
>> }
>>
>> -static int exynos_dp_detect_hpd(struct exynos_dp_device *dp)
>> +static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>> {
>> int timeout_loop = 0;
>>
>> - while (exynos_dp_get_plug_in_status(dp) != 0) {
>> + while (analogix_dp_get_plug_in_status(dp) != 0) {
>> timeout_loop++;
>> if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
>> dev_err(dp->dev, "failed to get hpd plug status\n");
>> @@ -88,7 +75,7 @@ static int exynos_dp_detect_hpd(struct exynos_dp_device *dp)
>> return 0;
>> }
>>
>> -static unsigned char exynos_dp_calc_edid_check_sum(unsigned char *edid_data)
>> +static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
>> {
>> int i;
>> unsigned char sum = 0;
>> @@ -99,7 +86,7 @@ static unsigned char exynos_dp_calc_edid_check_sum(unsigned char *edid_data)
>> return sum;
>> }
>>
>> -static int exynos_dp_read_edid(struct exynos_dp_device *dp)
>> +static int analogix_dp_read_edid(struct analogix_dp_device *dp)
>> {
>> unsigned char edid[EDID_BLOCK_LENGTH * 2];
>> unsigned int extend_block = 0;
>> @@ -114,9 +101,9 @@ static int exynos_dp_read_edid(struct exynos_dp_device *dp)
>> */
>>
>> /* Read Extension Flag, Number of 128-byte EDID extension blocks */
>> - retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>> - EDID_EXTENSION_FLAG,
>> - &extend_block);
>> + retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>> + EDID_EXTENSION_FLAG,
>> + &extend_block);
> Hi,
>
> What was the point of patch 1 (checkpatch -f --subjective) if you
> have to re-align the parameters again? It's meaningless.
>
> I could understand that idea if this patch touched only first
> line - name of the function. This would result in small
> and effective diff, like:
>
> - retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
> + retval = SAME_LEGNTH____byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
> EDID_EXTENSION_FLAG,
>
> But this is not the case. The reviewer have to look at 6 lines
> of changes instead of two. You fixed the alignment just for two
> patches and then you re-written it. It's meaningless.
Yes, this is the place that crazy me too. After rename "exynos"
to "analogix", there would bring some "over-80-char" problem :(
> Nope. Please drop all changes from patch 1 which:
> (all conditions apply):
> 1. change alignment,
> 2. are immediately changed in next patches (e.g. like here),
>
> and fix the alignment while renaming the function.
>
> This is actually another point for not accepting commits where
> the reason is "checkpatch told me".
Okay, so there would be two patches to fix the alignment, done.
Thanks for your careful remind.
>
>
>> if (retval)
>> return retval;
>>
>> @@ -124,7 +111,7 @@ static int exynos_dp_read_edid(struct exynos_dp_device *dp)
>> dev_dbg(dp->dev, "EDID data includes a single extension!\n");
>>
>> /* Read EDID data */
>> - retval = exynos_dp_read_bytes_from_i2c(
>> + retval = analogix_dp_read_bytes_from_i2c(
>> dp, I2C_EDID_DEVICE_ADDR,
>> EDID_HEADER_PATTERN,
>> EDID_BLOCK_LENGTH,
>> @@ -133,14 +120,14 @@ static int exynos_dp_read_edid(struct exynos_dp_device *dp)
> (...)
>
>> @@ -871,218 +862,204 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
>> return IRQ_HANDLED;
>> }
>>
>> -static void exynos_dp_hotplug(struct work_struct *work)
>> +static void analogix_dp_hotplug(struct work_struct *work)
>> {
>> - struct exynos_dp_device *dp;
>> + struct analogix_dp_device *dp;
>>
>> - dp = container_of(work, struct exynos_dp_device, hotplug_work);
>> + dp = container_of(work, struct analogix_dp_device, hotplug_work);
>>
>> if (dp->drm_dev)
>> drm_helper_hpd_irq_event(dp->drm_dev);
>> }
>>
>> -static void exynos_dp_commit(struct drm_encoder *encoder)
>> +static void analogix_dp_commit(struct analogix_dp_device *dp)
>> {
>> - struct exynos_dp_device *dp = encoder_to_dp(encoder);
>> int ret;
>>
>> /* Keep the panel disabled while we configure video */
>> - if (dp->panel) {
>> - if (drm_panel_disable(dp->panel))
>> + if (dp->plat_data && dp->plat_data->panel) {
>> + if (drm_panel_disable(dp->plat_data->panel))
>> DRM_ERROR("failed to disable the panel\n");
>> }
>>
>> - ret = exynos_dp_detect_hpd(dp);
>> + ret = analogix_dp_detect_hpd(dp);
>> if (ret) {
>> /* Cable has been disconnected, we're done */
>> return;
>> }
>>
>> - ret = exynos_dp_handle_edid(dp);
>> + ret = analogix_dp_handle_edid(dp);
>> if (ret) {
>> dev_err(dp->dev, "unable to handle edid\n");
>> return;
>> }
>>
>> - ret = exynos_dp_set_link_train(dp, dp->video_info->lane_count,
>> - dp->video_info->link_rate);
>> + ret = analogix_dp_set_link_train(dp, dp->video_info->lane_count,
>> + dp->video_info->link_rate);
>> if (ret) {
>> dev_err(dp->dev, "unable to do link train\n");
>> return;
>> }
>>
>> - exynos_dp_enable_scramble(dp, 1);
>> - exynos_dp_enable_rx_to_enhanced_mode(dp, 1);
>> - exynos_dp_enable_enhanced_mode(dp, 1);
>> + analogix_dp_enable_scramble(dp, 1);
>> + analogix_dp_enable_rx_to_enhanced_mode(dp, 1);
>> + analogix_dp_enable_enhanced_mode(dp, 1);
>>
>> - exynos_dp_set_lane_count(dp, dp->video_info->lane_count);
>> - exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
>> + analogix_dp_set_lane_count(dp, dp->video_info->lane_count);
>> + analogix_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
>>
>> - exynos_dp_init_video(dp);
>> - ret = exynos_dp_config_video(dp);
>> + analogix_dp_init_video(dp);
>> + ret = analogix_dp_config_video(dp);
>> if (ret)
>> dev_err(dp->dev, "unable to config video\n");
>>
>> /* Safe to enable the panel now */
>> - if (dp->panel) {
>> - if (drm_panel_enable(dp->panel))
>> + if (dp->plat_data && dp->plat_data->panel) {
>> + if (drm_panel_enable(dp->plat_data->panel))
>> DRM_ERROR("failed to enable the panel\n");
>> }
>>
>> /* Enable video */
>> - exynos_dp_start_video(dp);
>> + analogix_dp_start_video(dp);
>> }
>>
>> static enum drm_connector_status
>> -exynos_dp_detect(struct drm_connector *connector, bool force)
>> +analogix_dp_detect(struct drm_connector *connector, bool force)
>> {
>> return connector_status_connected;
>> }
>>
>> -static void exynos_dp_connector_destroy(struct drm_connector *connector)
>> +static void analogix_dp_connector_destroy(struct drm_connector *connector)
>> {
>> drm_connector_unregister(connector);
>> drm_connector_cleanup(connector);
>> }
>>
>> -static struct drm_connector_funcs exynos_dp_connector_funcs = {
>> +static struct drm_connector_funcs analogix_dp_connector_funcs = {
>> .dpms = drm_atomic_helper_connector_dpms,
>> .fill_modes = drm_helper_probe_single_connector_modes,
>> - .detect = exynos_dp_detect,
>> - .destroy = exynos_dp_connector_destroy,
>> + .detect = analogix_dp_detect,
>> + .destroy = analogix_dp_connector_destroy,
>> .reset = drm_atomic_helper_connector_reset,
>> .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> };
>>
>> -static int exynos_dp_get_modes(struct drm_connector *connector)
>> +static int analogix_dp_get_modes(struct drm_connector *connector)
>> {
>> - struct exynos_dp_device *dp = ctx_from_connector(connector);
>> - struct drm_display_mode *mode;
>> -
>> - if (dp->panel)
>> - return drm_panel_get_modes(dp->panel);
>> + struct analogix_dp_device *dp = connector_to_dp(connector);
>> + struct analogix_dp_plat_data *plat_data = dp->plat_data;
>> + int num_modes = 0;
>>
>> - mode = drm_mode_create(connector->dev);
>> - if (!mode) {
>> - DRM_ERROR("failed to create a new display mode.\n");
>> - return 0;
>> - }
>> -
>> - drm_display_mode_from_videomode(&dp->priv.vm, mode);
>> - mode->width_mm = dp->priv.width_mm;
>> - mode->height_mm = dp->priv.height_mm;
>> - connector->display_info.width_mm = mode->width_mm;
>> - connector->display_info.height_mm = mode->height_mm;
>> + if (plat_data && plat_data->panel)
>> + num_modes += drm_panel_get_modes(plat_data->panel);
>>
>> - mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>> - drm_mode_set_name(mode);
>> - drm_mode_probed_add(connector, mode);
>> + if (plat_data && plat_data->get_modes)
>> + num_modes += plat_data->get_modes(plat_data, connector);
>>
>> - return 1;
>> + return num_modes;
>> }
>>
>> static struct drm_encoder *
>> -exynos_dp_best_encoder(struct drm_connector *connector)
>> +analogix_dp_best_encoder(struct drm_connector *connector)
>> {
>> - struct exynos_dp_device *dp = ctx_from_connector(connector);
>> + struct analogix_dp_device *dp = connector_to_dp(connector);
>>
>> - return &dp->encoder;
>> + return dp->encoder;
>> }
>>
>> -static struct drm_connector_helper_funcs exynos_dp_connector_helper_funcs = {
>> - .get_modes = exynos_dp_get_modes,
>> - .best_encoder = exynos_dp_best_encoder,
>> +static struct drm_connector_helper_funcs analogix_dp_connector_helper_funcs = {
>> + .get_modes = analogix_dp_get_modes,
>> + .best_encoder = analogix_dp_best_encoder,
>> };
>>
>> -/* returns the number of bridges attached */
>> -static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
>> - struct drm_encoder *encoder)
>> -{
>> - int ret;
>> -
>> - encoder->bridge->next = dp->ptn_bridge;
>> - dp->ptn_bridge->encoder = encoder;
>> - ret = drm_bridge_attach(encoder->dev, dp->ptn_bridge);
>> - if (ret) {
>> - DRM_ERROR("Failed to attach bridge to drm\n");
>> - return ret;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> -static int exynos_dp_bridge_attach(struct drm_bridge *bridge)
>> +static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>> {
>> - struct exynos_dp_device *dp = bridge->driver_private;
>> - struct drm_encoder *encoder = &dp->encoder;
>> + struct analogix_dp_device *dp = bridge->driver_private;
>> + struct drm_encoder *encoder = dp->encoder;
>> struct drm_connector *connector = &dp->connector;
>> int ret;
>>
>> - /* Pre-empt DP connector creation if there's a bridge */
>> - if (dp->ptn_bridge) {
>> - ret = exynos_drm_attach_lcd_bridge(dp, encoder);
>> - if (!ret)
>> - return 0;
>> + if (!bridge->encoder) {
>> + DRM_ERROR("Parent encoder object not found");
>> + return -ENODEV;
>> }
>>
>> + encoder->bridge = bridge;
>> +
>> connector->polled = DRM_CONNECTOR_POLL_HPD;
>>
>> ret = drm_connector_init(dp->drm_dev, connector,
>> - &exynos_dp_connector_funcs,
>> + &analogix_dp_connector_funcs,
>> DRM_MODE_CONNECTOR_eDP);
>> if (ret) {
>> DRM_ERROR("Failed to initialize connector with drm\n");
>> return ret;
>> }
>>
>> - drm_connector_helper_add(connector, &exynos_dp_connector_helper_funcs);
>> + drm_connector_helper_add(connector,
>> + &analogix_dp_connector_helper_funcs);
>> drm_connector_register(connector);
>> drm_mode_connector_attach_encoder(connector, encoder);
>>
>> - if (dp->panel)
>> - ret = drm_panel_attach(dp->panel, &dp->connector);
>> + if (dp->plat_data && dp->plat_data->panel) {
>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>> + if (ret) {
>> + DRM_ERROR("Failed to attach panel\n");
>> + return ret;
>> + }
>> + }
>> +
>> + /*
>> + * This should be the end of attach function, caused
>> + * we should ensure dp bridge could attach first.
>> + */
>> + if (dp->plat_data && dp->plat_data->attach) {
>> + ret = dp->plat_data->attach(dp->plat_data, bridge);
>> + if (ret) {
>> + DRM_ERROR("Failed at platform attch func\n");
> Two new error paths appeared here and above. Don't you have to
> cleanup something? I don't know, just wondering...
Hmm... I think both panel & platform_attch need ERROR remind when
it failed. But if it still need clean, I though it is tge platform attch
error,
this is not relate to DRM directly, just analogix driver logic, so code
would
like.
- if (dp->panel)
- ret = drm_panel_attach(dp->panel, &dp->connector);
+ if (dp->plat_data && dp->plat_data->panel) {
+ ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
+ if (ret) {
+ DRM_ERROR("Failed to attach panel\n");
+ return ret;
+ }
+ }
+ /*
+ * This should be the end of attach function, caused
+ * we should ensure dp bridge could attach first.
+ */
+ if (dp->plat_data && dp->plat_data->attach) {
+ ret = dp->plat_data->attach(dp->plat_data, bridge);
return ret;
> (...)
>
>> @@ -1345,22 +1286,9 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
>> disable_irq(dp->irq);
>>
>> dp->drm_dev = drm_dev;
>> + dp->encoder = encoder;
>>
>> - pipe = exynos_drm_crtc_get_pipe_from_type(drm_dev,
>> - EXYNOS_DISPLAY_TYPE_LCD);
>> - if (pipe < 0)
>> - return pipe;
>> -
>> - encoder->possible_crtcs = 1 << pipe;
>> -
>> - DRM_DEBUG_KMS("possible_crtcs = 0x%x\n", encoder->possible_crtcs);
>> -
>> - drm_encoder_init(drm_dev, encoder, &exynos_dp_encoder_funcs,
>> - DRM_MODE_ENCODER_TMDS);
>> -
>> - drm_encoder_helper_add(encoder, &exynos_dp_encoder_helper_funcs);
>> -
>> - ret = exynos_dp_create_connector(encoder);
>> + ret = analogix_dp_create_bridge(drm_dev, dp);
>> if (ret) {
>> DRM_ERROR("failed to create connector ret = %d\n", ret);
>> drm_encoder_cleanup(encoder);
>> @@ -1369,103 +1297,36 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL_GPL(analogix_dp_bind);
> Ooo, a new export symbol. I suppose it is necessary for the split
> but the commit message does not mention exporting
> anything nor the reason for doing it. Could you extend the commit
> message describing more tha changes here and rationale for it?
Okay, got it.
> (...)
>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> new file mode 100644
>> index 0000000..b879d8c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> @@ -0,0 +1,1265 @@
>> +/*
>> + * Analogix DP (Display port) core register interface driver.
>> + *
>> + * Copyright (C) 2012 Samsung Electronics Co., Ltd.
>> + * Author: Jingoo Han <jg1.han@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or (at your
>> + * option) any later version.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio.h>
>> +
>> +#include "analogix_dp_core.h"
>> +#include "analogix_dp_reg.h"
>> +
>> +#define COMMON_INT_MASK_1 0
>> +#define COMMON_INT_MASK_2 0
>> +#define COMMON_INT_MASK_3 0
>> +#define COMMON_INT_MASK_4 (HOTPLUG_CHG | HPD_LOST | PLUG)
>> +#define INT_STA_MASK INT_HPD
>> +
>> +void analogix_dp_enable_video_mute(struct analogix_dp_device *dp, bool enable)
>> +{
>> + u32 reg;
>> +
>> + if (enable) {
>> + reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> + reg |= HDCP_VIDEO_MUTE;
>> + writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> + } else {
>> + reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> + reg &= ~HDCP_VIDEO_MUTE;
>> + writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> + }
>> +}
>> +
>> +void analogix_dp_stop_video(struct analogix_dp_device *dp)
>> +{
>> + u32 reg;
>> +
>> + reg = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> + reg &= ~VIDEO_EN;
>> + writel(reg, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_1);
>> +}
>> +
>> +void analogix_dp_lane_swap(struct analogix_dp_device *dp, bool enable)
>> +{
>> + u32 reg;
>> +
>> + if (enable)
>> + reg = LANE3_MAP_LOGIC_LANE_0 | LANE2_MAP_LOGIC_LANE_1 |
>> + LANE1_MAP_LOGIC_LANE_2 | LANE0_MAP_LOGIC_LANE_3;
>> + else
>> + reg = LANE3_MAP_LOGIC_LANE_3 | LANE2_MAP_LOGIC_LANE_2 |
>> + LANE1_MAP_LOGIC_LANE_1 | LANE0_MAP_LOGIC_LANE_0;
> Here and in few other places with new code: no alignment?
>
> You spent some time fixing these in patch 1, so I would assume
> that new code already comes after "checkpatch -f --subjective".
> That's actually the only valid reason for using checkpatch - new code.
> However my mail reader could corrupt the output so maybe this is aligned?
This is another mistaken I make, and surprise by that checkpatch
fine with this no-alignment style. Anyway I would just do rename
change in this split patch, and move the style change to another
separate patch (maybe I should adjust the order, correct style
problem after this patch).
> Code seems fine after looking at it but review was rather shallow.
> I don't know the Exynos DP driver.
I really appreciate for your careful review ;)
- Yakir
> Best regards,
> Krzysztof
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2015-09-04 02:50 +0200 |
| Subject | Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir |
| Message-ID | <q4Nmy-31s-7@gated-at.bofh.it> |
| In reply to | #1218046 |
On 03.09.2015 14:30, Yakir Yang wrote:
> Hi Krzysztof,
>
> 在 09/03/2015 08:58 AM, Krzysztof Kozlowski 写道:
>> On 01.09.2015 14:49, Yakir Yang wrote:
>>> Split the dp core driver from exynos directory to bridge
>>> directory, and rename the core driver to analogix_dp_*,
>>> leave the platform code to analogix_dp-exynos.
>>>
>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>> ---
>>> Changes in v4:
>>> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT
>>> propery.
>>> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to
>>> "exynos_dp.c"
>>> - Take Archit suggest, create a separate folder for analogix code in
>>> bridge/
>>>
>>> Changes in v3:
>>> - Take Thierry Reding suggest, move exynos's video_timing code
>>> to analogix_dp-exynos platform driver, add get_modes method
>>> to struct analogix_dp_plat_data.
>>> - Take Heiko suggest, rename some "samsung*" dts propery to "analogix*".
>>>
>>> Changes in v2:
>>> - Take Jingoo Han suggest, remove new copyright
>>> - Fix compiled failed dut to analogix_dp_device misspell
>>>
>>> drivers/gpu/drm/bridge/Kconfig | 2 +
>>> drivers/gpu/drm/bridge/Makefile | 1 +
>>> drivers/gpu/drm/bridge/analogix/Kconfig | 4 +
>>> drivers/gpu/drm/bridge/analogix/Makefile | 1 +
>>> .../analogix/analogix_dp_core.c} | 817
>>> ++++++-------
>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 283 +++++
>>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 1265
>>> ++++++++++++++++++++
>>> .../analogix/analogix_dp_reg.h} | 258 ++--
>>> drivers/gpu/drm/exynos/Kconfig | 3 +-
>>> drivers/gpu/drm/exynos/Makefile | 2 +-
>>> drivers/gpu/drm/exynos/exynos_dp.c | 306 +++++
>>> drivers/gpu/drm/exynos/exynos_dp_core.h | 282 -----
>>> drivers/gpu/drm/exynos/exynos_dp_reg.c | 1259
>>> -------------------
>>> include/drm/bridge/analogix_dp.h | 24 +
>>> 14 files changed, 2357 insertions(+), 2150 deletions(-)
>>> create mode 100644 drivers/gpu/drm/bridge/analogix/Kconfig
>>> create mode 100644 drivers/gpu/drm/bridge/analogix/Makefile
>>> rename drivers/gpu/drm/{exynos/exynos_dp_core.c =>
>>> bridge/analogix/analogix_dp_core.c} (50%)
>>> create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> create mode 100644 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> rename drivers/gpu/drm/{exynos/exynos_dp_reg.h =>
>>> bridge/analogix/analogix_dp_reg.h} (64%)
>>> create mode 100644 drivers/gpu/drm/exynos/exynos_dp.c
>>> delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_core.h
>>> delete mode 100644 drivers/gpu/drm/exynos/exynos_dp_reg.c
>>> create mode 100644 include/drm/bridge/analogix_dp.h
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig
>>> b/drivers/gpu/drm/bridge/Kconfig
>>> index 2de52a5..7b5b77a 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -29,4 +29,6 @@ config DRM_PARADE_PS8622
>>> ---help---
>>> Parade eDP-LVDS bridge chip driver.
>>> +source "drivers/gpu/drm/bridge/analogix/Kconfig"
>>> +
>>> endmenu
>>> diff --git a/drivers/gpu/drm/bridge/Makefile
>>> b/drivers/gpu/drm/bridge/Makefile
>>> index e2eef1c..5366c6b 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -3,3 +3,4 @@ ccflags-y := -Iinclude/drm
>>> obj-$(CONFIG_DRM_DW_HDMI) += dw_hdmi.o
>>> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>>> obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig
>>> b/drivers/gpu/drm/bridge/analogix/Kconfig
>>> new file mode 100644
>>> index 0000000..5ff6551
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
>>> @@ -0,0 +1,4 @@
>>> +config DRM_ANALOGIX_DP
>>> + tristate
>>> + depends on DRM
>>> + select DRM_KMS_HELPER
>>> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile
>>> b/drivers/gpu/drm/bridge/analogix/Makefile
>>> new file mode 100644
>>> index 0000000..9107b86
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp_core.o analogix_dp_reg.o
>>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> similarity index 50%
>>> rename from drivers/gpu/drm/exynos/exynos_dp_core.c
>>> rename to drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index bed0252..7d62f22 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -1,14 +1,14 @@
>>> /*
>>> - * Samsung SoC DP (Display Port) interface driver.
>>> - *
>>> - * Copyright (C) 2012 Samsung Electronics Co., Ltd.
>>> - * Author: Jingoo Han <jg1.han@samsung.com>
>>> - *
>>> - * This program is free software; you can redistribute it and/or
>>> modify it
>>> - * under the terms of the GNU General Public License as published by
>>> the
>>> - * Free Software Foundation; either version 2 of the License, or (at
>>> your
>>> - * option) any later version.
>>> - */
>>> +* Analogix DP (Display Port) core interface driver.
>>> +*
>>> +* Copyright (C) 2012 Samsung Electronics Co., Ltd.
>>> +* Author: Jingoo Han <jg1.han@samsung.com>
>>> +*
>>> +* This program is free software; you can redistribute it and/or
>>> modify it
>>> +* under the terms of the GNU General Public License as published by the
>>> +* Free Software Foundation; either version 2 of the License, or (at
>>> your
>>> +* option) any later version.
>>> +*/
>>> #include <linux/module.h>
>>> #include <linux/platform_device.h>
>>> @@ -18,12 +18,9 @@
>>> #include <linux/interrupt.h>
>>> #include <linux/of.h>
>>> #include <linux/of_gpio.h>
>>> -#include <linux/of_graph.h>
>>> #include <linux/gpio.h>
>>> #include <linux/component.h>
>>> #include <linux/phy/phy.h>
>>> -#include <video/of_display_timing.h>
>>> -#include <video/of_videomode.h>
>>> #include <drm/drmP.h>
>>> #include <drm/drm_crtc.h>
>>> @@ -31,52 +28,42 @@
>>> #include <drm/drm_atomic_helper.h>
>>> #include <drm/drm_panel.h>
>>> -#include "exynos_dp_core.h"
>>> -#include "exynos_drm_crtc.h"
>>> +#include <drm/bridge/analogix_dp.h>
>>> -#define ctx_from_connector(c) container_of(c, struct
>>> exynos_dp_device, \
>>> - connector)
>>> +#include "analogix_dp_core.h"
>>> -static inline struct exynos_drm_crtc *dp_to_crtc(struct
>>> exynos_dp_device *dp)
>>> -{
>>> - return to_exynos_crtc(dp->encoder.crtc);
>>> -}
>>> -
>>> -static inline struct exynos_dp_device *encoder_to_dp(
>>> - struct drm_encoder *e)
>>> -{
>>> - return container_of(e, struct exynos_dp_device, encoder);
>>> -}
>>> +#define connector_to_dp(c) \
>>> + container_of(c, struct analogix_dp_device, connector)
>>> struct bridge_init {
>>> struct i2c_client *client;
>>> struct device_node *node;
>>> };
>>> -static void exynos_dp_init_dp(struct exynos_dp_device *dp)
>>> +static void analogix_dp_init_dp(struct analogix_dp_device *dp)
>>> {
>>> - exynos_dp_reset(dp);
>>> + analogix_dp_reset(dp);
>>> - exynos_dp_swreset(dp);
>>> + analogix_dp_swreset(dp);
>>> - exynos_dp_init_analog_param(dp);
>>> - exynos_dp_init_interrupt(dp);
>>> + analogix_dp_init_analog_param(dp);
>>> + analogix_dp_init_interrupt(dp);
>>> /* SW defined function Normal operation */
>>> - exynos_dp_enable_sw_function(dp);
>>> + analogix_dp_enable_sw_function(dp);
>>> - exynos_dp_config_interrupt(dp);
>>> - exynos_dp_init_analog_func(dp);
>>> + analogix_dp_config_interrupt(dp);
>>> + analogix_dp_init_analog_func(dp);
>>> - exynos_dp_init_hpd(dp);
>>> - exynos_dp_init_aux(dp);
>>> + analogix_dp_init_hpd(dp);
>>> + analogix_dp_init_aux(dp);
>>> }
>>> -static int exynos_dp_detect_hpd(struct exynos_dp_device *dp)
>>> +static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>>> {
>>> int timeout_loop = 0;
>>> - while (exynos_dp_get_plug_in_status(dp) != 0) {
>>> + while (analogix_dp_get_plug_in_status(dp) != 0) {
>>> timeout_loop++;
>>> if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
>>> dev_err(dp->dev, "failed to get hpd plug status\n");
>>> @@ -88,7 +75,7 @@ static int exynos_dp_detect_hpd(struct
>>> exynos_dp_device *dp)
>>> return 0;
>>> }
>>> -static unsigned char exynos_dp_calc_edid_check_sum(unsigned char
>>> *edid_data)
>>> +static unsigned char analogix_dp_calc_edid_check_sum(unsigned char
>>> *edid_data)
>>> {
>>> int i;
>>> unsigned char sum = 0;
>>> @@ -99,7 +86,7 @@ static unsigned char
>>> exynos_dp_calc_edid_check_sum(unsigned char *edid_data)
>>> return sum;
>>> }
>>> -static int exynos_dp_read_edid(struct exynos_dp_device *dp)
>>> +static int analogix_dp_read_edid(struct analogix_dp_device *dp)
>>> {
>>> unsigned char edid[EDID_BLOCK_LENGTH * 2];
>>> unsigned int extend_block = 0;
>>> @@ -114,9 +101,9 @@ static int exynos_dp_read_edid(struct
>>> exynos_dp_device *dp)
>>> */
>>> /* Read Extension Flag, Number of 128-byte EDID extension
>>> blocks */
>>> - retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>>> - EDID_EXTENSION_FLAG,
>>> - &extend_block);
>>> + retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>>> + EDID_EXTENSION_FLAG,
>>> + &extend_block);
>> Hi,
>>
>> What was the point of patch 1 (checkpatch -f --subjective) if you
>> have to re-align the parameters again? It's meaningless.
>>
>> I could understand that idea if this patch touched only first
>> line - name of the function. This would result in small
>> and effective diff, like:
>>
>> - retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>> + retval = SAME_LEGNTH____byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
>> EDID_EXTENSION_FLAG,
>>
>> But this is not the case. The reviewer have to look at 6 lines
>> of changes instead of two. You fixed the alignment just for two
>> patches and then you re-written it. It's meaningless.
>
> Yes, this is the place that crazy me too. After rename "exynos"
> to "analogix", there would bring some "over-80-char" problem :(
If you can't avoid it then it's okay. When you rename the function this
could happen and then you just re-align the block.
>
>> Nope. Please drop all changes from patch 1 which:
>> (all conditions apply):
>> 1. change alignment,
>> 2. are immediately changed in next patches (e.g. like here),
>>
>> and fix the alignment while renaming the function.
>>
>> This is actually another point for not accepting commits where
>> the reason is "checkpatch told me".
>
> Okay, so there would be two patches to fix the alignment, done.
>
> Thanks for your careful remind.
Two patches but fixing alignment in different places. Do not fix the
alignment in 1/16 and then re-align the same line at 3/16.
>
>>
>>
>>> if (retval)
>>> return retval;
>>> @@ -124,7 +111,7 @@ static int exynos_dp_read_edid(struct
>>> exynos_dp_device *dp)
>>> dev_dbg(dp->dev, "EDID data includes a single extension!\n");
>>> /* Read EDID data */
>>> - retval = exynos_dp_read_bytes_from_i2c(
>>> + retval = analogix_dp_read_bytes_from_i2c(
>>> dp, I2C_EDID_DEVICE_ADDR,
>>> EDID_HEADER_PATTERN,
>>> EDID_BLOCK_LENGTH,
>>> @@ -133,14 +120,14 @@ static int exynos_dp_read_edid(struct
>>> exynos_dp_device *dp)
>> (...)
>>
>>> @@ -871,218 +862,204 @@ static irqreturn_t exynos_dp_irq_handler(int
>>> irq, void *arg)
>>> return IRQ_HANDLED;
>>> }
>>> -static void exynos_dp_hotplug(struct work_struct *work)
>>> +static void analogix_dp_hotplug(struct work_struct *work)
>>> {
>>> - struct exynos_dp_device *dp;
>>> + struct analogix_dp_device *dp;
>>> - dp = container_of(work, struct exynos_dp_device, hotplug_work);
>>> + dp = container_of(work, struct analogix_dp_device, hotplug_work);
>>> if (dp->drm_dev)
>>> drm_helper_hpd_irq_event(dp->drm_dev);
>>> }
>>> -static void exynos_dp_commit(struct drm_encoder *encoder)
>>> +static void analogix_dp_commit(struct analogix_dp_device *dp)
>>> {
>>> - struct exynos_dp_device *dp = encoder_to_dp(encoder);
>>> int ret;
>>> /* Keep the panel disabled while we configure video */
>>> - if (dp->panel) {
>>> - if (drm_panel_disable(dp->panel))
>>> + if (dp->plat_data && dp->plat_data->panel) {
>>> + if (drm_panel_disable(dp->plat_data->panel))
>>> DRM_ERROR("failed to disable the panel\n");
>>> }
>>> - ret = exynos_dp_detect_hpd(dp);
>>> + ret = analogix_dp_detect_hpd(dp);
>>> if (ret) {
>>> /* Cable has been disconnected, we're done */
>>> return;
>>> }
>>> - ret = exynos_dp_handle_edid(dp);
>>> + ret = analogix_dp_handle_edid(dp);
>>> if (ret) {
>>> dev_err(dp->dev, "unable to handle edid\n");
>>> return;
>>> }
>>> - ret = exynos_dp_set_link_train(dp, dp->video_info->lane_count,
>>> - dp->video_info->link_rate);
>>> + ret = analogix_dp_set_link_train(dp, dp->video_info->lane_count,
>>> + dp->video_info->link_rate);
>>> if (ret) {
>>> dev_err(dp->dev, "unable to do link train\n");
>>> return;
>>> }
>>> - exynos_dp_enable_scramble(dp, 1);
>>> - exynos_dp_enable_rx_to_enhanced_mode(dp, 1);
>>> - exynos_dp_enable_enhanced_mode(dp, 1);
>>> + analogix_dp_enable_scramble(dp, 1);
>>> + analogix_dp_enable_rx_to_enhanced_mode(dp, 1);
>>> + analogix_dp_enable_enhanced_mode(dp, 1);
>>> - exynos_dp_set_lane_count(dp, dp->video_info->lane_count);
>>> - exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
>>> + analogix_dp_set_lane_count(dp, dp->video_info->lane_count);
>>> + analogix_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
>>> - exynos_dp_init_video(dp);
>>> - ret = exynos_dp_config_video(dp);
>>> + analogix_dp_init_video(dp);
>>> + ret = analogix_dp_config_video(dp);
>>> if (ret)
>>> dev_err(dp->dev, "unable to config video\n");
>>> /* Safe to enable the panel now */
>>> - if (dp->panel) {
>>> - if (drm_panel_enable(dp->panel))
>>> + if (dp->plat_data && dp->plat_data->panel) {
>>> + if (drm_panel_enable(dp->plat_data->panel))
>>> DRM_ERROR("failed to enable the panel\n");
>>> }
>>> /* Enable video */
>>> - exynos_dp_start_video(dp);
>>> + analogix_dp_start_video(dp);
>>> }
>>> static enum drm_connector_status
>>> -exynos_dp_detect(struct drm_connector *connector, bool force)
>>> +analogix_dp_detect(struct drm_connector *connector, bool force)
>>> {
>>> return connector_status_connected;
>>> }
>>> -static void exynos_dp_connector_destroy(struct drm_connector
>>> *connector)
>>> +static void analogix_dp_connector_destroy(struct drm_connector
>>> *connector)
>>> {
>>> drm_connector_unregister(connector);
>>> drm_connector_cleanup(connector);
>>> }
>>> -static struct drm_connector_funcs exynos_dp_connector_funcs = {
>>> +static struct drm_connector_funcs analogix_dp_connector_funcs = {
>>> .dpms = drm_atomic_helper_connector_dpms,
>>> .fill_modes = drm_helper_probe_single_connector_modes,
>>> - .detect = exynos_dp_detect,
>>> - .destroy = exynos_dp_connector_destroy,
>>> + .detect = analogix_dp_detect,
>>> + .destroy = analogix_dp_connector_destroy,
>>> .reset = drm_atomic_helper_connector_reset,
>>> .atomic_duplicate_state =
>>> drm_atomic_helper_connector_duplicate_state,
>>> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>> };
>>> -static int exynos_dp_get_modes(struct drm_connector *connector)
>>> +static int analogix_dp_get_modes(struct drm_connector *connector)
>>> {
>>> - struct exynos_dp_device *dp = ctx_from_connector(connector);
>>> - struct drm_display_mode *mode;
>>> -
>>> - if (dp->panel)
>>> - return drm_panel_get_modes(dp->panel);
>>> + struct analogix_dp_device *dp = connector_to_dp(connector);
>>> + struct analogix_dp_plat_data *plat_data = dp->plat_data;
>>> + int num_modes = 0;
>>> - mode = drm_mode_create(connector->dev);
>>> - if (!mode) {
>>> - DRM_ERROR("failed to create a new display mode.\n");
>>> - return 0;
>>> - }
>>> -
>>> - drm_display_mode_from_videomode(&dp->priv.vm, mode);
>>> - mode->width_mm = dp->priv.width_mm;
>>> - mode->height_mm = dp->priv.height_mm;
>>> - connector->display_info.width_mm = mode->width_mm;
>>> - connector->display_info.height_mm = mode->height_mm;
>>> + if (plat_data && plat_data->panel)
>>> + num_modes += drm_panel_get_modes(plat_data->panel);
>>> - mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>>> - drm_mode_set_name(mode);
>>> - drm_mode_probed_add(connector, mode);
>>> + if (plat_data && plat_data->get_modes)
>>> + num_modes += plat_data->get_modes(plat_data, connector);
>>> - return 1;
>>> + return num_modes;
>>> }
>>> static struct drm_encoder *
>>> -exynos_dp_best_encoder(struct drm_connector *connector)
>>> +analogix_dp_best_encoder(struct drm_connector *connector)
>>> {
>>> - struct exynos_dp_device *dp = ctx_from_connector(connector);
>>> + struct analogix_dp_device *dp = connector_to_dp(connector);
>>> - return &dp->encoder;
>>> + return dp->encoder;
>>> }
>>> -static struct drm_connector_helper_funcs
>>> exynos_dp_connector_helper_funcs = {
>>> - .get_modes = exynos_dp_get_modes,
>>> - .best_encoder = exynos_dp_best_encoder,
>>> +static struct drm_connector_helper_funcs
>>> analogix_dp_connector_helper_funcs = {
>>> + .get_modes = analogix_dp_get_modes,
>>> + .best_encoder = analogix_dp_best_encoder,
>>> };
>>> -/* returns the number of bridges attached */
>>> -static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
>>> - struct drm_encoder *encoder)
>>> -{
>>> - int ret;
>>> -
>>> - encoder->bridge->next = dp->ptn_bridge;
>>> - dp->ptn_bridge->encoder = encoder;
>>> - ret = drm_bridge_attach(encoder->dev, dp->ptn_bridge);
>>> - if (ret) {
>>> - DRM_ERROR("Failed to attach bridge to drm\n");
>>> - return ret;
>>> - }
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static int exynos_dp_bridge_attach(struct drm_bridge *bridge)
>>> +static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>>> {
>>> - struct exynos_dp_device *dp = bridge->driver_private;
>>> - struct drm_encoder *encoder = &dp->encoder;
>>> + struct analogix_dp_device *dp = bridge->driver_private;
>>> + struct drm_encoder *encoder = dp->encoder;
>>> struct drm_connector *connector = &dp->connector;
>>> int ret;
>>> - /* Pre-empt DP connector creation if there's a bridge */
>>> - if (dp->ptn_bridge) {
>>> - ret = exynos_drm_attach_lcd_bridge(dp, encoder);
>>> - if (!ret)
>>> - return 0;
>>> + if (!bridge->encoder) {
>>> + DRM_ERROR("Parent encoder object not found");
>>> + return -ENODEV;
>>> }
>>> + encoder->bridge = bridge;
>>> +
>>> connector->polled = DRM_CONNECTOR_POLL_HPD;
>>> ret = drm_connector_init(dp->drm_dev, connector,
>>> - &exynos_dp_connector_funcs,
>>> + &analogix_dp_connector_funcs,
>>> DRM_MODE_CONNECTOR_eDP);
>>> if (ret) {
>>> DRM_ERROR("Failed to initialize connector with drm\n");
>>> return ret;
>>> }
>>> - drm_connector_helper_add(connector,
>>> &exynos_dp_connector_helper_funcs);
>>> + drm_connector_helper_add(connector,
>>> + &analogix_dp_connector_helper_funcs);
>>> drm_connector_register(connector);
>>> drm_mode_connector_attach_encoder(connector, encoder);
>>> - if (dp->panel)
>>> - ret = drm_panel_attach(dp->panel, &dp->connector);
>>> + if (dp->plat_data && dp->plat_data->panel) {
>>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>>> + if (ret) {
>>> + DRM_ERROR("Failed to attach panel\n");
>>> + return ret;
>>> + }
>>> + }
>>> +
>>> + /*
>>> + * This should be the end of attach function, caused
>>> + * we should ensure dp bridge could attach first.
>>> + */
>>> + if (dp->plat_data && dp->plat_data->attach) {
>>> + ret = dp->plat_data->attach(dp->plat_data, bridge);
>>> + if (ret) {
>>> + DRM_ERROR("Failed at platform attch func\n");
>> Two new error paths appeared here and above. Don't you have to
>> cleanup something? I don't know, just wondering...
>
> Hmm... I think both panel & platform_attch need ERROR remind when
> it failed. But if it still need clean, I though it is tge platform attch
> error,
> this is not relate to DRM directly, just analogix driver logic, so code
> would
> like.
>
> - if (dp->panel)
> - ret = drm_panel_attach(dp->panel, &dp->connector);
> + if (dp->plat_data && dp->plat_data->panel) {
> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
> + if (ret) {
> + DRM_ERROR("Failed to attach panel\n");
> + return ret;
> + }
> + }
>
> + /*
> + * This should be the end of attach function, caused
> + * we should ensure dp bridge could attach first.
> + */
> + if (dp->plat_data && dp->plat_data->attach) {
> + ret = dp->plat_data->attach(dp->plat_data, bridge);
>
> return ret;
I am lost... the code looks the same. What did you change?
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Yakir Yang <ykk@rock-chips.com> |
|---|---|
| Date | 2015-09-06 10:00 +0200 |
| Subject | Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir |
| Message-ID | <q5D1L-YF-7@gated-at.bofh.it> |
| In reply to | #1218626 |
Hi Krzysztof,
在 09/04/2015 08:41 AM, Krzysztof Kozlowski 写道:
> On 03.09.2015 14:30, Yakir Yang wrote:
>> Hi Krzysztof,
>>
>> 在 09/03/2015 08:58 AM, Krzysztof Kozlowski 写道:
>>> On 01.09.2015 14:49, Yakir Yang wrote:
>>>> Split the dp core driver from exynos directory to bridge
>>>> directory, and rename the core driver to analogix_dp_*,
>>>> leave the platform code to analogix_dp-exynos.
>>>>
>>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>>> ---
>>>> Changes in v4:
>>>> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT
>>>> propery.
>>>> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to
>>>> "exynos_dp.c"
>>>> - Take Archit suggest, create a separate folder for analogix code in
>>>> bridge/
>>>>
>>>> Changes in v3:
>>>> - Take Thierry Reding suggest, move exynos's video_timing code
>>>> to analogix_dp-exynos platform driver, add get_modes method
>>>> to struct analogix_dp_plat_data.
>>>> - Take Heiko suggest, rename some "samsung*" dts propery to "analogix*".
>>>>
>>>> Changes in v2:
>>>> - Take Jingoo Han suggest, remove new copyright
>>>> - Fix compiled failed dut to analogix_dp_device misspell
[.....]
>>>> -static int exynos_dp_bridge_attach(struct drm_bridge *bridge)
>>>> +static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>>>> {
>>>> - struct exynos_dp_device *dp = bridge->driver_private;
>>>> - struct drm_encoder *encoder = &dp->encoder;
>>>> + struct analogix_dp_device *dp = bridge->driver_private;
>>>> + struct drm_encoder *encoder = dp->encoder;
>>>> struct drm_connector *connector = &dp->connector;
>>>> int ret;
>>>> - /* Pre-empt DP connector creation if there's a bridge */
>>>> - if (dp->ptn_bridge) {
>>>> - ret = exynos_drm_attach_lcd_bridge(dp, encoder);
>>>> - if (!ret)
>>>> - return 0;
>>>> + if (!bridge->encoder) {
>>>> + DRM_ERROR("Parent encoder object not found");
>>>> + return -ENODEV;
>>>> }
>>>> + encoder->bridge = bridge;
>>>> +
>>>> connector->polled = DRM_CONNECTOR_POLL_HPD;
>>>> ret = drm_connector_init(dp->drm_dev, connector,
>>>> - &exynos_dp_connector_funcs,
>>>> + &analogix_dp_connector_funcs,
>>>> DRM_MODE_CONNECTOR_eDP);
>>>> if (ret) {
>>>> DRM_ERROR("Failed to initialize connector with drm\n");
>>>> return ret;
>>>> }
>>>> - drm_connector_helper_add(connector,
>>>> &exynos_dp_connector_helper_funcs);
>>>> + drm_connector_helper_add(connector,
>>>> + &analogix_dp_connector_helper_funcs);
>>>> drm_connector_register(connector);
>>>> drm_mode_connector_attach_encoder(connector, encoder);
>>>> - if (dp->panel)
>>>> - ret = drm_panel_attach(dp->panel, &dp->connector);
>>>> + if (dp->plat_data && dp->plat_data->panel) {
>>>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>>>> + if (ret) {
>>>> + DRM_ERROR("Failed to attach panel\n");
>>>> + return ret;
>>>> + }
>>>> + }
>>>> +
>>>> + /*
>>>> + * This should be the end of attach function, caused
>>>> + * we should ensure dp bridge could attach first.
>>>> + */
>>>> + if (dp->plat_data && dp->plat_data->attach) {
>>>> + ret = dp->plat_data->attach(dp->plat_data, bridge);
>>>> + if (ret) {
>>>> + DRM_ERROR("Failed at platform attch func\n");
>>> Two new error paths appeared here and above. Don't you have to
>>> cleanup something? I don't know, just wondering...
>> Hmm... I think both panel & platform_attch need ERROR remind when
>> it failed. But if it still need clean, I though it should clean the platform attch
>> error,
>> this is not relate to DRM directly, just analogix driver logic, so code would like,
>>
>> - if (dp->panel)
>> - ret = drm_panel_attach(dp->panel, &dp->connector);
>> + if (dp->plat_data && dp->plat_data->panel) {
>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>> + if (ret) {
>> + DRM_ERROR("Failed to attach panel\n");
>> + return ret;
>> + }
>> + }
>>
>> + /*
>> + * This should be the end of attach function, caused
>> + * we should ensure dp bridge could attach first.
>> + */
>> + if (dp->plat_data && dp->plat_data->attach) {
>> + ret = dp->plat_data->attach(dp->plat_data, bridge);
>>
>> return ret;
> I am lost... the code looks the same. What did you change?
I just remove the DRM_ERROR after dp->plat_data->attach(),
maybe I should paste the change that rebase on this patch,
here are they,
/*
* This should be the end of attach function, caused
* we should ensure dp bridge could attach first.
*/
- if (dp->plat_data && dp->plat_data->attach) {
+ if (dp->plat_data && dp->plat_data->attach)
ret = dp->plat_data->attach(dp->plat_data, bridge);
- if (ret) {
- DRM_ERROR("Failed at platform attch func\n");
- return ret;
- }
- }
- return 0;
+ return ret;
If this haven't meet your comment, I maybe start to think that
your comment "Two new error paths appeared here and above"
indicated that those two function is the same.
"dp->plat_data->attach(dp->plat_data, bridge); "
"drm_panel_attach(dp->plat_data->panel, &dp->connector); "
Thanks,
- Yakir
> Best regards,
> Krzysztof
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2015-09-07 02:30 +0200 |
| Subject | Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir |
| Message-ID | <q5StQ-6dW-7@gated-at.bofh.it> |
| In reply to | #1219771 |
On 06.09.2015 16:49, Yakir Yang wrote:
> Hi Krzysztof,
>
> 在 09/04/2015 08:41 AM, Krzysztof Kozlowski 写道:
>> On 03.09.2015 14:30, Yakir Yang wrote:
>>> Hi Krzysztof,
>>>
>>> 在 09/03/2015 08:58 AM, Krzysztof Kozlowski 写道:
>>>> On 01.09.2015 14:49, Yakir Yang wrote:
>>>>> Split the dp core driver from exynos directory to bridge
>>>>> directory, and rename the core driver to analogix_dp_*,
>>>>> leave the platform code to analogix_dp-exynos.
>>>>>
>>>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>>>> ---
>>>>> Changes in v4:
>>>>> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT
>>>>> propery.
>>>>> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to
>>>>> "exynos_dp.c"
>>>>> - Take Archit suggest, create a separate folder for analogix code in
>>>>> bridge/
>>>>>
>>>>> Changes in v3:
>>>>> - Take Thierry Reding suggest, move exynos's video_timing code
>>>>> to analogix_dp-exynos platform driver, add get_modes method
>>>>> to struct analogix_dp_plat_data.
>>>>> - Take Heiko suggest, rename some "samsung*" dts propery to
>>>>> "analogix*".
>>>>>
>>>>> Changes in v2:
>>>>> - Take Jingoo Han suggest, remove new copyright
>>>>> - Fix compiled failed dut to analogix_dp_device misspell
>
> [.....]
>
>>>>> -static int exynos_dp_bridge_attach(struct drm_bridge *bridge)
>>>>> +static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>>>>> {
>>>>> - struct exynos_dp_device *dp = bridge->driver_private;
>>>>> - struct drm_encoder *encoder = &dp->encoder;
>>>>> + struct analogix_dp_device *dp = bridge->driver_private;
>>>>> + struct drm_encoder *encoder = dp->encoder;
>>>>> struct drm_connector *connector = &dp->connector;
>>>>> int ret;
>>>>> - /* Pre-empt DP connector creation if there's a bridge */
>>>>> - if (dp->ptn_bridge) {
>>>>> - ret = exynos_drm_attach_lcd_bridge(dp, encoder);
>>>>> - if (!ret)
>>>>> - return 0;
>>>>> + if (!bridge->encoder) {
>>>>> + DRM_ERROR("Parent encoder object not found");
>>>>> + return -ENODEV;
>>>>> }
>>>>> + encoder->bridge = bridge;
>>>>> +
>>>>> connector->polled = DRM_CONNECTOR_POLL_HPD;
>>>>> ret = drm_connector_init(dp->drm_dev, connector,
>>>>> - &exynos_dp_connector_funcs,
>>>>> + &analogix_dp_connector_funcs,
>>>>> DRM_MODE_CONNECTOR_eDP);
>>>>> if (ret) {
>>>>> DRM_ERROR("Failed to initialize connector with drm\n");
>>>>> return ret;
>>>>> }
>>>>> - drm_connector_helper_add(connector,
>>>>> &exynos_dp_connector_helper_funcs);
>>>>> + drm_connector_helper_add(connector,
>>>>> + &analogix_dp_connector_helper_funcs);
>>>>> drm_connector_register(connector);
>>>>> drm_mode_connector_attach_encoder(connector, encoder);
>>>>> - if (dp->panel)
>>>>> - ret = drm_panel_attach(dp->panel, &dp->connector);
>>>>> + if (dp->plat_data && dp->plat_data->panel) {
>>>>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>>>>> + if (ret) {
>>>>> + DRM_ERROR("Failed to attach panel\n");
>>>>> + return ret;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + /*
>>>>> + * This should be the end of attach function, caused
>>>>> + * we should ensure dp bridge could attach first.
>>>>> + */
>>>>> + if (dp->plat_data && dp->plat_data->attach) {
>>>>> + ret = dp->plat_data->attach(dp->plat_data, bridge);
>>>>> + if (ret) {
>>>>> + DRM_ERROR("Failed at platform attch func\n");
>>>> Two new error paths appeared here and above. Don't you have to
>>>> cleanup something? I don't know, just wondering...
>>> Hmm... I think both panel & platform_attch need ERROR remind when
>>> it failed. But if it still need clean, I though it should clean the
>>> platform attch
>>> error,
>>> this is not relate to DRM directly, just analogix driver logic, so
>>> code would like,
>>>
>>> - if (dp->panel)
>>> - ret = drm_panel_attach(dp->panel, &dp->connector);
>>> + if (dp->plat_data && dp->plat_data->panel) {
>>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>>> + if (ret) {
>>> + DRM_ERROR("Failed to attach panel\n");
>>> + return ret;
>>> + }
>>> + }
>>>
>>> + /*
>>> + * This should be the end of attach function, caused
>>> + * we should ensure dp bridge could attach first.
>>> + */
>>> + if (dp->plat_data && dp->plat_data->attach) {
>>> + ret = dp->plat_data->attach(dp->plat_data, bridge);
>>>
>>> return ret;
>> I am lost... the code looks the same. What did you change?
>
> I just remove the DRM_ERROR after dp->plat_data->attach(),
> maybe I should paste the change that rebase on this patch,
> here are they,
>
> /*
> * This should be the end of attach function, caused
> * we should ensure dp bridge could attach first.
> */
> - if (dp->plat_data && dp->plat_data->attach) {
> + if (dp->plat_data && dp->plat_data->attach)
> ret = dp->plat_data->attach(dp->plat_data, bridge);
> - if (ret) {
> - DRM_ERROR("Failed at platform attch func\n");
> - return ret;
> - }
> - }
>
> - return 0;
> + return ret;
>
>
> If this haven't meet your comment, I maybe start to think that
> your comment "Two new error paths appeared here and above"
> indicated that those two function is the same.
> "dp->plat_data->attach(dp->plat_data, bridge); "
> "drm_panel_attach(dp->plat_data->panel, &dp->connector); "
I wasn't talking about error message but rather about possible need of
clean up in error path. Previously there was only drm_panel_attach().
Now you have two of them (drm_panel_attach() and
dp->plat_data->attach()). If the second fails don't you have to clean up
before exit? I don't know, just asking.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Yakir Yang <ykk@rock-chips.com> |
|---|---|
| Date | 2015-09-07 04:30 +0200 |
| Subject | Re: [PATCH v4 03/16] drm: bridge: analogix/dp: split exynos dp driver to bridge dir |
| Message-ID | <q5UlY-wp-3@gated-at.bofh.it> |
| In reply to | #1219883 |
Hi Krzysztof,
在 09/07/2015 08:22 AM, Krzysztof Kozlowski 写道:
> On 06.09.2015 16:49, Yakir Yang wrote:
>> Hi Krzysztof,
>>
>> 在 09/04/2015 08:41 AM, Krzysztof Kozlowski 写道:
>>> On 03.09.2015 14:30, Yakir Yang wrote:
>>>> Hi Krzysztof,
>>>>
>>>> 在 09/03/2015 08:58 AM, Krzysztof Kozlowski 写道:
>>>>> On 01.09.2015 14:49, Yakir Yang wrote:
>>>>>> Split the dp core driver from exynos directory to bridge
>>>>>> directory, and rename the core driver to analogix_dp_*,
>>>>>> leave the platform code to analogix_dp-exynos.
>>>>>>
>>>>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>>>>> ---
>>>>>> Changes in v4:
>>>>>> - Take Rob suggest, update "analogix,hpd-gpios" to "hpd-gpios" DT
>>>>>> propery.
>>>>>> - Take Jingoo suggest, rename "analogix_dp-exynos.c" file name to
>>>>>> "exynos_dp.c"
>>>>>> - Take Archit suggest, create a separate folder for analogix code in
>>>>>> bridge/
>>>>>>
>>>>>> Changes in v3:
>>>>>> - Take Thierry Reding suggest, move exynos's video_timing code
>>>>>> to analogix_dp-exynos platform driver, add get_modes method
>>>>>> to struct analogix_dp_plat_data.
>>>>>> - Take Heiko suggest, rename some "samsung*" dts propery to
>>>>>> "analogix*".
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Take Jingoo Han suggest, remove new copyright
>>>>>> - Fix compiled failed dut to analogix_dp_device misspell
>> [.....]
>>
>>>>>> -static int exynos_dp_bridge_attach(struct drm_bridge *bridge)
>>>>>> +static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
>>>>>> {
>>>>>> - struct exynos_dp_device *dp = bridge->driver_private;
>>>>>> - struct drm_encoder *encoder = &dp->encoder;
>>>>>> + struct analogix_dp_device *dp = bridge->driver_private;
>>>>>> + struct drm_encoder *encoder = dp->encoder;
>>>>>> struct drm_connector *connector = &dp->connector;
>>>>>> int ret;
>>>>>> - /* Pre-empt DP connector creation if there's a bridge */
>>>>>> - if (dp->ptn_bridge) {
>>>>>> - ret = exynos_drm_attach_lcd_bridge(dp, encoder);
>>>>>> - if (!ret)
>>>>>> - return 0;
>>>>>> + if (!bridge->encoder) {
>>>>>> + DRM_ERROR("Parent encoder object not found");
>>>>>> + return -ENODEV;
>>>>>> }
>>>>>> + encoder->bridge = bridge;
>>>>>> +
>>>>>> connector->polled = DRM_CONNECTOR_POLL_HPD;
>>>>>> ret = drm_connector_init(dp->drm_dev, connector,
>>>>>> - &exynos_dp_connector_funcs,
>>>>>> + &analogix_dp_connector_funcs,
>>>>>> DRM_MODE_CONNECTOR_eDP);
>>>>>> if (ret) {
>>>>>> DRM_ERROR("Failed to initialize connector with drm\n");
>>>>>> return ret;
>>>>>> }
>>>>>> - drm_connector_helper_add(connector,
>>>>>> &exynos_dp_connector_helper_funcs);
>>>>>> + drm_connector_helper_add(connector,
>>>>>> + &analogix_dp_connector_helper_funcs);
>>>>>> drm_connector_register(connector);
>>>>>> drm_mode_connector_attach_encoder(connector, encoder);
>>>>>> - if (dp->panel)
>>>>>> - ret = drm_panel_attach(dp->panel, &dp->connector);
>>>>>> + if (dp->plat_data && dp->plat_data->panel) {
>>>>>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>>>>>> + if (ret) {
>>>>>> + DRM_ERROR("Failed to attach panel\n");
>>>>>> + return ret;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + /*
>>>>>> + * This should be the end of attach function, caused
>>>>>> + * we should ensure dp bridge could attach first.
>>>>>> + */
>>>>>> + if (dp->plat_data && dp->plat_data->attach) {
>>>>>> + ret = dp->plat_data->attach(dp->plat_data, bridge);
>>>>>> + if (ret) {
>>>>>> + DRM_ERROR("Failed at platform attch func\n");
>>>>> Two new error paths appeared here and above. Don't you have to
>>>>> cleanup something? I don't know, just wondering...
>>>> Hmm... I think both panel & platform_attch need ERROR remind when
>>>> it failed. But if it still need clean, I though it should clean the
>>>> platform attch
>>>> error,
>>>> this is not relate to DRM directly, just analogix driver logic, so
>>>> code would like,
>>>>
>>>> - if (dp->panel)
>>>> - ret = drm_panel_attach(dp->panel, &dp->connector);
>>>> + if (dp->plat_data && dp->plat_data->panel) {
>>>> + ret = drm_panel_attach(dp->plat_data->panel, &dp->connector);
>>>> + if (ret) {
>>>> + DRM_ERROR("Failed to attach panel\n");
>>>> + return ret;
>>>> + }
>>>> + }
>>>>
>>>> + /*
>>>> + * This should be the end of attach function, caused
>>>> + * we should ensure dp bridge could attach first.
>>>> + */
>>>> + if (dp->plat_data && dp->plat_data->attach) {
>>>> + ret = dp->plat_data->attach(dp->plat_data, bridge);
>>>>
>>>> return ret;
>>> I am lost... the code looks the same. What did you change?
>> I just remove the DRM_ERROR after dp->plat_data->attach(),
>> maybe I should paste the change that rebase on this patch,
>> here are they,
>>
>> /*
>> * This should be the end of attach function, caused
>> * we should ensure dp bridge could attach first.
>> */
>> - if (dp->plat_data && dp->plat_data->attach) {
>> + if (dp->plat_data && dp->plat_data->attach)
>> ret = dp->plat_data->attach(dp->plat_data, bridge);
>> - if (ret) {
>> - DRM_ERROR("Failed at platform attch func\n");
>> - return ret;
>> - }
>> - }
>>
>> - return 0;
>> + return ret;
>>
>>
>> If this haven't meet your comment, I maybe start to think that
>> your comment "Two new error paths appeared here and above"
>> indicated that those two function is the same.
>> "dp->plat_data->attach(dp->plat_data, bridge); "
>> "drm_panel_attach(dp->plat_data->panel, &dp->connector); "
> I wasn't talking about error message but rather about possible need of
> clean up in error path. Previously there was only drm_panel_attach().
> Now you have two of them (drm_panel_attach() and
> dp->plat_data->attach()). If the second fails don't you have to clean up
> before exit? I don't know, just asking.
Oh... Sorry about missing your words. Yes,
drm_panel_detach(dp->plat_data->panel)
was missed in this error path.
Thanks,
- Yakir
>
> Best regards,
> Krzysztof
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Page 4 of 4 — ← Prev page 1 2 3 [4]
Back to top | Article view | linux.kernel
csiph-web