[PATCH 0/3] firmware: exynos-acpm: read fix & reduce log verbosity

[PATCH 0/3] firmware: exynos-acpm: read fix & reduce log verbosity

[André Draszik]

While trying to use the ACPM driver, I stubmbled across two issues:

    * acpm_pmic_bulk_read() doesn't return the correct register values
    * superfluous log messages during boot

The patches attached are the result and hopefully self-explanatory.

This driver only exists in linux-next at the moment.

Cheers,
Andre'

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
André Draszik (3):
      firmware: exynos-acpm: fix reading longer results
      firmware: exynos-acpm: silence EPROBE_DEFER error on boot
      firmware: exynos-acpm: convert to dev_err_probe() in client API

 drivers/firmware/samsung/exynos-acpm-pmic.c | 16 ++++++++--------
 drivers/firmware/samsung/exynos-acpm.c      | 13 ++++++-------
 2 files changed, 14 insertions(+), 15 deletions(-)
---
base-commit: 0226d0ce98a477937ed295fb7df4cc30b46fc304
change-id: 20250314-acpm-fixes-348c058476a7

Best regards,
-- 
André Draszik <andre.draszik@linaro.org>

[PATCH 1/3] firmware: exynos-acpm: fix reading longer results

[André Draszik]

ACPM commands that return more than 8 bytes currently don't work
correctly, as this driver ignores any such returned bytes.

This is evident in at least acpm_pmic_bulk_read(), where up to 8
registers can be read back and those 8 register values are placed
starting at &xfer->rxd[8].

The reason is that xfter->rxlen is initialized with the size of a
pointer (8 bytes), rather than the size of the byte array that pointer
points to (16 bytes)

Update the code such that we set the number of bytes expected to be the
size of the rx buffer.

Note1: While different commands have different lengths rx buffers, we
have to specify the same length for all rx buffers since acpm_get_rx()
assumes they're all the same length.

Note2: The different commands also have different lengths tx buffers,
but before switching the code to use the minimum possible length, some
more testing would have to be done to ensure this works correctly in
all situations. It seems wiser to just apply this fix here without
additional logic changes for now.

Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
Signed-off-by: André Draszik <andre.draszik@linaro.org>

---
issue is in linux-next only afaics at this stage, as driver is not
merged into Linus' tree yet
---
 drivers/firmware/samsung/exynos-acpm-pmic.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/samsung/exynos-acpm-pmic.c b/drivers/firmware/samsung/exynos-acpm-pmic.c
index 85e90d236da21ed76f7adba59caec165138ad313..39b33a356ebd240506b6390163229a70a2d1fe68 100644
--- a/drivers/firmware/samsung/exynos-acpm-pmic.c
+++ b/drivers/firmware/samsung/exynos-acpm-pmic.c
@@ -43,13 +43,13 @@ static inline u32 acpm_pmic_get_bulk(u32 data, unsigned int i)
 	return (data >> (ACPM_PMIC_BULK_SHIFT * i)) & ACPM_PMIC_BULK_MASK;
 }
 
