[PATCH v2 17/27] drm/tegra: Add Tegra114 HDMI support

[Stephen Warren]

On 10/15/2013 02:13 AM, Thierry Reding wrote:
> On Mon, Oct 14, 2013 at 12:10:21PM -0600, Stephen Warren wrote:
>> On 10/12/2013 05:41 AM, Thierry Reding wrote:
>>> On Fri, Oct 11, 2013 at 04:19:19PM -0600, Stephen Warren
>>> wrote:
>>>> On 10/07/2013 02:34 AM, Thierry Reding wrote:
>>>>> From: Mikko Perttunen <mperttunen at nvidia.com>
>>>>> 
>>>>> Tegra114 TMDS configuration requires a new peak_current
>>>>> field and the driver current override bit has changed
>>>>> position.
>>>> 
>>>>> diff --git a/drivers/gpu/drm/tegra/hdmi.c 
>>>>> b/drivers/gpu/drm/tegra/hdmi.c
>>>> 
>>>>> static const struct tmds_config tegra2_tmds_config[] = {
>>>>> @@ -223,6 +224,85 @@ static const struct tmds_config 
>>>>> tegra3_tmds_config[] = {
>>>> 
>>>> Not related to this patch, but those should have been named 
>>>> tegra20_tmds_config[] and tegra30_tmds_config[].
>>>> 
>>>>> static void tegra_hdmi_setup_tmds(struct tegra_hdmi *hdmi,
>>>> 
>>>>> -	value = tmds->drive_current |
>>>>> DRIVE_CURRENT_FUSE_OVERRIDE; - tegra_hdmi_writel(hdmi,
>>>>> value, HDMI_NV_PDISP_SOR_LANE_DRIVE_CURRENT); +	if 
>>>>> (of_device_is_compatible(np, "nvidia,tegra114-hdmi")) {
>>>> 
>>>> Let's not check this at run-time. Instead,
>>>> host1x_drm_subdevs[]'s .data field should be used to contain
>>>> either flags or a pointer to a configuration structure,
>>>> either of which can be directly consulted to determine the
>>>> properties of the HW in a feature-oriented/semantic way.
>>>> 
>>>> drivers/gpio/gpio-tegra.c's 
>>>> tegra20_gpio_config/tegra30_gpio_config/tegra_gpio_of_match 
>>>> provide a good example of this.
>>>> 
>>>> This means that if Tegra124 is identical to Tegra114, yet a 
>>>> hypothetical Tegra999 is different, you don't have to keep 
>>>> adjusting these if conditions throughout the code; they can 
>>>> simply refer to the same feature bit forever.
>>> 
>>> Okay, I'll see what I can come up with. It's unfortunately not
>>> as simple as the GPIO driver's parameterization, and who knows
>>> what other differences will be introduced in some later
>>> versions of this block.
>>> 
>>> What I mean is that at some point it becomes questionable
>>> whether it makes sense to parameterize at all if you have to
>>> encode the register offset and bit position within that
>>> register for a large number of bits.
>> 
>> Well, I wasn't advocating that we shouldn't have an if statement
>> at all. Simply that the if statement shouldn't be doing string
>> compares of specific HW. Either of the following would be fine:
>> 
>> if (hdmi->soc_data->some_feature_flag) // just represents some
>> code; doesn't have to be a function call do_something(); else; 
>> do_something_else();
>> 
>> or:
>> 
>> do_something(hdmi->soc_data->some_feature_value);
> 
> But the fact that a bit has moved from one register to another can 
> hardly be defined as feature. At least I couldn't come up with any 
> sensible name for one.

The feature is the name/identification of the register the field is
in. For example, foo_field_in_bar_reg. Admittedly this isn't "feature"
in the typical sense, but more a "facet of the SW-visible interface".

> We could of course just add a version number into a per-SoC
> descriptor and use that, but that's not any better than checking
> for the compatible value, really.

Even that would be better; it'd avoid a strcmp() every time the code
ran, and also allow the of_match table's data to map from compatible
value to register layout ID. It's quite possible that we have 4 SoCs:

SoC / Register location of field X / Other features, etc.
100   A                              P
200   B                              Q
300   B                              Q
400   B                              R

Where P, Q, R need different compatible values, but the location of
the register field we're talking about isn't 1:1 with the compatible
values, but rather many:1.

> Ideally of course the hardware wouldn't change in these ways from
> one generation to the next...
> 
> That said, I've opted to go with putting the register and bit
> position into a per-SoC descriptor and parameterize on that, along
> with a boolean flag for the existence of the IO peak current
> register.

Sounds good.