All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] drm: bridge: adv7511: Add support For ADV7535
@ 2019-08-09 14:16 ` Bogdan Togorean
  0 siblings, 0 replies; 24+ messages in thread
From: Bogdan Togorean @ 2019-08-09 14:16 UTC (permalink / raw)
  To: dri-devel
  Cc: airlied, daniel, robh+dt, a.hajda, Laurent.pinchart, sam, gregkh,
	allison, tglx, matt.redfearn, linux-kernel, Bogdan Togorean

This patch-set add support for ADV7535 part in ADV7511 driver.

ADV7535 and ADV7533 are pin to pin compatible parts but ADV7535
support TMDS clock upto 148.5Mhz and resolutions up to 1080p@60Hz.

---
Changes in v2:
 - rename CONFIG_DRM_I2C_ADV7533 to CONFIG_DRM_I2C_ADV753X and 
update decription
 - removed "v1p2" index search and hardcoded it

Bogdan Togorean (2):
  dt-bindings: drm: bridge: adv7511: Add ADV7535 support
  drm: bridge: adv7511: Add support for ADV7535

 .../bindings/display/bridge/adi,adv7511.txt   | 23 +++++++------
 drivers/gpu/drm/bridge/adv7511/Kconfig        |  8 ++---
 drivers/gpu/drm/bridge/adv7511/Makefile       |  2 +-
 drivers/gpu/drm/bridge/adv7511/adv7511.h      |  4 ++-
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c  | 34 +++++++++++++------
 5 files changed, 44 insertions(+), 27 deletions(-)

-- 
2.22.0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 0/2] drm: bridge: adv7511: Add support For ADV7535
@ 2019-08-09 14:16 ` Bogdan Togorean
  0 siblings, 0 replies; 24+ messages in thread
From: Bogdan Togorean @ 2019-08-09 14:16 UTC (permalink / raw)
  To: dri-devel
  Cc: airlied, daniel, robh+dt, a.hajda, Laurent.pinchart, sam, gregkh,
	allison, tglx, matt.redfearn, linux-kernel, Bogdan Togorean

This patch-set add support for ADV7535 part in ADV7511 driver.

ADV7535 and ADV7533 are pin to pin compatible parts but ADV7535
support TMDS clock upto 148.5Mhz and resolutions up to 1080p@60Hz.

---
Changes in v2:
 - rename CONFIG_DRM_I2C_ADV7533 to CONFIG_DRM_I2C_ADV753X and 
update decription
 - removed "v1p2" index search and hardcoded it

Bogdan Togorean (2):
  dt-bindings: drm: bridge: adv7511: Add ADV7535 support
  drm: bridge: adv7511: Add support for ADV7535

 .../bindings/display/bridge/adi,adv7511.txt   | 23 +++++++------
 drivers/gpu/drm/bridge/adv7511/Kconfig        |  8 ++---
 drivers/gpu/drm/bridge/adv7511/Makefile       |  2 +-
 drivers/gpu/drm/bridge/adv7511/adv7511.h      |  4 ++-
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c  | 34 +++++++++++++------
 5 files changed, 44 insertions(+), 27 deletions(-)

-- 
2.22.0

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 1/2] dt-bindings: drm: bridge: adv7511: Add ADV7535 support
  2019-08-09 14:16 ` Bogdan Togorean
@ 2019-08-09 14:16   ` Bogdan Togorean
  -1 siblings, 0 replies; 24+ messages in thread
From: Bogdan Togorean @ 2019-08-09 14:16 UTC (permalink / raw)
  To: dri-devel
  Cc: airlied, daniel, robh+dt, a.hajda, Laurent.pinchart, sam, gregkh,
	allison, tglx, matt.redfearn, linux-kernel, Bogdan Togorean

ADV7535 is a part compatible with ADV7533 but it supports 1080p@60hz and
v1p2 supply is fixed to 1.8V

Signed-off-by: Bogdan Togorean <bogdan.togorean@analog.com>
---
 .../bindings/display/bridge/adi,adv7511.txt   | 23 ++++++++++---------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 2c887536258c..e8ddec5d9d91 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -1,10 +1,10 @@
-Analog Device ADV7511(W)/13/33 HDMI Encoders
+Analog Device ADV7511(W)/13/33/35 HDMI Encoders
 -----------------------------------------
 
-The ADV7511, ADV7511W, ADV7513 and ADV7533 are HDMI audio and video transmitters
-compatible with HDMI 1.4 and DVI 1.0. They support color space conversion,
-S/PDIF, CEC and HDCP. ADV7533 supports the DSI interface for input pixels, while
-the others support RGB interface.
+The ADV7511, ADV7511W, ADV7513, ADV7533 and ADV7535 are HDMI audio and video
+transmitters compatible with HDMI 1.4 and DVI 1.0. They support color space
+conversion, S/PDIF, CEC and HDCP. ADV7533/5 supports the DSI interface for input
+pixels, while the others support RGB interface.
 
 Required properties:
 
@@ -13,6 +13,7 @@ Required properties:
 		"adi,adv7511w"
 		"adi,adv7513"
 		"adi,adv7533"
+		"adi,adv7535"
 
 - reg: I2C slave addresses
   The ADV7511 internal registers are split into four pages exposed through
@@ -52,14 +53,14 @@ The following input format properties are required except in "rgb 1x" and
 - bgvdd-supply: A 1.8V supply that powers up the BGVDD pin. This is
   needed only for ADV7511.
 
-The following properties are required for ADV7533:
+The following properties are required for ADV7533 and ADV7535:
 
 - adi,dsi-lanes: Number of DSI data lanes connected to the DSI host. It should
   be one of 1, 2, 3 or 4.
 - a2vdd-supply: 1.8V supply that powers up the A2VDD pin on the chip.
 - v3p3-supply: A 3.3V supply that powers up the V3P3 pin on the chip.
 - v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be
-  either 1.2V or 1.8V.
+  either 1.2V or 1.8V for ADV7533 but only 1.8V for ADV7535.
 
 Optional properties:
 
@@ -71,9 +72,9 @@ Optional properties:
 - adi,embedded-sync: The input uses synchronization signals embedded in the
   data stream (similar to BT.656). Defaults to separate H/V synchronization
   signals.
-- adi,disable-timing-generator: Only for ADV7533. Disables the internal timing
-  generator. The chip will rely on the sync signals in the DSI data lanes,
-  rather than generate its own timings for HDMI output.
+- adi,disable-timing-generator: Only for ADV7533 and ADV7535. Disables the
+  internal timing generator. The chip will rely on the sync signals in the
+  DSI data lanes, rather than generate its own timings for HDMI output.
 - clocks: from common clock binding: reference to the CEC clock.
 - clock-names: from common clock binding: must be "cec".
 - reg-names : Names of maps with programmable addresses.
@@ -85,7 +86,7 @@ Required nodes:
 The ADV7511 has two video ports. Their connections are modelled using the OF
 graph bindings specified in Documentation/devicetree/bindings/graph.txt.
 
-- Video port 0 for the RGB, YUV or DSI input. In the case of ADV7533, the
+- Video port 0 for the RGB, YUV or DSI input. In the case of ADV7533/5, the
   remote endpoint phandle should be a reference to a valid mipi_dsi_host device
   node.
 - Video port 1 for the HDMI output
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 1/2] dt-bindings: drm: bridge: adv7511: Add ADV7535 support
@ 2019-08-09 14:16   ` Bogdan Togorean
  0 siblings, 0 replies; 24+ messages in thread
From: Bogdan Togorean @ 2019-08-09 14:16 UTC (permalink / raw)
  To: dri-devel
  Cc: airlied, daniel, robh+dt, a.hajda, Laurent.pinchart, sam, gregkh,
	allison, tglx, matt.redfearn, linux-kernel, Bogdan Togorean

ADV7535 is a part compatible with ADV7533 but it supports 1080p@60hz and
v1p2 supply is fixed to 1.8V

Signed-off-by: Bogdan Togorean <bogdan.togorean@analog.com>
---
 .../bindings/display/bridge/adi,adv7511.txt   | 23 ++++++++++---------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
index 2c887536258c..e8ddec5d9d91 100644
--- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
+++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
@@ -1,10 +1,10 @@
-Analog Device ADV7511(W)/13/33 HDMI Encoders
+Analog Device ADV7511(W)/13/33/35 HDMI Encoders
 -----------------------------------------
 
-The ADV7511, ADV7511W, ADV7513 and ADV7533 are HDMI audio and video transmitters
-compatible with HDMI 1.4 and DVI 1.0. They support color space conversion,
-S/PDIF, CEC and HDCP. ADV7533 supports the DSI interface for input pixels, while
-the others support RGB interface.
+The ADV7511, ADV7511W, ADV7513, ADV7533 and ADV7535 are HDMI audio and video
+transmitters compatible with HDMI 1.4 and DVI 1.0. They support color space
+conversion, S/PDIF, CEC and HDCP. ADV7533/5 supports the DSI interface for input
+pixels, while the others support RGB interface.
 
 Required properties:
 
@@ -13,6 +13,7 @@ Required properties:
 		"adi,adv7511w"
 		"adi,adv7513"
 		"adi,adv7533"
+		"adi,adv7535"
 
 - reg: I2C slave addresses
   The ADV7511 internal registers are split into four pages exposed through
@@ -52,14 +53,14 @@ The following input format properties are required except in "rgb 1x" and
 - bgvdd-supply: A 1.8V supply that powers up the BGVDD pin. This is
   needed only for ADV7511.
 
-The following properties are required for ADV7533:
+The following properties are required for ADV7533 and ADV7535:
 
 - adi,dsi-lanes: Number of DSI data lanes connected to the DSI host. It should
   be one of 1, 2, 3 or 4.
 - a2vdd-supply: 1.8V supply that powers up the A2VDD pin on the chip.
 - v3p3-supply: A 3.3V supply that powers up the V3P3 pin on the chip.
 - v1p2-supply: A supply that powers up the V1P2 pin on the chip. It can be
-  either 1.2V or 1.8V.
+  either 1.2V or 1.8V for ADV7533 but only 1.8V for ADV7535.
 
 Optional properties:
 
@@ -71,9 +72,9 @@ Optional properties:
 - adi,embedded-sync: The input uses synchronization signals embedded in the
   data stream (similar to BT.656). Defaults to separate H/V synchronization
   signals.
-- adi,disable-timing-generator: Only for ADV7533. Disables the internal timing
-  generator. The chip will rely on the sync signals in the DSI data lanes,
-  rather than generate its own timings for HDMI output.
+- adi,disable-timing-generator: Only for ADV7533 and ADV7535. Disables the
+  internal timing generator. The chip will rely on the sync signals in the
+  DSI data lanes, rather than generate its own timings for HDMI output.
 - clocks: from common clock binding: reference to the CEC clock.
 - clock-names: from common clock binding: must be "cec".
 - reg-names : Names of maps with programmable addresses.
@@ -85,7 +86,7 @@ Required nodes:
 The ADV7511 has two video ports. Their connections are modelled using the OF
 graph bindings specified in Documentation/devicetree/bindings/graph.txt.
 
-- Video port 0 for the RGB, YUV or DSI input. In the case of ADV7533, the
+- Video port 0 for the RGB, YUV or DSI input. In the case of ADV7533/5, the
   remote endpoint phandle should be a reference to a valid mipi_dsi_host device
   node.
 - Video port 1 for the HDMI output
-- 
2.22.0

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
  2019-08-09 14:16 ` Bogdan Togorean
