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

[Thierry Reding]

On Thu, Aug 16, 2012 at 07:33:27PM +0900, Alex Courbot wrote:
> 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.

I see. Perhaps I'm just bugged by the interface being a simple list. If
it was something just a little more sophisticated, like a very primitive
resource manager attached to one device, I would be appeased. Maybe an
opaque structure that carries the list and hides it for drivers would do
as well.

> >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.

I understand. You could use an idr (include/linux/idr.h) for this
purpose. However I don't know if this would be any better than the
above.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature