mbox series

[v2,00/12] drm/sun4i: Add A83T HDMI support

Message ID 20180110192512.19684-1-jernej.skrabec@siol.net
Headers show
Series drm/sun4i: Add A83T HDMI support | expand

Message

Jernej Škrabec Jan. 10, 2018, 7:25 p.m. UTC 
This patch series implements support for A83T DW HDMI and PHY. Contrary to
v1 series, this one is based on latest linux-next, since all needed patches
were merged.

While exactly this combination of HDMI controller and PHY is not common in
Allwinner SoCs, this patch series nevertheless makes groundwork for other
SoCs, which have same DW HDMI IP block, but different PHYs, like H3 and H5.

Please take a look.

Best regards,
Jernej

Changes from v1:
- Collected ACKs
- Separated bindings for controller and PHY
- Split driver into two parts - controller and PHY
- HDMI PHY driver now uses regmap for writes
- added defines for PHY registers and bits
- updated DT entries to accomodate new bindings
- removed already merged clock patch
- reworked first clock patch according to comments
- added new clock patch which changes NKMP formula
- split TCON patch in two, one for quirk and one for new compatible
- reworked patch which exports DW HDMI PHY functions:
  - remove "gen2" from some function names
  - removed parameter from dw_hdmi_phy_reset()
  - added address parameter to dw_hdmi_phy_i2c_set_addr()
- updated most of commit messages

Jernej Skrabec (12):
  clk: sunxi-ng: Mask nkmp factors when setting register
  clk: sunxi-ng: Change formula for NKMP PLLs
  drm/bridge/synopsys: dw-hdmi: Enable workaround for v1.32a
  drm/bridge/synopsys: dw-hdmi: Export some PHY related functions
  drm/bridge/synopsys: dw-hdmi: Add deinit callback
  dt-bindings: display: sun4i-drm: Add A83T HDMI pipeline
  drm/sun4i: Add has_channel_0 TCON quirk
  drm/sun4i: Add support for A83T second TCON
  drm/sun4i: Add support for A83T second DE2 mixer
  drm/sun4i: Implement A83T HDMI driver
  ARM: dts: sun8i: a83t: Add HDMI display pipeline
  ARM: dts: sun8i: a83t: Enable HDMI on BananaPi M3

 .../bindings/display/sunxi/sun4i-drm.txt           | 197 +++++++++++++-
 arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts       |  25 ++
 arch/arm/boot/dts/sun8i-a83t.dtsi                  | 119 +++++++-
 drivers/clk/sunxi-ng/ccu_nkmp.c                    |  27 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          |  55 ++--
 drivers/gpu/drm/sun4i/Kconfig                      |   9 +
 drivers/gpu/drm/sun4i/Makefile                     |   4 +
 drivers/gpu/drm/sun4i/sun4i_tcon.c                 |  46 +++-
 drivers/gpu/drm/sun4i/sun4i_tcon.h                 |   1 +
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c              | 183 +++++++++++++
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h              |  51 ++++
 drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c             | 302 +++++++++++++++++++++
 drivers/gpu/drm/sun4i/sun8i_mixer.c                |  11 +
 include/drm/bridge/dw_hdmi.h                       |  12 +
 14 files changed, 993 insertions(+), 49 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
 create mode 100644 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
 create mode 100644 drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c

Comments

Maxime Ripard Jan. 11, 2018, 12:18 p.m. UTC | #1 
On Wed, Jan 10, 2018 at 08:25:07PM +0100, Jernej Skrabec wrote:
> Some TCONs on newer SoCs doesn't support channel 0, since they are meant
> to be used only with TV or HDMI encoder.
> 
> Prepare support for them with adding has_channel_0 quirk.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime
Maxime Ripard Jan. 11, 2018, 12:18 p.m. UTC | #2 
On Wed, Jan 10, 2018 at 08:25:08PM +0100, Jernej Skrabec wrote:
> This TCON is connected to HDMI encoder.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime
Chen-Yu Tsai Jan. 12, 2018, 6:12 a.m. UTC | #3 
On Thu, Jan 11, 2018 at 3:25 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> Some SoCs, like Allwinner A83T, have to do additional cleanup when
> HDMI driver unloads. When using DW HDMI through DRM bridge API, there is
> no place to store driver's private data so it can be accessed in unbind
> function. Because of that, add deinit function which is called at the
> very end, so drivers can do a proper cleanup.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

