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@xxxxxxxxxx>
> >>>
> >>> 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.
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.
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.
Thierry
Attachment:
pgp6yw3EtKALR.pgp
Description: PGP signature
