Hi Hans, thanks for the input

On 22/02/2021 13:27, Hans de Goede wrote:

Hi,

On 2/22/21 2:19 PM, Daniel Scally wrote:

quoted

Hi all

On 22/02/2021 13:07, Daniel Scally wrote:

quoted

diff --git a/drivers/platform/x86/intel-int3472/Kconfig b/drivers/platform/x86/intel-int3472/Kconfig
new file mode 100644
index 000000000000..b94622245c21
--- /dev/null
+++ b/drivers/platform/x86/intel-int3472/Kconfig

@@ -0,0 +1,31 @@
+config INTEL_SKL_INT3472
+	tristate "Intel SkyLake ACPI INT3472 Driver"
+	depends on ACPI
+	depends on REGULATOR
+	depends on GPIOLIB
+	depends on COMMON_CLK && CLKDEV_LOOKUP
+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  This driver adds support for the INT3472 ACPI devices found on some
+	  Intel SkyLake devices.
+
+	  The INT3472 is an Intel camera power controller, a logical device
+	  found on some Skylake-based systems that can map to different
+	  hardware devices depending on the platform. On machines
+	  designed for Chrome OS, it maps to a TPS68470 camera PMIC. On
+	  machines designed for Windows, it maps to either a TP68470
+	  camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs
+	  and power gates.
+
+	  If your device was designed for Chrome OS, this driver will provide
+	  an ACPI OpRegion, which must be available before any of the devices
+	  using it are probed. For this reason, you should select Y if your
+	  device was designed for ChromeOS. For the same reason the
+	  I2C_DESIGNWARE_PLATFORM option must be set to Y too.
+
+	  Say Y or M here if you have a SkyLake device designed for use
+	  with Windows or ChromeOS. Say N here if you are not sure.
+
+	  The module will be named "intel-skl-int3472"

The Kconfig option for the existing tps68470 driver is a bool which
depends on I2C_DESIGNWARE_PLATFORM=y, giving the following reason:

This option is a bool as it provides an ACPI operation
region, which must be available before any of the devices
using this are probed. This option also configures the
designware-i2c driver to be built-in, for the same reason.

One problem I've faced is that that scenario only applies to some
devices that this new driver can support, so hard-coding it as built in
didn't make much sense. For that reason I opted to set it tristate, but
of course that issue still exists for ChromeOS devices where the
OpRegion will be registered. I opted for simply documenting that
requirement, as is done in aaac4a2eadaa6: "mfd: axp20x-i2c: Document
that this must be builtin on x86", but that's not entirely satisfactory.
Possible alternatives might be setting "depends on
I2C_DESIGNWARE_PLATFORM=y if CHROME_PLATFORMS" or something similar,
though of course the User would still have to realise they need to
build-in the INTEL_SKL_INT3472 Kconfig option too.

Feedback around this issue would be particularly welcome, as I'm not
sure what the best approach might be.

This is a tricky area, I actually wrote the "mfd: axp20x-i2c: Document
that this must be builtin on x86" patch you refer to. At first I tried
to express the dependency in Kconfig language but things got too complex
and Kconfig sometimes became unhappy about circular deps (or something
like that).

Yes, I had a go too; with similar results

The most important thing here is to make sure that the generic configs
shipped by distros get this right; and we can hope that people creating
those configs at least read the help text...

So all in all I believe that just documenting the requirement is fine.

OK - that's what I'm hoping is the consensus, as I don't think it can be
made _entirely_ seamless through dependencies or whatever anyway, in
which case documenting it seems like the cleanest approach to me.

The alternative would be to just change I2C_DESIGNWARE_PLATFORM (and the
core) to a bool, or at least make it not selectable as module when
X86 and ACPI are set... That would be a bit of a big hammer but might
not be the worst idea actually.

Regards,

Hans