Re: [PATCH v10 4/7] qcom-tgu: Add TGU decode support

[Songwei Chai <songwei.chai@oss.qualcomm.com>]

On 1/13/2026 7:13 PM, Konrad Dybcio wrote:
> On 1/9/26 3:11 AM, Songwei Chai wrote:
>> Decoding is when all the potential pieces for creating a trigger
>> are brought together for a given step. Example - there may be a
>> counter keeping track of some occurrences and a priority-group that
>> is being used to detect a pattern on the sense inputs. These 2
>> inputs to condition_decode must be programmed, for a given step,
>> to establish the condition for the trigger, or movement to another
>> steps.
>>
>> Signed-off-by: Songwei Chai <songwei.chai@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> @@ -18,8 +18,36 @@ static int calculate_array_location(struct tgu_drvdata *drvdata,
>>   				   int step_index, int operation_index,
>>   				   int reg_index)
>>   {
>> -	return operation_index * (drvdata->max_step) * (drvdata->max_reg) +
>> -		step_index * (drvdata->max_reg) + reg_index;
> 
> I think this type of calculations could use a wrapper
Agree, this calculation is already wrapped in the calculate_array_location.
> 
>> +	int ret = -EINVAL;
>> +
>> +	switch (operation_index) {
>> +	case TGU_PRIORITY0:
>> +	case TGU_PRIORITY1:
>> +	case TGU_PRIORITY2:
>> +	case TGU_PRIORITY3:
>> +		ret = operation_index * (drvdata->max_step) *
>> +			(drvdata->max_reg) +
>> +			step_index * (drvdata->max_reg) + reg_index;
>> +		break;
>> +	case TGU_CONDITION_DECODE:
>> +		ret = step_index * (drvdata->max_condition_decode) +
>> +			reg_index;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +	return ret;
> 
> The only thing your switch statement is assign a value to ret and break
> out. Change that to a direct return, and drop 'ret' altogether
> 

I kept a single return intentionally so the function has a single exit
point. This makes it easier to extend with common post-processing or
debug logic later if needed.

That said, I’m fine switching to direct returns if you prefer the 
simpler style here.

> 
>> +}
>> +
>> +static int check_array_location(struct tgu_drvdata *drvdata, int step,
>> +				int ops, int reg)
>> +{
>> +	int result = calculate_array_location(drvdata, step, ops, reg);
>> +
>> +	if (result == -EINVAL)
>> +		dev_err(drvdata->dev, "%s - Fail\n", __func__);
> 
> Avoid __func__.
> 
> The error message is very non-descriptive
Marked.Will update.
> 
> [...]
> 
>>   static int tgu_enable(struct device *dev)
>>   {
>>   	struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
>> +	int ret = 0;
>>   
>>   	guard(spinlock)(&drvdata->lock);
>>   	if (drvdata->enable)
>>   		return -EBUSY;
>>   
>> -	tgu_write_all_hw_regs(drvdata);
>> +	ret = tgu_write_all_hw_regs(drvdata);
>> +
>> +	if (ret == -EINVAL)
> 
> stray \n
Sure.
>> +		goto exit;
>> +
>>   	drvdata->enable = true;
>>   
>> -	return 0;
>> +exit:
>> +	return ret;
> 
> ret = tgu_write_all_hw_regs(drvdata);
> if (!ret)
> 	drvdata->enable = true;
> 
> return ret
Will update.
> 
> Konrad