Re: [PATCH v10 5/7] qcom-tgu: Add support to configure next action

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

On 1/13/2026 7:15 PM, Konrad Dybcio wrote:
> On 1/9/26 3:11 AM, Songwei Chai wrote:
>> Add "select" node for each step to determine if another step is taken,
>> trigger(s) are generated, counters/timers incremented/decremented, etc.
>>
>> Signed-off-by: Songwei Chai <songwei.chai@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +		case TGU_CONDITION_SELECT:
>> +		/* 'default' register is at the end of 'select' region */
>> +			if (tgu_attr->reg_num ==
>> +			    drvdata->max_condition_select - 1)
>> +				attr->name = "default";
>> +			ret = (tgu_attr->reg_num <
>> +				drvdata->max_condition_select) ?
>> +				attr->mode : 0;
> 
> similarly to my previous comments
> 
> [...]
> 
>> +	for (i = 0; i < drvdata->max_step; i++) {
>> +		for (j = 0; j < drvdata->max_condition_select; j++) {
>> +			index = check_array_location(drvdata, i,
>> +						TGU_CONDITION_SELECT, j);
>> +
>> +			if (index == -EINVAL)
> 
> stray \n
>> +				goto exit;
>> +
>> +			writel(drvdata->value_table->condition_select[index],
>> +				drvdata->base + CONDITION_SELECT_STEP(i, j));
>> +		}
>> +	}
>>   	/* Enable TGU to program the triggers */
>>   	writel(1, drvdata->base + TGU_CONTROL);
>>   exit:
>> @@ -225,6 +258,8 @@ static void tgu_set_conditions(struct tgu_drvdata *drvdata)
>>   
>>   	devid = readl(drvdata->base + TGU_DEVID);
>>   	drvdata->max_condition_decode = TGU_DEVID_CONDITIONS(devid);
>> +	/* select region has an additional 'default' register */
>> +	drvdata->max_condition_select = TGU_DEVID_CONDITIONS(devid) + 1;
>>   }
>>   
>>   static int tgu_enable(struct device *dev)
>> @@ -356,6 +391,14 @@ static const struct attribute_group *tgu_attr_groups[] = {
>>   	CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(5),
>>   	CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(6),
>>   	CONDITION_DECODE_ATTRIBUTE_GROUP_INIT(7),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(0),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(1),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(2),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(3),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(4),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(5),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(6),
>> +	CONDITION_SELECT_ATTRIBUTE_GROUP_INIT(7),
>>   	NULL,
>>   };
>>   
>> @@ -363,8 +406,8 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>>   {
>>   	struct device *dev = &adev->dev;
>>   	struct tgu_drvdata *drvdata;
>> -	size_t priority_size, condition_size;
>> -	unsigned int *priority, *condition;
>> +	size_t priority_size, condition_size, select_size;
>> +	unsigned int *priority, *condition, *select;
>>   	int ret;
>>   
>>   	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> @@ -417,6 +460,16 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>>   
>>   	drvdata->value_table->condition_decode = condition;
>>   
>> +	select_size = drvdata->max_condition_select * drvdata->max_step *
>> +		      sizeof(*(drvdata->value_table->condition_select));
>> +
>> +	select = devm_kzalloc(dev, select_size, GFP_KERNEL);
>> +
>> +	if (!select)
> 
> stray \n
Will improve this based on the comments above.
> 
>> +		return -ENOMEM;
>> +
>> +	drvdata->value_table->condition_select = select;
> 
> I don't see a need for an intemediate variable here

This was done intentionally, following the earlier suggestion in v9 to
introduce named intermediate variables for better readability when 
dealing with allocations.

I’m happy to inline the allocation if you prefer the simpler form here.

> 
> [...]
> 
>>    * @max_condition_decode: Maximum number of condition_decode
>> + * @max_condition_select: Maximum number of condition_select
> 
> Maximum value, perhaps? You haven't explained the feature very well
> so I'm not sure what this is supposed to reflect
Will add "Maximum value" description in cover letter.
> 
> Konrad