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

[Songwei Chai]

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@xxxxxxxxxxxxxxxx>
> > ---
>
> [...]
>
> > @@ -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