Re: [PATCH 1/2] pwm: lpss: Make builtin and add lookup-table so that i915 can find the pwm_backlight

From: Thierry Reding <thierry.reding@gmail.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: linux-pwm@vger.kernel.org,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 1/2] pwm: lpss: Make builtin and add lookup-table so that i915 can find the pwm_backlight
Date: Mon, 5 Dec 2016 08:46:30 +0100	[thread overview]
Message-ID: <20161205074630.GD18069@ulmo.ba.sec> (raw)
In-Reply-To: <20161202101736.12236-1-hdegoede@redhat.com>

[-- Attachment #1.1: Type: text/plain, Size: 1873 bytes --]

On Fri, Dec 02, 2016 at 11:17:35AM +0100, Hans de Goede wrote:
> The primary consumer of the lpss pwm is the i915 kms driver, but
> currently that driver cannot get the pwm because i915 platforms are
> not using devicetree and pwm-lpss does not call pwm_add_table.
> 
> Another problem is that i915 does not support get_pwm returning
> -EPROBE_DEFER and i915's init is very complex and this is almost
> impossible to fix.
> 
> This commit changes the PWM_LPSS Kconfig from a tristate to a bool, so
> that when the i915 driver loads the lpss pwm will be available avoiding
> the -EPROBE_DEFER issue. Note that this is identical to how the same
> problem was solved for the pwm-crc driver.
> 
> Being builtin also allows calling pwm_add_table directly from the
> pwm-lpss code, otherwise the pwm_add_table call would need to be put
> somewhere else to ensure it happens before i915 calls pwm_get,
> even if i915 would support -EPROBE_DEFER.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pwm/Kconfig    | 12 +++---------
>  drivers/pwm/pwm-lpss.c | 11 +++++++++++
>  2 files changed, 14 insertions(+), 9 deletions(-)

This is completely backwards. You're putting board-specific bits into a
generic driver.

And no, this is not the same as pwm-crc because the board-specific bits
went into the MFD driver. I don't like that very much either, but there
is documentation that suggests that the Crystal Cove PMIC is used on a
single platform, whereas there's no such evidence for LPSS.

Why can't you just add something to drivers/platform/x86 to register the
table? You could base that on some kind of identifier (ACPI, DMI, ...)
to only do so if necessary.

I could probably live with making this builtin, but your justification
sounds like a bit of a lame excuse rather than a good technical reason.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx