RE: [PATCH 1/2] drm/adi: axi-hdmi-tx: Add support for AXI HDMI TX IP core
Thank you Sam for your review, I'll send now V2 implementing all your remarks. Best regards, Bogdan > > Hi Bogdan > > On Mon, Oct 05, 2020 at 05:12:08PM +0300, Bogdan Togorean wrote: > > From: Lars-Peter Clausen > > > > The AXI HDMI HDL driver is the driver for the HDL graphics core which is > > used on various FPGA designs. It's mostly used to interface with the > > ADV7511 driver on some Zynq boards (e.g. ZC702 & ZedBoard). > > > > Link: > > https://wiki.analog.com/resources/tools-software/linux-drivers/drm/hdl- > axi-hdmi > > Link: https://wiki.analog.com/resources/fpga/docs/axi_hdmi_tx > > Thanks for submitting the driver - a few high level comments after > browsing the driver: > > - Use drmm_mode_config_init() to utilize new cleanup > - Look at other uses of drm_driver - there is macros that makes this > much simpler / smaller > - Use devm_drm_dev_alloc() to allocate axi_hdmi_tx_private > To do so embed drm_device in axi_hdmi_tx_private - which is the way to > do it today > - Do not use ddev->dev_private, it is deprecated > - Use dev_err_probe() when you risk to see a PROBE_DEFER > - In all include blocks sort the include alphabetically > - Use the new interface to drm_bridge_attach() - where display driver > creates the connector > - See if the Kconfig selects can be trimmed - the framebuffer releated > selects looks wrong (others get it wrong too) > - Check if you can use the simple encoder - see > drm_simple_encoder_init() > > If this is a simple one plane, one crtc display driver then it should > use the drm_simple_* support. Or the changelog should explain why not. > > We want the drivers as simple as we can - and they shall use as much of > the helper infrastructure as they can. > > We continue to develop the helper infrastructure so it is expected that > there is some lacking behind as is the case here. > > Sam > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [RESEND 2/2] drm: bridge: adv7511: Extend list of audio sample rates
> > ADV7511 support sample rates up to 192kHz. CTS and N parameters should > > be computed accordingly so this commit extend the list up to maximum > > supported sample rate. > > > > Signed-off-by: Bogdan Togorean > > --- > > drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 12 > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > > index 1e9b128d229b..13e8cee6e827 100644 > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > > @@ -27,6 +27,18 @@ static void adv7511_calc_cts_n(unsigned int f_tmds, > unsigned int fs, > > case 48000: > > *n = 6144; > > break; > > + case 88200: > > + *n = 12544; > > + break; > > + case 96000: > > + *n = 12288; > > + break; > > + case 176400: > > + *n = 25088; > > + break; > > + case 192000: > > + *n = 24576; > > + break; > > > I would put: > > case 32000: > case 48000: > case 96000: > case 192000: > *n = fs * 128 / 1000; > break; > case 44100: > case 88200: > case 176400: > *n = fs * 128 / 900; > break; > > To uncover the magic. Up to you. Great solution Andrzej, Thank you for your suggestion. Regards, Bogdan > > Reviewed-by: Andrzej Hajda > > > Regards > Andrzej > > } > > > > *cts = ((f_tmds * *n) / (128 * fs)) * 1000; > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND v3 2/2] drm: bridge: adv7511: Add support for ADV7535
On Fri, 2020-01-17 at 11:55 +0100, Andrzej Hajda wrote:
> [External]
>
> On 17.01.2020 11:27, Andrzej Hajda wrote:
> > On 07.01.2020 14:34, 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
> > > ---
> > > 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_SELECT0xe4
> > > +
> > > +#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
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 +, Schrempf Frieder wrote:
> [External]
>
> Hi Bogdan,
>
> On 21.08.19 07:34, Togorean, Bogdan wrote:
> > On Tue, 2019-08-20 at 10:53 +0200, Daniel Vetter wrote:
> > > [External]
> > >
> > > On Mon, Aug 19, 2019 at 12:46:16PM +0200, Sam Ravnborg wrote:
> > > > Hi Bogdan.
> > > >
> > > > > > > adv7533_detach_dsi(adv7511);
> > > > > > > i2c_unregister_device(adv7511->i2c_cec);
> > > > > > > if (adv7511->cec_clk)
> > > > > > > @@ -1266,8 +1278,9 @@ static const struct i2c_device_id
> > > > > > > adv7511_i2c_ids[] = {
> > > > > > > { "adv7511", ADV7511 },
> > > > > > > { "adv7511w", ADV7511 },
> > > > > > > { "adv7513", ADV7511 },
> > > > > > > -#ifdef CONFIG_DRM_I2C_ADV7533
> > > > > > > +#ifdef CONFIG_DRM_I2C_ADV753x
> > > > > > > { "adv7533", ADV7533 },
> > > > > > > + { "adv7535", ADV7535 },
> > > > > > > #endif
> > > > > >
> > > > > > This ifdef may not be needed??
> > > > > > If we did not get this type we will not look it up.
> > > > > But if we have defined in DT adv7533/5 device but
> > > > > CONFIG_DRM_I2C_ADV753x not selected probe will fail with
> > > > > ENODEV.
> > > > > That
> > > > > would be ok?
> > > >
> > > > What do we gain from this complexity in the end.
> > > > Why not let the driver always support all variants.
> > > >
> > > > If this result in a simpler driver, and less choices in Kconfig
> > > > then it is a win-win.
> > >
> > > Yeah in general we don't Kconfig within drivers in drm to disable
> > > specific
> > > code-paths. It's not worth the pain.
> >
> > Ack,
> > Thank you for clarification. Will remove in V3.
>
> Are you still working on this? Do you plan to send a v3?
> I will soon lay my hands on a board with the ADV7535 and would like
> to
> see this merged.
> Also for patch 1/2, it seems you already have a R-b for v1 from
> Laurent,
> but you didn't carry the tag to v2.
>
> Thanks,
> Frieder
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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
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_LOW0xf6
> >
> > @@ -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(>curr_mode, adj_mode);
> > @@ -874,7 +874,7 @@ static int adv7511_bridge_attach(struct
> > drm_bridge *bridge)
> > _connector_helper_funcs);
> > drm_connector_attach_encoder(>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 =
Re: [PATCH 2/2] drm: bridge: adv7511: Add support for ADV7535
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
> > ---
> > 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_LOW0xf6
> >
> > @@ -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(>curr_mode, adj_mode);
> > @@ -874,7 +874,7 @@ static int adv7511_bridge_attach(struct
> > drm_bridge *bridge)
> > _connector_helper_funcs);
> > drm_connector_attach_encoder(>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 = >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
> > +
Re: [PATCH 2/2] drm: bridge: adv7511: Add support for ADV7535
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
> > ---
> > 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_LOW0xf6
> >
> > @@ -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(>curr_mode, adj_mode);
> > @@ -874,7 +874,7 @@ static int adv7511_bridge_attach(struct
> > drm_bridge *bridge)
> > _connector_helper_funcs);
> > drm_connector_attach_encoder(>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 = >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 == 120) {
> > + regmap_update_bits(adv7511->regmap,
> > +
