mbox series

[v3,00/11] Analogix ANX6345 RGB-(e)DP bridge support

Message ID 20190215050957.20755-1-anarsoul@gmail.com
Headers show
Series Analogix ANX6345 RGB-(e)DP bridge support | expand

Message

Vasily Khoruzhick Feb. 15, 2019, 5:09 a.m. UTC
This patchset brings support for Analogix ANX6345 RGB-(e)DP bridge, which
is used by some Allwinner A64 laptops, such as Pinebook and Olimex
TERES-I.

It reuses some definitions from ANX78xx driver that already exists in the
kernel tree, but the driver code itself is rewritten due to significant
difference between ANX6345 and ANX78xx.

This patchset also adds support for LCD panels used in Pinebook, dt
property to disable strict dot clock check in sun4i drm driver and adds
nodes necessary for LCD support into Pinebook dts.

v2: - sort Kconfig and Makefile entries alphabetically
    - remove panel supply from anx6345
    - add support for panels into anx6345 driver
    - add compatible and binding for generic eDP panel
    - replace patch that adds 5% tolerance for dotclock check in sun4i
      driver for panel and bridges with patch that adds 1% tolerance
      in case if bridge is connected.
v3: - replace 1% tolerance patch with patch that adds dt property to
      disable strict dot clock check
    - add support for LCD panels used in Pinebook
    - drop generic eDP panel driver
    - drop Teres-I changes, I don't have hardware to test them anyway.

Icenowy Zheng (7):
  drm/bridge: move ANA78xx driver to analogix subdirectory
  drm/bridge: split some definitions of ANX78xx to dedicated headers
  drm/bridge: extract some Analogix I2C DP common code
  dt-bindings: Add ANX6345 DP/eDP transmitter binding
  drm/bridge: Add Analogix anx6345 support
  arm64: allwinner: a64: add pinmux for RGB666 LCD
  arm64: allwinner: a64: enable LCD-related hardware for Pinebook

Vasily Khoruzhick (4):
  drm/sun4i: rgb: Add DT property to disable strict clock rate check
  drm/panel: simple: Add BOE HB140WX1-501 panel support
  dt-bindings: Add Guangdong Neweast Optoelectronics CO. LTD vendor
    prefix
  drm/panel: simple: Add NewEast Optoelectronics CO., LTD WJFH116008A
    panel support

 .../bindings/display/bridge/anx6345.txt       |  56 ++
 .../display/panel/boe,hb140wx1-501.txt        |   7 +
 .../display/panel/neweast,wjfh116008a.txt     |   7 +
 .../bindings/display/sunxi/sun4i-drm.txt      |   2 +
 .../devicetree/bindings/vendor-prefixes.txt   |   1 +
 .../dts/allwinner/sun50i-a64-pinebook.dts     |  76 ++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |   9 +
 drivers/gpu/drm/bridge/Kconfig                |  10 -
 drivers/gpu/drm/bridge/Makefile               |   4 +-
 drivers/gpu/drm/bridge/analogix-anx78xx.h     | 719 ---------------
 drivers/gpu/drm/bridge/analogix/Kconfig       |  25 +
 drivers/gpu/drm/bridge/analogix/Makefile      |   4 +
 .../drm/bridge/analogix/analogix-anx6345.c    | 845 ++++++++++++++++++
 .../bridge/{ => analogix}/analogix-anx78xx.c  | 146 +--
 .../drm/bridge/analogix/analogix-anx78xx.h    | 265 ++++++
 .../drm/bridge/analogix/analogix-i2c-dptx.c   | 169 ++++
 .../drm/bridge/analogix/analogix-i2c-dptx.h   | 258 ++++++
 .../bridge/analogix/analogix-i2c-txcommon.h   | 240 +++++
 drivers/gpu/drm/panel/panel-simple.c          |  65 ++
 drivers/gpu/drm/sun4i/sun4i_rgb.c             |   5 +
 drivers/gpu/drm/sun4i/sun4i_tcon.c            |   3 +
 drivers/gpu/drm/sun4i/sun4i_tcon.h            |   1 +
 22 files changed, 2041 insertions(+), 876 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/anx6345.txt
 create mode 100644 Documentation/devicetree/bindings/display/panel/boe,hb140wx1-501.txt
 create mode 100644 Documentation/devicetree/bindings/display/panel/neweast,wjfh116008a.txt
 delete mode 100644 drivers/gpu/drm/bridge/analogix-anx78xx.h
 create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
 rename drivers/gpu/drm/bridge/{ => analogix}/analogix-anx78xx.c (90%)
 create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx78xx.h
 create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c
 create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.h
 create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-i2c-txcommon.h

Comments

Priit Laes Feb. 15, 2019, 8:23 a.m. UTC | #1
On Thu, Feb 14, 2019 at 09:09:51PM -0800, Vasily Khoruzhick wrote:
> From: Icenowy Zheng <icenowy@aosc.io>
> 
> The ANX6345 is an ultra-low power DisplayPower/eDP transmitter designed
> for portable devices. This driver adds initial support for RGB to eDP
> mode, without HPD and interrupts.
> 
> This is a configuration usually seen in eDP applications.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/gpu/drm/bridge/analogix/Kconfig       |  11 +
>  drivers/gpu/drm/bridge/analogix/Makefile      |   1 +
>  .../drm/bridge/analogix/analogix-anx6345.c    | 845 ++++++++++++++++++
>  .../drm/bridge/analogix/analogix-i2c-dptx.c   |   2 +-
>  .../drm/bridge/analogix/analogix-i2c-dptx.h   |   8 +
>  .../bridge/analogix/analogix-i2c-txcommon.h   |   3 +
>  6 files changed, 869 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> 

[ ... ]

> +
> +static int anx6345_start(struct anx6345 *anx6345)
> +{
> +	int err;
> +
> +	if (!anx6345->powered)
> +		anx6345_poweron(anx6345);
> +
> +	/* Power on needed modules */
> +	err = anx6345_clear_bits(anx6345->map[I2C_IDX_TXCOM],
> +				 SP_POWERDOWN_CTRL_REG,
> +				 SP_VIDEO_PD | SP_LINK_PD);
> +
> +	err = anx6345_tx_initialization(anx6345);
> +	if (err) {
> +		DRM_ERROR("Failed transmitter initialization: %d\n", err);
> +		goto err_poweroff;

You can move the whole err_poweroff section from below here and drop the goto.

> +	}
> +
> +	/*
> +	 * This delay seems to help keep the hardware in a good state. Without
> +	 * it, there are times where it fails silently.
> +	 */
> +	usleep_range(10000, 15000);
> +
> +	return 0;
> +
> +err_poweroff:
> +	DRM_ERROR("Failed DisplayPort transmitter initialization: %d\n", err);
> +	anx6345_poweroff(anx6345);
> +
> +	return err;
> +}
> +

