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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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.

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx