Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences

[Alex Courbot]

On 08/16/2012 06:52 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:
> > On 08/16/2012 04:42 PM, Thierry Reding wrote:
> > > > Old Signed by an unknown key
> > >
> > > On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
> [...]
> > > > +Usage by Drivers and Resources Management
> > > > +-----------------------------------------
> > > > +Power sequences make use of resources that must be properly allocated and
> > > > +managed. The power_seq_build() function builds a power sequence from the
> > > > +platform data. It also takes care of resolving and allocating the resources
> > > > +referenced by the sequence if needed:
> > > > +
> > > > +  struct power_seq *power_seq_build(struct device *dev, struct list_head *ress,
> > > > +                                    struct platform_power_seq *pseq);
> > > > +
> > > > +The 'dev' argument is the device in the name of which the resources are to be
> > > > +allocated.
> > > > +
> > > > +The 'ress' argument is a list to which the resolved resources are appended. This
> > > > +avoids allocating a resource referenced in several power sequences multiple
> > > > +times.
> > > > +
> > > > +On success, the function returns a devm allocated resolved sequence that is
> > > > +ready to be passed to power_seq_run(). In case of failure, and error code is
> > > > +returned.
> > > > +
> > > > +A resolved power sequence returned by power_seq_build can be run by
> > > > +power_run_run():
> > > > +
> > > > +  int power_seq_run(power_seq *seq);
> > > > +
> > > > +It returns 0 if the sequence has successfully been run, or an error code if a
> > > > +problem occured.
> > > > +
> > > > +There is no need to explicitly free the resources used by the sequence as they
> > > > +are devm-allocated.
> > >
> > > I had some comments about this particular interface for creating
> > > sequences in the last series. My point was that explicitly requiring
> > > drivers to manage a list of already allocated resources may be too much
> > > added complexity. Power sequences should be easy to use, and I find the
> > > requirement for a separately managed list of resources cumbersome.
> > >
> > > What I proposed last time was to collect all power sequences under a
> > > common parent object, which in turn would take care of managing the
> > > resources.
> >
> > Yes, I remember that. While I see why you don't like this list,
> > having a common parent object to all sequences will not reduce the
> > number of arguments to pass to power_seq_build() (which is the only
> > function that has to handle this list now). Also having the list of
> > resources at hand is needed for some drivers: for instance,
> > pwm-backlight needs to check that exactly one PWM has been
> > allocated, and takes a reference to it from this list in order to
> > control the brightness.
>
> I'm not complaining about the additional argument to power_seq_build()
> but about the missing encapsulation. I just think that keeping a list
> external to the power sequencing code is error-prone. Drivers could do
> just about anything with it between calls to power_seq_build(). If you
> do all of this internally, then you don't depend on the driver at all
> and power sequencing code can just do the right thing.

On the opposite side, I am concerned about over-encapsulation. :) IIRC 
you proposed to have a top structure to hold the power sequences, their 
resources and the associated device. Power sequences would then have a 
name and be run through a 2 arguments power_seq_run():

  power_seq_run(sequences, "up");

There are two things that bother me with this solution. First is that 
addressing power sequences by name looks a little bit overkill, when a 
single pointer should be enough. It would also complicate the design. 
Second thing is that this design would place the power sequences 
structure on top of the device - in effect, you could perfectly have 
several of these structures all using the same device and failing to see 
each other's resources. While that would be a error from the device 
driver's side, the design allows it.

> Obtaining a reference to the PWM, or any other resource for that matter,
> from the power sequence could be done via an explicit API.
>
> > Ideally we could embed the list into the device structure, but I
> > don't see how we can do that without modifying it (and we don't want
> > to modify it). Another solution would be to keep a static mapping
> > table that associates a device to its power_seq related resources
> > within power_seq.c. If we protect it for concurrent access this
> > should make it possible to make resources management transparent.
> > How does this sound? Only drawback I see is that we would need to
> > explicitly clean it up through a dedicated function when the driver
> > exits.
>
> I don't think that's much better. Since the power sequences will be very
> tightly coupled to a specific device, tying the sequences and their
> resources to the device makes a lot of sense. Keeping a global list of
> resources doesn't in my opinion.

That is not what would happen actually - what I proposed is to have a 
mapping (hash map, or more likely binary tree) between a device and the 
list_head of the resources for that device. In C++ (forgive me, this 
makes the types more explicit) that would be:

static std::map<struct device *, struct list_head> device_resources;

That way you would have exactly one list per device, could keep 
resource-management totally transparent without exposing the list_head, 
and keep the API and design simple.

For special cases (like pwm-backlight which needs to get the PWM), the 
list_head could be obtained through a dedicated API.

Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/