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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 13 Dec 2018, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Thu, Dec 13, 2018 at 01:40:27PM +0100, Hans de Goede wrote:
>> 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@xxxxxxxxxx>
>> >> ---
>> >> 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.
>
> Yeah, I think that's probably better. The spec has just the one
> interpretation for the sequence.

Agreed.

BR,
Jani.

>
>> 
>> 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.
>
> Yeah, probably not worth worrying about this until we
> encounter a machine that needs it.
>
> Oh, and we should probably change the DRM_DEBUG_KMS() for the
> PMIC_OPREGION=n case to a DRM_ERROR() which tells people to
> enable PMIC_OPREGION=y. Not sure why all these random knobs are
> even user configurable. No one can really be expected to know
> how to configure them properly. There was a recent problem with
> someone having set I2C_DESIGNWARE_BAYTRAIL=n as well because
> they had a CHT/BSW instead of a BYT :(

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux