| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 17 Jan 2020 11:27:30 +0100
From: Andrzej Hajda <a.hajda@...sung.com>
To: Bogdan Togorean <bogdan.togorean@...log.com>,
dri-devel@...ts.freedesktop.org
Cc: airlied@...ux.ie, daniel@...ll.ch, robh+dt@...nel.org,
mark.rutland@....com, narmstrong@...libre.com,
Laurent.pinchart@...asonboard.com, jonas@...boo.se,
jernej.skrabec@...l.net, gregkh@...uxfoundation.org,
tglx@...utronix.de, sam@...nborg.org, alexander.deucher@....com,
matt.redfearn@...nci.com, robdclark@...omium.org,
wsa+renesas@...g-engineering.com, linux-kernel@...r.kernel.org
Subject: Re: [RESEND v3 2/2] drm: bridge: adv7511: Add support for ADV7535
On 07.01.2020 14:34, Bogdan Togorean wrote:
> ADV7535 is a DSI to HDMI bridge chip like ADV7533 but it allows
> 1080p@...z. 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@...log.com>
> ---
> drivers/gpu/drm/bridge/adv7511/Kconfig | 13 ++----
> drivers/gpu/drm/bridge/adv7511/Makefile | 3 +-
> drivers/gpu/drm/bridge/adv7511/adv7511.h | 44 +++-----------------
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 35 ++++++++++------
> 4 files changed, 32 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig b/drivers/gpu/drm/bridge/adv7511/Kconfig
> index 8a56ff81f4fb..47d4eb9e845d 100644
> --- a/drivers/gpu/drm/bridge/adv7511/Kconfig
> +++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
> @@ -4,8 +4,9 @@ config DRM_I2C_ADV7511
> depends on OF
> select DRM_KMS_HELPER
> select REGMAP_I2C
> + select DRM_MIPI_DSI
> help
> - Support for the Analog Device ADV7511(W) and ADV7513 HDMI encoders.
> + Support for the Analog Device ADV7511(W)/13/33/35 HDMI encoders.
>
> config DRM_I2C_ADV7511_AUDIO
> bool "ADV7511 HDMI Audio driver"
> @@ -15,16 +16,8 @@ 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"
> - depends on DRM_I2C_ADV7511
> - select DRM_MIPI_DSI
> - default y
> - help
> - Support for the Analog Devices ADV7533 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..d8ceb534b51f 100644
> --- a/drivers/gpu/drm/bridge/adv7511/Makefile
> +++ b/drivers/gpu/drm/bridge/adv7511/Makefile
> @@ -1,6 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0-only
> -adv7511-y := adv7511_drv.o
> +adv7511-y := adv7511_drv.o adv7533.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
> 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..ed9cfd944098 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -220,6 +220,10 @@
>
> #define ADV7533_REG_CEC_OFFSET 0x70
>
> +#define ADV7533_REG_SUPPLY_SELECT 0xe4
> +
> +#define ADV7533_V1P2_ENABLE BIT(7)
> +
> enum adv7511_input_clock {
> ADV7511_INPUT_CLOCK_1X,
> ADV7511_INPUT_CLOCK_2X,
> @@ -320,6 +324,7 @@ struct adv7511_video_config {
> enum adv7511_type {
> ADV7511,
> ADV7533,
> + ADV7535,
> };
>
> #define ADV7511_MAX_ADDRS 3
> @@ -393,7 +398,6 @@ static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
> }
> #endif
>
> -#ifdef CONFIG_DRM_I2C_ADV7533
> 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);
> @@ -402,44 +406,6 @@ int adv7533_patch_cec_registers(struct adv7511 *adv);
> int adv7533_attach_dsi(struct adv7511 *adv);
> void adv7533_detach_dsi(struct adv7511 *adv);
> int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv);
> -#else
> -static inline void adv7533_dsi_power_on(struct adv7511 *adv)
> -{
> -}
> -
> -static inline void adv7533_dsi_power_off(struct adv7511 *adv)
> -{
> -}
> -
> -static inline void adv7533_mode_set(struct adv7511 *adv,
> - const struct drm_display_mode *mode)
> -{
> -}
> -
> -static inline int adv7533_patch_registers(struct adv7511 *adv)
> -{
> - return -ENODEV;
> -}
> -
> -static inline int adv7533_patch_cec_registers(struct adv7511 *adv)
> -{
> - return -ENODEV;
> -}
> -
> -static inline int adv7533_attach_dsi(struct adv7511 *adv)
> -{
> - return -ENODEV;
> -}
> -
> -static inline void adv7533_detach_dsi(struct adv7511 *adv)
> -{
> -}
> -
> -static inline int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv)
> -{
> - return -ENODEV;
> -}
> -#endif
>
> #ifdef CONFIG_DRM_I2C_ADV7511_AUDIO
> int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511);
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 9e13e466e72c..35595472e771 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,17 @@ 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,
> + ADV7533_REG_SUPPLY_SELECT, ADV7533_V1P2_ENABLE,
> + ADV7533_V1P2_ENABLE);
> + }
> + }
> +
In patch adding support for adv7535 you modifies adv7533 path. It looks
suspicious, maybe it would be better to extract it to separate patch.
Anyway this change requires tests from adv7533 users.
Beside this the patch looks OK.
Regards
Andrzej
> adv7511_packet_disable(adv7511, 0xffff);
>
> adv7511->i2c_edid = i2c_new_ancillary_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)
> @@ -1266,9 +1279,8 @@ static const struct i2c_device_id adv7511_i2c_ids[] = {
> { "adv7511", ADV7511 },
> { "adv7511w", ADV7511 },
> { "adv7513", ADV7511 },
> -#ifdef CONFIG_DRM_I2C_ADV7533
> { "adv7533", ADV7533 },
> -#endif
> + { "adv7535", ADV7535 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, adv7511_i2c_ids);
> @@ -1277,9 +1289,8 @@ 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
> { .compatible = "adi,adv7533", .data = (void *)ADV7533 },
> -#endif
> + { .compatible = "adi,adv7535", .data = (void *)ADV7535 },
> { }
> };
> MODULE_DEVICE_TABLE(of, adv7511_of_ids);
Powered by blists - more mailing lists