Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 28 Apr 2021 19:12:00 -0500
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
Message-ID: <YIn50NW+Pimqfsih@builder.lan>
References: <20201021201224.3430546-1-bjorn.andersson@linaro.org>
 <20201021201224.3430546-3-bjorn.andersson@linaro.org>
 <20201029181357.GE26053@duo.ucw.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201029181357.GE26053@duo.ucw.cz>
Precedence: bulk

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