From: Lee Jones <lee@kernel.org>
To: Bjorn Andersson <andersson@kernel.org>
Cc: Anjelique Melendez <quic_amelende@quicinc.com>,
pavel@ucw.cz, thierry.reding@gmail.com, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
agross@kernel.org, luca.weiss@fairphone.com,
konrad.dybcio@linaro.org, u.kleine-koenig@pengutronix.de,
quic_subbaram@quicinc.com, quic_gurus@quicinc.com,
linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-pwm@vger.kernel.org
Subject: Re: [PATCH v8 3/7] soc: qcom: add QCOM PBS driver
Date: Wed, 31 Jan 2024 08:58:42 +0000 [thread overview]
Message-ID: <20240131085842.GF8551@google.com> (raw)
In-Reply-To: <ut6jbawqqdgfyoxmt76hm67rbnv67x54eho3nae2dd2szbejfb@7joy57g4i3qt>
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 [李琼斯]
next prev parent reply other threads:[~2024-01-31 8:58 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-21 18:58 [PATCH v8 0/7] Add support for LUT PPG Anjelique Melendez
2023-12-21 18:58 ` [PATCH v8 1/7] dt-bindings: soc: qcom: Add qcom,pbs bindings Anjelique Melendez
2023-12-21 18:58 ` [PATCH v8 2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG Anjelique Melendez
2023-12-21 18:58 ` [PATCH v8 3/7] soc: qcom: add QCOM PBS driver Anjelique Melendez
2024-01-25 13:04 ` Lee Jones
2024-01-30 21:21 ` Bjorn Andersson
2024-01-31 8:58 ` Lee Jones [this message]
2024-02-01 20:50 ` Anjelique Melendez
2024-02-01 23:28 ` Bjorn Andersson
2024-02-02 12:43 ` Lee Jones
2024-02-08 11:07 ` Lee Jones
2023-12-21 18:58 ` [PATCH v8 4/7] leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM Anjelique Melendez
2024-01-11 10:06 ` Lee Jones
2023-12-21 18:58 ` [PATCH v8 5/7] leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG Anjelique Melendez
2023-12-21 18:58 ` [PATCH v8 6/7] leds: rgb: leds-qcom-lpg: Include support for PPG with dedicated LUT SDAM Anjelique Melendez
2023-12-21 18:58 ` [PATCH v8 7/7] leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme Anjelique Melendez
2024-01-11 10:05 ` (subset) [PATCH v8 0/7] Add support for LUT PPG Lee Jones
2024-01-11 10:07 ` Lee Jones
2024-01-16 18:49 ` Anjelique Melendez
2024-01-18 14:49 ` Lee Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240131085842.GF8551@google.com \
--to=lee@kernel.org \
--cc=agross@kernel.org \
--cc=andersson@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=luca.weiss@fairphone.com \
--cc=pavel@ucw.cz \
--cc=quic_amelende@quicinc.com \
--cc=quic_gurus@quicinc.com \
--cc=quic_subbaram@quicinc.com \
--cc=robh+dt@kernel.org \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).