[ ... ]
Andrzej Hajda Feb. 15, 2019, 9:13 a.m. UTC | #2
On 15.02.2019 06:09, Vasily Khoruzhick wrote:
> From: Icenowy Zheng <icenowy@aosc.io>
>
> The ANX6345 is an ultra-low power DisplayPower/eDP transmitter designed
> for portable devices. This driver adds initial support for RGB to eDP
> mode, without HPD and interrupts.
>
> This is a configuration usually seen in eDP applications.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/gpu/drm/bridge/analogix/Kconfig       |  11 +
>  drivers/gpu/drm/bridge/analogix/Makefile      |   1 +
>  .../drm/bridge/analogix/analogix-anx6345.c    | 845 ++++++++++++++++++
>  .../drm/bridge/analogix/analogix-i2c-dptx.c   |   2 +-
>  .../drm/bridge/analogix/analogix-i2c-dptx.h   |   8 +
>  .../bridge/analogix/analogix-i2c-txcommon.h   |   3 +
>  6 files changed, 869 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
>
> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig b/drivers/gpu/drm/bridge/analogix/Kconfig
> index ed2d05c12546..3c6ec535d361 100644
> --- a/drivers/gpu/drm/bridge/analogix/Kconfig
> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
> @@ -1,3 +1,14 @@
> +config DRM_ANALOGIX_ANX6345
> +	tristate "Analogix ANX6345 bridge"
> +	select DRM_ANALOGIX_DP_I2C
> +	select DRM_KMS_HELPER
> +	select REGMAP_I2C
> +	help
> +	  ANX6345 is an ultra-low Full-HD DisplayPort/eDP
> +	  transmitter designed for portable devices. The
> +	  ANX6345 transforms the LVTTL RGB output of an
> +	  application processor to eDP or DisplayPort.
> +
>  config DRM_ANALOGIX_ANX78XX
>  	tristate "Analogix ANX78XX bridge"
>  	select DRM_ANALOGIX_DP_I2C
> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile b/drivers/gpu/drm/bridge/analogix/Makefile
> index 2d523b67487d..12fed7b04e1e 100644
> --- a/drivers/gpu/drm/bridge/analogix/Makefile
> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
> @@ -1,5 +1,6 @@
>  analogix_dp-objs := analogix_dp_core.o analogix_dp_reg.o
>  analogix_dp_i2c-objs := analogix-i2c-dptx.o
> +obj-$(CONFIG_DRM_ANALOGIX_ANX6345) += analogix-anx6345.o
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP_I2C) += analogix_dp_i2c.o
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> new file mode 100644
> index 000000000000..6098e245e074
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
> @@ -0,0 +1,845 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright(c) Icenowy Zheng <icenowy@aosc.io>
> + * Based on analogix-anx6345.c, which is:
> + *   Copyright(c) 2016, Analogix Semiconductor.
> + */
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
Do you need this header?
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>


drmP.h is/should be deprecated.


> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_dp_helper.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +
> +#include "analogix-i2c-dptx.h"
> +#include "analogix-i2c-txcommon.h"
> +
> +#define I2C_NUM_ADDRESSES	2
> +#define I2C_IDX_DPTX		0
> +#define I2C_IDX_TXCOM		1
> +
> +#define XTAL_CLK		270 /* 27M */
> +
> +#define POLL_DELAY		50000 /* us */
> +#define POLL_TIMEOUT		5000000 /* us */
> +
> +static const u8 anx6345_i2c_addresses[] = {
> +	[I2C_IDX_DPTX]	= ANALOGIX_I2C_DPTX,
> +	[I2C_IDX_TXCOM]	= ANALOGIX_I2C_TXCOMMON,
> +};
> +
> +struct anx6345_platform_data {
> +	struct regulator *dvdd12;
> +	struct regulator *dvdd25;
> +	struct gpio_desc *gpiod_reset;
> +};


Why do you need this struct, why just do not embed it's fields directly
into struct anx6345 ?


