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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

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

> +		return -ENOMEM;
> +
> +	drvdata->value_table->condition_select = select;

I don't see a need for an intemediate variable 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

Konrad