8242ecbd597d ("drm/bridge/synopsys: stop clobbering drvdata"), which is
already in drm-misc-next, is a much saner solution. :)

ChenYu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jernej Škrabec Jan. 12, 2018, 8:42 p.m. UTC | #4 
Hi all,

Dne sreda, 10. januar 2018 ob 20:25:04 CET je Jernej Skrabec napisal(a):
> Parts of PHY code could be useful also for custom PHYs. For example,
> Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY
> with few additional memory mapped registers, so most of the Synopsys PHY
> related code could be reused.
> 
> Functions exported here are actually not specific to Synopsys PHYs but
> to DWC HDMI controller PHY interface. This means that even if the PHY is
> completely custom, i.e. not designed by Synopsys, exported functions can
> be useful.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 44
> +++++++++++++++++++++---------- include/drm/bridge/dw_hdmi.h              |
> 11 ++++++++
>  2 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> 7ca14d7325b5..7d80f4b56683 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1037,19 +1037,21 @@ static void dw_hdmi_phy_enable_svsret(struct dw_hdmi
> *hdmi, u8 enable) HDMI_PHY_CONF0_SVSRET_MASK);
>  }
> 
> -static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
> +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
>  {
>  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
>  			 HDMI_PHY_CONF0_GEN2_PDDQ_OFFSET,
>  			 HDMI_PHY_CONF0_GEN2_PDDQ_MASK);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_pddq);
> 
> -static void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable)
> +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable)
>  {
>  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
>  			 HDMI_PHY_CONF0_GEN2_TXPWRON_OFFSET,
>  			 HDMI_PHY_CONF0_GEN2_TXPWRON_MASK);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_txpwron);
> 
>  static void dw_hdmi_phy_sel_data_en_pol(struct dw_hdmi *hdmi, u8 enable)
>  {
> @@ -1065,6 +1067,22 @@ static void dw_hdmi_phy_sel_interface_control(struct
> dw_hdmi *hdmi, u8 enable) HDMI_PHY_CONF0_SELDIPIF_MASK);
>  }
> 
> +void dw_hdmi_phy_reset(struct dw_hdmi *hdmi)
> +{
> +	/* PHY reset. The reset signal is active high on Gen2 PHYs. */
> +	hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_PHYRSTZ, HDMI_MC_PHYRSTZ);
> +	hdmi_writeb(hdmi, 0, HDMI_MC_PHYRSTZ);
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_reset);

I just noticed that meson dw hdmi driver uses function with the same name, 
which break compilation. Is it ok if I rename meson specific reset to 
meson_dw_hdmi_phy_reset()?

Best regards,
Jernej

