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:   Mon, 25 Oct 2021 19:57:02 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Subbaraman Narayanamurthy <subbaram@codeaurora.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,
        Marijn Suijten <marijn.suijten@somainline.org>,
        Yassine Oudjana <y.oudjana@protonmail.com>,
        Luca Weiss <luca@z3ntu.xyz>
Subject: Re: [PATCH v10 2/2] leds: Add driver for Qualcomm LPG
Message-ID: <YXdufoQfnlrDvVVs@ripper>
References: <20211010043912.136640-1-bjorn.andersson@linaro.org>
 <20211010043912.136640-2-bjorn.andersson@linaro.org>
 <1ad508af-f7cb-a88f-07d8-5731c5a45403@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1ad508af-f7cb-a88f-07d8-5731c5a45403@codeaurora.org>
Precedence: bulk

On Mon 25 Oct 17:37 PDT 2021, Subbaraman Narayanamurthy wrote:

> Hi Bjorn,
> 
> > +#define LPG_RESOLUTION		512
> 
> Just a thought. Having this fixed to 9-bit resolution would require a
> lot of code churn if this driver ends up supporting higher resolution
> PWM later. Would it be possible to have this as a parameter in "struct
> lpg_channel" ?
> 

In earlier revisions I had support for picking 6 vs 9 bit resolution
dynamically, but looked ahead and iirc saw that we only support 9 bit
for the now current generation (it's been a while, so I might
misremember).

The end result was that I ripped that out to simplify the code from the
various heuristics that had been bolted on during the generations.


People seem okay with this version and it meets the need of a large
number of PMICs and use cases, so I would prefer that we land this and
then evolve it based on requirements I haven't yet seen.

> > +static const unsigned int lpg_clk_rates[] = {1024, 32768, 19200000};
> > +static const unsigned int lpg_pre_divs[] = {1, 3, 5, 6};
> > +
> > +static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
> > +{
> > +	unsigned int clk, best_clk = 0;
> > +	unsigned int div, best_div = 0;
> > +	unsigned int m, best_m = 0;
> > +	unsigned int error;
> > +	unsigned int best_err = UINT_MAX;
> > +	u64 best_period = 0;
> > +
> > +	/*
> > +	 * The PWM period is determined by:
> > +	 *
> > +	 *          resolution * pre_div * 2^M
> > +	 * period = --------------------------
> > +	 *                   refclk
> > +	 *
> > +	 * With resolution fixed at 2^9 bits, pre_div = {1, 3, 5, 6} and
> > +	 * M = [0..7].
> > +	 *
> > +	 * This allows for periods between 27uS and 384s, as the PWM framework
> > +	 * wants a period of equal or lower length than requested, reject
> > +	 * anything below 27uS.
> > +	 */
> > +	if (period <= (u64)NSEC_PER_SEC * LPG_RESOLUTION / 19200000)
> > +		return -EINVAL;
> > +
> > +	/* Limit period to largest possible value, to avoid overflows */
> > +	if (period > (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 1024)
> > +		period = (u64)NSEC_PER_SEC * LPG_RESOLUTION * 6 * (1 << LPG_MAX_M) / 2014;
> 
> s/2014/1024 ?
> 

Now that's an interesting typo... I will correct this and submit v11.

Thanks,
Bjorn