On Wed, 02 Dec 2020, "Navare, Manasi" <manasi.d.navare@xxxxxxxxx> wrote:
> On Tue, Dec 01, 2020 at 02:21:56PM -0800, Navare, Manasi wrote:
>> On Tue, Nov 10, 2020 at 12:39:08PM +0200, Jani Nikula wrote:
>> > > +{
>> > > + struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
>> >
>> > I kind of feel like either the function should a) ensure it's okay to do
>> > intel_attached_dp() and return false if not, or b) just use struct
>> > intel_dp as the parameter.
>> >
>> > As it is, passing a non-dp connector to this function will fail either
>> > subtly or spectacularly, but not graciously.
>
> Actually after doing some code rewriting, I think it is best to still pass
> the drm connector but I am adding the check now for connector type
> Only if its eDP or DP now I get the intel_dp.
> Future we can add HDMI there as well.
It's fine, as long as we don't cast to intel_dp unless we know it's
intel_dp.
>> > > +
>> > > + /*
>> > > + * DP Sink is capable of Variable refresh video timings if
>> > > + * Ignore MSA bit is set in DPCD.
>> > > + * EDID monitor range also should be atleast 10 for reasonable
>> > > + * Adaptive sync/ VRR end user experience.
>> > > + */
>> >
>> > Please fix typos etc.
>>
>> Did I miss some typos, I dont see any in the above comment?
Odd capitalization, "atleast", "sync/ VRR", maybe also reflow the
paragraph.
I know it's nitpicking, but other people will read the comment many,
many more times than you write it. ;)
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
