2019-08-09 14:18:26

by Togorean, Bogdan

[permalink] [raw]
Subject: [PATCH v2 0/2] drm: bridge: adv7511: Add support For ADV7535

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


2019-08-09 14:20:09

by Togorean, Bogdan

[permalink] [raw]
Subject: [PATCH v2 1/2] dt-bindings: drm: bridge: adv7511: Add ADV7535 support

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 <[email protected]>
---
.../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

2019-08-09 14:20:09

by Togorean, Bogdan

[permalink] [raw]
Subject: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535

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 <[email protected]>
---
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

2019-08-09 16:45:40

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535

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

2019-08-19 09:01:35

by Togorean, Bogdan

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535

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

2019-08-19 10:47:27

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535

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

2019-08-20 08:54:45

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535

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

2019-08-21 05:37:58

by Togorean, Bogdan

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535

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

2019-11-27 11:54:20

by Frieder Schrempf

[permalink] [raw]
Subject: Re: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535

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

2019-11-27 14:24:38

by Togorean, Bogdan

[permalink] [raw]
Subject: Re: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535

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

2019-11-27 14:48:21

by Frieder Schrempf

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] drm: bridge: adv7511: Add support for ADV7535

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