> +
> +void dw_hdmi_phy_i2c_set_addr(struct dw_hdmi *hdmi, u8 address)
> +{
> +	hdmi_phy_test_clear(hdmi, 1);
> +	hdmi_writeb(hdmi, address, HDMI_PHY_I2CM_SLAVE_ADDR);
> +	hdmi_phy_test_clear(hdmi, 0);
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_set_addr);
> +
>  static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
>  {
>  	const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
> @@ -1203,16 +1221,11 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>  	if (phy->has_svsret)
>  		dw_hdmi_phy_enable_svsret(hdmi, 1);
> 
> -	/* PHY reset. The reset signal is active high on Gen2 PHYs. */
> -	hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_PHYRSTZ, HDMI_MC_PHYRSTZ);
> -	hdmi_writeb(hdmi, 0, HDMI_MC_PHYRSTZ);
> +	dw_hdmi_phy_reset(hdmi);
> 
>  	hdmi_writeb(hdmi, HDMI_MC_HEACPHY_RST_ASSERT, HDMI_MC_HEACPHY_RST);
> 
> -	hdmi_phy_test_clear(hdmi, 1);
> -	hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2,
> -		    HDMI_PHY_I2CM_SLAVE_ADDR);
> -	hdmi_phy_test_clear(hdmi, 0);
> +	dw_hdmi_phy_i2c_set_addr(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2);
> 
>  	/* Write to the PHY as configured by the platform */
>  	if (pdata->configure_phy)
> @@ -1251,15 +1264,16 @@ static void dw_hdmi_phy_disable(struct dw_hdmi
> *hdmi, void *data) dw_hdmi_phy_power_off(hdmi);
>  }
> 
> -static enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
> -						      void *data)
> +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
> +					       void *data)
>  {
>  	return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ?
>  		connector_status_connected : connector_status_disconnected;
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_read_hpd);
> 
> -static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
> -				   bool force, bool disabled, bool rxsense)
> +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
> +			    bool force, bool disabled, bool rxsense)
>  {
>  	u8 old_mask = hdmi->phy_mask;
> 
> @@ -1271,8 +1285,9 @@ static void dw_hdmi_phy_update_hpd(struct dw_hdmi
> *hdmi, void *data, if (old_mask != hdmi->phy_mask)
>  		hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_update_hpd);
> 
> -static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data)
> +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data)
>  {
>  	/*
>  	 * Configure the PHY RX SENSE and HPD interrupts polarities and clear
> @@ -1291,6 +1306,7 @@ static void dw_hdmi_phy_setup_hpd(struct dw_hdmi
> *hdmi, void *data) hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD |
> HDMI_IH_PHY_STAT0_RX_SENSE), HDMI_IH_MUTE_PHY_STAT0);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_setup_hpd);
> 
>  static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = {
>  	.init = dw_hdmi_phy_init,
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 182f83283e24..4a35e5065f6f 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -157,7 +157,18 @@ void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
>  void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
> 
>  /* PHY configuration */
> +void dw_hdmi_phy_i2c_set_addr(struct dw_hdmi *hdmi, u8 address);
>  void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
>  			   unsigned char addr);
> 
> +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
> +					       void *data);
> +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
> +			    bool force, bool disabled, bool rxsense);
> +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
> +
> +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable);
> +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable);
> +void dw_hdmi_phy_reset(struct dw_hdmi *hdmi);
> +
>  #endif /* __IMX_HDMI_H__ */
> --
> 2.15.1




--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Jan. 12, 2018, 11:04 p.m. UTC | #5 
Hi Jernej,

Thank you for th epatch.

On Wednesday, 10 January 2018 21:25:04 EET Jernej Skrabec wrote:
> Parts of PHY code could be useful also for custom PHYs. For example,
> Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY
> with few additional memory mapped registers, so most of the Synopsys PHY
> related code could be reused.
> 
> Functions exported here are actually not specific to Synopsys PHYs but
> to DWC HDMI controller PHY interface. This means that even if the PHY is
> completely custom, i.e. not designed by Synopsys, exported functions can
> be useful.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 44 ++++++++++++++++++++--------
>  include/drm/bridge/dw_hdmi.h              | 11 ++++++++
>  2 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> 7ca14d7325b5..7d80f4b56683 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1037,19 +1037,21 @@ static void dw_hdmi_phy_enable_svsret(struct dw_hdmi
> *hdmi, u8 enable) HDMI_PHY_CONF0_SVSRET_MASK);
>  }
> 
> -static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
> +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
>  {
>  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
>  			 HDMI_PHY_CONF0_GEN2_PDDQ_OFFSET,
>  			 HDMI_PHY_CONF0_GEN2_PDDQ_MASK);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_pddq);
> 
> -static void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable)
> +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable)
>  {
>  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
>  			 HDMI_PHY_CONF0_GEN2_TXPWRON_OFFSET,
>  			 HDMI_PHY_CONF0_GEN2_TXPWRON_MASK);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_txpwron);
> 
>  static void dw_hdmi_phy_sel_data_en_pol(struct dw_hdmi *hdmi, u8 enable)
>  {
> @@ -1065,6 +1067,22 @@ static void dw_hdmi_phy_sel_interface_control(struct
> dw_hdmi *hdmi, u8 enable) HDMI_PHY_CONF0_SELDIPIF_MASK);
>  }
> 
> +void dw_hdmi_phy_reset(struct dw_hdmi *hdmi)
> +{
> +	/* PHY reset. The reset signal is active high on Gen2 PHYs. */
> +	hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_PHYRSTZ, HDMI_MC_PHYRSTZ);
> +	hdmi_writeb(hdmi, 0, HDMI_MC_PHYRSTZ);
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_reset);
> +
> +void dw_hdmi_phy_i2c_set_addr(struct dw_hdmi *hdmi, u8 address)
> +{
> +	hdmi_phy_test_clear(hdmi, 1);
> +	hdmi_writeb(hdmi, address, HDMI_PHY_I2CM_SLAVE_ADDR);
> +	hdmi_phy_test_clear(hdmi, 0);
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_set_addr);
> +
>  static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
>  {
>  	const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
> @@ -1203,16 +1221,11 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>  	if (phy->has_svsret)
>  		dw_hdmi_phy_enable_svsret(hdmi, 1);
> 
> -	/* PHY reset. The reset signal is active high on Gen2 PHYs. */
> -	hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_PHYRSTZ, HDMI_MC_PHYRSTZ);
> -	hdmi_writeb(hdmi, 0, HDMI_MC_PHYRSTZ);
> +	dw_hdmi_phy_reset(hdmi);
> 
>  	hdmi_writeb(hdmi, HDMI_MC_HEACPHY_RST_ASSERT, HDMI_MC_HEACPHY_RST);
> 
> -	hdmi_phy_test_clear(hdmi, 1);
> -	hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2,
> -		    HDMI_PHY_I2CM_SLAVE_ADDR);
> -	hdmi_phy_test_clear(hdmi, 0);
> +	dw_hdmi_phy_i2c_set_addr(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2);
> 
>  	/* Write to the PHY as configured by the platform */
>  	if (pdata->configure_phy)
> @@ -1251,15 +1264,16 @@ static void dw_hdmi_phy_disable(struct dw_hdmi
> *hdmi, void *data) dw_hdmi_phy_power_off(hdmi);
>  }
> 
> -static enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
> -						      void *data)
> +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
> +					       void *data)
>  {
>  	return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ?
>  		connector_status_connected : connector_status_disconnected;
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_read_hpd);
> 
> -static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
> -				   bool force, bool disabled, bool rxsense)
> +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
> +			    bool force, bool disabled, bool rxsense)
>  {
>  	u8 old_mask = hdmi->phy_mask;
> 
> @@ -1271,8 +1285,9 @@ static void dw_hdmi_phy_update_hpd(struct dw_hdmi
> *hdmi, void *data, if (old_mask != hdmi->phy_mask)
>  		hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_update_hpd);
> 
> -static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data)
> +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data)
>  {
>  	/*
>  	 * Configure the PHY RX SENSE and HPD interrupts polarities and clear
> @@ -1291,6 +1306,7 @@ static void dw_hdmi_phy_setup_hpd(struct dw_hdmi
> *hdmi, void *data) hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD |
> HDMI_IH_PHY_STAT0_RX_SENSE), HDMI_IH_MUTE_PHY_STAT0);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_setup_hpd);
> 
>  static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = {
>  	.init = dw_hdmi_phy_init,
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 182f83283e24..4a35e5065f6f 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -157,7 +157,18 @@ void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
>  void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
> 
>  /* PHY configuration */
> +void dw_hdmi_phy_i2c_set_addr(struct dw_hdmi *hdmi, u8 address);
>  void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
>  			   unsigned char addr);
> 
> +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
> +					       void *data);
> +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
> +			    bool force, bool disabled, bool rxsense);
> +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
> +
> +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable);
> +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable);
> +void dw_hdmi_phy_reset(struct dw_hdmi *hdmi);

