[PATCH v2 2/2] drm/bridge: add support for TI TDP158

[Dmitry Baryshkov]

On Wed, Jun 26, 2024 at 05:26:10PM GMT, Marc Gonzalez wrote:
> On 26/06/2024 06:47, Dmitry Baryshkov wrote:
> 
> > On Tue, Jun 25, 2024 at 06:38:13PM GMT, Marc Gonzalez wrote:
> >
> >> The TI TDP158 is an AC-Coupled HDMI signal to TMDS Redriver supporting
> >> DVI 1.0 and HDMI 1.4b and 2.0b output signals.
> >>
> >> The default settings work fine for our use-case.
> >>
> >> Signed-off-by: Marc Gonzalez <mgonzalez at freebox.fr>
> >> ---
> >>  drivers/gpu/drm/bridge/Kconfig     |   6 +++
> >>  drivers/gpu/drm/bridge/Makefile    |   1 +
> >>  drivers/gpu/drm/bridge/ti-tdp158.c | 103 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 110 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> >> index c621be1a99a89..0859f85cb4b69 100644
> >> --- a/drivers/gpu/drm/bridge/Kconfig
> >> +++ b/drivers/gpu/drm/bridge/Kconfig
> >> @@ -368,6 +368,12 @@ config DRM_TI_DLPC3433
> >>  	  It supports up to 720p resolution with 60 and 120 Hz refresh
> >>  	  rates.
> >>  
> >> +config DRM_TI_TDP158
> >> +	tristate "TI TDP158 HDMI/TMDS bridge"
> >> +	depends on OF
> >> +	help
> >> +	  Texas Instruments TDP158 HDMI/TMDS Bridge driver
> > 
> > Please run ./scripts/checkpatch.pl on your patch.
> 
> Oops, sorry, will do.
> For the record, I did run:
> $ make -j16 dt_binding_check DT_SCHEMA_FILES=ti,tdp158.yaml
> 
> 
> >> +	if ((err = regulator_enable(tdp158->vcc)))
> >> +		pr_err("%s: vcc: %d", __func__, err);
> > 
> > - dev_err
> > - please expand error messages
> > - ERROR: do not use assignment in if condition
> 
> simple-bridge.c uses DRM_ERROR, but it says:
> "NOTE: this is deprecated in favor of pr_err()"
> Hence, I used pr_err.
> Are you saying I need to record the dev,
> just to be able to call dev_err?

Yes, please. Otherwise it's just random 'foo: vcc: -1'. Note, that most
of the error-reporting codes doesn't use __func__.

> 
> 
> > empty line
> 
> Will do.
> 
> >> +	return drm_bridge_attach(bridge->encoder, tdp158->next, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > 
> > No. Pass flags directly.
> 
> Oh, just pass the flags argument here? OK.

Yes

> 
> 
> >> +	tdp158->next = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > 
> > Missing `select DRM_PANEL_BRIDGE`
> 
> OK.
> 
> >> +	if (IS_ERR(tdp158->next))
> >> +		return dev_err_probe(dev, PTR_ERR(tdp158->next), "next");
> > 
> > This results in a cryptic message 'foo: ESOMETHING: next'. Please
> > expand.
> 
> OK.
> 
> Thanks for the in-depth review & suggestions.
> Will respin with fixes.
> 
> Regards
> 

-- 
With best wishes
Dmitry