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