@ 2019-08-09 14:16   ` Bogdan Togorean
  -1 siblings, 0 replies; 24+ messages in thread
From: Bogdan Togorean @ 2019-08-09 14:16 UTC (permalink / raw)
  To: dri-devel
  Cc: airlied, daniel, robh+dt, a.hajda, Laurent.pinchart, sam, gregkh,
	allison, tglx, matt.redfearn, linux-kernel, Bogdan Togorean

ADV7535 is a DSI to HDMI bridge chip like ADV7533 but it allows
1080p@60Hz. v1p2 is fixed to 1.8V on ADV7535 but on ADV7533 can be 1.2V
or 1.8V and is configurable in a register.

Signed-off-by: Bogdan Togorean <bogdan.togorean@analog.com>
---
 drivers/gpu/drm/bridge/adv7511/Kconfig       |  8 ++---
 drivers/gpu/drm/bridge/adv7511/Makefile      |  2 +-
 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  4 ++-
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 34 ++++++++++++++------
 4 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig b/drivers/gpu/drm/bridge/adv7511/Kconfig
index 8a56ff81f4fb..fa43acd46ab7 100644
--- a/drivers/gpu/drm/bridge/adv7511/Kconfig
+++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
@@ -15,16 +15,16 @@ config DRM_I2C_ADV7511_AUDIO
 	  Support the ADV7511 HDMI Audio interface. This is used in
 	  conjunction with the AV7511  HDMI driver.
 
-config DRM_I2C_ADV7533
-	bool "ADV7533 encoder"
+config DRM_I2C_ADV753x
+	bool "ADV753x encoder"
 	depends on DRM_I2C_ADV7511
 	select DRM_MIPI_DSI
 	default y
 	help
-	  Support for the Analog Devices ADV7533 DSI to HDMI encoder.
+	  Support for the Analog Devices ADV7533/5 DSI to HDMI encoder.
 
 config DRM_I2C_ADV7511_CEC
-	bool "ADV7511/33 HDMI CEC driver"
+	bool "ADV7511/33/35 HDMI CEC driver"
 	depends on DRM_I2C_ADV7511
 	select CEC_CORE
 	default y
diff --git a/drivers/gpu/drm/bridge/adv7511/Makefile b/drivers/gpu/drm/bridge/adv7511/Makefile
index b46ebeb35fd4..319efddb268e 100644
--- a/drivers/gpu/drm/bridge/adv7511/Makefile
+++ b/drivers/gpu/drm/bridge/adv7511/Makefile
@@ -2,5 +2,5 @@
 adv7511-y := adv7511_drv.o
 adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
 adv7511-$(CONFIG_DRM_I2C_ADV7511_CEC) += adv7511_cec.o
-adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
+adv7511-$(CONFIG_DRM_I2C_ADV753x) += adv7533.o
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 52b2adfdc877..38288c3c3c53 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -91,6 +91,7 @@
 #define ADV7511_REG_ARC_CTRL			0xdf
 #define ADV7511_REG_CEC_I2C_ADDR		0xe1
 #define ADV7511_REG_CEC_CTRL			0xe2
+#define ADV7511_REG_SUPPLY_SELECT		0xe4
 #define ADV7511_REG_CHIP_ID_HIGH		0xf5
 #define ADV7511_REG_CHIP_ID_LOW			0xf6
 
@@ -320,6 +321,7 @@ struct adv7511_video_config {
 enum adv7511_type {
 	ADV7511,
 	ADV7533,
+	ADV7535,
 };
 
 #define ADV7511_MAX_ADDRS 3
@@ -393,7 +395,7 @@ static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
 }
 #endif
 
-#ifdef CONFIG_DRM_I2C_ADV7533
+#ifdef CONFIG_DRM_I2C_ADV753x
 void adv7533_dsi_power_on(struct adv7511 *adv);
 void adv7533_dsi_power_off(struct adv7511 *adv);
 void adv7533_mode_set(struct adv7511 *adv, const struct drm_display_mode *mode);
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index f6d2681f6927..b1501344df3e 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -367,7 +367,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 	 */
 	regcache_sync(adv7511->regmap);
 
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		adv7533_dsi_power_on(adv7511);
 	adv7511->powered = true;
 }
@@ -387,7 +387,7 @@ static void __adv7511_power_off(struct adv7511 *adv7511)
 static void adv7511_power_off(struct adv7511 *adv7511)
 {
 	__adv7511_power_off(adv7511);
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		adv7533_dsi_power_off(adv7511);
 	adv7511->powered = false;
 }
@@ -761,7 +761,7 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
 	regmap_update_bits(adv7511->regmap, 0x17,
 		0x60, (vsync_polarity << 6) | (hsync_polarity << 5));
 
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		adv7533_mode_set(adv7511, adj_mode);
 
 	drm_mode_copy(&adv7511->curr_mode, adj_mode);
@@ -874,7 +874,7 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
 				 &adv7511_connector_helper_funcs);
 	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
 
-	if (adv->type == ADV7533)
+	if (adv->type == ADV7533 || adv->type == ADV7535)
 		ret = adv7533_attach_dsi(adv);
 
 	if (adv->i2c_main->irq)
@@ -903,6 +903,7 @@ static const char * const adv7511_supply_names[] = {
 	"dvdd-3v",
 };
 
+/* The order of entries is important. If changed update hardcoded indices */
 static const char * const adv7533_supply_names[] = {
 	"avdd",
 	"dvdd",
@@ -952,7 +953,7 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
 	struct i2c_client *i2c = to_i2c_client(dev);
 	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		reg -= ADV7533_REG_CEC_OFFSET;
 
 	switch (reg) {
@@ -994,7 +995,7 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
 		goto err;
 	}
 
-	if (adv->type == ADV7533) {
+	if (adv->type == ADV7533 || adv->type == ADV7535) {
 		ret = adv7533_patch_cec_registers(adv);
 		if (ret)
 			goto err;
@@ -1094,8 +1095,9 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	struct adv7511_link_config link_config;
 	struct adv7511 *adv7511;
 	struct device *dev = &i2c->dev;
+	struct regulator *reg_v1p2;
 	unsigned int val;
-	int ret;
+	int ret, reg_v1p2_uV;
 
 	if (!dev->of_node)
 		return -EINVAL;
@@ -1163,6 +1165,16 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	if (ret)
 		goto uninit_regulators;
 
+	if (adv7511->type == ADV7533) {
+		reg_v1p2 = adv7511->supplies[5].consumer;
+		reg_v1p2_uV = regulator_get_voltage(reg_v1p2);
+
+		if (reg_v1p2_uV == 1200000) {
+			regmap_update_bits(adv7511->regmap,
+				ADV7511_REG_SUPPLY_SELECT, 0x80, 0x80);
+		}
+	}
+
 	adv7511_packet_disable(adv7511, 0xffff);
 
 	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
@@ -1242,7 +1254,7 @@ static int adv7511_remove(struct i2c_client *i2c)
 {
 	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		adv7533_detach_dsi(adv7511);
 	i2c_unregister_device(adv7511->i2c_cec);
 	if (adv7511->cec_clk)
@@ -1266,8 +1278,9 @@ static const struct i2c_device_id adv7511_i2c_ids[] = {
 	{ "adv7511", ADV7511 },
 	{ "adv7511w", ADV7511 },
 	{ "adv7513", ADV7511 },
-#ifdef CONFIG_DRM_I2C_ADV7533
+#ifdef CONFIG_DRM_I2C_ADV753x
 	{ "adv7533", ADV7533 },
+	{ "adv7535", ADV7535 },
 #endif
 	{ }
 };
@@ -1277,8 +1290,9 @@ static const struct of_device_id adv7511_of_ids[] = {
 	{ .compatible = "adi,adv7511", .data = (void *)ADV7511 },
 	{ .compatible = "adi,adv7511w", .data = (void *)ADV7511 },
 	{ .compatible = "adi,adv7513", .data = (void *)ADV7511 },
-#ifdef CONFIG_DRM_I2C_ADV7533
+#ifdef CONFIG_DRM_I2C_ADV753x
 	{ .compatible = "adi,adv7533", .data = (void *)ADV7533 },
+	{ .compatible = "adi,adv7535", .data = (void *)ADV7535 },
 #endif
 	{ }
 };
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-08-09 14:16   ` Bogdan Togorean
  0 siblings, 0 replies; 24+ messages in thread
From: Bogdan Togorean @ 2019-08-09 14:16 UTC (permalink / raw)
  To: dri-devel
  Cc: airlied, daniel, robh+dt, a.hajda, Laurent.pinchart, sam, gregkh,
	allison, tglx, matt.redfearn, linux-kernel, Bogdan Togorean

ADV7535 is a DSI to HDMI bridge chip like ADV7533 but it allows
1080p@60Hz. v1p2 is fixed to 1.8V on ADV7535 but on ADV7533 can be 1.2V
or 1.8V and is configurable in a register.

Signed-off-by: Bogdan Togorean <bogdan.togorean@analog.com>
---
 drivers/gpu/drm/bridge/adv7511/Kconfig       |  8 ++---
 drivers/gpu/drm/bridge/adv7511/Makefile      |  2 +-
 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  4 ++-
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 34 ++++++++++++++------
 4 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig b/drivers/gpu/drm/bridge/adv7511/Kconfig
index 8a56ff81f4fb..fa43acd46ab7 100644
--- a/drivers/gpu/drm/bridge/adv7511/Kconfig
+++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
@@ -15,16 +15,16 @@ config DRM_I2C_ADV7511_AUDIO
 	  Support the ADV7511 HDMI Audio interface. This is used in
 	  conjunction with the AV7511  HDMI driver.
 
-config DRM_I2C_ADV7533
-	bool "ADV7533 encoder"
+config DRM_I2C_ADV753x
+	bool "ADV753x encoder"
 	depends on DRM_I2C_ADV7511
 	select DRM_MIPI_DSI
 	default y
 	help
-	  Support for the Analog Devices ADV7533 DSI to HDMI encoder.
+	  Support for the Analog Devices ADV7533/5 DSI to HDMI encoder.
 
 config DRM_I2C_ADV7511_CEC
