Re: [Intel-gfx] [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling
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
Re: [Intel-gfx] [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling
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 {
...
}
?
--
With Best Regards,
Andy Shevchenko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling
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?
--
With Best Regards,
Andy Shevchenko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling
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?
--
With Best Regards,
Andy Shevchenko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 3/4] ACPI / PMIC: Add generic intel_soc_pmic_exec_mipi_pmic_seq_element handling
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
> ---
> 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ä
> };
>
> 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
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
