Re: [PATCH v3 1/3] ACPI / PMIC: Add support for executing PMIC MIPI sequence elements

[Hans de Goede <hdegoede@redhat.com>]

HI,

On 13-12-18 14:45, Jani Nikula wrote:
> On Thu, 13 Dec 2018, Hans de Goede <hdegoede@redhat.com> wrote:
>> DSI LCD panels describe an initialization sequence in the Video BIOS
>> Tables using so called MIPI sequences. One possible element in these
>> sequences is a PMIC specific element of 15 bytes.
>>
>> Although this is not really an ACPI opregion, the ACPI opregion code is the
>> closest thing we have. We need to have support for these PMIC specific MIPI
>> sequence elements somwhere. Since we already instantiate a special platform
>> device for Intel PMICs for the ACPI PMIC OpRegion handler to bind to,
>> with PMIC specific implementations of the OpRegion, the handling of MIPI
>> sequence PMIC elements fits very well in the ACPI PMIC OpRegion code.
>>
>> This commit adds a new intel_soc_pmic_exec_mipi_pmic_seq_element()
>> function, which is to be backed by a PMIC specific
>> exec_mipi_pmic_seq_element callback. This function will be called by the
>> i915 code to execture MIPI sequence PMIC elements.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3:
>> -Add kerneldoc for intel_soc_pmic_exec_mipi_pmic_seq_element
>> -Make intel_soc_pmic_exec_mipi_pmic_seq_element return errors
>> ---
>>   drivers/acpi/pmic/intel_pmic.c     | 42 ++++++++++++++++++++++++++++++
>>   drivers/acpi/pmic/intel_pmic.h     |  1 +
>>   include/linux/mfd/intel_soc_pmic.h |  2 ++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
>> index ca18e0d23df9..61f99735fd41 100644
>> --- a/drivers/acpi/pmic/intel_pmic.c
>> +++ b/drivers/acpi/pmic/intel_pmic.c
>> @@ -15,6 +15,7 @@
>>   
>>   #include <linux/export.h>
>>   #include <linux/acpi.h>
>> +#include <linux/mfd/intel_soc_pmic.h>
>>   #include <linux/regmap.h>
>>   #include <acpi/acpi_lpat.h>
>>   #include "intel_pmic.h"
>> @@ -36,6 +37,8 @@ struct intel_pmic_opregion {
>>   	struct intel_pmic_regs_handler_ctx ctx;
>>   };
>>   
>> +static struct intel_pmic_opregion *intel_pmic_opregion;
>> +
>>   static int pmic_get_reg_bit(int address, struct pmic_table *table,
>>   			    int count, int *reg, int *bit)
>>   {
>> @@ -304,6 +307,7 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
>>   	}
>>   
>>   	opregion->data = d;
>> +	intel_pmic_opregion = opregion;
>>   	return 0;
>>   
>>   out_remove_thermal_handler:
>> @@ -319,3 +323,41 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(intel_pmic_install_opregion_handler);
>> +
>> +/**
>> + * intel_soc_pmic_exec_mipi_pmic_seq_element - Execute PMIC MIPI sequence
>> + * @data: Pointer to *15* byte PMIC MIPI sequence element
>> + *
>> + * DSI LCD panels describe an initialization sequence in the i915 VBT (Video
>> + * BIOS Tables) using so called MIPI sequences. One possible element in these
>> + * sequences is a PMIC specific element of 15 bytes.
>> + *
>> + * This function executes these PMIC specific elements sending the embedded
>> + * commands to the PMIC.
>> + *
>> + * Return 0 on success, < 0 on failure.
>> + */
>> +int intel_soc_pmic_exec_mipi_pmic_seq_element(const u8 *data)
>> +{
>> +	struct intel_pmic_opregion_data *d;
>> +	int ret;
>> +
>> +	if (!intel_pmic_opregion) {
>> +		pr_warn("%s: No PMIC registered\n", __func__);
>> +		return -ENXIO;
>> +	}
>> +
>> +	d = intel_pmic_opregion->data;
>> +	if (!d->exec_mipi_pmic_seq_element) {
>> +		pr_warn("%s: Not implemented\n", __func__);
>> +		pr_warn("%s: Data: %15ph\n", __func__, data);
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	mutex_lock(&intel_pmic_opregion->lock);
>> +	ret = d->exec_mipi_pmic_seq_element(intel_pmic_opregion->regmap, data);
>> +	mutex_unlock(&intel_pmic_opregion->lock);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(intel_soc_pmic_exec_mipi_pmic_seq_element);
>> diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
>> index 095afc96952e..1cc5e69af0c3 100644
>> --- a/drivers/acpi/pmic/intel_pmic.h
>> +++ b/drivers/acpi/pmic/intel_pmic.h
>> @@ -15,6 +15,7 @@ struct intel_pmic_opregion_data {
>>   	int (*update_aux)(struct regmap *r, int reg, int raw_temp);
>>   	int (*get_policy)(struct regmap *r, int reg, int bit, u64 *value);
>>   	int (*update_policy)(struct regmap *r, int reg, int bit, int enable);
>> +	int (*exec_mipi_pmic_seq_element)(struct regmap *r, const u8 *data);
>>   	struct pmic_table *power_table;
>>   	int power_table_count;
>>   	struct pmic_table *thermal_table;
>> diff --git a/include/linux/mfd/intel_soc_pmic.h b/include/linux/mfd/intel_soc_pmic.h
>> index ed1dfba5e5f9..71a8607a0a8c 100644
>> --- a/include/linux/mfd/intel_soc_pmic.h
>> +++ b/include/linux/mfd/intel_soc_pmic.h
>> @@ -26,4 +26,6 @@ struct intel_soc_pmic {
>>   	struct device *dev;
>>   };
>>   
>> +int intel_soc_pmic_exec_mipi_pmic_seq_element(const u8 *data);
> 
> Add a static inline dummy implementation with error return for
> CONFIG_PMIC_OPREGION=n? Maybe even with pr_err or pr_warn or something?

There is only one caller and Ville has requested for the caller to
do a DRM_ERROR("Your hardware requires CONFIG_PMIC_OPREGION and it is not set")
if we hit that code-path and CONFIG_PMIC_OPREGION is not set.

We could do the DRM_ERROR from the wrapper, but that would make the
wrapper DRM specific which feels wrong to me.

Regards,

Hans

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx