On Mon, Aug 14, 2023 at 04:59:17PM -0700, Anjelique Melendez wrote:
> On PMICs such as PM8350C, the lookup table containing the pattern data
> is stored in a separate nvmem device from the one where the per-channel
I think it would be better to say "separate SDAM" instead of "separate
nvmem device", to make it clearer to the reader what's being done.
> data is stored.
>
> Add support for two separate nvmems to handle this case while maintaining
You're only adding the support for the dedicated LUT SDAM, not "for two
separate nvmems".
> backward compatibility for those targets that use only a single nvmem
> device.
>
> Signed-off-by: Guru Das Srinagesh <quic_gurus@xxxxxxxxxxx>
If Guru was the first to sign off the change, then he must have authored
the patch. Or add a Co-developed-by if that's appropriate.
> Signed-off-by: Anjelique Melendez <quic_amelende@xxxxxxxxxxx>
> ---
> drivers/leds/rgb/leds-qcom-lpg.c | 112 ++++++++++++++++++++++++-------
> 1 file changed, 89 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 822c7bff00df..f3f83925ab41 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -60,6 +60,7 @@
> #define RAMP_STEP_DURATION(x) (((x) * 1000 / DEFAULT_TICK_DURATION_US) & 0xff)
>
> /* LPG common config settings for PPG */
> +#define SDAM_START_BASE 0x40
I really wish that we hid this offset in the SDAM nvmem driver - but
that ship has sailed...
> #define SDAM_REG_RAMP_STEP_DURATION 0x47
> #define SDAM_LUT_COUNT_MAX 64
>
> @@ -69,6 +70,8 @@
> #define SDAM_END_INDEX_OFFSET 0x3
> #define SDAM_START_INDEX_OFFSET 0x4
> #define SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET 0x6
> +#define SDAM_PAUSE_HI_MULTIPLIER_OFFSET 0x8
> +#define SDAM_PAUSE_LO_MULTIPLIER_OFFSET 0x9
>
> struct lpg_channel;
> struct lpg_data;
> @@ -85,7 +88,9 @@ struct lpg_data;
> * @lut_bitmap: allocation bitmap for LUT entries
> * @pbs_dev: PBS device
> * @lpg_chan_nvmem: LPG nvmem peripheral device
> + * @lut_nvmem: LUT nvmem peripheral device
> * @pbs_en_bitmap: bitmap for tracking PBS triggers
> + * @nvmem_count: number of nvmems used for LUT and PPG config
I find the concept of "how many nvmem do we have" slightly unclear.
Perhaps we could split this that into two separate boolean properties,
something like use_sdam and use_sdam_lut?
On the other hand, use_sdam and use_sdam_lut should afaict always be
equal to lpg_chan_nvmem != NULL and lut_nvmem != NULL. So you could use
these directly instead.
> * @lut_sdam_base: offset where LUT pattern begins in nvmem
> * @ppg_en: Flag indicating whether PPG is enabled/used
> * @triled_base: base address of the TRILED block (optional)
> @@ -111,7 +116,9 @@ struct lpg {
>
> struct pbs_dev *pbs_dev;
> struct nvmem_device *lpg_chan_nvmem;
> + struct nvmem_device *lut_nvmem;
> unsigned long pbs_en_bitmap;
> + unsigned int nvmem_count;
> u32 lut_sdam_base;
> bool ppg_en;
>
> @@ -261,6 +268,8 @@ static int lpg_sdam_write(struct lpg *lpg, u16 addr, u8 val)
> }
>
> #define SDAM_REG_PBS_SEQ_EN 0x42
> +#define SDAM_PBS_TRIG_SET 0xe5
> +#define SDAM_PBS_TRIG_CLR 0xe6
Please add these at the top, like requested in the previous patch.
> #define PBS_SW_TRIG_BIT BIT(0)
>
[..]
> static void lpg_sdam_apply_lut_control(struct lpg_channel *chan)
> {
> u8 val, conf = 0;
> + unsigned int hi_pause, lo_pause;
> struct lpg *lpg = chan->lpg;
>
> + hi_pause = DIV_ROUND_UP(chan->ramp_hi_pause_ms, chan->ramp_tick_ms);
> + lo_pause = DIV_ROUND_UP(chan->ramp_lo_pause_ms, chan->ramp_tick_ms);
> +
> if (!chan->ramp_oneshot)
> conf |= LPG_PATTERN_CONFIG_REPEAT;
> + if (chan->ramp_hi_pause_ms && lpg->nvmem_count != 1)
> + conf |= LPG_PATTERN_CONFIG_PAUSE_HI;
> + if (chan->ramp_lo_pause_ms && lpg->nvmem_count != 1)
> + conf |= LPG_PATTERN_CONFIG_PAUSE_LO;
>
> lpg_sdam_write(lpg, SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 0);
> lpg_sdam_write(lpg, SDAM_PATTERN_CONFIG_OFFSET + chan->sdam_offset, conf);
>
> - lpg_sdam_write(lpg, SDAM_END_INDEX_OFFSET + chan->sdam_offset, chan->pattern_hi_idx);
> - lpg_sdam_write(lpg, SDAM_START_INDEX_OFFSET + chan->sdam_offset, chan->pattern_lo_idx);
> + val = lpg_get_sdam_lut_idx(chan, chan->pattern_hi_idx);
I'd suggest adding a local variable "lut_offset" to the values here,
instead of calling out to a separate function to perhaps adjust the
values.
> + lpg_sdam_write(lpg, SDAM_END_INDEX_OFFSET + chan->sdam_offset, val);
> +
> + val = lpg_get_sdam_lut_idx(chan, chan->pattern_lo_idx);
> + lpg_sdam_write(lpg, SDAM_START_INDEX_OFFSET + chan->sdam_offset, val);
>
> val = RAMP_STEP_DURATION(chan->ramp_tick_ms);
> if (val > 0)
> val--;
> lpg_sdam_write(lpg, SDAM_REG_RAMP_STEP_DURATION, val);
> +
> + if (lpg->nvmem_count != 1) {
> + lpg_sdam_write(lpg, SDAM_PAUSE_HI_MULTIPLIER_OFFSET + chan->sdam_offset, hi_pause);
> + lpg_sdam_write(lpg, SDAM_PAUSE_LO_MULTIPLIER_OFFSET + chan->sdam_offset, lo_pause);
> + }
> +
> }
>
> static void lpg_apply_lut_control(struct lpg_channel *chan)
> @@ -1000,8 +1050,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
> * enabled. In this scenario the delta_t of the middle entry (i.e. the
> * last in the programmed pattern) determines the "high pause".
> *
> - * NVMEM devices supporting LUT do not support "low pause", "high pause"
> - * or "ping pong"
> + * All NVMEM devices supporting LUT do not support "ping pong"
> + * Single NVMEM devices supporting LUT do not support "low pause" and "high pause"
How about: "SDAM-based devices does not support "ping pong", and only
supports "low pause" and "high pause" with dedicated SDAM LUT." ?
> */
>
> /* Detect palindromes and use "ping pong" to reduce LUT usage */
[..]
> @@ -1509,29 +1559,45 @@ static int lpg_parse_sdam(struct lpg *lpg)
> {
> int rc = 0;
>
> - if (lpg->data->nvmem_count == 0)
> + lpg->nvmem_count = lpg->data->nvmem_count;
> + if (lpg->nvmem_count == 0)
> return 0;
>
> - /* get the nvmem device for LPG/LUT config */
> + if (lpg->nvmem_count > 2)
> + return -EINVAL;
> +
> + /* get the 1st nvmem device for LPG/LUT config */
> lpg->lpg_chan_nvmem = devm_nvmem_device_get(lpg->dev, "lpg_chan_sdam");
> if (IS_ERR(lpg->lpg_chan_nvmem)) {
> rc = PTR_ERR(lpg->lpg_chan_nvmem);
> - if (rc != -EPROBE_DEFER)
> - dev_err(lpg->dev, "Failed to get nvmem device, rc=%d\n", rc);
> - return rc;
> + goto err;
> }
>
> - lpg->pbs_dev = get_pbs_client_device(lpg->dev);
> - if (IS_ERR(lpg->pbs_dev)) {
> - rc = PTR_ERR(lpg->pbs_dev);
> - if (rc != -EPROBE_DEFER)
> - dev_err(lpg->dev, "Failed to get PBS client device, rc=%d\n", rc);
> - return rc;
> + if (lpg->nvmem_count == 1) {
> + /* get PBS device node if single NVMEM device */
> + lpg->pbs_dev = get_pbs_client_device(lpg->dev);
> + if (IS_ERR(lpg->pbs_dev)) {
> + rc = PTR_ERR(lpg->pbs_dev);
> + if (rc != -EPROBE_DEFER)
> + dev_err(lpg->dev, "Failed to get PBS client device, rc=%d\n", rc);
> + return rc;
return dev_err_probe() please.
> + }
> + } else if (lpg->nvmem_count == 2) {
> + /* get the 2nd nvmem device for LUT pattern */
> + lpg->lut_nvmem = devm_nvmem_device_get(lpg->dev, "lut_sdam");
> + if (IS_ERR(lpg->lut_nvmem)) {
> + rc = PTR_ERR(lpg->lut_nvmem);
> + goto err;
> + }
> }
>
> lpg->ppg_en = true;
>
> return rc;
rc is still 0 here.
> +err:
> + if (rc != -EPROBE_DEFER)
> + dev_err(lpg->dev, "Failed to get nvmem device, rc=%d\n", rc);
The idiomatic usage of goto is to share error handling, and release
resources in a reverse order that they where acquired. It's therefor
going to confuse future readers when you do:
if (error)
goto err;
if (error2)
return rc;
if (error3)
goto err;
But more importantly, this error message gives no indication about which
nvmem device the driver failed to find. If you move the error print up,
and use dev_err_probe() the code will be easier to follow and you can
afford making the error message more helpful.
Regards,
Bjorn