-	bool "ADV7511/33 HDMI CEC driver"
+	bool "ADV7511/33/35 HDMI CEC driver"
 	depends on DRM_I2C_ADV7511
 	select CEC_CORE
 	default y
diff --git a/drivers/gpu/drm/bridge/adv7511/Makefile b/drivers/gpu/drm/bridge/adv7511/Makefile
index b46ebeb35fd4..319efddb268e 100644
--- a/drivers/gpu/drm/bridge/adv7511/Makefile
+++ b/drivers/gpu/drm/bridge/adv7511/Makefile
@@ -2,5 +2,5 @@
 adv7511-y := adv7511_drv.o
 adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
 adv7511-$(CONFIG_DRM_I2C_ADV7511_CEC) += adv7511_cec.o
-adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
+adv7511-$(CONFIG_DRM_I2C_ADV753x) += adv7533.o
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 52b2adfdc877..38288c3c3c53 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -91,6 +91,7 @@
 #define ADV7511_REG_ARC_CTRL			0xdf
 #define ADV7511_REG_CEC_I2C_ADDR		0xe1
 #define ADV7511_REG_CEC_CTRL			0xe2
+#define ADV7511_REG_SUPPLY_SELECT		0xe4
 #define ADV7511_REG_CHIP_ID_HIGH		0xf5
 #define ADV7511_REG_CHIP_ID_LOW			0xf6
 
@@ -320,6 +321,7 @@ struct adv7511_video_config {
 enum adv7511_type {
 	ADV7511,
 	ADV7533,
+	ADV7535,
 };
 
 #define ADV7511_MAX_ADDRS 3
@@ -393,7 +395,7 @@ static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
 }
 #endif
 
-#ifdef CONFIG_DRM_I2C_ADV7533
+#ifdef CONFIG_DRM_I2C_ADV753x
 void adv7533_dsi_power_on(struct adv7511 *adv);
 void adv7533_dsi_power_off(struct adv7511 *adv);
 void adv7533_mode_set(struct adv7511 *adv, const struct drm_display_mode *mode);
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index f6d2681f6927..b1501344df3e 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -367,7 +367,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 	 */
 	regcache_sync(adv7511->regmap);
 
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		adv7533_dsi_power_on(adv7511);
 	adv7511->powered = true;
 }
@@ -387,7 +387,7 @@ static void __adv7511_power_off(struct adv7511 *adv7511)
 static void adv7511_power_off(struct adv7511 *adv7511)
 {
 	__adv7511_power_off(adv7511);
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		adv7533_dsi_power_off(adv7511);
 	adv7511->powered = false;
 }
@@ -761,7 +761,7 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
 	regmap_update_bits(adv7511->regmap, 0x17,
 		0x60, (vsync_polarity << 6) | (hsync_polarity << 5));
 
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		adv7533_mode_set(adv7511, adj_mode);
 
 	drm_mode_copy(&adv7511->curr_mode, adj_mode);
@@ -874,7 +874,7 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
 				 &adv7511_connector_helper_funcs);
 	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
 
-	if (adv->type == ADV7533)
+	if (adv->type == ADV7533 || adv->type == ADV7535)
 		ret = adv7533_attach_dsi(adv);
 
 	if (adv->i2c_main->irq)
@@ -903,6 +903,7 @@ static const char * const adv7511_supply_names[] = {
 	"dvdd-3v",
 };
 
