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
