From: Marijn Suijten <marijn.suijten@somainline.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: "Pavel Machek" <pavel@ucw.cz>, "Rob Herring" <robh+dt@kernel.org>,
"Andy Gross" <agross@kernel.org>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"Lee Jones" <lee.jones@linaro.org>,
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,
"Yassine Oudjana" <y.oudjana@protonmail.com>,
"Luca Weiss" <luca@z3ntu.xyz>,
"Subbaraman Narayanamurthy" <subbaram@codeaurora.org>
Subject: Re: [PATCH v7 2/6] leds: Add driver for Qualcomm LPG
Date: Sat, 1 May 2021 23:09:21 +0200 [thread overview]
Message-ID: <55b56fd9-0ba5-58d9-2be8-98aa639e4496@somainline.org> (raw)
In-Reply-To: <20210429211517.312792-3-bjorn.andersson@linaro.org>
Hi Bjorn,
On 4/29/21 11:15 PM, Bjorn Andersson wrote:
> The Light Pulse Generator (LPG) is a PWM-block found in a wide range of
> PMICs from Qualcomm. It can operate on fixed parameters or based on a
> lookup-table, altering the duty cycle over time - which provides the
> means for e.g. hardware assisted transitions of LED brightness.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> Changes since v6:
> - Moved code into drivers/leds/rgb/
> - Reverted to earlier qcom,dtest handling to support routing pwm signals
> through dtest lines.
> - Remember the duration of each step of the pattern, rather than adding up and
> then dividing when the value is used.
> - Added missing error prints on DT parse errors.
> - Added sm8150[lb] and made led source and atc presence optional
> - Added missing parenthesis around (len + 1) / 2 in search for hi_pause in the
> pattern.
>
> drivers/leds/Kconfig | 3 +
> drivers/leds/Makefile | 3 +
> drivers/leds/rgb/leds-qcom-lpg.c | 1286 ++++++++++++++++++++++++++++++
> 3 files changed, 1292 insertions(+)
> create mode 100644 drivers/leds/rgb/leds-qcom-lpg.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 49d99cb084db..8ab06b3f162d 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -933,6 +933,9 @@ source "drivers/leds/blink/Kconfig"
> comment "Flash and Torch LED drivers"
> source "drivers/leds/flash/Kconfig"
>
> +comment "RGB LED drivers"
> +source "drivers/leds/rgb/Kconfig"
It looks like this file is not included in any of the patches.
> +
> comment "LED Triggers"
> source "drivers/leds/trigger/Kconfig"
>
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 7e604d3028c8..8cad0465aae0 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -106,6 +106,9 @@ obj-$(CONFIG_LEDS_USER) += uleds.o
> # Flash and Torch LED Drivers
> obj-$(CONFIG_LEDS_CLASS_FLASH) += flash/
>
> +# RGB LED Drivers
> +obj-$(CONFIG_LEDS_CLASS_MULTICOLOR) += rgb/
This file appears to be missing from this patch(set), too.
> +static int lpg_lut_store(struct lpg *lpg, struct led_pattern *pattern,
> + size_t len, unsigned int *lo_idx, unsigned int *hi_idx)
> +{
> + unsigned int idx;
> + u16 val;
> + int i;
> +
> + /* Hardware does not behave when LO_IDX == HI_IDX */
> + if (len == 1)
> + return -EINVAL;
> +
> + idx = bitmap_find_next_zero_area(lpg->lut_bitmap, lpg->lut_size,
> + 0, len, 0);
> + if (idx >= lpg->lut_size)
> + return -ENOMEM;
> +
> + for (i = 0; i < len; i++) {
> + val = pattern[i].brightness;
> +
> + regmap_bulk_write(lpg->map, lpg->lut_base + LPG_LUT_REG(idx + i), &val, 1);
This and the other regmap_bulk_write in lpg_apply_pwm_value used
sizeof(val) before. As far as I'm aware qcom-spmi-pmic specifies 16-bit
addresses (.reg_bits) but 8-bit register sizes (.val_bits). Writing one
register means only 8 out of 16 bits in u16 val are written?
> +static void lpg_apply_lut_control(struct lpg_channel *chan)
> +{
> + struct lpg *lpg = chan->lpg;
> + unsigned int hi_pause;
> + unsigned int lo_pause;
> + unsigned int step;
> + unsigned int conf = 0;
> + unsigned int lo_idx = chan->pattern_lo_idx;
> + unsigned int hi_idx = chan->pattern_hi_idx;
> + int pattern_len;
> +
> + if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx)
> + return;
> +
> + pattern_len = hi_idx - lo_idx + 1 > +
> + step = chan->ramp_tick_ms;
Since this is not dividing a full pattern duration by pattern_len
anymore, that variable is now never read and best removed.
> +static int lpg_parse_channel(struct lpg *lpg, struct device_node *np,
> + struct lpg_channel **channel)
> +{
> + struct lpg_channel *chan;
> + u32 color = LED_COLOR_ID_GREEN;
> + u32 reg;
> + int ret;
> +
> + ret = of_property_read_u32(np, "reg", ®);
> + if (ret || !reg || reg > lpg->num_channels) {
> + dev_err(lpg->dev, "invalid reg of %pOFn\n", np);
Like \"color\" below, escape reg with \"reg\"?
> +static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> +{
> + struct led_classdev *cdev;
> + struct device_node *child;
> + struct mc_subled *info;
> + struct lpg_led *led;
> + const char *state;
> + int num_channels;
> + u32 color = 0;
> + int ret;
> + int i;
> +
> + ret = of_property_read_u32(np, "color", &color);
> + if (ret < 0 && ret != -EINVAL) {
> + dev_err(lpg->dev, "failed to parse \"color\" of %pOF\n", np);
> + return ret;
> + }
> +
> + if (color == LED_COLOR_ID_MULTI)
Since this driver now lives under rgb/, and is specifically for RGB leds
(afaik), should this and the rest of the code use LED_COLOR_ID_RGB
instead? There was a patch floating around on (if I remember correctly)
##linux-msm by Luca Weiss that performs the conversion, with some
related changes.
> +static int lpg_init_lut(struct lpg *lpg)
> +{
> + const struct lpg_data *data = lpg->data;
> + size_t bitmap_size;
> +
> + if (!data->lut_base)
> + return 0;
> +
> + lpg->lut_base = data->lut_base;
> + lpg->lut_size = data->lut_size;
> +
> + bitmap_size = BITS_TO_BYTES(lpg->lut_size);
> + lpg->lut_bitmap = devm_kzalloc(lpg->dev, bitmap_size, GFP_KERNEL);
> + if (!lpg->lut_bitmap)
> + return -ENOMEM;
> +
> + bitmap_clear(lpg->lut_bitmap, 0, lpg->lut_size);
devm_kzalloc already zeroes the bitmap. Is it necessary to clear it
again (assuming a "cleared" bitmap is implementation-dependent and does
not imply zeroed memory) or could the memory be allocated with
devm_kalloc instead?
Thanks!
Marijn
next prev parent reply other threads:[~2021-05-01 21:09 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-29 21:15 [PATCH v7 0/6] Qualcomm Light Pulse Generator Bjorn Andersson
2021-04-29 21:15 ` [PATCH v7 1/6] dt-bindings: leds: Add Qualcomm Light Pulse Generator binding Bjorn Andersson
2021-04-30 20:05 ` Rob Herring
2021-05-02 20:53 ` Bjorn Andersson
2021-05-04 1:01 ` Subbaraman Narayanamurthy
2021-05-04 14:39 ` Pavel Machek
2021-05-04 16:29 ` Subbaraman Narayanamurthy
2021-04-29 21:15 ` [PATCH v7 2/6] leds: Add driver for Qualcomm LPG Bjorn Andersson
2021-04-30 2:22 ` kernel test robot
2021-04-30 2:48 ` kernel test robot
2021-05-01 21:09 ` Marijn Suijten [this message]
2021-05-02 21:15 ` Bjorn Andersson
2021-05-04 1:20 ` Subbaraman Narayanamurthy
2021-04-29 21:15 ` [PATCH v7 3/6] arm64: dts: qcom: Add LPG to pm8916, pm8994, pmi8994 and pmi8998 Bjorn Andersson
2021-04-29 21:15 ` [PATCH v7 4/6] arm64: dts: qcom: sdm845: Enable user LEDs on DB845c Bjorn Andersson
2021-04-29 21:15 ` [PATCH v7 5/6] arm64: dts: qcom: pmi8994: Define MPP block Bjorn Andersson
2021-04-29 21:15 ` [PATCH v7 6/6] arm64: dts: qcom: db820c: Add user LEDs Bjorn Andersson
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=55b56fd9-0ba5-58d9-2be8-98aa639e4496@somainline.org \
--to=marijn.suijten@somainline.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=lee.jones@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@z3ntu.xyz \
--cc=pavel@ucw.cz \
--cc=robh+dt@kernel.org \
--cc=subbaram@codeaurora.org \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
--cc=y.oudjana@protonmail.com \
/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 NNTP newsgroup(s).