linux-leds.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>, 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>,
	Martin Botka <martin.botka1@gmail.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 v6 2/4] leds: Add driver for Qualcomm LPG
Date: Wed, 28 Apr 2021 19:12:00 -0500	[thread overview]
Message-ID: <YIn50NW+Pimqfsih@builder.lan> (raw)
In-Reply-To: <20201029181357.GE26053@duo.ucw.cz>

On Thu 29 Oct 13:13 CDT 2020, Pavel Machek wrote:

> Hi!
> 
> > 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 v5:
> > - Make sure to not used the state of the last channel in a group to determine
> >   if the current sink should be active for all channels in the group.
> > - Replacement of unsigned -1 with UINT_MAX
> > - Work around potential overflow by using larger data types, instead of separate code paths
> > - Use cpu_to_l16() rather than hand rolling them
> > - Minor style cleanups
> > 
> >  drivers/leds/Kconfig         |    9 +
> >  drivers/leds/Makefile        |    1 +
> >  drivers/leds/leds-qcom-lpg.c | 1190 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 1200 insertions(+)
> >  create mode 100644 drivers/leds/leds-qcom-lpg.c
> 
> Let's put this into drivers/leds/rgb/. You may need to create it.
> 

Will do so.

> 
> > +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;
> > +	__le16 val;
> 
> No need for __XX variants outside of headers meant for userspace.
> 

__le16 is the in-kernel return type for cpu_to_le16(), but after further
review I believe I don't need to do this.

> > +#define LPG_ENABLE_GLITCH_REMOVAL	BIT(5)
> > +
> > +static void lpg_enable_glitch(struct lpg_channel *chan)
> > +{
> > +	struct lpg *lpg = chan->lpg;
> > +
> > +	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
> > +			   LPG_ENABLE_GLITCH_REMOVAL, 0);
> > +}
> > +
> > +static void lpg_disable_glitch(struct lpg_channel *chan)
> > +{
> > +	struct lpg *lpg = chan->lpg;
> > +
> > +	regmap_update_bits(lpg->map, chan->base + PWM_TYPE_CONFIG_REG,
> > +			   LPG_ENABLE_GLITCH_REMOVAL,
> > +			   LPG_ENABLE_GLITCH_REMOVAL);
> > +}
> 
> Helper functions for single register write is kind of overkill...
> 

Yes, it is, but it keep lpg_apply() tidy.

> > +static int lpg_blink_set(struct lpg_led *led,
> > +			 unsigned long delay_on, unsigned long delay_off)
> > +{
> > +	struct lpg_channel *chan;
> > +	unsigned int period_us;
> > +	unsigned int duty_us;
> > +	int i;
> > +
> > +	if (!delay_on && !delay_off) {
> > +		delay_on = 500;
> > +		delay_off = 500;
> > +	}
> 
> Aren't you supposed to modify the values passed to you, so that
> userspace knows what the default rate is?
> 

I had missed this.

> 
> > +	ret = lpg_lut_store(lpg, pattern, len, &lo_idx, &hi_idx);
> > +	if (ret < 0)
> > +		goto out;
> 
> Just do direct return.
> 

Will do.

> > +out:
> > +	return ret;
> > +}
> 
> > +static const struct pwm_ops lpg_pwm_ops = {
> > +	.request = lpg_pwm_request,
> > +	.apply = lpg_pwm_apply,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static int lpg_add_pwm(struct lpg *lpg)
> > +{
> > +	int ret;
> > +
> > +	lpg->pwm.base = -1;
> > +	lpg->pwm.dev = lpg->dev;
> > +	lpg->pwm.npwm = lpg->num_channels;
> > +	lpg->pwm.ops = &lpg_pwm_ops;
> > +
> > +	ret = pwmchip_add(&lpg->pwm);
> > +	if (ret)
> > +		dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret);
> > +
> > +	return ret;
> > +}
> 
> Do we need to do this? I'd rather have LED driver, than LED+PWM
> driver...
> 

Yes, I believe we need to do this.

Because each piece of hardware has N channels, which can be wired to
LEDs, grouped with other channels and wired to multicolor LEDs or be
used as PWM signals. And this configuration is board specific.

One such example is the laptop in front of me, which has 3 channels
wired to an RGB LED and 1 channel wired as a backlight control signal
(i.e. using pwm-backlight).  Another example is a devboard where the
4 channels are wired to 4 LEDs.

Regards,
Bjorn

  reply	other threads:[~2021-04-29  0:12 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-21 20:12 [PATCH v6 0/4] Qualcomm Light Pulse Generator Bjorn Andersson
2020-10-21 20:12 ` [PATCH v6 1/4] dt-bindings: leds: Add Qualcomm Light Pulse Generator binding Bjorn Andersson
2020-10-26 15:02   ` Rob Herring
2020-10-21 20:12 ` [PATCH v6 2/4] leds: Add driver for Qualcomm LPG Bjorn Andersson
2020-10-22 19:25   ` Luca Weiss
2020-10-29 18:13   ` Pavel Machek
2021-04-29  0:12     ` Bjorn Andersson [this message]
2021-04-29 21:12       ` Pavel Machek
2021-04-29 21:29         ` Bjorn Andersson
2021-05-04 15:43           ` Pavel Machek
2021-05-04 16:13             ` Bjorn Andersson
2021-05-05  5:21               ` Uwe Kleine-König
2021-04-18 21:54   ` Marijn Suijten
2021-04-28 22:39     ` Bjorn Andersson
2021-04-29 19:31       ` Marijn Suijten
2021-04-29 20:54         ` Bjorn Andersson
2021-05-05  5:15   ` Uwe Kleine-König
2021-05-05  5:19     ` Uwe Kleine-König
2021-05-13 17:43     ` Bjorn Andersson
2020-10-21 20:12 ` [PATCH v6 3/4] arm64: dts: qcom: pm(i)8994: Add mpp and lpg blocks Bjorn Andersson
2020-10-21 20:12 ` [PATCH v6 4/4] arm64: dts: qcom: Add user LEDs on db820c Bjorn Andersson
2021-04-15 10:46 [PATCH v6 2/4] leds: Add driver for Qualcomm LPG Yassine Oudjana

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=YIn50NW+Pimqfsih@builder.lan \
    --to=bjorn.andersson@linaro.org \
    --cc=agross@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dmurphy@ti.com \
    --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=martin.botka1@gmail.com \
    --cc=pavel@ucw.cz \
    --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 NNTP newsgroup(s).