Re: [PATCH 09/19] omap3+: sr: introduce class init,deinit and priv data

[Nishanth Menon <nm@ti.com>]

Kevin Hilman wrote, on 03/03/2011 06:27 AM:
> Nishanth Menon<nm@ti.com>  writes:
>
>> Kevin Hilman wrote, on 03/03/2011 05:38 AM:
>>> Nishanth Menon<nm@ti.com>   writes:
>>>
>>>> Certain class drivers such as class 1.5 drivers, will need specific
>>>> notification that they have to be started up or stopped independent
>>>> of smart reflex operation. They also may need private data to be
>>>> used for operations of their own, provide the same.
>>>>
>>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>>
>>> Basic principle looks fine, but some naming comments below...
>> k, thx.
>>
>>>
>>> [...]
>>>
>>>> diff --git a/arch/arm/plat-omap/include/plat/smartreflex.h b/arch/arm/plat-omap/include/plat/smartreflex.h
>>>> index 6568c88..8b6ecd9 100644
>>>> --- a/arch/arm/plat-omap/include/plat/smartreflex.h
>>>> +++ b/arch/arm/plat-omap/include/plat/smartreflex.h
>>>> @@ -167,6 +167,8 @@ struct omap_sr_pmic_data {
>>>>     *
>>>>     * @enable:		API to enable a particular class smaartreflex.
>>>>     * @disable:		API to disable a particular class smartreflex.
>>>> + * @class_init:		API to do class specific initialization (optional)
>>>> + * @class_deinit:	API to do class specific initialization (optional)
>>>
>>> The 'class_' prefix here is not needed.
>> ack.
>>
>>>
>>> The changelog uses 'start' and 'stop' instead of init&   deinit.  I
>>> prefer those names to [de]init.
>>
>> Would'nt start and stop cause confusion when mixed with existing
>> enable/disable? does enable/start actually start the SR? intent here
>> with init/deinit is to do class specific initialization or
>> deinitialization.
>
> Well, one way or another, make the changelog consistent with the
> function names.
>
> To me though, start/stop has more meaning.  They are called from
> sr_[start|stop]_vddautocomp, so using start/stop is consistent there.
> Also, init is usually something done once (and doesn't have a de-init
> counterpart) but here we're talking about something done for each
> sr_[start|stop]_vddautocomp()

fair enough - unless there are other dissenting voices, will switch to 
start/stop for the api name and add documentation in code to explain the 
need.

-- 
Regards,
Nishanth Menon