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-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
