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

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 30.08.2023 20:05, Anjelique Melendez wrote:
> Add the Qualcomm PBS (Programmable Boot Sequencer) driver. The QCOM PBS
> driver supports configuring software PBS trigger events through PBS RAM
> on Qualcomm Technologies, Inc (QTI) PMICs.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
[...]

> +static int qcom_pbs_read(struct pbs_dev *pbs, u32 address, u8 *val)
I've seen your answer in v2, but I'm still not convinced about
two things:

1. why are you using bulk APIs with count=1 instead of just
   regmap_write/read?
2. do we expect the accesses to ever fail (realistically), if not
   we don't have to care about the retval and skip the conditional
   error message (1-2 cycles less per invocation)

You insisted on keeping the error messages, but firstly you'll soon
get an angry response from Krzysztof saying register accesses can't
fail (hence making checking the retval useless) and secondly I think
spmi core already spits out some errors on disallowed r/w

If you agree access failures are very edge cases, you can simply
convert all r/w ops to regmap_read/write/modify_bits and pass
pbs->base + reg

> +{
> +	int ret;
> +
> +	address += pbs->base;
> +	ret = regmap_bulk_read(pbs->regmap, address, val, 1);
> +	if (ret)
> +		dev_err(pbs->dev, "Failed to read address=%#x sid=%#x ret=%d\n",
> +			address, to_spmi_device(pbs->dev->parent)->usid, ret);
> +
> +	return ret;
> +}
[...]

> +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
> +{
> +	int ret, retries = 2000, delay = 1000;
> +	u8 val;
> +
> +	while (retries--) {
> +		ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> +			/* PBS error - clear SCRATCH2 register */
> +			ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
> +			if (ret < 0)
> +				return ret;
> +
> +			dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
> +			return -EINVAL;
> +		}
> +
> +		if (val & BIT(bit_pos)) {
> +			dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
> +			return 0;
> +		}
> +
> +		usleep_range(delay, delay + 100);
> +	}
Since the SCRATCH2_ERROR path exits the loop, this can simply be
made into:

regmap_read_poll_timeout

if (SCARTCH2_ERROR)
  do something
  return einval

return etimedout

[...]

> +int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
> +{
> +	u8 val;
> +	u16 bit_pos;
> +	int ret;
Reverse-Christmas-tree?

> +
> +	if (!bitmap) {
> +		dev_err(pbs->dev, "Invalid bitmap passed by client\n");
> +		return -EINVAL;
> +	}
> +
> +	if (IS_ERR_OR_NULL(pbs))
> +		return -EINVAL;
> +
> +	mutex_lock(&pbs->lock);
> +	ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (val == PBS_CLIENT_SCRATCH2_ERROR) {
> +		/* PBS error - clear SCRATCH2 register */
> +		ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
> +		if (ret < 0)
> +			goto out;
> +	}
Probably deserves an error message?

> +
> +	for (bit_pos = 0; bit_pos < 8; bit_pos++) {
> +		if (bitmap & BIT(bit_pos)) {
if (!(bitmap & BIT(bit_pos))
    continue

would save you a level of indentation

Konrad