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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?


> 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.


>> +	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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux