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.
>
> 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 :(
--
Ville Syrjälä
Intel