Nitpicking, could you move these last three functions between the I2C and HPD 
functions ? They are, as the I2C functions, related to controlling the 
interface between the HDMI controller and the PHY (the internal I2C bus and 
direct signals), so it would make sense to group them together.

With that fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Regarding HPD I think we could refactor the code in a cleaner why that 
wouldn't require the three functions to be exported, but that shouldn't block 
this patch series, it can always be done later.

>  #endif /* __IMX_HDMI_H__ */
Neil Armstrong Jan. 15, 2018, 3:13 p.m. UTC | #6 
On 12/01/2018 21:42, Jernej Škrabec wrote:
> Hi all,
> 
> Dne sreda, 10. januar 2018 ob 20:25:04 CET je Jernej Skrabec napisal(a):
>> Parts of PHY code could be useful also for custom PHYs. For example,
>> Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY
>> with few additional memory mapped registers, so most of the Synopsys PHY
>> related code could be reused.
>>
>> Functions exported here are actually not specific to Synopsys PHYs but
>> to DWC HDMI controller PHY interface. This means that even if the PHY is
>> completely custom, i.e. not designed by Synopsys, exported functions can
>> be useful.
>>
>> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 44
>> +++++++++++++++++++++---------- include/drm/bridge/dw_hdmi.h              |
>> 11 ++++++++
>>  2 files changed, 41 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
>> 7ca14d7325b5..7d80f4b56683 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -1037,19 +1037,21 @@ static void dw_hdmi_phy_enable_svsret(struct dw_hdmi
>> *hdmi, u8 enable) HDMI_PHY_CONF0_SVSRET_MASK);
>>  }
>>
>> -static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
>> +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
>>  {
>>  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
>>  			 HDMI_PHY_CONF0_GEN2_PDDQ_OFFSET,
>>  			 HDMI_PHY_CONF0_GEN2_PDDQ_MASK);
>>  }
>> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_pddq);
>>
>> -static void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable)
>> +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable)
>>  {
>>  	hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
>>  			 HDMI_PHY_CONF0_GEN2_TXPWRON_OFFSET,
>>  			 HDMI_PHY_CONF0_GEN2_TXPWRON_MASK);
>>  }
>> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_txpwron);
>>
>>  static void dw_hdmi_phy_sel_data_en_pol(struct dw_hdmi *hdmi, u8 enable)
>>  {
>> @@ -1065,6 +1067,22 @@ static void dw_hdmi_phy_sel_interface_control(struct
>> dw_hdmi *hdmi, u8 enable) HDMI_PHY_CONF0_SELDIPIF_MASK);
>>  }
>>
>> +void dw_hdmi_phy_reset(struct dw_hdmi *hdmi)
>> +{
>> +	/* PHY reset. The reset signal is active high on Gen2 PHYs. */
>> +	hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_PHYRSTZ, HDMI_MC_PHYRSTZ);
>> +	hdmi_writeb(hdmi, 0, HDMI_MC_PHYRSTZ);
>> +}
>> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_reset);
> 
> I just noticed that meson dw hdmi driver uses function with the same name, 
> which break compilation. Is it ok if I rename meson specific reset to 
> meson_dw_hdmi_phy_reset()?

Hi Jernej,

Yes,

Neil

> 
> Best regards,
> Jernej
> 
[...]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html