[Intel-gfx] [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling

[Hans de Goede]

Hi,

On 08-01-19 18:33, Andy Shevchenko wrote:
> On Tue, Jan 08, 2019 at 04:35:45PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 08-01-19 15:51, Andy Shevchenko wrote:
>>> On Tue, Jan 08, 2019 at 02:45:39PM +0100, Hans de Goede wrote:
>>>> On 07-01-19 16:46, Andy Shevchenko wrote:
>>>>> On Mon, Jan 07, 2019 at 12:15:55PM +0100, Hans de Goede wrote:
>>>
>>>>>> +	} else if (d->pmic_i2c_address) {
>>>>>> +		if (i2c_address == d->pmic_i2c_address) {
>>>>>> +			ret = regmap_update_bits(intel_pmic_opregion->regmap,
>>>>>> +						 reg_address, mask, value);
>>>>>> +		} else {
>>>>>> +			pr_err("%s: Unexpected i2c-addr: 0x%02x (reg-addr 0x%x value 0x%x mask 0x%x)\n",
>>>>>> +			       __func__, i2c_address, reg_address, value, mask);
>>>>>> +			ret = -ENXIO;
>>>>>> +		}
>>>>>
>>>>>> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
>>>>>> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
>>>>>> +	.pmic_i2c_address = 0x34,
>>>>>
>>>>> Can we just have a hook here instead of exposing PMIC I2C address?
>>>>> Am I missing something in case it's not possible?
>>>>
>>>> We already have a hook, but it isn't really necessary to implement
>>>> that for each model PMIC. The MFD device which is the PMIC's parent
>>>> in most cases will give us a regmap to access the PMIC registers and
>>>> that allows us to do a generic implementation.
>>>>
>>>> But the MIPI PMIC sequence includes an i2c-address as some PMICs
>>>> span multiple i2c-addresses. For the simple single i2c-address case
>>>> the regmap gives us access to the registers behind that single address
>>>> and we can use a generic solution. In this case we should verify the
>>>> i2c-addr is what we expect, which is where the pmic_i2c_address comes in.
>>>
>>> But we also can have a generic hook in intel_pmic.c and assign it whenever
>>> it's the case?
>>
>> We could, but then we still need the pmic_i2c_address member and now the
>> hook would need to passed both the regmap and the intel_pmic_opregion_data
>> pointer so that it can verify the i2c address so handling the generic case
>> directly inside intel_soc_pmic_exec_mipi_pmic_seq_element is easier.
> 
> I see.
> 
> One more question, can we unify somehow error messages and do something like
> 
> if (...) {
> 	...
> } else if (pmic_i2c_address && i2c_address == pmic_i2c_address) {
> 	ret = regmap_update_bits(...);
> } else {
> 	...
> }

I can understand where you are coming from with this request but I would
prefer to keep the messages separate, merging them doing something like this:

if (...) {
  	...
} else if (pmic_i2c_address && i2c_address == pmic_i2c_address) {
  	ret = regmap_update_bits(...);
} else {
  	...
}

if (ret)
	pr_err()

Would mean that the messages become less clear and the user would need to
go by the error code to figure out what is going on. Also currently one
of the 2 messages this would merge is a pr_err, while the other is a pr_warn.

I hope that the clear error messages lead to clear bug reports (or help
the user over the threshold to report a bug at all when they are hit).

Regards,

Hans