[PATCH v3 2/3] ACPI / PMIC: Implement exec_mipi_pmic_seq_element for CHT Whiskey Cove PMIC

[Hans de Goede]

Hi,

On 13-12-18 13:14, Ville Syrjälä wrote:
> On Thu, Dec 13, 2018 at 12:21:35PM +0100, Hans de Goede wrote:
>> Implement the exec_mipi_pmic_seq_element callback for the CHT Whiskey Cove
>> PMIC.
>>
>> On some CHT devices this fixes the LCD panel not lighting up when it was
>> not initialized by the GOP, because an external monitor was plugged in and
>> the GOP initialized only the external monitor.
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>> Changes in v2:
>> -Interpret data passed to the PMIC MIPI elements according to the docs
>>   instead of my own reverse engineered interpretation
>> Changes in v3:
>> -Use hex values for out of range checks
>> -Make intel_cht_wc_exec_mipi_pmic_seq_element return errors
>> ---
>>   drivers/acpi/pmic/intel_pmic_chtwc.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
>> index 078b0448f30a..8ede74e9b89f 100644
>> --- a/drivers/acpi/pmic/intel_pmic_chtwc.c
>> +++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/mfd/intel_soc_pmic.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/regmap.h>
>> +#include <asm/unaligned.h>
>>   #include "intel_pmic.h"
>>   
>>   #define CHT_WC_V1P05A_CTRL		0x6e3b
>> @@ -231,6 +232,29 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
>>   	return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0);
>>   }
>>   
>> +static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap,
>> +						   const u8 *data)
>> +{
>> +	u32 value, mask, reg_address, address;
>> +	u16 i2c_client_address;
>> +
>> +	/* byte 0 aka PMIC Flag is reserved */
>> +	i2c_client_address	= get_unaligned_le16(data + 1);
>> +	reg_address		= get_unaligned_le32(data + 3);
>> +	value			= get_unaligned_le32(data + 7);
>> +	mask			= get_unaligned_le32(data + 11);
> 
> Upon further reflection maybe it would better to do this decoding in
> the i915 code and just pass each parameter to this hook separately?
> That way we wouldn't be spreading the vbt details all over the place.

Interesting point, if the VBT spec says that this encoding is PMIC
independent, then yes we should probably fo the decoding in the VBT
code and change the intel_soc_pmic_exec_mipi_pmic_seq_element
prototype to:

int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address,
					      u32 value, u32 mask);

If you agree please let me know and I will do a v4 of the patchset.

I've also been thinking about trying to make the implementation
under drivers/acpi/pmic pmic independent, but not all pmic
drivers use the regmap the same way. The CHT Whiskey Cove PMIC
mfd driver uses a regmap with 16 bit addresses where the upper
byte is the i2c client address and the lower byte is the register
address (this PMIC listens on multiple addresses, with different
registers behind each i2c address).

Where as most PMIC mfd drivers simply use the standard
devm_regmap_init_i2c() method of creating a regmap.  For these
others we could do a standard implementation where we check the
passed in i2c_address is what we expect (for that type PMIC) and
then pass the other 3 parameters to regmap_update_bits.

But I think it would be best to wait with such a generic implementation
until we encounter a device using the PMIC MIPI sequence element
with another type of PMIC.  Since we still need the special
implementation for the CHT WC case, we still need an operation
pointer for this in intel_pmic_opregion_data anyways, so we can
easily plug in the generic implementation for others later.

Regards,

Hans





>  
>> +
>> +	if (i2c_client_address > 0xff || reg_address > 0xff) {
>> +		pr_warn("%s warning addresses too big client 0x%x reg 0x%x\n",
>> +			__func__, i2c_client_address, reg_address);
>> +		return -ERANGE;
>> +	}
>> +
>> +	address = (i2c_client_address << 8) | reg_address;
>> +
>> +	return regmap_update_bits(regmap, address, mask, value);
>> +}
>> +
>>   /*
>>    * The thermal table and ops are empty, we do not support the Thermal opregion
>>    * (DPTF) due to lacking documentation.
>> @@ -238,6 +262,7 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
>>   static struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
>>   	.get_power		= intel_cht_wc_pmic_get_power,
>>   	.update_power		= intel_cht_wc_pmic_update_power,
>> +	.exec_mipi_pmic_seq_element = intel_cht_wc_exec_mipi_pmic_seq_element,
>>   	.power_table		= power_table,
>>   	.power_table_count	= ARRAY_SIZE(power_table),
>>   };
>> -- 
>> 2.19.2
>