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 [李琼斯]
