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: >> Hi all >> >> On 22/02/2021 13:07, Daniel Scally wrote: >>> 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 >