+/* The order of entries is important. If changed update hardcoded indices */
 static const char * const adv7533_supply_names[] = {
 	"avdd",
 	"dvdd",
@@ -952,7 +953,7 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
 	struct i2c_client *i2c = to_i2c_client(dev);
 	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		reg -= ADV7533_REG_CEC_OFFSET;
 
 	switch (reg) {
@@ -994,7 +995,7 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
 		goto err;
 	}
 
-	if (adv->type == ADV7533) {
+	if (adv->type == ADV7533 || adv->type == ADV7535) {
 		ret = adv7533_patch_cec_registers(adv);
 		if (ret)
 			goto err;
@@ -1094,8 +1095,9 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	struct adv7511_link_config link_config;
 	struct adv7511 *adv7511;
 	struct device *dev = &i2c->dev;
+	struct regulator *reg_v1p2;
 	unsigned int val;
-	int ret;
+	int ret, reg_v1p2_uV;
 
 	if (!dev->of_node)
 		return -EINVAL;
@@ -1163,6 +1165,16 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	if (ret)
 		goto uninit_regulators;
 
+	if (adv7511->type == ADV7533) {
+		reg_v1p2 = adv7511->supplies[5].consumer;
+		reg_v1p2_uV = regulator_get_voltage(reg_v1p2);
+
+		if (reg_v1p2_uV == 1200000) {
+			regmap_update_bits(adv7511->regmap,
+				ADV7511_REG_SUPPLY_SELECT, 0x80, 0x80);
+		}
+	}
+
 	adv7511_packet_disable(adv7511, 0xffff);
 
 	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
@@ -1242,7 +1254,7 @@ static int adv7511_remove(struct i2c_client *i2c)
 {
 	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
 
-	if (adv7511->type == ADV7533)
+	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
 		adv7533_detach_dsi(adv7511);
 	i2c_unregister_device(adv7511->i2c_cec);
 	if (adv7511->cec_clk)
@@ -1266,8 +1278,9 @@ static const struct i2c_device_id adv7511_i2c_ids[] = {
 	{ "adv7511", ADV7511 },
 	{ "adv7511w", ADV7511 },
 	{ "adv7513", ADV7511 },
-#ifdef CONFIG_DRM_I2C_ADV7533
+#ifdef CONFIG_DRM_I2C_ADV753x
 	{ "adv7533", ADV7533 },
+	{ "adv7535", ADV7535 },
 #endif
 	{ }
 };
@@ -1277,8 +1290,9 @@ static const struct of_device_id adv7511_of_ids[] = {
 	{ .compatible = "adi,adv7511", .data = (void *)ADV7511 },
 	{ .compatible = "adi,adv7511w", .data = (void *)ADV7511 },
 	{ .compatible = "adi,adv7513", .data = (void *)ADV7511 },
-#ifdef CONFIG_DRM_I2C_ADV7533
+#ifdef CONFIG_DRM_I2C_ADV753x
 	{ .compatible = "adi,adv7533", .data = (void *)ADV7533 },
+	{ .compatible = "adi,adv7535", .data = (void *)ADV7535 },
 #endif
 	{ }
 };
-- 
2.22.0

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
  2019-08-09 14:16   ` Bogdan Togorean
  (?)
@ 2019-08-09 15:25   ` Sam Ravnborg
  2019-08-19  8:59       ` Togorean, Bogdan
  -1 siblings, 1 reply; 24+ messages in thread
From: Sam Ravnborg @ 2019-08-09 15:25 UTC (permalink / raw)
  To: Bogdan Togorean
  Cc: dri-devel, airlied, daniel, robh+dt, a.hajda, Laurent.pinchart,
	gregkh, allison, tglx, matt.redfearn, linux-kernel

Hi Bogdan.

This patch triggered a few general comments.

> --- a/drivers/gpu/drm/bridge/adv7511/Makefile
> +++ b/drivers/gpu/drm/bridge/adv7511/Makefile
> @@ -2,5 +2,5 @@
>  adv7511-y := adv7511_drv.o
>  adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
>  adv7511-$(CONFIG_DRM_I2C_ADV7511_CEC) += adv7511_cec.o
> -adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
> +adv7511-$(CONFIG_DRM_I2C_ADV753x) += adv7533.o
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index 52b2adfdc877..38288c3c3c53 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -91,6 +91,7 @@
>  #define ADV7511_REG_ARC_CTRL			0xdf
>  #define ADV7511_REG_CEC_I2C_ADDR		0xe1
>  #define ADV7511_REG_CEC_CTRL			0xe2
> +#define ADV7511_REG_SUPPLY_SELECT		0xe4
>  #define ADV7511_REG_CHIP_ID_HIGH		0xf5
>  #define ADV7511_REG_CHIP_ID_LOW			0xf6
>  
> @@ -320,6 +321,7 @@ struct adv7511_video_config {
>  enum adv7511_type {
>  	ADV7511,
>  	ADV7533,
> +	ADV7535,
>  };
>  
>  #define ADV7511_MAX_ADDRS 3
> @@ -393,7 +395,7 @@ static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>  }
>  #endif
>  
> -#ifdef CONFIG_DRM_I2C_ADV7533
> +#ifdef CONFIG_DRM_I2C_ADV753x
>  void adv7533_dsi_power_on(struct adv7511 *adv);
>  void adv7533_dsi_power_off(struct adv7511 *adv);
>  void adv7533_mode_set(struct adv7511 *adv, const struct drm_display_mode *mode);

The else part here define dummy functions.

> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index f6d2681f6927..b1501344df3e 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -367,7 +367,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
>  	 */
>  	regcache_sync(adv7511->regmap);
>  
> -	if (adv7511->type == ADV7533)
> +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
>  		adv7533_dsi_power_on(adv7511);

In the driver we check for adv7511->type - and call
adv7533_dsi_power_on() only for the two types where we have this
function defined.
A simpler approach would be to always call adv7533_dsi_power_on(), and
let the existing logic pick up the right version.
The dummy version should then return 0 to say OK.

Same goes for several places below.


>  	adv7511->powered = true;
>  }
> @@ -387,7 +387,7 @@ static void __adv7511_power_off(struct adv7511 *adv7511)
>  static void adv7511_power_off(struct adv7511 *adv7511)
>  {
>  	__adv7511_power_off(adv7511);
> -	if (adv7511->type == ADV7533)
> +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
>  		adv7533_dsi_power_off(adv7511);
>  	adv7511->powered = false;
>  }
> @@ -761,7 +761,7 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
>  	regmap_update_bits(adv7511->regmap, 0x17,
>  		0x60, (vsync_polarity << 6) | (hsync_polarity << 5));
>  
> -	if (adv7511->type == ADV7533)
> +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
>  		adv7533_mode_set(adv7511, adj_mode);
>  
>  	drm_mode_copy(&adv7511->curr_mode, adj_mode);
> @@ -874,7 +874,7 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
>  				 &adv7511_connector_helper_funcs);
>  	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
>  
> -	if (adv->type == ADV7533)
> +	if (adv->type == ADV7533 || adv->type == ADV7535)
>  		ret = adv7533_attach_dsi(adv);
>  
>  	if (adv->i2c_main->irq)
> @@ -903,6 +903,7 @@ static const char * const adv7511_supply_names[] = {
>  	"dvdd-3v",
>  };
>  
> +/* The order of entries is important. If changed update hardcoded indices */
>  static const char * const adv7533_supply_names[] = {
>  	"avdd",
>  	"dvdd",
> @@ -952,7 +953,7 @@ static bool adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
>  	struct i2c_client *i2c = to_i2c_client(dev);
>  	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>  
> -	if (adv7511->type == ADV7533)
> +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
>  		reg -= ADV7533_REG_CEC_OFFSET;
>  
>  	switch (reg) {
> @@ -994,7 +995,7 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>  		goto err;
>  	}
>  
> -	if (adv->type == ADV7533) {
> +	if (adv->type == ADV7533 || adv->type == ADV7535) {
>  		ret = adv7533_patch_cec_registers(adv);
>  		if (ret)
>  			goto err;
> @@ -1094,8 +1095,9 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  	struct adv7511_link_config link_config;
>  	struct adv7511 *adv7511;
>  	struct device *dev = &i2c->dev;
> +	struct regulator *reg_v1p2;
>  	unsigned int val;
> -	int ret;
> +	int ret, reg_v1p2_uV;
>  
>  	if (!dev->of_node)
>  		return -EINVAL;
> @@ -1163,6 +1165,16 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  	if (ret)
>  		goto uninit_regulators;
>  
> +	if (adv7511->type == ADV7533) {
> +		reg_v1p2 = adv7511->supplies[5].consumer;
> +		reg_v1p2_uV = regulator_get_voltage(reg_v1p2);
> +
> +		if (reg_v1p2_uV == 1200000) {
> +			regmap_update_bits(adv7511->regmap,
> +				ADV7511_REG_SUPPLY_SELECT, 0x80, 0x80);
> +		}
> +	}
> +
>  	adv7511_packet_disable(adv7511, 0xffff);
>  
>  	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
> @@ -1242,7 +1254,7 @@ static int adv7511_remove(struct i2c_client *i2c)
>  {
>  	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>  
> -	if (adv7511->type == ADV7533)
> +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
>  		adv7533_detach_dsi(adv7511);
>  	i2c_unregister_device(adv7511->i2c_cec);
>  	if (adv7511->cec_clk)
> @@ -1266,8 +1278,9 @@ static const struct i2c_device_id adv7511_i2c_ids[] = {
>  	{ "adv7511", ADV7511 },
>  	{ "adv7511w", ADV7511 },
>  	{ "adv7513", ADV7511 },
> -#ifdef CONFIG_DRM_I2C_ADV7533
> +#ifdef CONFIG_DRM_I2C_ADV753x
>  	{ "adv7533", ADV7533 },
> +	{ "adv7535", ADV7535 },
>  #endif

This ifdef may not be needed??
If we did not get this type we will not look it up.

>  	{ }
>  };
> @@ -1277,8 +1290,9 @@ static const struct of_device_id adv7511_of_ids[] = {
>  	{ .compatible = "adi,adv7511", .data = (void *)ADV7511 },
>  	{ .compatible = "adi,adv7511w", .data = (void *)ADV7511 },
>  	{ .compatible = "adi,adv7513", .data = (void *)ADV7511 },
> -#ifdef CONFIG_DRM_I2C_ADV7533
> +#ifdef CONFIG_DRM_I2C_ADV753x
>  	{ .compatible = "adi,adv7533", .data = (void *)ADV7533 },
> +	{ .compatible = "adi,adv7535", .data = (void *)ADV7535 },
>  #endif
>  	{ }
>  };

	Sam

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
  2019-08-09 15:25   ` Sam Ravnborg
@ 2019-08-19  8:59       ` Togorean, Bogdan
  0 siblings, 0 replies; 24+ messages in thread
From: Togorean, Bogdan @ 2019-08-19  8:59 UTC (permalink / raw)
  To: sam
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, daniel, robh+dt

Thank you Sam for review,

On Fri, 2019-08-09 at 17:25 +0200, Sam Ravnborg wrote:
> [External]
> 
> Hi Bogdan.
> 
> This patch triggered a few general comments.
> 
> > --- a/drivers/gpu/drm/bridge/adv7511/Makefile
> > +++ b/drivers/gpu/drm/bridge/adv7511/Makefile
> > @@ -2,5 +2,5 @@
> >  adv7511-y := adv7511_drv.o
> >  adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
> >  adv7511-$(CONFIG_DRM_I2C_ADV7511_CEC) += adv7511_cec.o
> > -adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
> > +adv7511-$(CONFIG_DRM_I2C_ADV753x) += adv7533.o
> >  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > index 52b2adfdc877..38288c3c3c53 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > @@ -91,6 +91,7 @@
> >  #define ADV7511_REG_ARC_CTRL			0xdf
> >  #define ADV7511_REG_CEC_I2C_ADDR		0xe1
> >  #define ADV7511_REG_CEC_CTRL			0xe2
> > +#define ADV7511_REG_SUPPLY_SELECT		0xe4
> >  #define ADV7511_REG_CHIP_ID_HIGH		0xf5
> >  #define ADV7511_REG_CHIP_ID_LOW			0xf6
> >  
> > @@ -320,6 +321,7 @@ struct adv7511_video_config {
> >  enum adv7511_type {
> >  	ADV7511,
> >  	ADV7533,
> > +	ADV7535,
> >  };
> >  
> >  #define ADV7511_MAX_ADDRS 3
> > @@ -393,7 +395,7 @@ static inline int adv7511_cec_init(struct
> > device *dev, struct adv7511 *adv7511)
> >  }
> >  #endif
> >  
> > -#ifdef CONFIG_DRM_I2C_ADV7533
> > +#ifdef CONFIG_DRM_I2C_ADV753x
> >  void adv7533_dsi_power_on(struct adv7511 *adv);
> >  void adv7533_dsi_power_off(struct adv7511 *adv);
> >  void adv7533_mode_set(struct adv7511 *adv, const struct
> > drm_display_mode *mode);
> 
> The else part here define dummy functions.
> 
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index f6d2681f6927..b1501344df3e 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -367,7 +367,7 @@ static void adv7511_power_on(struct adv7511
> > *adv7511)
> >  	 */
> >  	regcache_sync(adv7511->regmap);
> >  
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		adv7533_dsi_power_on(adv7511);
> 
> In the driver we check for adv7511->type - and call
> adv7533_dsi_power_on() only for the two types where we have this
> function defined.
> A simpler approach would be to always call adv7533_dsi_power_on(),
> and
> let the existing logic pick up the right version.
> The dummy version should then return 0 to say OK.
> 
> Same goes for several places below.
> 
I agree with your remarks. Will implement them in v3
> 
> >  	adv7511->powered = true;
> >  }
> > @@ -387,7 +387,7 @@ static void __adv7511_power_off(struct adv7511
> > *adv7511)
> >  static void adv7511_power_off(struct adv7511 *adv7511)
> >  {
> >  	__adv7511_power_off(adv7511);
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		adv7533_dsi_power_off(adv7511);
> >  	adv7511->powered = false;
> >  }
> > @@ -761,7 +761,7 @@ static void adv7511_mode_set(struct adv7511
> > *adv7511,
> >  	regmap_update_bits(adv7511->regmap, 0x17,
> >  		0x60, (vsync_polarity << 6) | (hsync_polarity << 5));
> >  
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		adv7533_mode_set(adv7511, adj_mode);
> >  
> >  	drm_mode_copy(&adv7511->curr_mode, adj_mode);
> > @@ -874,7 +874,7 @@ static int adv7511_bridge_attach(struct
> > drm_bridge *bridge)
> >  				 &adv7511_connector_helper_funcs);
> >  	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
> >  
> > -	if (adv->type == ADV7533)
> > +	if (adv->type == ADV7533 || adv->type == ADV7535)
> >  		ret = adv7533_attach_dsi(adv);
> >  
> >  	if (adv->i2c_main->irq)
> > @@ -903,6 +903,7 @@ static const char * const
> > adv7511_supply_names[] = {
> >  	"dvdd-3v",
> >  };
> >  
> > +/* The order of entries is important. If changed update hardcoded
> > indices */
> >  static const char * const adv7533_supply_names[] = {
> >  	"avdd",
> >  	"dvdd",
> > @@ -952,7 +953,7 @@ static bool
> > adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
> >  	struct i2c_client *i2c = to_i2c_client(dev);
> >  	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
> >  
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		reg -= ADV7533_REG_CEC_OFFSET;
> >  
> >  	switch (reg) {
> > @@ -994,7 +995,7 @@ static int adv7511_init_cec_regmap(struct
> > adv7511 *adv)
> >  		goto err;
> >  	}
> >  
> > -	if (adv->type == ADV7533) {
> > +	if (adv->type == ADV7533 || adv->type == ADV7535) {
> >  		ret = adv7533_patch_cec_registers(adv);
> >  		if (ret)
> >  			goto err;
> > @@ -1094,8 +1095,9 @@ static int adv7511_probe(struct i2c_client
> > *i2c, const struct i2c_device_id *id)
> >  	struct adv7511_link_config link_config;
> >  	struct adv7511 *adv7511;
> >  	struct device *dev = &i2c->dev;
> > +	struct regulator *reg_v1p2;
> >  	unsigned int val;
> > -	int ret;
> > +	int ret, reg_v1p2_uV;
> >  
> >  	if (!dev->of_node)
> >  		return -EINVAL;
> > @@ -1163,6 +1165,16 @@ static int adv7511_probe(struct i2c_client
> > *i2c, const struct i2c_device_id *id)
> >  	if (ret)
> >  		goto uninit_regulators;
> >  
> > +	if (adv7511->type == ADV7533) {
> > +		reg_v1p2 = adv7511->supplies[5].consumer;
> > +		reg_v1p2_uV = regulator_get_voltage(reg_v1p2);
> > +
> > +		if (reg_v1p2_uV == 1200000) {
> > +			regmap_update_bits(adv7511->regmap,
> > +				ADV7511_REG_SUPPLY_SELECT, 0x80, 0x80);
> > +		}
> > +	}
> > +
> >  	adv7511_packet_disable(adv7511, 0xffff);
> >  
> >  	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
> > @@ -1242,7 +1254,7 @@ static int adv7511_remove(struct i2c_client
> > *i2c)
> >  {
> >  	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
> >  
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		adv7533_detach_dsi(adv7511);
> >  	i2c_unregister_device(adv7511->i2c_cec);
> >  	if (adv7511->cec_clk)
> > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > adv7511_i2c_ids[] = {
> >  	{ "adv7511", ADV7511 },
> >  	{ "adv7511w", ADV7511 },
> >  	{ "adv7513", ADV7511 },
> > -#ifdef CONFIG_DRM_I2C_ADV7533
> > +#ifdef CONFIG_DRM_I2C_ADV753x
> >  	{ "adv7533", ADV7533 },
> > +	{ "adv7535", ADV7535 },
> >  #endif
> 
> This ifdef may not be needed??
> If we did not get this type we will not look it up.
But if we have defined in DT adv7533/5 device but
CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV. That
would be ok?
> 
> >  	{ }
> >  };
> > @@ -1277,8 +1290,9 @@ static const struct of_device_id
> > adv7511_of_ids[] = {
> >  	{ .compatible = "adi,adv7511", .data = (void *)ADV7511 },
> >  	{ .compatible = "adi,adv7511w", .data = (void *)ADV7511 },
> >  	{ .compatible = "adi,adv7513", .data = (void *)ADV7511 },
> > -#ifdef CONFIG_DRM_I2C_ADV7533
> > +#ifdef CONFIG_DRM_I2C_ADV753x
> >  	{ .compatible = "adi,adv7533", .data = (void *)ADV7533 },
> > +	{ .compatible = "adi,adv7535", .data = (void *)ADV7535 },
> >  #endif
> >  	{ }
> >  };
> 
> 	Sam

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-08-19  8:59       ` Togorean, Bogdan
  0 siblings, 0 replies; 24+ messages in thread
From: Togorean, Bogdan @ 2019-08-19  8:59 UTC (permalink / raw)
  To: sam
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, daniel, robh+dt

Thank you Sam for review,

On Fri, 2019-08-09 at 17:25 +0200, Sam Ravnborg wrote:
> [External]
> 
> Hi Bogdan.
> 
> This patch triggered a few general comments.
> 
> > --- a/drivers/gpu/drm/bridge/adv7511/Makefile
> > +++ b/drivers/gpu/drm/bridge/adv7511/Makefile
> > @@ -2,5 +2,5 @@
> >  adv7511-y := adv7511_drv.o
> >  adv7511-$(CONFIG_DRM_I2C_ADV7511_AUDIO) += adv7511_audio.o
> >  adv7511-$(CONFIG_DRM_I2C_ADV7511_CEC) += adv7511_cec.o
> > -adv7511-$(CONFIG_DRM_I2C_ADV7533) += adv7533.o
> > +adv7511-$(CONFIG_DRM_I2C_ADV753x) += adv7533.o
> >  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511.o
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > index 52b2adfdc877..38288c3c3c53 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> > @@ -91,6 +91,7 @@
> >  #define ADV7511_REG_ARC_CTRL			0xdf
> >  #define ADV7511_REG_CEC_I2C_ADDR		0xe1
> >  #define ADV7511_REG_CEC_CTRL			0xe2
> > +#define ADV7511_REG_SUPPLY_SELECT		0xe4
> >  #define ADV7511_REG_CHIP_ID_HIGH		0xf5
> >  #define ADV7511_REG_CHIP_ID_LOW			0xf6
> >  
> > @@ -320,6 +321,7 @@ struct adv7511_video_config {
> >  enum adv7511_type {
> >  	ADV7511,
> >  	ADV7533,
> > +	ADV7535,
> >  };
> >  
> >  #define ADV7511_MAX_ADDRS 3
> > @@ -393,7 +395,7 @@ static inline int adv7511_cec_init(struct
> > device *dev, struct adv7511 *adv7511)
> >  }
> >  #endif
> >  
> > -#ifdef CONFIG_DRM_I2C_ADV7533
> > +#ifdef CONFIG_DRM_I2C_ADV753x
> >  void adv7533_dsi_power_on(struct adv7511 *adv);
> >  void adv7533_dsi_power_off(struct adv7511 *adv);
> >  void adv7533_mode_set(struct adv7511 *adv, const struct
> > drm_display_mode *mode);
> 
> The else part here define dummy functions.
> 
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index f6d2681f6927..b1501344df3e 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -367,7 +367,7 @@ static void adv7511_power_on(struct adv7511
> > *adv7511)
> >  	 */
> >  	regcache_sync(adv7511->regmap);
> >  
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		adv7533_dsi_power_on(adv7511);
> 
> In the driver we check for adv7511->type - and call
> adv7533_dsi_power_on() only for the two types where we have this
> function defined.
> A simpler approach would be to always call adv7533_dsi_power_on(),
> and
> let the existing logic pick up the right version.
> The dummy version should then return 0 to say OK.
> 
> Same goes for several places below.
> 
I agree with your remarks. Will implement them in v3
> 
> >  	adv7511->powered = true;
> >  }
> > @@ -387,7 +387,7 @@ static void __adv7511_power_off(struct adv7511
> > *adv7511)
> >  static void adv7511_power_off(struct adv7511 *adv7511)
> >  {
> >  	__adv7511_power_off(adv7511);
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		adv7533_dsi_power_off(adv7511);
> >  	adv7511->powered = false;
> >  }
> > @@ -761,7 +761,7 @@ static void adv7511_mode_set(struct adv7511
> > *adv7511,
> >  	regmap_update_bits(adv7511->regmap, 0x17,
> >  		0x60, (vsync_polarity << 6) | (hsync_polarity << 5));
> >  
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		adv7533_mode_set(adv7511, adj_mode);
> >  
> >  	drm_mode_copy(&adv7511->curr_mode, adj_mode);
> > @@ -874,7 +874,7 @@ static int adv7511_bridge_attach(struct
> > drm_bridge *bridge)
> >  				 &adv7511_connector_helper_funcs);
> >  	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
> >  
> > -	if (adv->type == ADV7533)
> > +	if (adv->type == ADV7533 || adv->type == ADV7535)
> >  		ret = adv7533_attach_dsi(adv);
> >  
> >  	if (adv->i2c_main->irq)
> > @@ -903,6 +903,7 @@ static const char * const
> > adv7511_supply_names[] = {
> >  	"dvdd-3v",
> >  };
> >  
> > +/* The order of entries is important. If changed update hardcoded
> > indices */
> >  static const char * const adv7533_supply_names[] = {
> >  	"avdd",
> >  	"dvdd",
> > @@ -952,7 +953,7 @@ static bool
> > adv7511_cec_register_volatile(struct device *dev, unsigned int reg)
> >  	struct i2c_client *i2c = to_i2c_client(dev);
> >  	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
> >  
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		reg -= ADV7533_REG_CEC_OFFSET;
> >  
> >  	switch (reg) {
> > @@ -994,7 +995,7 @@ static int adv7511_init_cec_regmap(struct
> > adv7511 *adv)
> >  		goto err;
> >  	}
> >  
> > -	if (adv->type == ADV7533) {
> > +	if (adv->type == ADV7533 || adv->type == ADV7535) {
> >  		ret = adv7533_patch_cec_registers(adv);
> >  		if (ret)
> >  			goto err;
> > @@ -1094,8 +1095,9 @@ static int adv7511_probe(struct i2c_client
> > *i2c, const struct i2c_device_id *id)
> >  	struct adv7511_link_config link_config;
> >  	struct adv7511 *adv7511;
> >  	struct device *dev = &i2c->dev;
> > +	struct regulator *reg_v1p2;
> >  	unsigned int val;
> > -	int ret;
> > +	int ret, reg_v1p2_uV;
> >  
> >  	if (!dev->of_node)
> >  		return -EINVAL;
> > @@ -1163,6 +1165,16 @@ static int adv7511_probe(struct i2c_client
> > *i2c, const struct i2c_device_id *id)
> >  	if (ret)
> >  		goto uninit_regulators;
> >  
> > +	if (adv7511->type == ADV7533) {
> > +		reg_v1p2 = adv7511->supplies[5].consumer;
> > +		reg_v1p2_uV = regulator_get_voltage(reg_v1p2);
> > +
> > +		if (reg_v1p2_uV == 1200000) {
> > +			regmap_update_bits(adv7511->regmap,
> > +				ADV7511_REG_SUPPLY_SELECT, 0x80, 0x80);
> > +		}
> > +	}
> > +
> >  	adv7511_packet_disable(adv7511, 0xffff);
> >  
> >  	adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
> > @@ -1242,7 +1254,7 @@ static int adv7511_remove(struct i2c_client
> > *i2c)
> >  {
> >  	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
> >  
> > -	if (adv7511->type == ADV7533)
> > +	if (adv7511->type == ADV7533 || adv7511->type == ADV7535)
> >  		adv7533_detach_dsi(adv7511);
> >  	i2c_unregister_device(adv7511->i2c_cec);
> >  	if (adv7511->cec_clk)
> > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > adv7511_i2c_ids[] = {
> >  	{ "adv7511", ADV7511 },
> >  	{ "adv7511w", ADV7511 },
> >  	{ "adv7513", ADV7511 },
> > -#ifdef CONFIG_DRM_I2C_ADV7533
> > +#ifdef CONFIG_DRM_I2C_ADV753x
> >  	{ "adv7533", ADV7533 },
> > +	{ "adv7535", ADV7535 },
> >  #endif
> 
> This ifdef may not be needed??
> If we did not get this type we will not look it up.
But if we have defined in DT adv7533/5 device but
CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV. That
would be ok?
> 
> >  	{ }
> >  };
> > @@ -1277,8 +1290,9 @@ static const struct of_device_id
> > adv7511_of_ids[] = {
> >  	{ .compatible = "adi,adv7511", .data = (void *)ADV7511 },
> >  	{ .compatible = "adi,adv7511w", .data = (void *)ADV7511 },
> >  	{ .compatible = "adi,adv7513", .data = (void *)ADV7511 },
> > -#ifdef CONFIG_DRM_I2C_ADV7533
> > +#ifdef CONFIG_DRM_I2C_ADV753x
> >  	{ .compatible = "adi,adv7533", .data = (void *)ADV7533 },
> > +	{ .compatible = "adi,adv7535", .data = (void *)ADV7535 },
> >  #endif
> >  	{ }
> >  };
> 
> 	Sam

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
  2019-08-19  8:59       ` Togorean, Bogdan
@ 2019-08-19 10:46         ` Sam Ravnborg
  -1 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2019-08-19 10:46 UTC (permalink / raw)
  To: Togorean, Bogdan
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, daniel, robh+dt

Hi Bogdan.

> > >  		adv7533_detach_dsi(adv7511);
> > >  	i2c_unregister_device(adv7511->i2c_cec);
> > >  	if (adv7511->cec_clk)
> > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > adv7511_i2c_ids[] = {
> > >  	{ "adv7511", ADV7511 },
> > >  	{ "adv7511w", ADV7511 },
> > >  	{ "adv7513", ADV7511 },
> > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > >  	{ "adv7533", ADV7533 },
> > > +	{ "adv7535", ADV7535 },
> > >  #endif
> > 
> > This ifdef may not be needed??
> > If we did not get this type we will not look it up.
> But if we have defined in DT adv7533/5 device but
> CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV. That
> would be ok?

What do we gain from this complexity in the end.
Why not let the driver always support all variants.

If this result in a simpler driver, and less choices in Kconfig
then it is a win-win.

	Sam

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-08-19 10:46         ` Sam Ravnborg
  0 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2019-08-19 10:46 UTC (permalink / raw)
  To: Togorean, Bogdan
  Cc: airlied, gregkh, linux-kernel, dri-devel, robh+dt,
	Laurent.pinchart, tglx, matt.redfearn, allison

Hi Bogdan.

> > >  		adv7533_detach_dsi(adv7511);
> > >  	i2c_unregister_device(adv7511->i2c_cec);
> > >  	if (adv7511->cec_clk)
> > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > adv7511_i2c_ids[] = {
> > >  	{ "adv7511", ADV7511 },
> > >  	{ "adv7511w", ADV7511 },
> > >  	{ "adv7513", ADV7511 },
> > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > >  	{ "adv7533", ADV7533 },
> > > +	{ "adv7535", ADV7535 },
> > >  #endif
> > 
> > This ifdef may not be needed??
> > If we did not get this type we will not look it up.
> But if we have defined in DT adv7533/5 device but
> CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV. That
> would be ok?

What do we gain from this complexity in the end.
Why not let the driver always support all variants.

If this result in a simpler driver, and less choices in Kconfig
then it is a win-win.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
  2019-08-19 10:46         ` Sam Ravnborg
@ 2019-08-20  8:53           ` Daniel Vetter
  -1 siblings, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2019-08-20  8:53 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Togorean, Bogdan, Laurent.pinchart, a.hajda, airlied, gregkh,
	dri-devel, linux-kernel, allison, tglx, matt.redfearn, daniel,
	robh+dt

On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
> Hi Bogdan.
> 
> > > >  		adv7533_detach_dsi(adv7511);
> > > >  	i2c_unregister_device(adv7511->i2c_cec);
> > > >  	if (adv7511->cec_clk)
> > > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > > adv7511_i2c_ids[] = {
> > > >  	{ "adv7511", ADV7511 },
> > > >  	{ "adv7511w", ADV7511 },
> > > >  	{ "adv7513", ADV7511 },
> > > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > > >  	{ "adv7533", ADV7533 },
> > > > +	{ "adv7535", ADV7535 },
> > > >  #endif
> > > 
> > > This ifdef may not be needed??
> > > If we did not get this type we will not look it up.
> > But if we have defined in DT adv7533/5 device but
> > CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV. That
> > would be ok?
> 
> What do we gain from this complexity in the end.
> Why not let the driver always support all variants.
> 
> If this result in a simpler driver, and less choices in Kconfig
> then it is a win-win.

Yeah in general we don't Kconfig within drivers in drm to disable specific
code-paths. It's not worth the pain.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-08-20  8:53           ` Daniel Vetter
  0 siblings, 0 replies; 24+ messages in thread
From: Daniel Vetter @ 2019-08-20  8:53 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Togorean, Bogdan, Laurent.pinchart, a.hajda, airlied, gregkh,
	dri-devel, linux-kernel, allison, tglx, matt.redfearn, daniel,
	robh+dt

On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
> Hi Bogdan.
> 
> > > >  		adv7533_detach_dsi(adv7511);
> > > >  	i2c_unregister_device(adv7511->i2c_cec);
> > > >  	if (adv7511->cec_clk)
> > > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > > adv7511_i2c_ids[] = {
> > > >  	{ "adv7511", ADV7511 },
> > > >  	{ "adv7511w", ADV7511 },
> > > >  	{ "adv7513", ADV7511 },
> > > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > > >  	{ "adv7533", ADV7533 },
> > > > +	{ "adv7535", ADV7535 },
> > > >  #endif
> > > 
> > > This ifdef may not be needed??
> > > If we did not get this type we will not look it up.
> > But if we have defined in DT adv7533/5 device but
> > CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV. That
> > would be ok?
> 
> What do we gain from this complexity in the end.
> Why not let the driver always support all variants.
> 
> If this result in a simpler driver, and less choices in Kconfig
> then it is a win-win.

Yeah in general we don't Kconfig within drivers in drm to disable specific
code-paths. It's not worth the pain.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
  2019-08-20  8:53           ` Daniel Vetter
@ 2019-08-21  5:34             ` Togorean, Bogdan
  -1 siblings, 0 replies; 24+ messages in thread
From: Togorean, Bogdan @ 2019-08-21  5:34 UTC (permalink / raw)
  To: daniel, sam
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, robh+dt

On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
> [External]
> 
> On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
> > Hi Bogdan.
> > 
> > > > >  		adv7533_detach_dsi(adv7511);
> > > > >  	i2c_unregister_device(adv7511->i2c_cec);
> > > > >  	if (adv7511->cec_clk)
> > > > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > > > adv7511_i2c_ids[] = {
> > > > >  	{ "adv7511", ADV7511 },
> > > > >  	{ "adv7511w", ADV7511 },
> > > > >  	{ "adv7513", ADV7511 },
> > > > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > > > >  	{ "adv7533", ADV7533 },
> > > > > +	{ "adv7535", ADV7535 },
> > > > >  #endif
> > > > 
> > > > This ifdef may not be needed??
> > > > If we did not get this type we will not look it up.
> > > But if we have defined in DT adv7533/5 device but
> > > CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV.
> > > That
> > > would be ok?
> > 
> > What do we gain from this complexity in the end.
> > Why not let the driver always support all variants.
> > 
> > If this result in a simpler driver, and less choices in Kconfig
> > then it is a win-win.
> 
> Yeah in general we don't Kconfig within drivers in drm to disable
> specific
> code-paths. It's not worth the pain.
Ack,
Thank you for clarification. Will remove in V3.
> -Daniel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-08-21  5:34             ` Togorean, Bogdan
  0 siblings, 0 replies; 24+ messages in thread
From: Togorean, Bogdan @ 2019-08-21  5:34 UTC (permalink / raw)
  To: daniel, sam
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, robh+dt

On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
> [External]
> 
> On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
> > Hi Bogdan.
> > 
> > > > >  		adv7533_detach_dsi(adv7511);
> > > > >  	i2c_unregister_device(adv7511->i2c_cec);
> > > > >  	if (adv7511->cec_clk)
> > > > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > > > adv7511_i2c_ids[] = {
> > > > >  	{ "adv7511", ADV7511 },
> > > > >  	{ "adv7511w", ADV7511 },
> > > > >  	{ "adv7513", ADV7511 },
> > > > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > > > >  	{ "adv7533", ADV7533 },
> > > > > +	{ "adv7535", ADV7535 },
> > > > >  #endif
> > > > 
> > > > This ifdef may not be needed??
> > > > If we did not get this type we will not look it up.
> > > But if we have defined in DT adv7533/5 device but
> > > CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV.
> > > That
> > > would be ok?
> > 
> > What do we gain from this complexity in the end.
> > Why not let the driver always support all variants.
> > 
> > If this result in a simpler driver, and less choices in Kconfig
> > then it is a win-win.
> 
> Yeah in general we don't Kconfig within drivers in drm to disable
> specific
> code-paths. It's not worth the pain.
Ack,
Thank you for clarification. Will remove in V3.
> -Daniel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
  2019-08-21  5:34             ` Togorean, Bogdan
  (?)
@ 2019-11-27 11:52               ` Schrempf Frieder
  -1 siblings, 0 replies; 24+ messages in thread
From: Schrempf Frieder @ 2019-11-27 11:52 UTC (permalink / raw)
  To: Togorean, Bogdan, daniel, sam
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, robh+dt

Hi Bogdan,

On 21.08.19 07:34, Togorean, Bogdan wrote:
> On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
>> [External]
>>
>> On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
>>> Hi Bogdan.
>>>
>>>>>>   		adv7533_detach_dsi(adv7511);
>>>>>>   	i2c_unregister_device(adv7511->i2c_cec);
>>>>>>   	if (adv7511->cec_clk)
>>>>>> @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
>>>>>> adv7511_i2c_ids[] = {
>>>>>>   	{ "adv7511", ADV7511 },
>>>>>>   	{ "adv7511w", ADV7511 },
>>>>>>   	{ "adv7513", ADV7511 },
>>>>>> -#ifdef CONFIG_DRM_I2C_ADV7533
>>>>>> +#ifdef CONFIG_DRM_I2C_ADV753x
>>>>>>   	{ "adv7533", ADV7533 },
>>>>>> +	{ "adv7535", ADV7535 },
>>>>>>   #endif
>>>>>
>>>>> This ifdef may not be needed??
>>>>> If we did not get this type we will not look it up.
>>>> But if we have defined in DT adv7533/5 device but
>>>> CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV.
>>>> That
>>>> would be ok?
>>>
>>> What do we gain from this complexity in the end.
>>> Why not let the driver always support all variants.
>>>
>>> If this result in a simpler driver, and less choices in Kconfig
>>> then it is a win-win.
>>
>> Yeah in general we don't Kconfig within drivers in drm to disable
>> specific
>> code-paths. It's not worth the pain.
 >
> Ack,
> Thank you for clarification. Will remove in V3.

Are you still working on this? Do you plan to send a v3?
I will soon lay my hands on a board with the ADV7535 and would like to 
see this merged.
Also for patch 1/2, it seems you already have a R-b for v1 from Laurent, 
but you didn't carry the tag to v2.

Thanks,
Frieder

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-11-27 11:52               ` Schrempf Frieder
  0 siblings, 0 replies; 24+ messages in thread
From: Schrempf Frieder @ 2019-11-27 11:52 UTC (permalink / raw)
  To: Togorean, Bogdan, daniel, sam
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, robh+dt

Hi Bogdan,

On 21.08.19 07:34, Togorean, Bogdan wrote:
> On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
>> [External]
>>
>> On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
>>> Hi Bogdan.
>>>
>>>>>>   		adv7533_detach_dsi(adv7511);
>>>>>>   	i2c_unregister_device(adv7511->i2c_cec);
>>>>>>   	if (adv7511->cec_clk)
>>>>>> @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
>>>>>> adv7511_i2c_ids[] = {
>>>>>>   	{ "adv7511", ADV7511 },
>>>>>>   	{ "adv7511w", ADV7511 },
>>>>>>   	{ "adv7513", ADV7511 },
>>>>>> -#ifdef CONFIG_DRM_I2C_ADV7533
>>>>>> +#ifdef CONFIG_DRM_I2C_ADV753x
>>>>>>   	{ "adv7533", ADV7533 },
>>>>>> +	{ "adv7535", ADV7535 },
>>>>>>   #endif
>>>>>
>>>>> This ifdef may not be needed??
>>>>> If we did not get this type we will not look it up.
>>>> But if we have defined in DT adv7533/5 device but
>>>> CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV.
>>>> That
>>>> would be ok?
>>>
>>> What do we gain from this complexity in the end.
>>> Why not let the driver always support all variants.
>>>
>>> If this result in a simpler driver, and less choices in Kconfig
>>> then it is a win-win.
>>
>> Yeah in general we don't Kconfig within drivers in drm to disable
>> specific
>> code-paths. It's not worth the pain.
 >
> Ack,
> Thank you for clarification. Will remove in V3.

Are you still working on this? Do you plan to send a v3?
I will soon lay my hands on a board with the ADV7535 and would like to 
see this merged.
Also for patch 1/2, it seems you already have a R-b for v1 from Laurent, 
but you didn't carry the tag to v2.

Thanks,
Frieder

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-11-27 11:52               ` Schrempf Frieder
  0 siblings, 0 replies; 24+ messages in thread
From: Schrempf Frieder @ 2019-11-27 11:52 UTC (permalink / raw)
  To: Togorean, Bogdan, daniel, sam
  Cc: airlied, gregkh, linux-kernel, dri-devel, robh+dt,
	Laurent.pinchart, tglx, matt.redfearn, allison

Hi Bogdan,

On 21.08.19 07:34, Togorean, Bogdan wrote:
> On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
>> [External]
>>
>> On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
>>> Hi Bogdan.
>>>
>>>>>>   		adv7533_detach_dsi(adv7511);
>>>>>>   	i2c_unregister_device(adv7511->i2c_cec);
>>>>>>   	if (adv7511->cec_clk)
>>>>>> @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
>>>>>> adv7511_i2c_ids[] = {
>>>>>>   	{ "adv7511", ADV7511 },
>>>>>>   	{ "adv7511w", ADV7511 },
>>>>>>   	{ "adv7513", ADV7511 },
>>>>>> -#ifdef CONFIG_DRM_I2C_ADV7533
>>>>>> +#ifdef CONFIG_DRM_I2C_ADV753x
>>>>>>   	{ "adv7533", ADV7533 },
>>>>>> +	{ "adv7535", ADV7535 },
>>>>>>   #endif
>>>>>
>>>>> This ifdef may not be needed??
>>>>> If we did not get this type we will not look it up.
>>>> But if we have defined in DT adv7533/5 device but
>>>> CONFIG_DRM_I2C_ADV753x not selected probe will fail with ENODEV.
>>>> That
>>>> would be ok?
>>>
>>> What do we gain from this complexity in the end.
>>> Why not let the driver always support all variants.
>>>
>>> If this result in a simpler driver, and less choices in Kconfig
>>> then it is a win-win.
>>
>> Yeah in general we don't Kconfig within drivers in drm to disable
>> specific
>> code-paths. It's not worth the pain.
 >
> Ack,
> Thank you for clarification. Will remove in V3.

Are you still working on this? Do you plan to send a v3?
I will soon lay my hands on a board with the ADV7535 and would like to 
see this merged.
Also for patch 1/2, it seems you already have a R-b for v1 from Laurent, 
but you didn't carry the tag to v2.

Thanks,
Frieder
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
  2019-11-27 11:52               ` Schrempf Frieder
  (?)
@ 2019-11-27 14:22                 ` Togorean, Bogdan
  -1 siblings, 0 replies; 24+ messages in thread
From: Togorean, Bogdan @ 2019-11-27 14:22 UTC (permalink / raw)
  To: frieder.schrempf
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, sam, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, robh+dt, daniel

Hi Frieder,

I'm glad to find there are other persons interested in this driver and
especially support for ADV7535. Unfortunately I had to put on hold the
development due to other activities but I'll send V3 tomorrow.

I also started work on HDCP support for this driver and hope to send
soon a patch for that.

Best regards,
Bogdan 

On Wed, 2019-11-27 at 11:52 +0000, Schrempf Frieder wrote:
> [External]
> 
> Hi Bogdan,
> 
> On 21.08.19 07:34, Togorean, Bogdan wrote:
> > On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
> > > [External]
> > > 
> > > On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
> > > > Hi Bogdan.
> > > > 
> > > > > > >   		adv7533_detach_dsi(adv7511);
> > > > > > >   	i2c_unregister_device(adv7511->i2c_cec);
> > > > > > >   	if (adv7511->cec_clk)
> > > > > > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > > > > > adv7511_i2c_ids[] = {
> > > > > > >   	{ "adv7511", ADV7511 },
> > > > > > >   	{ "adv7511w", ADV7511 },
> > > > > > >   	{ "adv7513", ADV7511 },
> > > > > > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > > > > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > > > > > >   	{ "adv7533", ADV7533 },
> > > > > > > +	{ "adv7535", ADV7535 },
> > > > > > >   #endif
> > > > > > 
> > > > > > This ifdef may not be needed??
> > > > > > If we did not get this type we will not look it up.
> > > > > But if we have defined in DT adv7533/5 device but
> > > > > CONFIG_DRM_I2C_ADV753x not selected probe will fail with
> > > > > ENODEV.
> > > > > That
> > > > > would be ok?
> > > > 
> > > > What do we gain from this complexity in the end.
> > > > Why not let the driver always support all variants.
> > > > 
> > > > If this result in a simpler driver, and less choices in Kconfig
> > > > then it is a win-win.
> > > 
> > > Yeah in general we don't Kconfig within drivers in drm to disable
> > > specific
> > > code-paths. It's not worth the pain.
>  >
> > Ack,
> > Thank you for clarification. Will remove in V3.
> 
> Are you still working on this? Do you plan to send a v3?
> I will soon lay my hands on a board with the ADV7535 and would like
> to 
> see this merged.
> Also for patch 1/2, it seems you already have a R-b for v1 from
> Laurent, 
> but you didn't carry the tag to v2.
> 
> Thanks,
> Frieder

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-11-27 14:22                 ` Togorean, Bogdan
  0 siblings, 0 replies; 24+ messages in thread
From: Togorean, Bogdan @ 2019-11-27 14:22 UTC (permalink / raw)
  To: frieder.schrempf
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, sam, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, robh+dt, daniel

Hi Frieder,

I'm glad to find there are other persons interested in this driver and
especially support for ADV7535. Unfortunately I had to put on hold the
development due to other activities but I'll send V3 tomorrow.

I also started work on HDCP support for this driver and hope to send
soon a patch for that.

Best regards,
Bogdan 

On Wed, 2019-11-27 at 11:52 +0000, Schrempf Frieder wrote:
> [External]
> 
> Hi Bogdan,
> 
> On 21.08.19 07:34, Togorean, Bogdan wrote:
> > On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
> > > [External]
> > > 
> > > On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
> > > > Hi Bogdan.
> > > > 
> > > > > > >   		adv7533_detach_dsi(adv7511);
> > > > > > >   	i2c_unregister_device(adv7511->i2c_cec);
> > > > > > >   	if (adv7511->cec_clk)
> > > > > > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > > > > > adv7511_i2c_ids[] = {
> > > > > > >   	{ "adv7511", ADV7511 },
> > > > > > >   	{ "adv7511w", ADV7511 },
> > > > > > >   	{ "adv7513", ADV7511 },
> > > > > > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > > > > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > > > > > >   	{ "adv7533", ADV7533 },
> > > > > > > +	{ "adv7535", ADV7535 },
> > > > > > >   #endif
> > > > > > 
> > > > > > This ifdef may not be needed??
> > > > > > If we did not get this type we will not look it up.
> > > > > But if we have defined in DT adv7533/5 device but
> > > > > CONFIG_DRM_I2C_ADV753x not selected probe will fail with
> > > > > ENODEV.
> > > > > That
> > > > > would be ok?
> > > > 
> > > > What do we gain from this complexity in the end.
> > > > Why not let the driver always support all variants.
> > > > 
> > > > If this result in a simpler driver, and less choices in Kconfig
> > > > then it is a win-win.
> > > 
> > > Yeah in general we don't Kconfig within drivers in drm to disable
> > > specific
> > > code-paths. It's not worth the pain.
>  >
> > Ack,
> > Thank you for clarification. Will remove in V3.
> 
> Are you still working on this? Do you plan to send a v3?
> I will soon lay my hands on a board with the ADV7535 and would like
> to 
> see this merged.
> Also for patch 1/2, it seems you already have a R-b for v1 from
> Laurent, 
> but you didn't carry the tag to v2.
> 
> Thanks,
> Frieder

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-11-27 14:22                 ` Togorean, Bogdan
  0 siblings, 0 replies; 24+ messages in thread
From: Togorean, Bogdan @ 2019-11-27 14:22 UTC (permalink / raw)
  To: frieder.schrempf
  Cc: airlied, gregkh, linux-kernel, dri-devel, robh+dt,
	Laurent.pinchart, tglx, sam, matt.redfearn, allison

Hi Frieder,

I'm glad to find there are other persons interested in this driver and
especially support for ADV7535. Unfortunately I had to put on hold the
development due to other activities but I'll send V3 tomorrow.

I also started work on HDCP support for this driver and hope to send
soon a patch for that.

Best regards,
Bogdan 

On Wed, 2019-11-27 at 11:52 +0000, Schrempf Frieder wrote:
> [External]
> 
> Hi Bogdan,
> 
> On 21.08.19 07:34, Togorean, Bogdan wrote:
> > On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
> > > [External]
> > > 
> > > On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
> > > > Hi Bogdan.
> > > > 
> > > > > > >   		adv7533_detach_dsi(adv7511);
> > > > > > >   	i2c_unregister_device(adv7511->i2c_cec);
> > > > > > >   	if (adv7511->cec_clk)
> > > > > > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > > > > > adv7511_i2c_ids[] = {
> > > > > > >   	{ "adv7511", ADV7511 },
> > > > > > >   	{ "adv7511w", ADV7511 },
> > > > > > >   	{ "adv7513", ADV7511 },
> > > > > > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > > > > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > > > > > >   	{ "adv7533", ADV7533 },
> > > > > > > +	{ "adv7535", ADV7535 },
> > > > > > >   #endif
> > > > > > 
> > > > > > This ifdef may not be needed??
> > > > > > If we did not get this type we will not look it up.
> > > > > But if we have defined in DT adv7533/5 device but
> > > > > CONFIG_DRM_I2C_ADV753x not selected probe will fail with
> > > > > ENODEV.
> > > > > That
> > > > > would be ok?
> > > > 
> > > > What do we gain from this complexity in the end.
> > > > Why not let the driver always support all variants.
> > > > 
> > > > If this result in a simpler driver, and less choices in Kconfig
> > > > then it is a win-win.
> > > 
> > > Yeah in general we don't Kconfig within drivers in drm to disable
> > > specific
> > > code-paths. It's not worth the pain.
>  >
> > Ack,
> > Thank you for clarification. Will remove in V3.
> 
> Are you still working on this? Do you plan to send a v3?
> I will soon lay my hands on a board with the ADV7535 and would like
> to 
> see this merged.
> Also for patch 1/2, it seems you already have a R-b for v1 from
> Laurent, 
> but you didn't carry the tag to v2.
> 
> Thanks,
> Frieder
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
  2019-11-27 14:22                 ` Togorean, Bogdan
  (?)
@ 2019-11-27 14:46                   ` Schrempf Frieder
  -1 siblings, 0 replies; 24+ messages in thread
From: Schrempf Frieder @ 2019-11-27 14:46 UTC (permalink / raw)
  To: Togorean, Bogdan
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, sam, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, robh+dt, daniel

On 27.11.19 15:22, Togorean, Bogdan wrote:
> Hi Frieder,
> 
> I'm glad to find there are other persons interested in this driver and
> especially support for ADV7535. Unfortunately I had to put on hold the
> development due to other activities but I'll send V3 tomorrow.

Great to hear that. Thanks for your effort. I will try to test your v3 
when I have received the new hardware and got it up and running.

> 
> I also started work on HDCP support for this driver and hope to send
> soon a patch for that.
> 
> Best regards,
> Bogdan
> 
> On Wed, 2019-11-27 at 11:52 +0000, Schrempf Frieder wrote:
>> [External]
>>
>> Hi Bogdan,
>>
>> On 21.08.19 07:34, Togorean, Bogdan wrote:
>>> On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
>>>> [External]
>>>>
>>>> On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
>>>>> Hi Bogdan.
>>>>>
>>>>>>>>    		adv7533_detach_dsi(adv7511);
>>>>>>>>    	i2c_unregister_device(adv7511->i2c_cec);
>>>>>>>>    	if (adv7511->cec_clk)
>>>>>>>> @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
>>>>>>>> adv7511_i2c_ids[] = {
>>>>>>>>    	{ "adv7511", ADV7511 },
>>>>>>>>    	{ "adv7511w", ADV7511 },
>>>>>>>>    	{ "adv7513", ADV7511 },
>>>>>>>> -#ifdef CONFIG_DRM_I2C_ADV7533
>>>>>>>> +#ifdef CONFIG_DRM_I2C_ADV753x
>>>>>>>>    	{ "adv7533", ADV7533 },
>>>>>>>> +	{ "adv7535", ADV7535 },
>>>>>>>>    #endif
>>>>>>>
>>>>>>> This ifdef may not be needed??
>>>>>>> If we did not get this type we will not look it up.
>>>>>> But if we have defined in DT adv7533/5 device but
>>>>>> CONFIG_DRM_I2C_ADV753x not selected probe will fail with
>>>>>> ENODEV.
>>>>>> That
>>>>>> would be ok?
>>>>>
>>>>> What do we gain from this complexity in the end.
>>>>> Why not let the driver always support all variants.
>>>>>
>>>>> If this result in a simpler driver, and less choices in Kconfig
>>>>> then it is a win-win.
>>>>
>>>> Yeah in general we don't Kconfig within drivers in drm to disable
>>>> specific
>>>> code-paths. It's not worth the pain.
>>   >
>>> Ack,
>>> Thank you for clarification. Will remove in V3.
>>
>> Are you still working on this? Do you plan to send a v3?
>> I will soon lay my hands on a board with the ADV7535 and would like
>> to
>> see this merged.
>> Also for patch 1/2, it seems you already have a R-b for v1 from
>> Laurent,
>> but you didn't carry the tag to v2.
>>
>> Thanks,
>> Frieder

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-11-27 14:46                   ` Schrempf Frieder
  0 siblings, 0 replies; 24+ messages in thread
From: Schrempf Frieder @ 2019-11-27 14:46 UTC (permalink / raw)
  To: Togorean, Bogdan
  Cc: Laurent.pinchart, a.hajda, airlied, gregkh, sam, dri-devel,
	linux-kernel, allison, tglx, matt.redfearn, robh+dt, daniel

On 27.11.19 15:22, Togorean, Bogdan wrote:
> Hi Frieder,
> 
> I'm glad to find there are other persons interested in this driver and
> especially support for ADV7535. Unfortunately I had to put on hold the
> development due to other activities but I'll send V3 tomorrow.

Great to hear that. Thanks for your effort. I will try to test your v3 
when I have received the new hardware and got it up and running.

> 
> I also started work on HDCP support for this driver and hope to send
> soon a patch for that.
> 
> Best regards,
> Bogdan
> 
> On Wed, 2019-11-27 at 11:52 +0000, Schrempf Frieder wrote:
>> [External]
>>
>> Hi Bogdan,
>>
>> On 21.08.19 07:34, Togorean, Bogdan wrote:
>>> On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
>>>> [External]
>>>>
>>>> On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
>>>>> Hi Bogdan.
>>>>>
>>>>>>>>    		adv7533_detach_dsi(adv7511);
>>>>>>>>    	i2c_unregister_device(adv7511->i2c_cec);
>>>>>>>>    	if (adv7511->cec_clk)
>>>>>>>> @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
>>>>>>>> adv7511_i2c_ids[] = {
>>>>>>>>    	{ "adv7511", ADV7511 },
>>>>>>>>    	{ "adv7511w", ADV7511 },
>>>>>>>>    	{ "adv7513", ADV7511 },
>>>>>>>> -#ifdef CONFIG_DRM_I2C_ADV7533
>>>>>>>> +#ifdef CONFIG_DRM_I2C_ADV753x
>>>>>>>>    	{ "adv7533", ADV7533 },
>>>>>>>> +	{ "adv7535", ADV7535 },
>>>>>>>>    #endif
>>>>>>>
>>>>>>> This ifdef may not be needed??
>>>>>>> If we did not get this type we will not look it up.
>>>>>> But if we have defined in DT adv7533/5 device but
>>>>>> CONFIG_DRM_I2C_ADV753x not selected probe will fail with
>>>>>> ENODEV.
>>>>>> That
>>>>>> would be ok?
>>>>>
>>>>> What do we gain from this complexity in the end.
>>>>> Why not let the driver always support all variants.
>>>>>
>>>>> If this result in a simpler driver, and less choices in Kconfig
>>>>> then it is a win-win.
>>>>
>>>> Yeah in general we don't Kconfig within drivers in drm to disable
>>>> specific
>>>> code-paths. It's not worth the pain.
>>   >
>>> Ack,
>>> Thank you for clarification. Will remove in V3.
>>
>> Are you still working on this? Do you plan to send a v3?
>> I will soon lay my hands on a board with the ADV7535 and would like
>> to
>> see this merged.
>> Also for patch 1/2, it seems you already have a R-b for v1 from
>> Laurent,
>> but you didn't carry the tag to v2.
>>
>> Thanks,
>> Frieder

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535
@ 2019-11-27 14:46                   ` Schrempf Frieder
  0 siblings, 0 replies; 24+ messages in thread
From: Schrempf Frieder @ 2019-11-27 14:46 UTC (permalink / raw)
  To: Togorean, Bogdan
  Cc: airlied, gregkh, linux-kernel, dri-devel, robh+dt,
	Laurent.pinchart, tglx, sam, matt.redfearn, allison

On 27.11.19 15:22, Togorean, Bogdan wrote:
> Hi Frieder,
> 
> I'm glad to find there are other persons interested in this driver and
> especially support for ADV7535. Unfortunately I had to put on hold the
> development due to other activities but I'll send V3 tomorrow.

Great to hear that. Thanks for your effort. I will try to test your v3 
when I have received the new hardware and got it up and running.

> 
> I also started work on HDCP support for this driver and hope to send
> soon a patch for that.
> 
> Best regards,
> Bogdan
> 
> On Wed, 2019-11-27 at 11:52 +0000, Schrempf Frieder wrote:
>> [External]
>>
>> Hi Bogdan,
>>
>> On 21.08.19 07:34, Togorean, Bogdan wrote:
>>> On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
>>>> [External]
>>>>
>>>> On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
>>>>> Hi Bogdan.
>>>>>
>>>>>>>>    		adv7533_detach_dsi(adv7511);
>>>>>>>>    	i2c_unregister_device(adv7511->i2c_cec);
>>>>>>>>    	if (adv7511->cec_clk)
>>>>>>>> @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
>>>>>>>> adv7511_i2c_ids[] = {
>>>>>>>>    	{ "adv7511", ADV7511 },
>>>>>>>>    	{ "adv7511w", ADV7511 },
>>>>>>>>    	{ "adv7513", ADV7511 },
>>>>>>>> -#ifdef CONFIG_DRM_I2C_ADV7533
>>>>>>>> +#ifdef CONFIG_DRM_I2C_ADV753x
>>>>>>>>    	{ "adv7533", ADV7533 },
>>>>>>>> +	{ "adv7535", ADV7535 },
>>>>>>>>    #endif
>>>>>>>
>>>>>>> This ifdef may not be needed??
>>>>>>> If we did not get this type we will not look it up.
>>>>>> But if we have defined in DT adv7533/5 device but
>>>>>> CONFIG_DRM_I2C_ADV753x not selected probe will fail with
>>>>>> ENODEV.
>>>>>> That
>>>>>> would be ok?
>>>>>
>>>>> What do we gain from this complexity in the end.
>>>>> Why not let the driver always support all variants.
>>>>>
>>>>> If this result in a simpler driver, and less choices in Kconfig
>>>>> then it is a win-win.
>>>>
>>>> Yeah in general we don't Kconfig within drivers in drm to disable
>>>> specific
>>>> code-paths. It's not worth the pain.
>>   >
>>> Ack,
>>> Thank you for clarification. Will remove in V3.
>>
>> Are you still working on this? Do you plan to send a v3?
>> I will soon lay my hands on a board with the ADV7535 and would like
>> to
>> see this merged.
>> Also for patch 1/2, it seems you already have a R-b for v1 from
>> Laurent,
>> but you didn't carry the tag to v2.
>>
>> Thanks,
>> Frieder
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2019-11-28  8:24 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 14:16 [PATCH v2 0/2] drm: bridge: adv7511: Add support For ADV7535 Bogdan Togorean
2019-08-09 14:16 ` Bogdan Togorean
2019-08-09 14:16 ` [PATCH v2 1/2] dt-bindings: drm: bridge: adv7511: Add ADV7535 support Bogdan Togorean
2019-08-09 14:16   ` Bogdan Togorean
2019-08-09 14:16 ` [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535 Bogdan Togorean
2019-08-09 14:16   ` Bogdan Togorean
2019-08-09 15:25   ` Sam Ravnborg
2019-08-19  8:59     ` Togorean, Bogdan
2019-08-19  8:59       ` Togorean, Bogdan
2019-08-19 10:46       ` Sam Ravnborg
2019-08-19 10:46         ` Sam Ravnborg
2019-08-20  8:53         ` Daniel Vetter
2019-08-20  8:53           ` Daniel Vetter
2019-08-21  5:34           ` Togorean, Bogdan
2019-08-21  5:34             ` Togorean, Bogdan
2019-11-27 11:52             ` Schrempf Frieder
2019-11-27 11:52               ` Schrempf Frieder
2019-11-27 11:52               ` Schrempf Frieder
2019-11-27 14:22               ` Togorean, Bogdan
2019-11-27 14:22                 ` Togorean, Bogdan
2019-11-27 14:22                 ` Togorean, Bogdan
2019-11-27 14:46                 ` Schrempf Frieder
2019-11-27 14:46                   ` Schrempf Frieder
2019-11-27 14:46                   ` Schrempf Frieder

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.