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