Re: [PATCH 3/4] remoteproc: Add a sysfs interface for firmware and state

[Bjorn Andersson]

On Thu 13 Oct 07:39 PDT 2016, loic pallardy wrote:

> 
> 
> On 10/13/2016 04:25 PM, Matt Redfearn wrote:
> >Hi Loic,
> >
> >
> >On 13/10/16 14:56, loic pallardy wrote:
> >>
> >>
> >>On 10/11/2016 03:39 PM, Matt Redfearn wrote:
[..]
> >>>diff --git a/drivers/remoteproc/remoteproc_internal.h
> >>>b/drivers/remoteproc/remoteproc_internal.h
> >>>index 837faf2677a6..2beb86ddfacc 100644
> >>>--- a/drivers/remoteproc/remoteproc_internal.h
> >>>+++ b/drivers/remoteproc/remoteproc_internal.h
> >>>@@ -64,6 +64,11 @@ void rproc_create_debug_dir(struct rproc *rproc);
> >>> void rproc_init_debugfs(void);
> >>> void rproc_exit_debugfs(void);
> >>>
> >>>+/* from remoteproc_sysfs.c */
> >>>+extern struct class rproc_class;
> >>struct class rproc_class should be static to remoteproc_sysfs.c file.
> >>It will be better to create a new interface like rproc_register_sysfs
> >>to associate rproc_class to rproc device.
> >
> >It would be nice if that were possible, but since the class has to
> >associated with the devices created within remoteproc_core, as it stands
> >we must have visibility of this symbol there, see above the change to
> >drivers/remoteproc/remoteproc_core.c line 1455.
> >The alternative would be a utility function in remoteproc_sysfs.c to
> >associate the class with an rproc device, it depends what the preference
> >would be.
> 
> Yes it will be my preference. remoteproc_sysfs.c file to provide a new
> service like:
> void rproc_register_sysfs(struct rproc *rproc) {
> 	rproc->dev.class = &rproc_class;
> }
> 
> and to call this function from rproc_alloc
> 

Compare:
  rproc->dev.class = &rproc_class;
to:
  rproc_register_sysfs(rproc);

The meaning of the prior is obvious, the latter require the jump to a
different file to follow. And we would only be trading one global
variable for a global function.

A viable alternative would be to keep a local pointer to the class in
core.c, that we assign during the call to rproc_init_sysfs().

But I'm fine with the way it's written.

Regards,
Bjorn