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