On Mon, 30 Dec 2024, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
> On Mon, Dec 30, 2024 at 03:18:35PM +0200, Jani Nikula wrote:
>> On Thu, 26 Dec 2024, Abel Vesa <abel.vesa@xxxxxxxxxx> 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
