Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver

[Hans de Goede]

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

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.

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