lkml.org 
[lkml]   [2017]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs
On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:

> +static int keystone_rproc_start(struct rproc *rproc)
> +{
> + struct keystone_rproc *ksproc = rproc->priv;
> + int ret;
> +
> + INIT_WORK(&ksproc->workqueue, handle_event);
> +
> + ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> + dev_name(ksproc->dev), ksproc);
> + if (ret) {
> + dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> + ret);
> + goto out;
> + }
> +
> + ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> + 0, dev_name(ksproc->dev), ksproc);
> + if (ret) {
> + dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> + ret);
> + goto free_vring_irq;
> + }

I do prefer that your request any resources during probe() and
potentially enable/disable them here. If below concern about using a
GPIO driver is cleared already I'll take it as is though.

[..]
> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> +{
> + struct keystone_rproc *ksproc = rproc->priv;
> +
> + if (WARN_ON(ksproc->kick_gpio < 0))
> + return;
> +
> + gpio_set_value(ksproc->kick_gpio, 1);
> +}
> +

This doesn't sound like a gpio-controller and the GPIO maintainer did
reject an attempt by me to use the GPIO framework to abstract a similar
thing. Do you already have this driver upstream or have you clarified
with the maintainer that the GPIO framework is an acceptable abstraction
for this?

It looks equivalent to the "APCS IPC" register found in Qualcomm
platforms, previously implemented through a syscon but in v4.13 being
pushed to being a mailbox driver.


Apart from this I think the series looks good.

Regards,
Bjorn

\
 
 \ /
  Last update: 2017-06-25 22:15    [W:0.106 / U:1.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site