| Message ID | 20190107111556.4510-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ACPI/i915: Add support for PMIC MIPI sequence elements | expand |
On Mon, Jan 07, 2019 at 12:15:55PM +0100, Hans de Goede wrote: > Most PMIC-s use only a single i2c-address, so after verifying the > i2c-address matches, we can simply pass the call to regmap_update_bits. > > This commit adds support for this and hooks this up for the xpower AXP288 > PMIC by setting the new pmic_i2c_address field. > > This fixes the following errors on display on / off on a Jumper Ezpad > mini 3 and an Onda V80 plus tablet, both of which use the AXP288: > > intel_soc_pmic_exec_mipi_pmic_seq_element: Not implemented > intel_soc_pmic_exec_mipi_pmic_seq_element: i2c-addr: 0x34 reg-addr ... > [drm:mipi_exec_pmic [i915]] *ERROR* mipi_exec_pmic failed, error: -95 > > Instead of these errors on both devices we now correctly turn on / off > DLDO3 (through direct register manipulation). On the Onda V80 plus this > fixes an issue with the backlight being brighter around the borders after > an off / on cycle. This should also help to save some power when the > display is off. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v6: > -This is a new patch in v6 of this patch-set > --- > drivers/acpi/pmic/intel_pmic.c | 9 +++++++++ > drivers/acpi/pmic/intel_pmic.h | 2 ++ > drivers/acpi/pmic/intel_pmic_xpower.c | 1 + > 3 files changed, 12 insertions(+) > > diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c > index 471afeea87c2..c14cfaea92e2 100644 > --- a/drivers/acpi/pmic/intel_pmic.c > +++ b/drivers/acpi/pmic/intel_pmic.c > @@ -359,6 +359,15 @@ int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address, > ret = d->exec_mipi_pmic_seq_element(intel_pmic_opregion->regmap, > i2c_address, reg_address, > value, mask); > + } 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; > + } > } else { > pr_warn("%s: Not implemented\n", __func__); > pr_warn("%s: i2c-addr: 0x%x reg-addr 0x%x value 0x%x mask 0x%x\n", > diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h > index 5cd195fabca8..89379476a1df 100644 > --- a/drivers/acpi/pmic/intel_pmic.h > +++ b/drivers/acpi/pmic/intel_pmic.h > @@ -21,6 +21,8 @@ struct intel_pmic_opregion_data { > int power_table_count; > struct pmic_table *thermal_table; > int thermal_table_count; > + /* For generic exec_mipi_pmic_seq_element handling */ > + int pmic_i2c_address; > }; > > int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle, struct regmap *regmap, struct intel_pmic_opregion_data *d); > diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c > index e7c0006e6602..a091d5a8392c 100644 > --- a/drivers/acpi/pmic/intel_pmic_xpower.c > +++ b/drivers/acpi/pmic/intel_pmic_xpower.c > @@ -265,6 +265,7 @@ static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = { > .power_table_count = ARRAY_SIZE(power_table), > .thermal_table = thermal_table, > .thermal_table_count = ARRAY_SIZE(thermal_table), > + .pmic_i2c_address = 0x34, Seems to match the axp288 datasheet. FWIW Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > }; > > static acpi_status intel_xpower_pmic_gpio_handler(u32 function, > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jan 07, 2019 at 12:15:55PM +0100, Hans de Goede wrote: > Most PMIC-s use only a single i2c-address, so after verifying the > i2c-address matches, we can simply pass the call to regmap_update_bits. > > This commit adds support for this and hooks this up for the xpower AXP288 > PMIC by setting the new pmic_i2c_address field. > > This fixes the following errors on display on / off on a Jumper Ezpad > mini 3 and an Onda V80 plus tablet, both of which use the AXP288: > > intel_soc_pmic_exec_mipi_pmic_seq_element: Not implemented > intel_soc_pmic_exec_mipi_pmic_seq_element: i2c-addr: 0x34 reg-addr ... > [drm:mipi_exec_pmic [i915]] *ERROR* mipi_exec_pmic failed, error: -95 > > Instead of these errors on both devices we now correctly turn on / off > DLDO3 (through direct register manipulation). On the Onda V80 plus this > fixes an issue with the backlight being brighter around the borders after > an off / on cycle. This should also help to save some power when the > display is off. > + } 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?
Hi, On 07-01-19 16:46, Andy Shevchenko wrote: > On Mon, Jan 07, 2019 at 12:15:55PM +0100, Hans de Goede wrote: >> Most PMIC-s use only a single i2c-address, so after verifying the >> i2c-address matches, we can simply pass the call to regmap_update_bits. >> >> This commit adds support for this and hooks this up for the xpower AXP288 >> PMIC by setting the new pmic_i2c_address field. >> >> This fixes the following errors on display on / off on a Jumper Ezpad >> mini 3 and an Onda V80 plus tablet, both of which use the AXP288: >> >> intel_soc_pmic_exec_mipi_pmic_seq_element: Not implemented >> intel_soc_pmic_exec_mipi_pmic_seq_element: i2c-addr: 0x34 reg-addr ... >> [drm:mipi_exec_pmic [i915]] *ERROR* mipi_exec_pmic failed, error: -95 >> >> Instead of these errors on both devices we now correctly turn on / off >> DLDO3 (through direct register manipulation). On the Onda V80 plus this >> fixes an issue with the backlight being brighter around the borders after >> an off / on cycle. This should also help to save some power when the >> display is off. > >> + } 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. Regards, Hans
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?
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. Regards, Hans
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 { ... } ?
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
diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c index 471afeea87c2..c14cfaea92e2 100644 --- a/drivers/acpi/pmic/intel_pmic.c +++ b/drivers/acpi/pmic/intel_pmic.c @@ -359,6 +359,15 @@ int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address, ret = d->exec_mipi_pmic_seq_element(intel_pmic_opregion->regmap, i2c_address, reg_address, value, mask); + } 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; + } } else { pr_warn("%s: Not implemented\n", __func__); pr_warn("%s: i2c-addr: 0x%x reg-addr 0x%x value 0x%x mask 0x%x\n", diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h index 5cd195fabca8..89379476a1df 100644 --- a/drivers/acpi/pmic/intel_pmic.h +++ b/drivers/acpi/pmic/intel_pmic.h @@ -21,6 +21,8 @@ struct intel_pmic_opregion_data { int power_table_count; struct pmic_table *thermal_table; int thermal_table_count; + /* For generic exec_mipi_pmic_seq_element handling */ + int pmic_i2c_address; }; int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle, struct regmap *regmap, struct intel_pmic_opregion_data *d); diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c index e7c0006e6602..a091d5a8392c 100644 --- a/drivers/acpi/pmic/intel_pmic_xpower.c +++ b/drivers/acpi/pmic/intel_pmic_xpower.c @@ -265,6 +265,7 @@ static struct intel_pmic_opregion_data intel_xpower_pmic_opregion_data = { .power_table_count = ARRAY_SIZE(power_table), .thermal_table = thermal_table, .thermal_table_count = ARRAY_SIZE(thermal_table), + .pmic_i2c_address = 0x34, }; static acpi_status intel_xpower_pmic_gpio_handler(u32 function,
Most PMIC-s use only a single i2c-address, so after verifying the i2c-address matches, we can simply pass the call to regmap_update_bits. This commit adds support for this and hooks this up for the xpower AXP288 PMIC by setting the new pmic_i2c_address field. This fixes the following errors on display on / off on a Jumper Ezpad mini 3 and an Onda V80 plus tablet, both of which use the AXP288: intel_soc_pmic_exec_mipi_pmic_seq_element: Not implemented intel_soc_pmic_exec_mipi_pmic_seq_element: i2c-addr: 0x34 reg-addr ... [drm:mipi_exec_pmic [i915]] *ERROR* mipi_exec_pmic failed, error: -95 Instead of these errors on both devices we now correctly turn on / off DLDO3 (through direct register manipulation). On the Onda V80 plus this fixes an issue with the backlight being brighter around the borders after an off / on cycle. This should also help to save some power when the display is off. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v6: -This is a new patch in v6 of this patch-set --- drivers/acpi/pmic/intel_pmic.c | 9 +++++++++ drivers/acpi/pmic/intel_pmic.h | 2 ++ drivers/acpi/pmic/intel_pmic_xpower.c | 1 + 3 files changed, 12 insertions(+)