Message-ID: <ed1e0e2f-25a0-91a9-9f0b-a41b391ee869@redhat.com>
Date:   Wed, 24 Nov 2021 17:25:17 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH] mfd: Kconfig: change INTEL_SOC_PMIC_CHTDC_TI to bool
Content-Language: en-US
To:     Lee Jones <lee.jones@linaro.org>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc:     linuxarm@huawei.com, mauro.chehab@huawei.com,
        linux-kernel@vger.kernel.org
References: <f12ed3dea697e6fdcfcaf9b6cc862ffa7aac005b.1635490923.git.mchehab+huawei@kernel.org>
 <YZ5c0LO5Ocaq+B7c@google.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <YZ5c0LO5Ocaq+B7c@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 11/24/21 16:40, Lee Jones wrote:
> On Fri, 29 Oct 2021, Mauro Carvalho Chehab wrote:
> 
>> The INTEL_SOC_PMIC_CHTDC_TI should be initialized early, before
>> loading the fbcon driver, as otherwise the i915 driver will
>> fail to configure pwm:
>>
>> [   13.674287] fb0: switching to inteldrmfb from EFI VGA
>> [   13.682380] Console: switching to colour dummy device 80x25
>> [   13.682468] i915 0000:00:02.0: vgaarb: deactivate vga console
>> [   13.682686] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>> [   13.685773] i915 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
>> [   13.686219] i915 0000:00:02.0: [drm] *ERROR* Failed to configure the pwm chip
>> [   13.699572] [drm] Initialized i915 1.6.0 20200313 for 0000:00:02.0 on minor 0
>> [   13.739044] fbcon: i915drmfb (fb0) is primary device
>> [   14.037792] intel_soc_pmic_exec_mipi_pmic_seq_element: No PMIC registered
>> ...
>> [   24.621403] intel_pmic_install_opregion_handler: Ask to register OpRegion for bus ID=PMI2, HID=INT33F5
>> [   24.630540] intel_pmic_install_opregion_handler: OpRegion registered
>>
>> (some extra debug printk's were added to the above)
>>
>> As suggested by Hans, this patch also addresses an issue with
>> the dependencies, as, for this driver to be a bool, it also
>> need the I2C core and the I2C_DESIGNWARE driver to be builtin.
>>
>> Suggested-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>> ---
>>  drivers/mfd/Kconfig | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index ca0edab91aeb..f9092c79c4e8 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -632,7 +632,7 @@ config INTEL_SOC_PMIC_CHTWC
>>  config INTEL_SOC_PMIC_CHTDC_TI
>>  	tristate "Support for Intel Cherry Trail Dollar Cove TI PMIC"
>>  	depends on GPIOLIB
>> -	depends on I2C
>> +	depends on I2C = y && I2C_DESIGNWARE_PLATFORM=y
> 
> The lack of formatting consistency here is eating me up inside!
> 
>>  	depends on ACPI
>>  	depends on X86
>>  	select MFD_CORE
>> @@ -642,6 +642,10 @@ config INTEL_SOC_PMIC_CHTDC_TI
>>  	  Select this option for supporting Dollar Cove (TI version) PMIC
>>  	  device that is found on some Intel Cherry Trail systems.
>>  
>> +	  This option is a bool as it provides an ACPI OpRegion which must be
>> +	  available before any devices using it are probed. This option also
>> +	  needs the designware-i2c driver to be builtin for the same reason.
> 
> Is there no way around this?

No unfortunately not, the ACPI device-scan is done really early,
and in ACPI everything is a function, including e.g. _HID,
so I've seen _HID method-s reading e.g. GPIOs. So while the
initial ACPI scan is figuring out what sort of devices it
is dealing with, we already need working GPIOs (for example).

Various early ACPI code (AML / the DSDT) tends to access the
PMICs OpRegion and various problems happen when it is not
available. For the same reason the other BYT/CHT related
INTEL_SOC_PMIC_FOO options are already bools.

I guess that the DSDTs for other Intel SoCs then the BYT/CHT
SoCs might be a bit cleaner. BYT/CHT was Intel's first serious
attempt at standard x86 tablet SoC and this shows in various
places.

Regards,

Hans