Skip to content

Cleanups and minor fixes to the ADI remoteproc driver - #3428

Open
ukleinek wants to merge 9 commits into
analogdevicesinc:adsp-6.18.31-yfrom
ukleinek:remoteproc-adi
Open

Cleanups and minor fixes to the ADI remoteproc driver#3428
ukleinek wants to merge 9 commits into
analogdevicesinc:adsp-6.18.31-yfrom
ukleinek:remoteproc-adi

Conversation

@ukleinek

Copy link
Copy Markdown
Contributor

PR Description

Some low-hanging fruit patch opportunities I found while ramping up on the driver while not yet having the needed hardware.

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have compiled my changes, including the documentation
  • I have tested the changes on the relevant hardware
  • I have updated the documentation outside this repo accordingly
  • I have provided links for the relevant upstream lore

ukleinek added 2 commits July 13, 2026 09:58
The workqueue might still be in use until rproc_del() returns, so better
don't release before calling rproc_del(). Ditto for the mbox channels.

Fixes: ca6c93f ("remoteproc: adi: Add driver for ADSP SHARC cores")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
As soon as rproc_add() is called, the dma operations in the callbacks
might be used. So call dmaengine_get() before rproc_add().

Reorder the function calls in the remove callback accordingly.

Fixes: ca6c93f ("remoteproc: adi: Add driver for ADSP SHARC cores")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
@ukleinek ukleinek changed the title Remoteproc adi Cleanups and minor fixes to the ADI remoteproc driver Jul 13, 2026
@pamolloy
pamolloy requested review from a team July 13, 2026 11:12
@pamolloy pamolloy added this to ADSP Jul 13, 2026

@nunojsa nunojsa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ukleinek! LGTM! Maybe one nice addition would be completely get rid of .remove(). Right now it seems we have a mix of manual and devm calls.

Comment thread drivers/remoteproc/adi_remoteproc.c
Comment thread drivers/remoteproc/adi_remoteproc.c Outdated
dev_err(dev, "Error: Core%d not idle\n", core_id);
ret = -EBUSY;
goto free_rproc;
return -EBUSY;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this could be an opportunity to get rid of that redundant else

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want the following?:

    if (ret < 0)
        return dev_err_probe(...);
    if (!ret)
        return dev_err_probe(...);

?

}
if (ret < 0)
return dev_err_probe(dev, ret, "Unable to read core status\n");
else if (ret == 0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, style wise I really prefer just if (!ret) return. But that's me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean s/ret == 0/!ret/, right, and not to have the return on the same line as the if statement?

@pamolloy pamolloy moved this to In Development in ADSP Jul 13, 2026
Comment thread drivers/remoteproc/adi_remoteproc.c Outdated
In the error case devm_ioremap_wc() returns NULL not an error pointer.
Adapt the check accordingly.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
@ukleinek

Copy link
Copy Markdown
Contributor Author

Hi @ukleinek! LGTM! Maybe one nice addition would be completely get rid of .remove(). Right now it seems we have a mix of manual and devm calls.

I already checked, not all resource allocation functions used have a devm equivalent. So converting completely would need some work in other subsystems.

ukleinek added 6 commits July 13, 2026 16:41
This fixes a missing of_node_put in the last hunk.

Fixes: ca6c93f ("remoteproc: adi: Add driver for ADSP SHARC cores")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Since commit 5ab23c7 ("modpost: Create modalias for builtin
modules") MODULE_DEVICE_TABLE() is enough to reference a match table and
the data isn't discarded by the linker even when the driver is built-in
and CONFIG_OF is disabled. So there is no valid reason to not store the
address of that table in the driver struct.

Drop the of_match_ptr() accordingly.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
New additions after the terminator are not expected, so usually there is
no comma added either.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
With the devm managed version of rproc_alloc() the error codes can be
simplified a bit. Also given that after rproc_alloc() some devm managed
resources are allocated this improves the order of resource freeing on
device remove.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Compared to dev_err() dev_err_probe() is more compact and improves the
error message by mentioning the error code. Also it has EPROBE_DEFER
handing built-in. For out-of-memory errors no message should be emitted
---the kernel is loud enough in this case already---so the respective
messages are dropped.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Multiline comments start with /* on its own line.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Development

Development

Successfully merging this pull request may close these issues.

5 participants