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:
pgp9Wqn7wsi1x.pgp
Description: PGP signature
