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.
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/adv7511.h | 2 ++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 31 ++++++++++++++----- 3 files changed, 37 insertions(+), 19 deletions(-)
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
Hi Bogdan,
Thank you for the patch.
On Tue, Jul 30, 2019 at 04:17:35PM +0300, Bogdan Togorean wrote:
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
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.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
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/adv7511.h | 2 ++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 31 +++++++++++++++----- 2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 52b2adfdc877..702432615ec8 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 diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index f6d2681f6927..941072decb73 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) @@ -952,7 +952,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 +994,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 +1094,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 +1164,18 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (ret) goto uninit_regulators;
+ if (adv7511->type == ADV7533) { + ret = match_string(adv7533_supply_names, adv7511->num_supplies, + "v1p2"); + reg_v1p2 = adv7511->supplies[ret].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 +1255,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) @@ -1268,6 +1281,7 @@ static const struct i2c_device_id adv7511_i2c_ids[] = { { "adv7513", ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533 { "adv7533", ADV7533 }, + { "adv7535", ADV7535 }, #endif { } }; @@ -1279,6 +1293,7 @@ static const struct of_device_id adv7511_of_ids[] = { { .compatible = "adi,adv7513", .data = (void *)ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533 { .compatible = "adi,adv7533", .data = (void *)ADV7533 }, + { .compatible = "adi,adv7535", .data = (void *)ADV7535 }, #endif { } };
Hi Bogdan,
On 30/07/2019 15:17, Bogdan Togorean wrote:
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/adv7511.h | 2 ++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 31 +++++++++++++++----- 2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 52b2adfdc877..702432615ec8 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 diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index f6d2681f6927..941072decb73 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)
@@ -952,7 +952,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 +994,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 +1094,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 +1164,18 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (ret) goto uninit_regulators;
if (adv7511->type == ADV7533) {
ret = match_string(adv7533_supply_names, adv7511->num_supplies,"v1p2");reg_v1p2 = adv7511->supplies[ret].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 +1255,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)
@@ -1268,6 +1281,7 @@ static const struct i2c_device_id adv7511_i2c_ids[] = { { "adv7513", ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533 { "adv7533", ADV7533 },
- { "adv7535", ADV7535 },
Maybe you should add a new CONFIG_DRM_I2C_ADV7535 or update the current CONFIG_DRM_I2C_ADV7533 description.
#endif { } }; @@ -1279,6 +1293,7 @@ static const struct of_device_id adv7511_of_ids[] = { { .compatible = "adi,adv7513", .data = (void *)ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533 { .compatible = "adi,adv7533", .data = (void *)ADV7533 },
- { .compatible = "adi,adv7535", .data = (void *)ADV7535 },
Ditto
I'm not a adv75xx expert but it looks sane.
Neil
#endif { } };
Hi Neil,
Thank you for review.
On Wed, 2019-07-31 at 09:42 +0200, Neil Armstrong wrote:
[External]
Hi Bogdan,
On 30/07/2019 15:17, Bogdan Togorean wrote:
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/adv7511.h | 2 ++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 31 +++++++++++++++-
2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 52b2adfdc877..702432615ec8 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 diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index f6d2681f6927..941072decb73 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)
@@ -952,7 +952,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 +994,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 +1094,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 +1164,18 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (ret) goto uninit_regulators;
- if (adv7511->type == ADV7533) {
ret = match_string(adv7533_supply_names, adv7511-num_supplies,
"v1p2");
reg_v1p2 = adv7511->supplies[ret].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 +1255,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)
@@ -1268,6 +1281,7 @@ static const struct i2c_device_id adv7511_i2c_ids[] = { { "adv7513", ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533 { "adv7533", ADV7533 },
- { "adv7535", ADV7535 },
Maybe you should add a new CONFIG_DRM_I2C_ADV7535 or update the current CONFIG_DRM_I2C_ADV7533 description.
I'll rename the config option as Laurent also suggested to CONFIG_DRM_I2C_ADV753X and update its description.
#endif { } }; @@ -1279,6 +1293,7 @@ static const struct of_device_id adv7511_of_ids[] = { { .compatible = "adi,adv7513", .data = (void *)ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533 { .compatible = "adi,adv7533", .data = (void *)ADV7533 },
- { .compatible = "adi,adv7535", .data = (void *)ADV7535 },
Ditto
I'm not a adv75xx expert but it looks sane.
Neil
#endif { } };
Hi Bogdan,
Thank you for the patch.
On Tue, Jul 30, 2019 at 04:17:36PM +0300, Bogdan Togorean wrote:
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/adv7511.h | 2 ++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 31 +++++++++++++++----- 2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 52b2adfdc877..702432615ec8 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 diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index f6d2681f6927..941072decb73 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)
@@ -952,7 +952,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 +994,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 +1094,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 +1164,18 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (ret) goto uninit_regulators;
- if (adv7511->type == ADV7533) {
ret = match_string(adv7533_supply_names, adv7511->num_supplies,
Although they're equivalent, I would use ARRAY_SIZE(adv7533_supply_names) instead of adv7511->num_supplies to make the code easier to read (otherwise one has to check where num_supplies is set to validate this function call).
"v1p2");
You should align "v1p2" left, with adv7533_supply_names.
I wonder if you couldn't simply hardcode the index, with a comment above the adv7533_supply_names array to mention that the order of the entries shall not be modified without updating the locations that hardcode indices.
reg_v1p2 = adv7511->supplies[ret].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);}
Shouldn't you explicitly clear bit 7 when reg_v1p2_uV is not 1200000 ? Or is there a guarantee it gets reset after a reboot ?
}
adv7511_packet_disable(adv7511, 0xffff);
adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
@@ -1242,7 +1255,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)
@@ -1268,6 +1281,7 @@ static const struct i2c_device_id adv7511_i2c_ids[] = { { "adv7513", ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533
As noted by Neil, I think this config option should be renamed (possibly to CONFIG_DRM_I2C_ADV753X) and its description updated.
{ "adv7533", ADV7533 },
- { "adv7535", ADV7535 },
#endif { } }; @@ -1279,6 +1293,7 @@ static const struct of_device_id adv7511_of_ids[] = { { .compatible = "adi,adv7513", .data = (void *)ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533 { .compatible = "adi,adv7533", .data = (void *)ADV7533 },
- { .compatible = "adi,adv7535", .data = (void *)ADV7535 },
#endif { } };
Hi Laurent,
Thnk you for your review.
On Wed, 2019-07-31 at 12:14 +0300, Laurent Pinchart wrote:
[External]
Hi Bogdan,
Thank you for the patch.
On Tue, Jul 30, 2019 at 04:17:36PM +0300, Bogdan Togorean wrote:
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/adv7511.h | 2 ++ drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 31 +++++++++++++++-
2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 52b2adfdc877..702432615ec8 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 diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index f6d2681f6927..941072decb73 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)
@@ -952,7 +952,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 +994,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 +1094,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 +1164,18 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (ret) goto uninit_regulators;
- if (adv7511->type == ADV7533) {
ret = match_string(adv7533_supply_names, adv7511-num_supplies,
Although they're equivalent, I would use ARRAY_SIZE(adv7533_supply_names) instead of adv7511->num_supplies to make the code easier to read (otherwise one has to check where num_supplies is set to validate this function call).
Ack. Will modify it in V2
"v1p2");
You should align "v1p2" left, with adv7533_supply_names.
I wonder if you couldn't simply hardcode the index, with a comment above the adv7533_supply_names array to mention that the order of the entries shall not be modified without updating the locations that hardcode indices.
Ok, my intention was to have robust code but as you said a comment before the supply names array is enough.
reg_v1p2 = adv7511->supplies[ret].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);}Shouldn't you explicitly clear bit 7 when reg_v1p2_uV is not 1200000 ? Or is there a guarantee it gets reset after a reboot ?
It is guaranteed that it starts cleared to be safe for the chip if the voltage is 1.8V.
}
adv7511_packet_disable(adv7511, 0xffff);
adv7511->i2c_edid = i2c_new_secondary_device(i2c, "edid",
@@ -1242,7 +1255,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)
@@ -1268,6 +1281,7 @@ static const struct i2c_device_id adv7511_i2c_ids[] = { { "adv7513", ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533
As noted by Neil, I think this config option should be renamed (possibly to CONFIG_DRM_I2C_ADV753X) and its description updated.
Ack. Will rename to CONFIG_DRM_I2C_ADV753X and update decription.
{ "adv7533", ADV7533 },
- { "adv7535", ADV7535 },
#endif { } }; @@ -1279,6 +1293,7 @@ static const struct of_device_id adv7511_of_ids[] = { { .compatible = "adi,adv7513", .data = (void *)ADV7511 }, #ifdef CONFIG_DRM_I2C_ADV7533 { .compatible = "adi,adv7533", .data = (void *)ADV7533 },
- { .compatible = "adi,adv7535", .data = (void *)ADV7535 },
#endif { } };
dri-devel@lists.freedesktop.org
-
Bogdan Togorean -
Laurent Pinchart -
Neil Armstrong -
Togorean, Bogdan