Re: [PATCH v8 3/7] soc: qcom: add QCOM PBS driver

[Lee Jones]

Intentional generic top-post reply.

Please work quickly to resolve Bjorn's comments.

I'm being hounded over a broken LEDs tree due to the missing headerfile.

/end

On Tue, 30 Jan 2024, Bjorn Andersson wrote:

> On Thu, Dec 21, 2023 at 10:58:33AM -0800, Anjelique Melendez wrote:
> > diff --git a/drivers/soc/qcom/qcom-pbs.c b/drivers/soc/qcom/qcom-pbs.c
> [..]
> > +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
> > +{
> > +	int ret, retries = 2000, delay = 1100;
> 
> retries and delay are not variable, please use defines instead.
> 
> > +	unsigned int val;
> > +
> > +	ret = regmap_read_poll_timeout(pbs->regmap,  pbs->base + PBS_CLIENT_SCRATCH2,
> > +					val, val & BIT(bit_pos), delay, delay * retries);
> > +
> > +	if (ret < 0) {
> > +		dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> > +		ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
> > +		dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
> > +	return 0;
> > +}
> > +
> > +/**
> > + * qcom_pbs_trigger_event() - Trigger the PBS RAM sequence
> > + * @pbs: Pointer to PBS device
> > + * @bitmap: bitmap
> > + *
> > + * This function is used to trigger the PBS RAM sequence to be
> > + * executed by the client driver.
> > + *
> > + * The PBS trigger sequence involves
> > + * 1. setting the PBS sequence bit in PBS_CLIENT_SCRATCH1
> > + * 2. Initiating the SW PBS trigger
> > + * 3. Checking the equivalent bit in PBS_CLIENT_SCRATCH2 for the
> > + *    completion of the sequence.
> > + * 4. If PBS_CLIENT_SCRATCH2 == 0xFF, the PBS sequence failed to execute
> > + *
> > + * Returns: 0 on success, < 0 on failure
> 
> Return: without the 's' is the appropriate form here.
> 
> > + */
> > +int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
> > +{
> > +	unsigned int val;
> > +	u16 bit_pos;
> > +	int ret;
> > +
> > +	if (!bitmap) {
> > +		dev_err(pbs->dev, "Invalid bitmap passed by client\n");
> 
> No one is going to spot that hidden in the kernel log, and if someone
> sees it it does not give an indication to which client it is that's
> broken (if there are multiple clients...)
> 
> Instead do:
> 
> 	if (WARN_ON(!bitmap))
> 		return -EINVAL;
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (IS_ERR_OR_NULL(pbs))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&pbs->lock);
> > +	ret = regmap_read(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, &val);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> > +		/* PBS error - clear SCRATCH2 register */
> > +		ret = regmap_write(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2, 0);
> > +		if (ret < 0)
> > +			goto out;
> > +	}
> > +
> > +	for (bit_pos = 0; bit_pos < 8; bit_pos++) {
> > +		if (!(bitmap & BIT(bit_pos)))
> > +			continue;
> > +
> > +		/* Clear the PBS sequence bit position */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> > +					BIT(bit_pos), 0);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		/* Set the PBS sequence bit position */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
> > +					BIT(bit_pos), BIT(bit_pos));
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		/* Initiate the SW trigger */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_TRIG_CTL,
> > +					PBS_CLIENT_SW_TRIG_BIT, PBS_CLIENT_SW_TRIG_BIT);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		ret = qcom_pbs_wait_for_ack(pbs, bit_pos);
> > +		if (ret < 0)
> > +			goto error;
> 
> In the case that this fails, you're jumping to error, which clears all
> of SCRATCH1, but you're leaving SCRATCH2 untouched.
> 
> > +
> > +		/* Clear the PBS sequence bit position */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1,
> > +					BIT(bit_pos), 0);
> > +		if (ret < 0)
> > +			goto error;
> 
> Does it make sense to handle this error by jumping to error and trying
> to clear it once more - while leaving SCRATCH2?
> 
> Perhaps you should just ignore the errors from clearing SCRATCH1 and
> SCRATCH2? You where able to trigger the PBS and you got your ack...
> 
> > +
> > +		/* Clear the PBS sequence bit position */
> > +		ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH2,
> > +					BIT(bit_pos), 0);
> > +		if (ret < 0)
> > +			goto error;
> > +	}
> > +
> > +error:
> 
> We're passing "error" in the successful case as well, please name this
> "out_clear_scratch1" (or something) instead, to not confuse the reader.
> 
> > +	/* Clear all the requested bitmap */
> > +	ret = regmap_update_bits(pbs->regmap, pbs->base + PBS_CLIENT_SCRATCH1, bitmap, 0);
> > +
> > +out:
> > +	mutex_unlock(&pbs->lock);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_pbs_trigger_event);
> > +
> > +/**
> > + * get_pbs_client_device() - Get the PBS device used by client
> > + * @dev: Client device
> > + *
> > + * This function is used to get the PBS device that is being
> > + * used by the client.
> > + *
> > + * Returns: pbs_dev on success, ERR_PTR on failure
> 
> Return:
> 
> Regards,
> Bjorn

-- 
Lee Jones [李琼斯]