Re: [PATCH v4 10/11] ARM: OMAP2+: AM33XX: Basic suspend resume support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 10, 2014 at 12:10 AM, Kevin Hilman <khilman@xxxxxxxxxx> wrote:
> What I think you need to do (and what I've recommended at least once in
> earlier reviews) put all the (non-rproc) wkup_m3 IPC into into one
> driver and create a well-described, well-documented API that the
> platform PM code will use.
>
> IMO, the current "split" is very difficult to read/understand, which
> means it will even more difficult to maintain.

I strongly agree.

A remoteproc driver should generally only register its
hardware-specific implementation of the rproc_ops via the remoteproc
framework. It is not expected to expose public API of its own - that's
why we have the generic remoteproc layer for. It makes perfect sense
not to use rpmsg for communications if it's not lightweight enough,
but exposing a new set of IPC API should take place in a separate
well-documented driver.

In addition, the suggested remoteproc driver seems to act both as a
low-level hardware-specific driver that plugs into remoteproc (by
registering rproc_ops via the remoteproc framework), and also as a
high-level driver that utilizes the remoteproc public API (by calling
rproc_boot). This alone calls for scrutinizing the design as this is
not very intuitive.

At least for the remoteproc part: if you could provide a simple and
straight-forward remoteproc driver that just implements the rproc_ops,
this could be merged very quickly. The rest of the code most probably
belongs in a different entity, just like Kevin suggested.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux