Re: [PATCHv2 3/8] OMAP3: PM: Adding smartreflex driver support.

[Kevin Hilman <khilman@deeprootsystems.com>]

"Gopinath, Thara" <thara@ti.com> writes:

[...]

>>>> +   sr->is_sr_enable = 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * omap_smartreflex_enable : API to enable SR clocks and to call into the
>>>> + * registered smartreflex class enable API.
>>>> + * @voltdm - VDD pointer to which the SR module to be configured belongs to.
>>>> + *
>>>> + * This API is to be called from the kernel in order to enable
>>>> + * a particular smartreflex module. This API will do the initial
>>>> + * configurations to turn on the smartreflex module and in turn call
>>>> + * into the registered smartreflex class enable API.
>>>> + */
>>>> +void omap_smartreflex_enable(struct voltagedomain *voltdm)
>>>> +{
>>>> +   struct omap_sr *sr = _sr_lookup(voltdm);
>>>> +
>>>> +   if (IS_ERR(sr)) {
>>>> +           pr_warning("%s: omap_sr struct for sr_%s not found\n",
>>>> +                   __func__, voltdm->name);
>>>> +           return;
>>>> +   }
>>>> +
>>>> +   if (!sr->is_autocomp_active)
>>>> +           return;
>>>
>>>from here
>>>
>>>> +   if (!sr_class || !(sr_class->enable) || !(sr_class->configure)) {
>>>> +           dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
>>>> +                   "registered\n", __func__);
>>>> +           return;
>>>> +   }
>>>> +   sr_class->enable(voltdm);
>>>
>>>to here, this looks an awful lot like sr_start_autocomp()
>
> Yes both will enable smartreflex. One is called from user side and other
> from kernel side. Are you suggesting creating a third API to just keep the above
> piece of code ??

No, I'm suggesting you just call sr_start_vddautocomp() here.

[...]

>>>> +};
>>>> +
>>>> +/*
>>>> + * Smartreflex module enable/disable interface.
>>>> + * NOTE: if smartreflex is not enabled from sysfs, these functions will not
>>>> + * do anything.
>>>> + */
>>>> +void omap_smartreflex_enable(struct voltagedomain *voltdm);
>>>> +void omap_smartreflex_disable(struct voltagedomain *voltdm);
>>>> +void omap_smartreflex_disable_reset_volt(struct voltagedomain *voltdm);
>>>> +
>>>> +/* Smartreflex driver hooks to be called from Smartreflex class driver */
>>>> +int sr_enable(struct voltagedomain *voltdm, unsigned long volt);
>>>> +void sr_disable(struct voltagedomain *voltdm);
>>>> +int sr_configure_errgen(struct voltagedomain *voltdm);
>>>> +int sr_configure_minmax(struct voltagedomain *voltdm);
>>>> +
>>>> +/* API to register the smartreflex class driver with the smartreflex driver */
>>>> +int omap_sr_register_class(struct omap_smartreflex_class_data *class_data);
>>>> +
>>>> +/* API to register the pmic specific data with the smartreflex driver. */
>>>> +void omap_sr_register_pmic(struct omap_smartreflex_pmic_data *pmic_data);
>>>> +#else
>>>> +static inline void omap_smartreflex_enable(struct voltagedomain *voltdm) {}
>>>> +static inline void omap_smartreflex_disable(struct voltagedomain *voltdm) {}
>>>> +static inline void omap_smartreflex_disable_reset_volt(
>>>> +           struct voltagedomain *voltdm) {}
>>>> +static inline void omap_sr_register_pmic(
>>>> +           struct omap_smartreflex_pmic_data *pmic_data) {}
>>>> +#endif
>>>> +#endif
>>>
>>>These function names could all be unified a bit better.  Some are
>>>'omap_smartreflex_', some are 'sr_' and some are 'omap_sr'.
>>>
>>>How about using 'omap_sr' for all the public functions.
>
> Yes sr_ are the APIs that will be called from smartreflex class driver
> and omap_smartreflex are the ones that will be called from rest of the code.
> I can change omap_smartreflex_ ones to omap_sr.

OK.

Kevin