-static void acpm_pmic_set_xfer(struct acpm_xfer *xfer, u32 *cmd,
+static void acpm_pmic_set_xfer(struct acpm_xfer *xfer, u32 *cmd, size_t cmdlen,
 			       unsigned int acpm_chan_id)
 {
 	xfer->txd = cmd;
 	xfer->rxd = cmd;
-	xfer->txlen = sizeof(cmd);
-	xfer->rxlen = sizeof(cmd);
+	xfer->txlen = cmdlen;
+	xfer->rxlen = cmdlen;
 	xfer->acpm_chan_id = acpm_chan_id;
 }
 
@@ -71,7 +71,7 @@ int acpm_pmic_read_reg(const struct acpm_handle *handle,
 	int ret;
 
 	acpm_pmic_init_read_cmd(cmd, type, reg, chan);
-	acpm_pmic_set_xfer(&xfer, cmd, acpm_chan_id);
+	acpm_pmic_set_xfer(&xfer, cmd, sizeof(cmd), acpm_chan_id);
 
 	ret = acpm_do_xfer(handle, &xfer);
 	if (ret)
@@ -104,7 +104,7 @@ int acpm_pmic_bulk_read(const struct acpm_handle *handle,
 		return -EINVAL;
 
 	acpm_pmic_init_bulk_read_cmd(cmd, type, reg, chan, count);
-	acpm_pmic_set_xfer(&xfer, cmd, acpm_chan_id);
+	acpm_pmic_set_xfer(&xfer, cmd, sizeof(cmd), acpm_chan_id);
 
 	ret = acpm_do_xfer(handle, &xfer);
 	if (ret)
@@ -144,7 +144,7 @@ int acpm_pmic_write_reg(const struct acpm_handle *handle,
 	int ret;
 
 	acpm_pmic_init_write_cmd(cmd, type, reg, chan, value);
-	acpm_pmic_set_xfer(&xfer, cmd, acpm_chan_id);
+	acpm_pmic_set_xfer(&xfer, cmd, sizeof(cmd), acpm_chan_id);
 
 	ret = acpm_do_xfer(handle, &xfer);
 	if (ret)
@@ -184,7 +184,7 @@ int acpm_pmic_bulk_write(const struct acpm_handle *handle,
 		return -EINVAL;
 
 	acpm_pmic_init_bulk_write_cmd(cmd, type, reg, chan, count, buf);
-	acpm_pmic_set_xfer(&xfer, cmd, acpm_chan_id);
+	acpm_pmic_set_xfer(&xfer, cmd, sizeof(cmd), acpm_chan_id);
 
 	ret = acpm_do_xfer(handle, &xfer);
 	if (ret)
@@ -214,7 +214,7 @@ int acpm_pmic_update_reg(const struct acpm_handle *handle,
 	int ret;
 
 	acpm_pmic_init_update_cmd(cmd, type, reg, chan, value, mask);
-	acpm_pmic_set_xfer(&xfer, cmd, acpm_chan_id);
+	acpm_pmic_set_xfer(&xfer, cmd, sizeof(cmd), acpm_chan_id);
 
 	ret = acpm_do_xfer(handle, &xfer);
 	if (ret)

-- 
2.49.0.rc1.451.g8f38331e32-goog

Re: [PATCH 1/3] firmware: exynos-acpm: fix reading longer results

[Tudor Ambarus]

On 3/14/25 4:40 PM, André Draszik wrote:
> ACPM commands that return more than 8 bytes currently don't work
> correctly, as this driver ignores any such returned bytes.
> 
> This is evident in at least acpm_pmic_bulk_read(), where up to 8
> registers can be read back and those 8 register values are placed
> starting at &xfer->rxd[8].
> 
> The reason is that xfter->rxlen is initialized with the size of a
> pointer (8 bytes), rather than the size of the byte array that pointer
> points to (16 bytes)
> 
> Update the code such that we set the number of bytes expected to be the
> size of the rx buffer.
> 
> Note1: While different commands have different lengths rx buffers, we
> have to specify the same length for all rx buffers since acpm_get_rx()
> assumes they're all the same length.
> 
> Note2: The different commands also have different lengths tx buffers,
> but before switching the code to use the minimum possible length, some
> more testing would have to be done to ensure this works correctly in
> all situations. It seems wiser to just apply this fix here without
> additional logic changes for now.
> 
> Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

[PATCH 2/3] firmware: exynos-acpm: silence EPROBE_DEFER error on boot

[André Draszik]

This driver emits error messages when client drivers are trying to get
an interface handle to this driver here before this driver has
completed _probe().

Given this driver returns -EPROBE_DEFER in that case, this is not an
error and shouldn't be emitted to the log, so just remove them.

Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
Signed-off-by: André Draszik <andre.draszik@linaro.org>

---
issue is in linux-next only afaics at this stage, as driver is not
merged into Linus' tree yet
---
 drivers/firmware/samsung/exynos-acpm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
index a85b2dbdd9f0d7b1f327f54a0a283e4f32587a98..48f1e3cacaa709ae703115169df138b659ddae44 100644
--- a/drivers/firmware/samsung/exynos-acpm.c
+++ b/drivers/firmware/samsung/exynos-acpm.c
@@ -690,14 +690,11 @@ static const struct acpm_handle *acpm_get_by_phandle(struct device *dev,
 
 	acpm = platform_get_drvdata(pdev);
 	if (!acpm) {
-		dev_err(dev, "Cannot get drvdata from %s\n",
-			dev_name(&pdev->dev));
 		platform_device_put(pdev);
 		return ERR_PTR(-EPROBE_DEFER);
 	}
 
 	if (!try_module_get(pdev->dev.driver->owner)) {
-		dev_err(dev, "Cannot get module reference.\n");
 		platform_device_put(pdev);
 		return ERR_PTR(-EPROBE_DEFER);
 	}

-- 
2.49.0.rc1.451.g8f38331e32-goog

Re: [PATCH 2/3] firmware: exynos-acpm: silence EPROBE_DEFER error on boot

[Tudor Ambarus]

On 3/14/25 4:40 PM, André Draszik wrote:
> This driver emits error messages when client drivers are trying to get
> an interface handle to this driver here before this driver has
> completed _probe().
> 
> Given this driver returns -EPROBE_DEFER in that case, this is not an
> error and shouldn't be emitted to the log, so just remove them.
> 
> Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
> Signed-off-by: André Draszik <andre.draszik@linaro.org>

Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Re: [PATCH 2/3] firmware: exynos-acpm: silence EPROBE_DEFER error on boot

[Tudor Ambarus]

On 3/17/25 10:26 AM, Tudor Ambarus wrote:
> 
> 
> On 3/14/25 4:40 PM, André Draszik wrote:
>> This driver emits error messages when client drivers are trying to get
>> an interface handle to this driver here before this driver has
>> completed _probe().
>>
>> Given this driver returns -EPROBE_DEFER in that case, this is not an
>> error and shouldn't be emitted to the log, so just remove them.
>>
>> Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
>> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

I see you kept the error message though for of_find_device_by_node()
failure. You either get rid of that too, or maybe transform all to dev_dbg?

Re: [PATCH 2/3] firmware: exynos-acpm: silence EPROBE_DEFER error on boot

[André Draszik]

On Mon, 2025-03-17 at 10:48 +0000, Tudor Ambarus wrote:
> 
> 
> On 3/17/25 10:26 AM, Tudor Ambarus wrote:
> > 
> > 
> > On 3/14/25 4:40 PM, André Draszik wrote:
> > > This driver emits error messages when client drivers are trying to get
> > > an interface handle to this driver here before this driver has
> > > completed _probe().
> > > 
> > > Given this driver returns -EPROBE_DEFER in that case, this is not an
> > > error and shouldn't be emitted to the log, so just remove them.
> > > 
> > > Fixes: a88927b534ba ("firmware: add Exynos ACPM protocol driver")
> > > Signed-off-by: André Draszik <andre.draszik@linaro.org>
> > 
> > Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> 
> I see you kept the error message though for of_find_device_by_node()
> failure. You either get rid of that too, or maybe transform all to dev_dbg?

Thanks Tudor. Seems I missed that, I'll remove it, too.


Cheers,
A.

[PATCH 3/3] firmware: exynos-acpm: convert to dev_err_probe() in client API

[André Draszik]

dev_err_probe() exists to simplify code and unify error messages by
using its message template.

Convert the remaining dev_err() in acpm_get_by_phandle() to
dev_err_probe().

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/firmware/samsung/exynos-acpm.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
index 48f1e3cacaa709ae703115169df138b659ddae44..03f907a95c6acd66d89cd8af2f52e7c6dadf492a 100644
--- a/drivers/firmware/samsung/exynos-acpm.c
+++ b/drivers/firmware/samsung/exynos-acpm.c
@@ -701,12 +701,14 @@ static const struct acpm_handle *acpm_get_by_phandle(struct device *dev,
 
 	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
 	if (!link) {
-		dev_err(&pdev->dev,
-			"Failed to create device link to consumer %s.\n",
-			dev_name(dev));
+		int ret = -EINVAL;
+
+		dev_err_probe(&pdev->dev, ret,
+			      "Failed to create device link to consumer %s.\n",
+			      dev_name(dev));
 		platform_device_put(pdev);
 		module_put(pdev->dev.driver->owner);
-		return ERR_PTR(-EINVAL);
+		return ERR_PTR(ret);
 	}
 
 	return &acpm->handle;

-- 
2.49.0.rc1.451.g8f38331e32-goog

Re: [PATCH 3/3] firmware: exynos-acpm: convert to dev_err_probe() in client API

[Tudor Ambarus]

On 3/14/25 4:40 PM, André Draszik wrote:
> dev_err_probe() exists to simplify code and unify error messages by
> using its message template.
> 
> Convert the remaining dev_err() in acpm_get_by_phandle() to
> dev_err_probe().
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> ---
>  drivers/firmware/samsung/exynos-acpm.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
> index 48f1e3cacaa709ae703115169df138b659ddae44..03f907a95c6acd66d89cd8af2f52e7c6dadf492a 100644
> --- a/drivers/firmware/samsung/exynos-acpm.c
> +++ b/drivers/firmware/samsung/exynos-acpm.c
> @@ -701,12 +701,14 @@ static const struct acpm_handle *acpm_get_by_phandle(struct device *dev,
>  
>  	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
>  	if (!link) {
> -		dev_err(&pdev->dev,
> -			"Failed to create device link to consumer %s.\n",
> -			dev_name(dev));
> +		int ret = -EINVAL;
> +
> +		dev_err_probe(&pdev->dev, ret,
> +			      "Failed to create device link to consumer %s.\n",
> +			      dev_name(dev));
>  		platform_device_put(pdev);
>  		module_put(pdev->dev.driver->owner);
> -		return ERR_PTR(-EINVAL);
> +		return ERR_PTR(ret);
>  	}
>  
>  	return &acpm->handle;
> 

The clients are indeed expected to call this method in their probe
method. Shall we make such assumption? I'm in the middle here, but I
don't mind if this gets queued:
Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Re: [PATCH 3/3] firmware: exynos-acpm: convert to dev_err_probe() in client API

[Krzysztof Kozlowski]

On 14/03/2025 17:40, André Draszik wrote:
> dev_err_probe() exists to simplify code and unify error messages by
> using its message template.
> 
> Convert the remaining dev_err() in acpm_get_by_phandle() to
> dev_err_probe().
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> ---
>  drivers/firmware/samsung/exynos-acpm.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
> index 48f1e3cacaa709ae703115169df138b659ddae44..03f907a95c6acd66d89cd8af2f52e7c6dadf492a 100644
> --- a/drivers/firmware/samsung/exynos-acpm.c
> +++ b/drivers/firmware/samsung/exynos-acpm.c
> @@ -701,12 +701,14 @@ static const struct acpm_handle *acpm_get_by_phandle(struct device *dev,
>  
>  	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
>  	if (!link) {
> -		dev_err(&pdev->dev,
> -			"Failed to create device link to consumer %s.\n",
> -			dev_name(dev));
> +		int ret = -EINVAL;
> +
> +		dev_err_probe(&pdev->dev, ret,
> +			      "Failed to create device link to consumer %s.\n",
> +			      dev_name(dev));

I do not see how it is simpler. Three lines (statement) is now 5 lines
with two statements.

What's more important, dev_err_probe is supposed to be used only in
probe context, while this could be called in other contexts.

Best regards,
Krzysztof

Re: [PATCH 3/3] firmware: exynos-acpm: convert to dev_err_probe() in client API

[André Draszik]

Hi Krzysztof,

On Tue, 2025-03-18 at 20:23 +0100, Krzysztof Kozlowski wrote:
> On 14/03/2025 17:40, André Draszik wrote:
> > dev_err_probe() exists to simplify code and unify error messages by
> > using its message template.
> > 
> > Convert the remaining dev_err() in acpm_get_by_phandle() to
> > dev_err_probe().
> > 
> > Signed-off-by: André Draszik <andre.draszik@linaro.org>
> > ---
> >  drivers/firmware/samsung/exynos-acpm.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/firmware/samsung/exynos-acpm.c b/drivers/firmware/samsung/exynos-acpm.c
> > index 48f1e3cacaa709ae703115169df138b659ddae44..03f907a95c6acd66d89cd8af2f52e7c6dadf492a 100644
> > --- a/drivers/firmware/samsung/exynos-acpm.c
> > +++ b/drivers/firmware/samsung/exynos-acpm.c
> > @@ -701,12 +701,14 @@ static const struct acpm_handle *acpm_get_by_phandle(struct device *dev,
> >  
> >  	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
> >  	if (!link) {
> > -		dev_err(&pdev->dev,
> > -			"Failed to create device link to consumer %s.\n",
> > -			dev_name(dev));
> > +		int ret = -EINVAL;
> > +
> > +		dev_err_probe(&pdev->dev, ret,
> > +			      "Failed to create device link to consumer %s.\n",
> > +			      dev_name(dev));
> 
> I do not see how it is simpler. Three lines (statement) is now 5 lines
> with two statements.

This was part of some patches converting to scoped cleanup, and
there it was shorter. Shouldn't have taken this change out of
that context...

> What's more important, dev_err_probe is supposed to be used only in
> probe context, while this could be called in other contexts.

True. dev_err_probe is nice though in that it gives us unified
error messages.

Happy to drop for now.

Cheers,
A.