Re: [PATCH] power: introduce library for device-specific OPPs

[Kevin Hilman <khilman@deeprootsystems.com>]

"Menon, Nishanth" <nm@ti.com> writes:

>> -----Original Message-----
>> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
>> Sent: Thursday, October 07, 2010 4:55 PM
>
>
>> 
>> Hi,
>> 
>> On Wednesday, October 06, 2010, Nishanth Menon wrote:
>> > SoCs have a standard set of tuples consisting of frequency and
>> > voltage pairs that the device will support per voltage domain. These
>> > are called Operating Performance Points or OPPs. The actual
>> > definitions of OPP varies over silicon versions. For a specific domain,
>> > we can have a set of {frequency, voltage} pairs. As the kernel boots
>> > and more information is available, a default set of these are activated
>> > based on the precise nature of device. Further on operation, based on
>> > conditions prevailing in the system (such as temperature), some OPP
>> > availability may be temporarily controlled by the SoC frameworks.
>> >
>> > To implement an OPP, some sort of power management support is necessary
>> > hence this library depends on CONFIG_PM.
>> 
>> The patch generally looks good to me, I only have a couple of cosmetic
> Thanks for the great reviews.. It did bump up the resultant patch.
>
>> remarks
>> (below).
>> 
>> ...
>> > +static int opp_set_availability(struct device *dev, unsigned long freq,
>> > +		bool availability_req)
>> > +{
>> > +	struct device_opp *tmp_dev_opp, *dev_opp = NULL;
>> > +	struct opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV);
>> > +	int r = 0;
>> > +
>> > +	/* keep the node allocated */
>> > +	new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
>> > +	if (!new_opp) {
>> > +		pr_warning("Unable to allocate opp\n");
>> 
>> Please add an identification string to the messages, something like
>> "OPP: Unable to allocat object\n" (and similarly in the other messages).
>> That would help to find the source of a message in case there's any
>> problem.
>
> pr_fmt has been reformatted for this. The actual message which will appear
> is as follows:
> opp_set_availability: Unable to allocate opp
>
> is'nt that good enough considering that all functions are opp_ prefixed? 
> I can modify pr_fmt to add "OPP:" but I kinda think it is redundant. But I
> have no strong opinions on that and look forward to your recommendations.

Even more informative would be to use dev_warn() and include the func.
That way we'll even know which device has the problem.

Kevin