Re: [PATCH v10 05/11] drm: bridge/dw_hdmi:split some phy configuration to platform driver
On Fri, Nov 14, 2014 at 7:13 PM, Zubair Lutfullah Kakakhel wrote: > > > On 14/11/14 11:08, Andy Yan wrote: >> >> On 2014年11月14日 18:55, Zubair Lutfullah Kakakhel wrote: >>> >>> On 14/11/14 10:53, Andy Yan wrote: Hi ZubairLK: Thanks for your review. On 2014年11月14日 18:19, Zubair Lutfullah Kakakhel wrote: > Hi Andy, > > Nice work on this patch series. Its getting better and better :). > > On 14/11/14 03:27, Andy Yan wrote: >> hdmi phy clock symbol and transmission termination value >> can adjust platform specific to get the best SI > ^Is this signal integrity? yes , SI is signal integrity, such as eye diagram measurement > Are these two disjoint features in separate patches? > >> also add mode_valid interface for some platform may not support >> all the display mode > Sounds like another separate patch to me. :) they can seperate > Also, This series is becoming quite large. With major changes and fixes > mixed together. > > Patch 3 splits imx-drm. > Patch 4 moves dw-drm out of imx-drm folder. > Patch 7 adds binding > Patch 9 converts to drm bridge. > > Can these be placed together easily? > And in the start. i.e. patch 1, 2, 3, 4, > > Then all fixes etc can come afterwards? > > It helps when checking histories later as to how a driver was made and > how fixes happened. > > Especially when file moves happen.. Do you mean we can rearrange the patch series? put patch 3, 4 ,7, 9 together one bye one than followed by the fixes patches 5 ,6, 8, 11 ? >>> Yes. Rearrange so that the split imx-drm/imx-hdmi and conversion to >>> drm-bridge is at the start of the series. >>> Then the rest are bug fixes and feature additions. >> Can I put patch#1(make checkpatch happy) and patch#2 (defer probe) as the >> first two patch. >> Daniel from Google chromium think it's better to put the two slightly >> changes in the front for easy review. Sorry, I didn't see this conversation before my earlier emails. I was suggesting to Andy to arrange his patches like this: (1) fixes that directly apply to imx-drm as it is today. (2) split out the "generic dw_hdmi" parts from imx-hdmi into dw_hdmi.c (3) convert dw_hdmi.c to a drm_bridge (4) move the dw_hdmi.c bridge implementation to drm/bridge (5) modifications to dw_hdmi.c required to support hdmi on rk3288 (5) add rk3288-hdmi.c to drm/rockchip (at least whenever drm/rockchip lands) The idea being that we can start landing the patches for (1) even while we are still debating / reviewing (2)+ upstream. > Sure. > > I am not the maintainer. They have to make the final decision. imx-drm is still in staging. I do not see anyone listed explicitly under MAINTAINERS for drivers/staging/imx-drm, so by default I guess it falls back to gregkh (drivers/staging/)? The "Commit" field in git log seems to back this up. Although, based on Author, I think we also want the opinions of Philipp Zabel and Russel King. Thanks, -djk > > ZubairLK ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v18 12/12] drm: bridge/dw_hdmi: add rockchip rk3288 support
Hi Andy,
This driver adds HDMI to rockchip/drm. The fact that rockchip's hdmi
uses dw_hdmi is an implementation detail. I do not think that the names
used for rk3288-hdmi should include "dw" in them.
See inline for what I mean...
On Thu, Dec 4, 2014 at 10:34 PM, Andy Yan wrote:
> Rockchip RK3288 hdmi is compatible with dw_hdmi
>
> this patch is depend on patch by Mark Yao
> drm: rockchip: Add basic drm driver
> see https://lkml.org/lkml/2014/12/2/161
>
> Signed-off-by: Andy Yan
>
> ---
>
> Changes in v18: None
> Changes in v17:
> - parse resource and irq in platform driver
>
> Changes in v16: None
> Changes in v15:
> - remove THIS_MODULE in platform driver
>
> Changes in v14: None
> Changes in v13: None
> Changes in v12:
> - add comment for the depend on patch
>
> Changes in v11: None
> Changes in v10:
> - add more display mode support mpll configuration for rk3288
>
> Changes in v9:
> - move some phy configuration to platform driver
>
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
>
> drivers/gpu/drm/bridge/dw_hdmi.c| 3 +
> drivers/gpu/drm/rockchip/Kconfig| 10 +
> drivers/gpu/drm/rockchip/Makefile | 2 +
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 341
>
> include/drm/bridge/dw_hdmi.h| 1 +
> 5 files changed, 357 insertions(+)
> create mode 100644 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c
> b/drivers/gpu/drm/bridge/dw_hdmi.c
> index cecc46a..01c95a8 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
> @@ -852,6 +852,9 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> unsigned char prep,
> dw_hdmi_phy_gen2_txpwron(hdmi, 1);
> dw_hdmi_phy_gen2_pddq(hdmi, 0);
>
> + if (hdmi->dev_type == RK3288_HDMI)
> + dw_hdmi_phy_enable_spare(hdmi, 1);
> +
> /*Wait for PHY PLL lock */
> msec = 5;
> do {
> diff --git a/drivers/gpu/drm/rockchip/Kconfig
> b/drivers/gpu/drm/rockchip/Kconfig
> index ca9f085..6ebebe8 100644
> --- a/drivers/gpu/drm/rockchip/Kconfig
> +++ b/drivers/gpu/drm/rockchip/Kconfig
> @@ -15,3 +15,13 @@ config DRM_ROCKCHIP
> management to userspace. This driver does not provide
> 2D or 3D acceleration; acceleration is performed by other
> IP found on the SoC.
> +
> +config ROCKCHIP_DW_HDMI
I would rather call this ROCKCHIP_HDMI, since this driver implements
the HDMI for Rockchip. The fact that it uses dw_hdmi is an
implementation detail.
> +bool "Rockchip specific extensions for Synopsys DW HDMI"
> +depends on DRM_ROCKCHIP
> +select DRM_DW_HDMI
> +help
> + This selects support for Rockchip SoC specific extensions
> + for the Synopsys DesignWare HDMI driver. If you want to
> + enable HDMI on RK3288 based SoC, you should selet this
> + option.
This could become simply:
Select this option to enable HDMI support for Rockchip SoCs.
> diff --git a/drivers/gpu/drm/rockchip/Makefile
> b/drivers/gpu/drm/rockchip/Makefile
> index 2cb0672..beed7df 100644
> --- a/drivers/gpu/drm/rockchip/Makefile
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -5,4 +5,6 @@
> rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o rockchip_drm_fbdev.o \
> rockchip_drm_gem.o
>
> +rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o
> +
> obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o rockchip_drm_vop.o
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> new file mode 100644
> index 000..11d54b0
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
Similarly, I'd rather this file be called drm_rockchip_hdmi.c to be
consistent with the rest of the files in drm/rockchip.
> @@ -0,0 +1,341 @@
> +/*
> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "rockchip_drm_drv.h"
> +#include "rockchip_drm_vop.h"
> +
> +#define GRF_SOC_CON60x025c
> +#define HDMI_SEL_VOP_LIT(1 << 4)
> +
> +struct rockchip_hdmi {
> + struct device *dev;
> + struct regmap *regmap;
> + struct drm_encoder encoder;
> +};
> +
> +#define to_rockchip_hdmi(x)container_of(x, struct rockchip_hdmi, x)
> +
> +static const struct dw_hdmi_mpll_config rockchip_mpll_cfg[] = {
Let's stick to mpll_config. Not much value to abbreviate an abbreviation.
> + {
Re: [PATCH 3/3] staging: slimport: Add anx7814 driver support by analogix.
Hi Eric,
Thanks for starting to upstream this Analogix Slimport driver!
As Greg says, please move this driver to its intended directory, I presume:
/drivers/gpu/drm/bridge
And when you submit, use get_maintainer.pl to add the proper reviewers
and lists.
At present, you have no DRM folks, nor dri-devel, so you probably
won't receive any additional feedback on this version, since the
relevant folks have not seen your emails.
Some more very detailed feedback inline...
On Sep 7, 2015 05:15, "Enric Balletbo i Serra" wrote:
>
> The ANX7814 is an ultra-low power Full-HD (1080p60) SlimPort transmitter
> designed for portable devices.
>
> This driver adds initial support and supports HDMI to DP pass-through mode.
>
> Signed-off-by: Enric Balletbo i Serra
> ---
> drivers/staging/Kconfig|2 +
> drivers/staging/Makefile |1 +
> drivers/staging/slimport/Kconfig |7 +
> drivers/staging/slimport/Makefile |4 +
> drivers/staging/slimport/slimport.c| 301 +++
> drivers/staging/slimport/slimport.h| 49 +
> drivers/staging/slimport/slimport_tx_drv.c | 3293
>
> drivers/staging/slimport/slimport_tx_drv.h | 254 +++
> drivers/staging/slimport/slimport_tx_reg.h | 786 +++
> 9 files changed, 4697 insertions(+)
> create mode 100644 drivers/staging/slimport/Kconfig
> create mode 100644 drivers/staging/slimport/Makefile
> create mode 100644 drivers/staging/slimport/slimport.c
> create mode 100644 drivers/staging/slimport/slimport.h
> create mode 100644 drivers/staging/slimport/slimport_tx_drv.c
> create mode 100644 drivers/staging/slimport/slimport_tx_drv.h
> create mode 100644 drivers/staging/slimport/slimport_tx_reg.h
>
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index e29293c..24ccd7c 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -110,4 +110,6 @@ source "drivers/staging/wilc1000/Kconfig"
>
> source "drivers/staging/most/Kconfig"
>
> +source "drivers/staging/slimport/Kconfig"
> +
> endif # STAGING
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 50824dd..942e886 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -47,3 +47,4 @@ obj-$(CONFIG_FB_TFT) += fbtft/
> obj-$(CONFIG_FSL_MC_BUS) += fsl-mc/
> obj-$(CONFIG_WILC1000) += wilc1000/
> obj-$(CONFIG_MOST) += most/
> +obj-$(CONFIG_SLIMPORT_ANX78XX) += slimport/
> diff --git a/drivers/staging/slimport/Kconfig
> b/drivers/staging/slimport/Kconfig
> new file mode 100644
> index 000..f5233ef
> --- /dev/null
> +++ b/drivers/staging/slimport/Kconfig
> @@ -0,0 +1,7 @@
> +config SLIMPORT_ANX78XX
I think the "SLIMPORT_" here is a bit overkill, and just adds to
confusion over what name to use where. I'd recommend just
CONFIG_ANX78XX.
Likewise, for consistency, the rename the files as "anx78xx*" instead
of "slimport*".
> + tristate "Analogix Slimport transmitter ANX78XX support"
> + help
> + Slimport Transmitter is a HD video transmitter chip
> + over micro-USB connector for smartphone device.
> +
> +
> diff --git a/drivers/staging/slimport/Makefile
> b/drivers/staging/slimport/Makefile
> new file mode 100644
> index 000..9bb6ce2
> --- /dev/null
> +++ b/drivers/staging/slimport/Makefile
> @@ -0,0 +1,4 @@
> +obj-${CONFIG_SLIMPORT_ANX78XX} := anx78xx.o
> +
> +anx78xx-y := slimport.o
> +anx78xx-y += slimport_tx_drv.o
> diff --git a/drivers/staging/slimport/slimport.c
> b/drivers/staging/slimport/slimport.c
> new file mode 100644
> index 000..95c5114
> --- /dev/null
> +++ b/drivers/staging/slimport/slimport.c
> @@ -0,0 +1,301 @@
> +/*
> + * Copyright(c) 2015, Analogix Semiconductor. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "slimport.h"
> +#include "slimport_tx_drv.h"
> +
> +/* HDCP switch for external block */
> +/* external_block_en = 1: enable, 0: disable */
This comment is a bit superfluous.
> +int external_block_en = 1;
> +
> +struct i2c_client *anx7814_client;
A single global client isn't going to work. It is easy to imagine a
machine with more than one anx78xx, requiring multiple instances of
the driver and hence multiple i2c clients.
Also, you'll want to be a bit more consistent with naming; is the
driver for anx78xx? Or anx7814
