Re: [PATCH v2 1/4] drm/dp: Add helper to set LTTPRs in transparent mode

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 30 Dec 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Mon, Dec 30, 2024 at 03:18:35PM +0200, Jani Nikula wrote:
>> On Thu, 26 Dec 2024, Abel Vesa <abel.vesa@linaro.org> wrote:
>> > On 24-12-11 15:42:27, Johan Hovold wrote:
>> >> On Wed, Dec 11, 2024 at 03:04:12PM +0200, Abel Vesa wrote:
>> >>  
>> >> > +/**
>> >> > + * drm_dp_lttpr_set_transparent_mode - set the LTTPR in transparent mode
>> >> > + * @aux: DisplayPort AUX channel
>> >> > + * @enable: Enable or disable transparent mode
>> >> > + *
>> >> > + * Returns 0 on success or a negative error code on failure.
>> >> > + */
>> >> > +int drm_dp_lttpr_set_transparent_mode(struct drm_dp_aux *aux, bool enable)
>> >> > +{
>> >> > +	u8 val = enable ? DP_PHY_REPEATER_MODE_TRANSPARENT :
>> >> > +			  DP_PHY_REPEATER_MODE_NON_TRANSPARENT;
>> >> > +	int ret = drm_dp_dpcd_writeb(aux, DP_PHY_REPEATER_MODE, val);
>> >> > +
>> >> > +	return ret == 1 ? 0 : ret;
>> >> 
>> >> This looks correct, but I had to go look at drm_dp_dpcd_writeb() to make
>> >> sure it never returns 0 (for short transfers).
>> >
>> > Will follow Dmitry's proposal here.
>> >
>> > 	if (ret < 0)
>> >         	return ret;
>> >
>> > 	return (ret == 1) ? 0 : -EIO;
>> 
>> Arguably this (well, with ret == len) is what we should've done with
>> *all* of the drm_dp_dpcd_*() functions. I don't think there's a single
>> case where we'd actually need to know that some but not all data was
>> transferred. And if there are, they could be special cased. Now we have
>> hundreds of cases where we check against length and it's just cumbersome
>> all over the place.
>> 
>> The question is, how confusing is it going to be to have some of the new
>> functions return 0 instead of len? Very? Extremely?
>> 
>> As painful as it would be, I'd be in favor of changing them all to
>> return 0 on ret == len. If we find a volunteer.
>
> Maybe a correct Coccinelle script can do a significant part of such a
> conversion for us?
>
> Anyway, I think it a right thing to do. Could you possibly add a new set
> of API and use it inside i915 driver? Then during the next cycle we can
> start using new functions for all other drivers. Or would you rather add
> new API through drm-misc? Then we can concert e.g. existing helpers in
> the first place and then start working on the drivers.

There are hundreds of drm_dp_dpcd_{read,readb,write,writeb} uses across
drm, and then all the higher level helpers on top. I'm not sure adding a
new API and using it in i915 achieves much.

BR,
Jani.


-- 
Jani Nikula, Intel