Re: [PATCH v3 06/12] soc: qcom: geni-se: Introduce helper APIs for performance control

[Konrad Dybcio]

On 2/4/26 6:42 AM, Praveen Talari wrote:
> Hi Konrad,
> 
> On 2/3/2026 4:44 PM, Konrad Dybcio wrote:
>> On 1/30/26 5:54 PM, Praveen Talari wrote:
>>> Hi Konrad
>>>
>>> On 1/30/2026 5:53 PM, Konrad Dybcio wrote:
>>>> On 1/12/26 11:47 AM, Praveen Talari wrote:
>>>>> The GENI Serial Engine (SE) drivers (I2C, SPI, and SERIAL) currently
>>>>> manage performance levels and operating points directly. This resulting
>>>>> in code duplication across drivers. such as configuring a specific level
>>>>> or find and apply an OPP based on a clock frequency.
>>>>>
>>>>> Introduce two new helper APIs, geni_se_set_perf_level() and
>>>>> geni_se_set_perf_opp(), addresses this issue by providing a streamlined
>>>>> method for the GENI Serial Engine (SE) drivers to find and set the OPP
>>>>> based on the desired performance level, thereby eliminating redundancy.
>>>>>
>>>>> Signed-off-by: Praveen Talari <praveen.talari@xxxxxxxxxxxxxxxx>
>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>> +/**
>>>>> + * geni_se_set_perf_level() - Set performance level for GENI SE.
>>>>> + * @se: Pointer to the struct geni_se instance.
>>>>> + * @level: The desired performance level.
>>>>> + *
>>>>> + * Sets the performance level by directly calling dev_pm_opp_set_level
>>>>> + * on the performance device associated with the SE.
>>>>> + *
>>>>> + * Return: 0 on success, or a negative error code on failure.
>>>>> + */
>>>>> +int geni_se_set_perf_level(struct geni_se *se, unsigned long level)
>>>>> +{
>>>>> +    return dev_pm_opp_set_level(se->pd_list->pd_devs[DOMAIN_IDX_PERF], level);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(geni_se_set_perf_level);
>>>>
>>>> This function is never used
>>>
>>> it will be used by UART driver, not for I2C/SPI.
>>
>> Adding unused exported symbols is "eeeh"..
> 
> I keep in mind for UART, i have added this API.
>>
>>>>
>>>>> +
>>>>> +/**
>>>>> + * geni_se_set_perf_opp() - Set performance OPP for GENI SE by frequency.
>>>>> + * @se: Pointer to the struct geni_se instance.
>>>>> + * @clk_freq: The requested clock frequency.
>>>>> + *
>>>>> + * Finds the nearest operating performance point (OPP) for the given
>>>>> + * clock frequency and applies it to the SE's performance device.
>>>>> + *
>>>>> + * Return: 0 on success, or a negative error code on failure.
>>>>> + */
>>>>> +int geni_se_set_perf_opp(struct geni_se *se, unsigned long clk_freq)
>>>>
>>>> I think with the SPI driver in mind (which seems to do a simple rateset
>>>
>>> APIs were added as generic interfaces shared across I²C/SPI which is specific to firmware control, not Linux control.
>>>
>>>> for both backends) we could do:
>>>>
>>>>> +{
>>>>> +    struct device *perf_dev = se->pd_list->pd_devs[DOMAIN_IDX_PERF];
>>>>
>>>> Then, we can do struct device * perf_dev = se->dev;
>>> I don't think, it is needed since this is specific to firmware control, not Linux control.
>>
>> My point is that it doesn't have to be specific to the auto usecase,
>> further commonizing the code.
> 
> This API will not useful for non-auto cases as well.

This is only because you make it so, with the above suggestion we could
do without the .set_rate abstraction in the SPI driver which only does
an opp_set_rate in the generic case

Konrad