Re: [PATCH v10 7/7] qcom-tgu: Add reset node to initialize

[Songwei Chai]

On 1/13/2026 7:22 PM, Konrad Dybcio wrote:
> On 1/9/26 3:11 AM, Songwei Chai wrote:
> > Add reset node to initialize the value of
> > priority/condition_decode/condition_select/timer/counter nodes.
> >
> > Signed-off-by: Songwei Chai <songwei.chai@xxxxxxxxxxxxxxxx>
> > ---
>
> [...]
>
> > +/* reset_tgu_store - Reset Trace and Gating Unit (TGU) configuration. */
> > +static ssize_t reset_tgu_store(struct device *dev,
> > +			       struct device_attribute *attr, const char *buf,
> > +			       size_t size)
> > +{
> > +	struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
> > +	unsigned long value;
> > +	int i, j, ret;
> > +
> > +	if (kstrtoul(buf, 0, &value) || value == 0)
> > +		return -EINVAL;
>
> Your documentation blurb promises that only 1 is accepted, but this is not
> the case. I think the previous additions had a similar flaw

I’ll fix this to only accept 1 and review the previous additions
for similar issues.

> > +
> > +	if (!drvdata->enable) {
>
> I think this check needs to be made under a lock, otherwise something else
> may pull the plug inbetween
Will move "guard(spinlock)(&drvdata->lock);" before "drvdata->enable" check.
> > +		ret = pm_runtime_get_sync(drvdata->dev);
> > +		if (ret < 0) {
> > +			pm_runtime_put(drvdata->dev);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	guard(spinlock)(&drvdata->lock);
> > +	TGU_UNLOCK(drvdata->base);
> > +
> > +	writel(0, drvdata->base + TGU_CONTROL);
> > +
> > +	TGU_LOCK(drvdata->base);
>
> This is tgu_disable()
will use tgu_disable instead.
> > +
> > +	if (drvdata->value_table->priority)
> > +		memset(drvdata->value_table->priority, 0,
> > +			    MAX_PRIORITY * drvdata->max_step *
> > +				drvdata->max_reg * sizeof(unsigned int));
> > +
> > +	if (drvdata->value_table->condition_decode)
> > +		memset(drvdata->value_table->condition_decode, 0,
> > +			    drvdata->max_condition_decode * drvdata->max_step *
> > +				sizeof(unsigned int));
> > +
> > +		/* Initialize all condition registers to NOT(value=0x1000000) */
>
> One \t too much
will update.
> > +	for (i = 0; i < drvdata->max_step; i++) {
> > +		for (j = 0; j < drvdata->max_condition_decode; j++) {
> > +			drvdata->value_table
> > +			->condition_decode[calculate_array_location(
> > +			drvdata, i, TGU_CONDITION_DECODE, j)] =
> > +			0x1000000;
>
> This is unreadable, take a pointer to condition_decode[]
sure.
> > +		}
> > +	}
> > +
> > +	if (drvdata->value_table->condition_select)
> > +		memset(drvdata->value_table->condition_select, 0,
> > +				drvdata->max_condition_select * drvdata->max_step *
> > +				sizeof(unsigned int));
> > +
> > +	if (drvdata->value_table->timer)
> > +		memset(drvdata->value_table->timer, 0,
> > +			    (drvdata->max_step) *
> > +				(drvdata->max_timer) *
> > +				sizeof(unsigned int));
> > +
> > +	if (drvdata->value_table->counter)
> > +		memset(drvdata->value_table->counter, 0,
> > +			    (drvdata->max_step) *
> > +				(drvdata->max_counter) *
> > +				sizeof(unsigned int));
>
> This is similarly difficult to read with almost random indentation

I agree, the indentation hurts readability. I’ll rework this to make the
expression clearer.

> Konrad