Re: [PATCH v4 05/13] soc: qcom: geni-se: Add resources activation/deactivation helpers

[Praveen Talari <praveen.talari@oss.qualcomm.com>]

Hi Konrad,

On 2/3/2026 5:50 PM, Konrad Dybcio wrote:
> On 2/2/26 7:09 PM, Praveen Talari wrote:
>> The GENI SE protocol drivers (I2C, SPI, UART) implement similar resource
>> activation/deactivation sequences independently, leading to code
>> duplication.
>>
>> Introduce geni_se_resources_activate()/geni_se_resources_deactivate() to
>> power on/off resources.The activate function enables ICC, clocks, and TLMM
>> whereas the deactivate function disables resources in reverse order
>> including OPP rate reset, clocks, ICC and TLMM.
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +int geni_se_resources_deactivate(struct geni_se *se)
>> +{
>> +	int ret;
>> +
>> +	if (has_acpi_companion(se->dev))
>> +		return 0;
>> +
>> +	if (se->has_opp)
>> +		dev_pm_opp_set_rate(se->dev, 0);
> 
> This is still unbalanced at this point of abstraction, notably
> keeping the RPMh vote at 0 permanently after the first
> geni_se_resources_deactivate()  since there's no counterpart in
> _activate()

I don’t think we need a counterpart for this in the activate path, since 
it is specific to dropping the vote during suspend. The vote will anyway 
be taken again as part of the transfer after the device resumes.

Thanks,
Praveen Talari

> 
> That said, the serial and UART drivers do rate calculations internally,
> so perhaps trying to be overly smart about it wouldn't be a good thing
> either.. Let's add a note in kerneldoc that the activate must be preceded
> by a dev_pm_opp_set_xyz()
> 
> [...]
> 
>> +int geni_se_resources_activate(struct geni_se *se)
>> +{
>> +	int ret;
>> +
>> +	if (has_acpi_companion(se->dev))
>> +		return 0;
>> +
>> +	ret = geni_icc_enable(se);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = geni_se_clks_on(se);
>> +	if (ret)
>> +		goto out_icc_disable;
>> +
>> +	ret = pinctrl_pm_select_default_state(se->dev);
>> +	if (ret) {
>> +		geni_se_clks_off(se);
>> +		goto out_icc_disable;
>> +	}
>> +
>> +	return ret;
> 
> nit: this 'return' always returns 0
> 
> Konrad
> 
>> +
>> +out_icc_disable:
>> +	geni_icc_disable(se);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(geni_se_resources_activate);
>> +
>>   /**
>>    * geni_se_resources_init() - Initialize resources for a GENI SE device.
>>    * @se: Pointer to the geni_se structure representing the GENI SE device.
>> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
>> index c182dd0f0bde..36a68149345c 100644
>> --- a/include/linux/soc/qcom/geni-se.h
>> +++ b/include/linux/soc/qcom/geni-se.h
>> @@ -541,6 +541,10 @@ int geni_icc_disable(struct geni_se *se);
>>   
>>   int geni_se_resources_init(struct geni_se *se);
>>   
>> +int geni_se_resources_activate(struct geni_se *se);
>> +
>> +int geni_se_resources_deactivate(struct geni_se *se);
>> +
>>   int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
>>   #endif
>>   #endif