> +
> +struct anx6345 {
> +	struct drm_dp_aux aux;
> +	struct drm_bridge bridge;
> +	struct i2c_client *client;
> +	struct edid *edid;
> +	struct drm_connector connector;
> +	struct drm_dp_link link;
> +	struct drm_panel *panel;
> +	struct anx6345_platform_data pdata;
> +	struct mutex lock;
> +
> +	/*
> +	 * I2C Slave addresses of ANX6345 are mapped as DPTX and SYS
> +	 */
> +	struct i2c_client *i2c_clients[I2C_NUM_ADDRESSES];
> +	struct regmap *map[I2C_NUM_ADDRESSES];
> +
> +	u16 chipid;
> +	u8 dpcd[DP_RECEIVER_CAP_SIZE];
> +
> +	bool powered;
> +};
> +
> +static inline struct anx6345 *connector_to_anx6345(struct drm_connector *c)
> +{
> +	return container_of(c, struct anx6345, connector);
> +}
> +
> +static inline struct anx6345 *bridge_to_anx6345(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct anx6345, bridge);
> +}
> +
> +static int anx6345_set_bits(struct regmap *map, u8 reg, u8 mask)
> +{
> +	return regmap_update_bits(map, reg, mask, mask);
> +}
> +
> +static int anx6345_clear_bits(struct regmap *map, u8 reg, u8 mask)
> +{
> +	return regmap_update_bits(map, reg, mask, 0);
> +}
> +
> +static ssize_t anx6345_aux_transfer(struct drm_dp_aux *aux,
> +				    struct drm_dp_aux_msg *msg)
> +{
> +	struct anx6345 *anx6345 = container_of(aux, struct anx6345, aux);
> +
> +	return anx_aux_transfer(anx6345->map[I2C_IDX_DPTX], msg);
> +}
> +
> +static int anx6345_dp_link_training(struct anx6345 *anx6345)
> +{
> +	unsigned int value;
> +	u8 dp_bw;
> +	int err;
> +
> +	err = anx6345_clear_bits(anx6345->map[I2C_IDX_TXCOM],
> +				 SP_POWERDOWN_CTRL_REG,
> +				 SP_TOTAL_PD);
> +	if (err)
> +		return err;
> +
> +	err = drm_dp_dpcd_readb(&anx6345->aux, DP_MAX_LINK_RATE, &dp_bw);
> +	if (err < 0)
> +		return err;
> +
> +	switch (dp_bw) {
> +	case DP_LINK_BW_1_62:
> +	case DP_LINK_BW_2_7:
> +		break;
> +
> +	default:
> +		DRM_DEBUG_KMS("DP bandwidth (%#02x) not supported\n", dp_bw);
> +		return -EINVAL;
> +	}
> +
> +	err = anx6345_set_bits(anx6345->map[I2C_IDX_TXCOM], SP_VID_CTRL1_REG,
> +			       SP_VIDEO_MUTE);
> +	if (err)
> +		return err;
> +
> +	err = anx6345_clear_bits(anx6345->map[I2C_IDX_TXCOM],
> +				 SP_VID_CTRL1_REG, SP_VIDEO_EN);
> +	if (err)
> +		return err;
> +
> +	/* Get DPCD info */
> +	err = drm_dp_dpcd_read(&anx6345->aux, DP_DPCD_REV,
> +			       &anx6345->dpcd, DP_RECEIVER_CAP_SIZE);
> +	if (err < 0) {
> +		DRM_ERROR("Failed to read DPCD: %d\n", err);
> +		return err;
> +	}
> +
> +	/* Clear channel x SERDES power down */
> +	err = anx6345_clear_bits(anx6345->map[I2C_IDX_DPTX],
> +				 SP_DP_ANALOG_POWER_DOWN_REG, SP_CH0_PD);
> +	if (err)
> +		return err;
> +
> +	/* Check link capabilities */
> +	err = drm_dp_link_probe(&anx6345->aux, &anx6345->link);
> +	if (err < 0) {
> +		DRM_ERROR("Failed to probe link capabilities: %d\n", err);
> +		return err;
> +	}
> +
> +	/* Power up the sink */
> +	err = drm_dp_link_power_up(&anx6345->aux, &anx6345->link);
> +	if (err < 0) {
> +		DRM_ERROR("Failed to power up DisplayPort link: %d\n", err);
> +		return err;
> +	}
> +
> +	/* Possibly enable downspread on the sink */
> +	err = regmap_write(anx6345->map[I2C_IDX_DPTX],
> +			   SP_DP_DOWNSPREAD_CTRL1_REG, 0);
> +	if (err)
> +		return err;
> +
> +	if (anx6345->dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5) {
> +		DRM_DEBUG("Enable downspread on the sink\n");
> +		/* 4000PPM */
> +		err = regmap_write(anx6345->map[I2C_IDX_DPTX],
> +				   SP_DP_DOWNSPREAD_CTRL1_REG, 8);
> +		if (err)
> +			return err;
> +
> +		err = drm_dp_dpcd_writeb(&anx6345->aux, DP_DOWNSPREAD_CTRL,
> +					 DP_SPREAD_AMP_0_5);
> +		if (err < 0)
> +			return err;
> +	} else {
> +		err = drm_dp_dpcd_writeb(&anx6345->aux, DP_DOWNSPREAD_CTRL, 0);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	/* Set the lane count and the link rate on the sink */
> +	if (drm_dp_enhanced_frame_cap(anx6345->dpcd))
> +		err = anx6345_set_bits(anx6345->map[I2C_IDX_DPTX],
> +				       SP_DP_SYSTEM_CTRL_BASE + 4,
> +				       SP_ENHANCED_MODE);
> +	else
> +		err = anx6345_clear_bits(anx6345->map[I2C_IDX_DPTX],
> +					 SP_DP_SYSTEM_CTRL_BASE + 4,
> +					 SP_ENHANCED_MODE);
> +	if (err)
> +		return err;
> +
> +	value = drm_dp_link_rate_to_bw_code(anx6345->link.rate);
> +	err = regmap_write(anx6345->map[I2C_IDX_DPTX],
> +			   SP_DP_MAIN_LINK_BW_SET_REG, value);
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(anx6345->map[I2C_IDX_DPTX],
> +			   SP_DP_LANE_COUNT_SET_REG, anx6345->link.num_lanes);
> +	if (err)
> +		return err;
> +
> +	err = drm_dp_link_configure(&anx6345->aux, &anx6345->link);
> +	if (err < 0) {
> +		DRM_ERROR("Failed to configure DisplayPort link: %d\n", err);
> +		return err;
> +	}
> +
> +	/* Start training on the source */
> +	err = regmap_write(anx6345->map[I2C_IDX_DPTX], SP_DP_LT_CTRL_REG,
> +			   SP_LT_EN);
> +	if (err)
> +		return err;
> +
> +	err = regmap_read_poll_timeout(anx6345->map[I2C_IDX_DPTX],
> +				       SP_DP_LT_CTRL_REG,
> +				       value, !(value & SP_DP_LT_INPROGRESS),
> +				       POLL_DELAY, POLL_TIMEOUT);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int anx6345_tx_initialization(struct anx6345 *anx6345)
> +{
> +	int err, i;
> +
> +	/* FIXME: hardcode color depth now */
> +	err = regmap_write(anx6345->map[I2C_IDX_TXCOM], SP_VID_CTRL2_REG,
> +			   SP_IN_BPC_6BIT << SP_IN_BPC_SHIFT);
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(anx6345->map[I2C_IDX_DPTX], SP_DP_PLL_CTRL_REG, 0);
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(anx6345->map[I2C_IDX_TXCOM],
> +			   SP_ANALOG_DEBUG1_REG, 0);
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(anx6345->map[I2C_IDX_DPTX],
> +			   SP_DP_LINK_DEBUG_CTRL_REG,
> +			   SP_NEW_PRBS7 | SP_M_VID_DEBUG);
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(anx6345->map[I2C_IDX_DPTX],
> +			   SP_DP_ANALOG_POWER_DOWN_REG, 0);
> +	if (err)
> +		return err;
> +
> +	/* Force HPD */
> +	err = anx6345_set_bits(anx6345->map[I2C_IDX_DPTX],
> +			       SP_DP_SYSTEM_CTRL_BASE + 3,
> +			       SP_HPD_FORCE | SP_HPD_CTRL);
> +	if (err)
> +		return err;
> +
> +	for (i = 0; i < 4; i++) {
> +		/* 4 lanes */
> +		err = regmap_write(anx6345->map[I2C_IDX_DPTX],
> +				   SP_DP_LANE0_LT_CTRL_REG + i, 0);
> +		if (err)
> +			return err;
> +	}
> +
> +	/* Reset AUX */
> +	err = anx6345_set_bits(anx6345->map[I2C_IDX_TXCOM],
> +			       SP_RESET_CTRL2_REG, SP_AUX_RST);
> +	if (err)
> +		return err;
> +
> +	err = anx6345_clear_bits(anx6345->map[I2C_IDX_TXCOM],
> +				 SP_RESET_CTRL2_REG, SP_AUX_RST);
> +	if (err)
> +		return err;
> +
> +	err = anx6345_dp_link_training(anx6345);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void anx6345_poweron(struct anx6345 *anx6345)
> +{
> +	struct anx6345_platform_data *pdata = &anx6345->pdata;
> +	int err;
> +
> +	if (WARN_ON(anx6345->powered))
> +		return;

It should not happen, you can remove this warn.

> +
> +	if (pdata->dvdd12) {


If regulators are required this will be never null.


> +		err = regulator_enable(pdata->dvdd12);
> +		if (err) {
> +			DRM_ERROR("Failed to enable DVDD12 regulator: %d\n",
> +				  err);
> +			return;
> +		}
> +
> +		usleep_range(1000, 2000);
> +	}
> +
> +	if (pdata->dvdd25) {


ditto


> +		err = regulator_enable(pdata->dvdd25);
> +		if (err) {
> +			DRM_ERROR("Failed to enable DVDD25 regulator: %d\n",
> +				  err);
> +			return;
> +		}
> +
> +		usleep_range(5000, 10000);
> +	}
> +
> +	if (anx6345->panel)
> +		drm_panel_prepare(anx6345->panel);


again, here and below: panel is never null, check can be removed.


> +
> +	gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
> +	usleep_range(1000, 2000);
> +
> +	gpiod_set_value_cansleep(pdata->gpiod_reset, 1);


Start/stop sequence seems odd regarding reset gpio:

1. In probe reset is set to low, in poweroff to high - incosistent.

2. If in case of disabled device reset should be 0, there is no point to
set it again to 0 three lines above.

3. I suspect in dts reset gpio should be declared as active_low, and the
logic in the driver should be reverted, in power off it should be set to
high, in power on it should be lowered (logically).


> +
> +	/* Power on registers module */
> +	anx6345_set_bits(anx6345->map[I2C_IDX_TXCOM], SP_POWERDOWN_CTRL_REG,
> +			 SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);
> +	anx6345_clear_bits(anx6345->map[I2C_IDX_TXCOM], SP_POWERDOWN_CTRL_REG,
> +			   SP_REGISTER_PD | SP_TOTAL_PD);
> +
> +	anx6345->powered = true;
> +}
> +
> +static void anx6345_poweroff(struct anx6345 *anx6345)
> +{
> +	struct anx6345_platform_data *pdata = &anx6345->pdata;
> +	int err;
> +
> +	if (WARN_ON(!anx6345->powered))
> +		return;
> +
> +	gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
> +	usleep_range(1000, 2000);
> +
> +	if (anx6345->panel)
> +		drm_panel_unprepare(anx6345->panel);
> +
> +	if (pdata->dvdd25) {
> +		err = regulator_disable(pdata->dvdd25);
> +		if (err) {
> +			DRM_ERROR("Failed to disable DVDD25 regulator: %d\n",
> +				  err);
> +			return;
> +		}
> +
> +		usleep_range(5000, 10000);
> +	}
> +
> +	if (pdata->dvdd12) {
> +		err = regulator_disable(pdata->dvdd12);
> +		if (err) {
> +			DRM_ERROR("Failed to disable DVDD12 regulator: %d\n",
> +				  err);
> +			return;
> +		}
> +
> +		usleep_range(1000, 2000);
> +	}
> +
> +	anx6345->powered = false;
> +}
> +
> +static int anx6345_start(struct anx6345 *anx6345)
> +{
> +	int err;
> +
> +	if (!anx6345->powered)
> +		anx6345_poweron(anx6345);
> +
> +	/* Power on needed modules */
> +	err = anx6345_clear_bits(anx6345->map[I2C_IDX_TXCOM],
> +				 SP_POWERDOWN_CTRL_REG,
> +				 SP_VIDEO_PD | SP_LINK_PD);
> +
> +	err = anx6345_tx_initialization(anx6345);
> +	if (err) {
> +		DRM_ERROR("Failed transmitter initialization: %d\n", err);
> +		goto err_poweroff;
> +	}
> +
> +	/*
> +	 * This delay seems to help keep the hardware in a good state. Without
> +	 * it, there are times where it fails silently.
> +	 */
> +	usleep_range(10000, 15000);
> +
> +	return 0;
> +
> +err_poweroff:
> +	DRM_ERROR("Failed DisplayPort transmitter initialization: %d\n", err);


redundant message


> +	anx6345_poweroff(anx6345);
> +
> +	return err;
> +}
> +
> +static int anx6345_init_pdata(struct anx6345 *anx6345)
> +{
> +	struct anx6345_platform_data *pdata = &anx6345->pdata;
> +	struct device *dev = &anx6345->client->dev;
> +
> +	/* 1.2V digital core power regulator  */
> +	pdata->dvdd12 = devm_regulator_get(dev, "dvdd12");
> +	if (IS_ERR(pdata->dvdd12)) {
> +		DRM_ERROR("DVDD12 regulator not found\n");
> +		return PTR_ERR(pdata->dvdd12);
> +	}
> +
> +	/* 2.5V digital core power regulator  */
> +	pdata->dvdd25 = devm_regulator_get(dev, "dvdd25");
> +	if (IS_ERR(pdata->dvdd25)) {
> +		DRM_ERROR("DVDD25 regulator not found\n");
> +		return PTR_ERR(pdata->dvdd25);
> +	}
> +
> +	/* GPIO for chip reset */
> +	pdata->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +
> +	return PTR_ERR_OR_ZERO(pdata->gpiod_reset);
> +}
> +
> +static int anx6345_config_dp_output(struct anx6345 *anx6345)
> +{
> +	int err;
> +
> +	err = anx6345_clear_bits(anx6345->map[I2C_IDX_TXCOM], SP_VID_CTRL1_REG,
> +				 SP_VIDEO_MUTE);
> +	if (err)
> +		return err;
> +
> +	/* Enable DP output */
> +	err = anx6345_set_bits(anx6345->map[I2C_IDX_TXCOM], SP_VID_CTRL1_REG,
> +			       SP_VIDEO_EN);
> +	if (err)
> +		return err;
> +
> +	/* Force stream valid */
> +	err = anx6345_set_bits(anx6345->map[I2C_IDX_DPTX],
> +			       SP_DP_SYSTEM_CTRL_BASE + 3,
> +			       SP_STRM_FORCE | SP_STRM_CTRL);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int anx6345_get_downstream_info(struct anx6345 *anx6345)
> +{
> +	u8 value;
> +	int err;
> +
> +	err = drm_dp_dpcd_readb(&anx6345->aux, DP_SINK_COUNT, &value);
> +	if (err < 0) {
> +		DRM_ERROR("Get sink count failed %d\n", err);


The rule of thumb I heard is that if you start message capitalized you
should end with dot. Since I do not know if it is enforced in kernel I
leave the decision up to you.


> +		return err;
> +	}
> +
> +	if (!DP_GET_SINK_COUNT(value)) {
> +		DRM_ERROR("Downstream disconnected\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int anx6345_get_modes(struct drm_connector *connector)
> +{
> +	struct anx6345 *anx6345 = connector_to_anx6345(connector);
> +	int err, num_modes = 0;
> +
> +	if (WARN_ON(!anx6345->powered))
> +		return 0;
> +
> +	mutex_lock(&anx6345->lock);
> +
> +	if (!anx6345->edid) {
> +		err = anx6345_get_downstream_info(anx6345);
> +		if (err) {
> +			DRM_ERROR("Failed to get downstream info: %d\n", err);
> +			goto unlock;
> +		}
> +
> +		anx6345->edid = drm_get_edid(connector, &anx6345->aux.ddc);
> +		if (!anx6345->edid)
> +			DRM_ERROR("Failed to read EDID from panel\n");
> +
> +		err = drm_connector_update_edid_property(connector,
> +							 anx6345->edid);
> +		if (err) {
> +			DRM_ERROR("Failed to update EDID property: %d\n", err);
> +			goto unlock;
> +		}
> +	}
> +
> +	num_modes += drm_add_edid_modes(connector, anx6345->edid);
> +
> +unlock:
> +	mutex_unlock(&anx6345->lock);
> +
> +	if (!num_modes && anx6345->panel)
> +		num_modes += drm_panel_get_modes(anx6345->panel);
> +
> +	return num_modes;
> +}
> +
> +static const struct drm_connector_helper_funcs anx6345_connector_helper_funcs = {
> +	.get_modes = anx6345_get_modes,
> +};
> +
> +static enum drm_connector_status anx6345_detect(struct drm_connector *connector,
> +						bool force)
> +{
> +	return connector_status_connected;
> +}
> +
> +static void
> +anx6345_connector_destroy(struct drm_connector *connector)
> +{
> +	struct anx6345 *anx6345 = connector_to_anx6345(connector);
> +
> +	if (anx6345->panel)
> +		drm_panel_detach(anx6345->panel);
> +	drm_connector_cleanup(connector);
> +}
> +
> +static const struct drm_connector_funcs anx6345_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = anx6345_detect,
> +	.destroy = anx6345_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 anx6345_bridge_attach(struct drm_bridge *bridge)
> +{
> +	struct anx6345 *anx6345 = bridge_to_anx6345(bridge);
> +	int err;
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found");
> +		return -ENODEV;
> +	}
> +
> +	/* Register aux channel */
> +	anx6345->aux.name = "DP-AUX";
> +	anx6345->aux.dev = &anx6345->client->dev;
> +	anx6345->aux.transfer = anx6345_aux_transfer;
> +
> +	err = drm_dp_aux_register(&anx6345->aux);
> +	if (err < 0) {
> +		DRM_ERROR("Failed to register aux channel: %d\n", err);
> +		return err;
> +	}
> +
> +	err = drm_connector_init(bridge->dev, &anx6345->connector,
> +				 &anx6345_connector_funcs,
> +				 DRM_MODE_CONNECTOR_eDP);
> +	if (err) {
> +		DRM_ERROR("Failed to initialize connector: %d\n", err);
> +		return err;
> +	}
> +
> +	drm_connector_helper_add(&anx6345->connector,
> +				 &anx6345_connector_helper_funcs);
> +
> +	err = drm_connector_register(&anx6345->connector);
> +	if (err) {
> +		DRM_ERROR("Failed to register connector: %d\n", err);
> +		return err;
> +	}
> +
> +	anx6345->connector.polled = DRM_CONNECTOR_POLL_HPD;
> +
> +	err = drm_connector_attach_encoder(&anx6345->connector,
> +					   bridge->encoder);
> +	if (err) {
> +		DRM_ERROR("Failed to link up connector to encoder: %d\n", err);
> +		return err;
> +	}
> +
> +	if (anx6345->panel) {
> +		err = drm_panel_attach(anx6345->panel, &anx6345->connector);
> +		if (err) {
> +			DRM_ERROR("Failed to attach panel: %d\n", err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static bool anx6345_bridge_mode_fixup(struct drm_bridge *bridge,
> +				      const struct drm_display_mode *mode,
> +				      struct drm_display_mode *adjusted_mode)
> +{
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		return false;
> +
> +	/* Max 1200p at 5.4 Ghz, one lane */
> +	if (mode->clock > 154000)
> +		return false;


These checks should be in mode_valid callback.


> +
> +	return true;
> +}
> +
> +static void anx6345_bridge_disable(struct drm_bridge *bridge)
> +{
> +	struct anx6345 *anx6345 = bridge_to_anx6345(bridge);
> +
> +	/* Power off all modules except configuration registers access */
> +	anx6345_set_bits(anx6345->map[I2C_IDX_TXCOM], SP_POWERDOWN_CTRL_REG,
> +			 SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);
> +	if (anx6345->panel)
> +		drm_panel_disable(anx6345->panel);
> +}
> +
> +static void anx6345_bridge_mode_set(struct drm_bridge *bridge,
> +				    struct drm_display_mode *mode,
> +				    struct drm_display_mode *adjusted_mode)
> +{
> +	struct anx6345 *anx6345 = bridge_to_anx6345(bridge);
> +
> +	if (WARN_ON(!anx6345->powered))
> +		return;
> +}
> +
> +static void anx6345_bridge_enable(struct drm_bridge *bridge)
> +{
> +	struct anx6345 *anx6345 = bridge_to_anx6345(bridge);
> +	int err;
> +
> +	if (anx6345->panel)
> +		drm_panel_enable(anx6345->panel);
> +
> +	err = anx6345_start(anx6345);
> +	if (err) {
> +		DRM_ERROR("Failed to initialize: %d\n", err);
> +		return;
> +	}
> +
> +	err = anx6345_config_dp_output(anx6345);
> +	if (err)
> +		DRM_ERROR("Failed to enable DP output: %d\n", err);
> +}
> +
> +static const struct drm_bridge_funcs anx6345_bridge_funcs = {
> +	.attach = anx6345_bridge_attach,
> +	.mode_fixup = anx6345_bridge_mode_fixup,
> +	.disable = anx6345_bridge_disable,
> +	.mode_set = anx6345_bridge_mode_set,
> +	.enable = anx6345_bridge_enable,
> +};
> +
> +static void unregister_i2c_dummy_clients(struct anx6345 *anx6345)
> +{
> +	unsigned int i;
> +
> +	for (i = 1; i < ARRAY_SIZE(anx6345->i2c_clients); i++)
> +		if (anx6345->i2c_clients[i] &&
> +		    anx6345->i2c_clients[i]->addr != anx6345->client->addr)
> +			i2c_unregister_device(anx6345->i2c_clients[i]);
> +}
> +
> +static const struct regmap_config anx6345_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xff,
> +};
> +
> +static const u16 anx6345_chipid_list[] = {
> +	0x6345,
> +};
> +
> +static int anx6345_i2c_probe(struct i2c_client *client,
> +			     const struct i2c_device_id *id)
> +{
> +	struct anx6345 *anx6345;
> +	struct anx6345_platform_data *pdata;
> +	unsigned int i, idl, idh, version;
> +	bool found = false;
> +	int err;
> +
> +	anx6345 = devm_kzalloc(&client->dev, sizeof(*anx6345), GFP_KERNEL);
> +	if (!anx6345)
> +		return -ENOMEM;
> +
> +	pdata = &anx6345->pdata;
> +
> +	mutex_init(&anx6345->lock);
> +
> +	anx6345->bridge.of_node = client->dev.of_node;
> +
> +	anx6345->client = client;
> +	i2c_set_clientdata(client, anx6345);
> +
> +	err = drm_of_find_panel_or_bridge(client->dev.of_node, 1, 0,
> +					  &anx6345->panel, NULL);
> +	if (err == -EPROBE_DEFER)
> +		return err;
> +
> +	if (err)
> +		DRM_DEBUG("No panel found\n");
> +
> +	err = anx6345_init_pdata(anx6345);
> +	if (err) {
> +		DRM_ERROR("Failed to initialize pdata: %d\n", err);
> +		return err;
> +	}
> +
> +	/* Map slave addresses of ANX6345 */
> +	for (i = 0; i < I2C_NUM_ADDRESSES; i++) {
> +		if (anx6345_i2c_addresses[i] >> 1 != client->addr)
> +			anx6345->i2c_clients[i] = i2c_new_dummy(client->adapter,
> +						anx6345_i2c_addresses[i] >> 1);
> +		else
> +			anx6345->i2c_clients[i] = client;


I see this contredanse is copy/pasted from anx78*, but it looks quite
complicated. As I understand there are two i2c addresses, why we cannot
assume one address is for control interfaces and another

is dummy? It would simplify the code here and in other places.


> +
> +		if (!anx6345->i2c_clients[i]) {
> +			err = -ENOMEM;
> +			DRM_ERROR("Failed to reserve I2C bus %02x\n",
> +				  anx6345_i2c_addresses[i]);
> +			goto err_unregister_i2c;
> +		}
> +
> +		anx6345->map[i] = devm_regmap_init_i2c(anx6345->i2c_clients[i],
> +						       &anx6345_regmap_config);
> +		if (IS_ERR(anx6345->map[i])) {
> +			err = PTR_ERR(anx6345->map[i]);
> +			DRM_ERROR("Failed regmap initialization %02x\n",
> +				  anx6345_i2c_addresses[i]);
> +			goto err_unregister_i2c;
> +		}
> +	}
> +
> +	/* Look for supported chip ID */
> +	anx6345_poweron(anx6345);
> +
> +	err = regmap_read(anx6345->map[I2C_IDX_TXCOM], SP_DEVICE_IDL_REG,
> +			  &idl);
> +	if (err)
> +		goto err_poweroff;
> +
> +	err = regmap_read(anx6345->map[I2C_IDX_TXCOM], SP_DEVICE_IDH_REG,
> +			  &idh);
> +	if (err)
> +		goto err_poweroff;
> +
> +	anx6345->chipid = (u8)idl | ((u8)idh << 8);
> +
> +	err = regmap_read(anx6345->map[I2C_IDX_TXCOM], SP_DEVICE_VERSION_REG,
> +			  &version);
> +	if (err)
> +		goto err_poweroff;
> +
> +	for (i = 0; i < ARRAY_SIZE(anx6345_chipid_list); i++) {
> +		if (anx6345->chipid == anx6345_chipid_list[i]) {
> +			DRM_INFO("Found ANX%x (ver. %d) eDP Transmitter\n",
> +				 anx6345->chipid, version);
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		DRM_ERROR("ANX%x (ver. %d) not supported by this driver\n",
> +			  anx6345->chipid, version);
> +		err = -ENODEV;
> +		goto err_poweroff;
> +	}


As I see chip becomes powered forever, is it OK? Usually it should be
powered only when pipeline starts, and powered-off after pipeline stops.


Regards

Andrzej


> +
> +	anx6345->bridge.funcs = &anx6345_bridge_funcs;
> +
> +	drm_bridge_add(&anx6345->bridge);
> +
> +	return 0;
> +
> +err_poweroff:
> +	anx6345_poweroff(anx6345);
> +
> +err_unregister_i2c:
> +	unregister_i2c_dummy_clients(anx6345);
> +	return err;
> +}
> +
> +static int anx6345_i2c_remove(struct i2c_client *client)
> +{
> +	struct anx6345 *anx6345 = i2c_get_clientdata(client);
> +
> +	drm_bridge_remove(&anx6345->bridge);
> +
> +	unregister_i2c_dummy_clients(anx6345);
> +
> +	kfree(anx6345->edid);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id anx6345_id[] = {
> +	{ "anx6345", 0 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, anx6345_id);
> +
> +static const struct of_device_id anx6345_match_table[] = {
> +	{ .compatible = "analogix,anx6345", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, anx6345_match_table);
> +
> +static struct i2c_driver anx6345_driver = {
> +	.driver = {
> +		   .name = "anx6345",
> +		   .of_match_table = of_match_ptr(anx6345_match_table),
> +		  },
> +	.probe = anx6345_i2c_probe,
> +	.remove = anx6345_i2c_remove,
> +	.id_table = anx6345_id,
> +};
> +module_i2c_driver(anx6345_driver);
> +
> +MODULE_DESCRIPTION("ANX6345 eDP Transmitter driver");
> +MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c b/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c
> index 9cb30962032e..53b0e73d6a24 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c
> @@ -117,7 +117,7 @@ ssize_t anx_aux_transfer(struct regmap *map_dptx, struct drm_dp_aux_msg *msg)
>  	else	/* For non-zero-sized set the length field. */
>  		ctrl1 |= (msg->size - 1) << SP_AUX_LENGTH_SHIFT;
>  
> -	if ((msg->request & DP_AUX_I2C_READ) == 0) {
> +	if ((msg->size > 0) && ((msg->request & DP_AUX_I2C_READ) == 0)) {
>  		/* When WRITE | MOT write values to data buffer */
>  		err = regmap_bulk_write(map_dptx,
>  					SP_DP_BUF_DATA0_REG, buffer,
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.h b/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.h
> index c2ca854613a0..b29a0b3bc23c 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.h
> @@ -75,7 +75,11 @@
>  #define SP_CHA_STA			BIT(2)
>  /* Bits for DP System Control Register 3 */
>  #define SP_HPD_STATUS			BIT(6)
> +#define SP_HPD_FORCE			BIT(5)
> +#define SP_HPD_CTRL			BIT(4)
>  #define SP_STRM_VALID			BIT(2)
> +#define SP_STRM_FORCE			BIT(1)
> +#define SP_STRM_CTRL			BIT(0)
>  /* Bits for DP System Control Register 4 */
>  #define SP_ENHANCED_MODE		BIT(3)
>  
> @@ -120,6 +124,9 @@
>  #define SP_LINK_BW_SET_MASK		0x1f
>  #define SP_INITIAL_SLIM_M_AUD_SEL	BIT(5)
>  
> +/* DP Lane Count Setting Register */
> +#define SP_DP_LANE_COUNT_SET_REG	0xa1
> +
>  /* DP Training Pattern Set Register */
>  #define SP_DP_TRAINING_PATTERN_SET_REG	0xa2
>  
> @@ -133,6 +140,7 @@
>  
>  /* DP Link Training Control Register */
>  #define SP_DP_LT_CTRL_REG		0xa8
> +#define SP_DP_LT_INPROGRESS		0x80
>  #define SP_LT_ERROR_TYPE_MASK		0x70
>  #  define SP_LT_NO_ERROR		0x00
>  #  define SP_LT_AUX_WRITE_ERROR		0x01
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-i2c-txcommon.h b/drivers/gpu/drm/bridge/analogix/analogix-i2c-txcommon.h
> index 7d683573e970..480c98a225b1 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix-i2c-txcommon.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-i2c-txcommon.h
> @@ -183,6 +183,9 @@
>  #define SP_VBIT				BIT(1)
>  #define SP_AUDIO_LAYOUT			BIT(0)
>  
> +/* Analog Debug Register 1 */
> +#define SP_ANALOG_DEBUG1_REG		0xdc
> +
>  /* Analog Debug Register 2 */
>  #define SP_ANALOG_DEBUG2_REG		0xdd
>  #define SP_FORCE_SW_OFF_BYPASS		0x20
Vasily Khoruzhick Feb. 15, 2019, 7:21 p.m. UTC | #3
On Fri, Feb 15, 2019 at 12:23 AM Priit Laes <plaes@plaes.org> wrote:

> > +     err = anx6345_tx_initialization(anx6345);
> > +     if (err) {
> > +             DRM_ERROR("Failed transmitter initialization: %d\n", err);
> > +             goto err_poweroff;
>
> You can move the whole err_poweroff section from below here and drop the goto.

Thanks, will do.
Vasily Khoruzhick Feb. 15, 2019, 7:36 p.m. UTC | #4
On Fri, Feb 15, 2019 at 1:13 AM Andrzej Hajda <a.hajda@samsung.com> wrote:

Hi Andrzej,

Thanks for review!

> > +#include <linux/of_gpio.h>
> Do you need this header?

I'll drop it.

> > +#include <drm/drmP.h>
>
> drmP.h is/should be deprecated.

Same here

> > +struct anx6345_platform_data {
> > +     struct regulator *dvdd12;
> > +     struct regulator *dvdd25;
> > +     struct gpio_desc *gpiod_reset;
> > +};
>
> Why do you need this struct, why just do not embed it's fields directly
> into struct anx6345 ?

OK, I'll embed it into struct anx6345

> > +     if (WARN_ON(anx6345->powered))
> > +             return;
>
> It should not happen, you can remove this warn.

OK

> > +     if (pdata->dvdd12) {
>
> If regulators are required this will be never null.

Right, and regulator subsystem will return dummy regulator if it's
missing in dts.
I'll remove redundant checks.

> > +
> > +     if (pdata->dvdd25) {
>
> ditto

OK

> > +
> > +     if (anx6345->panel)
> > +             drm_panel_prepare(anx6345->panel);
>
> again, here and below: panel is never null, check can be removed.

That's not true, panel is optional. It can be DP connector, not a panel.

> > +
> > +     gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
> > +     usleep_range(1000, 2000);
> > +
> > +     gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
>
>
> Start/stop sequence seems odd regarding reset gpio:
>
> 1. In probe reset is set to low, in poweroff to high - incosistent.
>
> 2. If in case of disabled device reset should be 0, there is no point to
> set it again to 0 three lines above.
>
> 3. I suspect in dts reset gpio should be declared as active_low, and the
> logic in the driver should be reverted, in power off it should be set to
> high, in power on it should be lowered (logically).

OK, I'll look into it.

> > +err_poweroff:
> > +     DRM_ERROR("Failed DisplayPort transmitter initialization: %d\n", err);
>
> redundant message

OK, will drop.

> > +             DRM_ERROR("Get sink count failed %d\n", err);
>
> The rule of thumb I heard is that if you start message capitalized you
> should end with dot. Since I do not know if it is enforced in kernel I
> leave the decision up to you.

I grepped DRM_ERROR in driver/gpu/drm and they do exactly the same as here.
So I'll just keep it as is for consistency.

> > +static bool anx6345_bridge_mode_fixup(struct drm_bridge *bridge,
> > +                                   const struct drm_display_mode *mode,
> > +                                   struct drm_display_mode *adjusted_mode)
> > +{
> > +     if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > +             return false;
> > +
> > +     /* Max 1200p at 5.4 Ghz, one lane */
> > +     if (mode->clock > 154000)
> > +             return false;
>
> These checks should be in mode_valid callback.

OK

> > +     /* Map slave addresses of ANX6345 */
> > +     for (i = 0; i < I2C_NUM_ADDRESSES; i++) {
> > +             if (anx6345_i2c_addresses[i] >> 1 != client->addr)
> > +                     anx6345->i2c_clients[i] = i2c_new_dummy(client->adapter,
> > +                                             anx6345_i2c_addresses[i] >> 1);
> > +             else
> > +                     anx6345->i2c_clients[i] = client;
>
>
> I see this contredanse is copy/pasted from anx78*, but it looks quite
> complicated. As I understand there are two i2c addresses, why we cannot
> assume one address is for control interfaces and another  is dummy? It would
> simplify the code here and in other places.

Sorry, I don't get you, could you elaborate? Note that anx6345 uses
both addresses,
i2c_new_dummy() just registers new i2c device bound to a dummy driver and it's
supposed to be used for devices that consume more than one i2c address.

> > +     if (!found) {
> > +             DRM_ERROR("ANX%x (ver. %d) not supported by this driver\n",
> > +                       anx6345->chipid, version);
> > +             err = -ENODEV;
> > +             goto err_poweroff;
> > +     }
>
>
> As I see chip becomes powered forever, is it OK? Usually it should be
> powered only when pipeline starts, and powered-off after pipeline stops.

I'll look into how hard it would be to implement but personally I
think it's OK for now.
We can add more sophisticated power management once this driver is merged.
Andrzej Hajda Feb. 26, 2019, 7:12 a.m. UTC | #5
On 15.02.2019 20:36, Vasily Khoruzhick wrote:
> On Fri, Feb 15, 2019 at 1:13 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> Hi Andrzej,
>
> Thanks for review!
>
>>> +#include <linux/of_gpio.h>
>> Do you need this header?
> I'll drop it.
>
>>> +#include <drm/drmP.h>
>> drmP.h is/should be deprecated.
> Same here
>
>>> +struct anx6345_platform_data {
>>> +     struct regulator *dvdd12;
>>> +     struct regulator *dvdd25;
>>> +     struct gpio_desc *gpiod_reset;
>>> +};
>> Why do you need this struct, why just do not embed it's fields directly
>> into struct anx6345 ?
> OK, I'll embed it into struct anx6345
>
>>> +     if (WARN_ON(anx6345->powered))
>>> +             return;
>> It should not happen, you can remove this warn.
> OK
>
>>> +     if (pdata->dvdd12) {
>> If regulators are required this will be never null.
> Right, and regulator subsystem will return dummy regulator if it's
> missing in dts.
> I'll remove redundant checks.
>
>>> +
>>> +     if (pdata->dvdd25) {
>> ditto
> OK
>
>>> +
>>> +     if (anx6345->panel)
>>> +             drm_panel_prepare(anx6345->panel);
>> again, here and below: panel is never null, check can be removed.
> That's not true, panel is optional. It can be DP connector, not a panel.
>
>>> +
>>> +     gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
>>> +     usleep_range(1000, 2000);
>>> +
>>> +     gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
>>
>> Start/stop sequence seems odd regarding reset gpio:
>>
>> 1. In probe reset is set to low, in poweroff to high - incosistent.
>>
>> 2. If in case of disabled device reset should be 0, there is no point to
>> set it again to 0 three lines above.
>>
>> 3. I suspect in dts reset gpio should be declared as active_low, and the
>> logic in the driver should be reverted, in power off it should be set to
>> high, in power on it should be lowered (logically).
> OK, I'll look into it.
>
>>> +err_poweroff:
>>> +     DRM_ERROR("Failed DisplayPort transmitter initialization: %d\n", err);
>> redundant message
> OK, will drop.
>
>>> +             DRM_ERROR("Get sink count failed %d\n", err);
>> The rule of thumb I heard is that if you start message capitalized you
>> should end with dot. Since I do not know if it is enforced in kernel I
>> leave the decision up to you.
> I grepped DRM_ERROR in driver/gpu/drm and they do exactly the same as here.
> So I'll just keep it as is for consistency.
>
>>> +static bool anx6345_bridge_mode_fixup(struct drm_bridge *bridge,
>>> +                                   const struct drm_display_mode *mode,
>>> +                                   struct drm_display_mode *adjusted_mode)
>>> +{
>>> +     if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>> +             return false;
>>> +
>>> +     /* Max 1200p at 5.4 Ghz, one lane */
>>> +     if (mode->clock > 154000)
>>> +             return false;
>> These checks should be in mode_valid callback.
> OK
>
>>> +     /* Map slave addresses of ANX6345 */
>>> +     for (i = 0; i < I2C_NUM_ADDRESSES; i++) {
>>> +             if (anx6345_i2c_addresses[i] >> 1 != client->addr)
>>> +                     anx6345->i2c_clients[i] = i2c_new_dummy(client->adapter,
>>> +                                             anx6345_i2c_addresses[i] >> 1);
>>> +             else
>>> +                     anx6345->i2c_clients[i] = client;
>>
>> I see this contredanse is copy/pasted from anx78*, but it looks quite
>> complicated. As I understand there are two i2c addresses, why we cannot
>> assume one address is for control interfaces and another  is dummy? It would
>> simplify the code here and in other places.
> Sorry, I don't get you, could you elaborate? Note that anx6345 uses
> both addresses,
> i2c_new_dummy() just registers new i2c device bound to a dummy driver and it's
> supposed to be used for devices that consume more than one i2c address.


My idea was to assume that ANALOGIX_I2C_DPTX is the main address, ie.
address which should be set in dts in device node reg property.

Other addresses should be registered as dummy devices during probe -
simple loop without conditionals, without redundant fields in anx6345
context - i2c_clients, client.

I do not insist on this change but I suggest as it should simplify the code.

And moreover, you can consider removing direction bit from i2c
addresses, it could be also confusing and against i2c kernel api.

>
>>> +     if (!found) {
>>> +             DRM_ERROR("ANX%x (ver. %d) not supported by this driver\n",
>>> +                       anx6345->chipid, version);
>>> +             err = -ENODEV;
>>> +             goto err_poweroff;
>>> +     }
>>
>> As I see chip becomes powered forever, is it OK? Usually it should be
>> powered only when pipeline starts, and powered-off after pipeline stops.
> I'll look into how hard it would be to implement but personally I
> think it's OK for now.
> We can add more sophisticated power management once this driver is merged.


But the rule is every resource allocated/set during lifetime of the
driver should be dropped on driver removal, so please do it at least in
remove callback.


Regards

Andrzej