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
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
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
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
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
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
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
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
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
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
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