On Thu, 28 Sep 2023, Anjelique Melendez wrote: > In some PMICs like pmi632, the pattern look up table (LUT) and LPG > configuration is stored in a single SDAM module instead of LUT > peripheral. Currently, PMICs without LUT peripheral will not be > able to produce LED patterns. > > Add support for PBS Pattern Generation (PPG), which is the feature > that allows PMICs without LUT peripherals to produce LED patterns > from SDAM module. PPG uses the Qualcomm Programmable Boot > Sequencer (PBS) to trigger LED pattern sequences. > > Signed-off-by: Anjelique Melendez <quic_amelende@xxxxxxxxxxx> > --- > drivers/leds/rgb/leds-qcom-lpg.c | 282 ++++++++++++++++++++++++++++--- > 1 file changed, 261 insertions(+), 21 deletions(-) Looking good. Just a couple of nits and throughts. > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c > index df469aaa7e6e..4d87686f916c 100644 > --- a/drivers/leds/rgb/leds-qcom-lpg.c > +++ b/drivers/leds/rgb/leds-qcom-lpg.c > @@ -8,11 +8,13 @@ > #include <linux/bitfield.h> > #include <linux/led-class-multicolor.h> > #include <linux/module.h> > +#include <linux/nvmem-consumer.h> > #include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/pwm.h> > #include <linux/regmap.h> > #include <linux/slab.h> > +#include <linux/soc/qcom/qcom-pbs.h> > > #define LPG_SUBTYPE_REG 0x05 > #define LPG_SUBTYPE_LPG 0x2 > @@ -39,6 +41,8 @@ > #define PWM_SEC_ACCESS_REG 0xd0 > #define PWM_DTEST_REG(x) (0xe2 + (x) - 1) > > +#define SDAM_REG_PBS_SEQ_EN 0x42 > + > #define TRI_LED_SRC_SEL 0x45 > #define TRI_LED_EN_CTL 0x46 > #define TRI_LED_ATC_CTL 0x47 > @@ -48,9 +52,25 @@ > > #define LPG_RESOLUTION_9BIT BIT(9) > #define LPG_RESOLUTION_15BIT BIT(15) > +#define PPG_MAX_LED_BRIGHTNESS 255 > + > #define LPG_MAX_M 7 > #define LPG_MAX_PREDIV 6 > > +#define DEFAULT_TICK_DURATION_US 7800 > +#define RAMP_STEP_DURATION(x) (((x) * 1000 / DEFAULT_TICK_DURATION_US) & 0xff) > + > +/* LPG common config settings for PPG */ > +#define SDAM_REG_RAMP_STEP_DURATION 0x47 > +#define SDAM_LPG_SDAM_LUT_PATTERN_OFFSET 0x80 > + > +/* LPG per channel config settings for PPG */ > +#define SDAM_LUT_EN_OFFSET 0x0 > +#define SDAM_PATTERN_CONFIG_OFFSET 0x1 > +#define SDAM_END_INDEX_OFFSET 0x3 > +#define SDAM_START_INDEX_OFFSET 0x4 > +#define SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET 0x6 > + > struct lpg_channel; > struct lpg_data; > > @@ -64,7 +84,10 @@ struct lpg_data; > * @lut_base: base address of the LUT block (optional) > * @lut_size: number of entries in the LUT block > * @lut_bitmap: allocation bitmap for LUT entries > - * @triled_base: base address of the TRILED block (optional) > + * @pbs_dev: PBS device > + * @lpg_chan_sdam: LPG SDAM peripheral device > + * @pbs_en_bitmap: bitmap for tracking PBS triggers > + * @triled_base: base address of the TRILED block (optional) > * @triled_src: power-source for the TRILED > * @triled_has_atc_ctl: true if there is TRI_LED_ATC_CTL register > * @triled_has_src_sel: true if there is TRI_LED_SRC_SEL register > @@ -85,6 +108,10 @@ struct lpg { > u32 lut_size; > unsigned long *lut_bitmap; > > + struct pbs_dev *pbs_dev; > + struct nvmem_device *lpg_chan_sdam; > + unsigned long pbs_en_bitmap; > + > u32 triled_base; > u32 triled_src; > bool triled_has_atc_ctl; > @@ -101,6 +128,8 @@ struct lpg { > * @triled_mask: mask in TRILED to enable this channel > * @lut_mask: mask in LUT to start pattern generator for this channel > * @subtype: PMIC hardware block subtype > + * @sdam_offset: Channel offset in LPG SDAM Nit: De-capitalise Channel. > + * @lpg_idx: index of the channel > * @in_use: channel is exposed to LED framework > * @color: color of the LED attached to this channel > * @dtest_line: DTEST line for output, or 0 if disabled > @@ -112,6 +141,7 @@ struct lpg { > * @pre_div_sel: divider selector of the reference clock > * @pre_div_exp: exponential divider of the reference clock > * @pwm_resolution_sel: pwm resolution selector > + * @pattern_set: true when setting pattern > * @ramp_enabled: duty cycle is driven by iterating over lookup table > * @ramp_ping_pong: reverse through pattern, rather than wrapping to start > * @ramp_oneshot: perform only a single pass over the pattern > @@ -129,6 +159,8 @@ struct lpg_channel { > unsigned int triled_mask; > unsigned int lut_mask; > unsigned int subtype; > + u32 sdam_offset; > + u32 lpg_idx; > > bool in_use; > > @@ -146,6 +178,7 @@ struct lpg_channel { > unsigned int pre_div_exp; > unsigned int pwm_resolution_sel; > > + bool pattern_set; > bool ramp_enabled; > bool ramp_ping_pong; > bool ramp_oneshot; > @@ -180,8 +213,10 @@ struct lpg_led { > * struct lpg_channel_data - per channel initialization data > * @base: base address for PWM channel registers > * @triled_mask: bitmask for controlling this channel in TRILED > + * @sdam_offset: Channel offset in LPG SDAM Nit: Please keep these ordered. > */ > struct lpg_channel_data { > + unsigned int sdam_offset; > unsigned int base; > u8 triled_mask; > }; > @@ -206,6 +241,75 @@ struct lpg_data { > const struct lpg_channel_data *channels; > }; > > +#define PBS_SW_TRIG_BIT BIT(0) > + > +static int lpg_clear_pbs_trigger(struct lpg_channel *chan) > +{ > + u8 val = 0; > + int rc; > + > + clear_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap); > + if (!chan->lpg->pbs_en_bitmap) { > + rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, SDAM_REG_PBS_SEQ_EN, 1, &val); > + if (rc < 0) > + return rc; > + } > + > + return 0; > +} > + > +static int lpg_set_pbs_trigger(struct lpg_channel *chan) > +{ > + u8 val = PBS_SW_TRIG_BIT; > + int rc; > + > + if (!chan->lpg->pbs_en_bitmap) { > + rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, SDAM_REG_PBS_SEQ_EN, 1, &val); > + if (rc < 0) > + return rc; > + > + rc = qcom_pbs_trigger_event(chan->lpg->pbs_dev, val); > + if (rc < 0) > + return rc; > + } > + set_bit(chan->lpg_idx, &chan->lpg->pbs_en_bitmap); > + > + return 0; > +} > + > +static int lpg_sdam_configure_triggers(struct lpg_channel *chan, bool set_trig) > +{ > + u32 addr = SDAM_LUT_EN_OFFSET + chan->sdam_offset; > + u8 val; > + int rc; > + > + if (chan->lpg->lut_base) Because? I think I comment here would be handy. > + return 0; > + > + if (set_trig) { > + val = 1; > + rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, addr, 1, &val); > + if (rc < 0) > + return rc; > + > + rc = lpg_set_pbs_trigger(chan); > + if (rc < 0) > + return rc; > + chan->pattern_set = false; > + } else { > + val = 0; > + rc = nvmem_device_write(chan->lpg->lpg_chan_sdam, addr, 1, &val); > + if (rc < 0) > + return rc; > + > + rc = lpg_clear_pbs_trigger(chan); > + if (rc < 0) > + return rc; Nothing to be done for pattern_set? > + } > + > + return 0; > +} > + > static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable) > { > /* Skip if we don't have a triled block */ > @@ -216,6 +320,41 @@ static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable) > mask, enable); > } > > +static int lpg_lut_store_sdam(struct lpg *lpg, struct led_pattern *pattern, > + size_t len, unsigned int *lo_idx, unsigned int *hi_idx) > +{ > + unsigned int idx; > + u8 brightness; > + int i, rc; > + u16 addr; > + > + if (len > lpg->lut_size) { > + dev_err(lpg->dev, "Pattern length (%zu) exceeds maximum pattern length (%d)\n", > + len, lpg->lut_size); > + return -EINVAL; > + } > + > + idx = bitmap_find_next_zero_area(lpg->lut_bitmap, lpg->lut_size, > + 0, len, 0); These line wraps are inconsistent with some others. Pick 80 or 100, but be consistent throughout. > + if (idx >= lpg->lut_size) > + return -ENOSPC; > + > + for (i = 0; i < len; i++) { > + brightness = pattern[i].brightness; > + addr = SDAM_LPG_SDAM_LUT_PATTERN_OFFSET + i + idx; > + rc = nvmem_device_write(lpg->lpg_chan_sdam, addr, 1, &brightness); > + if (rc < 0) > + return rc; > + } > + > + bitmap_set(lpg->lut_bitmap, idx, len); > + > + *lo_idx = idx; > + *hi_idx = idx + len - 1; > + > + return 0; > +} > + > static int lpg_lut_store(struct lpg *lpg, struct led_pattern *pattern, > size_t len, unsigned int *lo_idx, unsigned int *hi_idx) > { > @@ -462,6 +601,28 @@ static void lpg_apply_pwm_value(struct lpg_channel *chan) > #define LPG_PATTERN_CONFIG_PAUSE_HI BIT(1) > #define LPG_PATTERN_CONFIG_PAUSE_LO BIT(0) > > +static void lpg_sdam_apply_lut_control(struct lpg_channel *chan) > +{ > + struct nvmem_device *lpg_chan_sdam = chan->lpg->lpg_chan_sdam; > + unsigned int lo_idx = chan->pattern_lo_idx; > + unsigned int hi_idx = chan->pattern_hi_idx; > + u8 val = 0, conf = 0; > + > + if (!chan->ramp_enabled || chan->pattern_lo_idx == chan->pattern_hi_idx) > + return; > + > + if (!chan->ramp_oneshot) > + conf |= LPG_PATTERN_CONFIG_REPEAT; > + > + nvmem_device_write(lpg_chan_sdam, SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 1, &val); > + nvmem_device_write(lpg_chan_sdam, SDAM_PATTERN_CONFIG_OFFSET + chan->sdam_offset, 1, &conf); > + nvmem_device_write(lpg_chan_sdam, SDAM_END_INDEX_OFFSET + chan->sdam_offset, 1, &hi_idx); > + nvmem_device_write(lpg_chan_sdam, SDAM_START_INDEX_OFFSET + chan->sdam_offset, 1, &lo_idx); > + > + val = RAMP_STEP_DURATION(chan->ramp_tick_ms); > + nvmem_device_write(lpg_chan_sdam, SDAM_REG_RAMP_STEP_DURATION, 1, &val); > +} > + > static void lpg_apply_lut_control(struct lpg_channel *chan) > { > struct lpg *lpg = chan->lpg; > @@ -597,7 +758,10 @@ static void lpg_apply(struct lpg_channel *chan) > lpg_apply_pwm_value(chan); > lpg_apply_control(chan); > lpg_apply_sync(chan); > - lpg_apply_lut_control(chan); > + if (chan->lpg->lpg_chan_sdam) > + lpg_sdam_apply_lut_control(chan); > + else > + lpg_apply_lut_control(chan); > lpg_enable_glitch(chan); > } > > @@ -642,6 +806,9 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev, > triled_mask |= chan->triled_mask; > > lpg_apply(chan); > + > + if (chan->pattern_set) > + lpg_sdam_configure_triggers(chan, true); > } > > /* Toggle triled lines */ > @@ -836,18 +1003,24 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, > * If the specified pattern is a palindrome the ping pong mode is > * enabled. In this scenario the delta_t of the middle entry (i.e. the > * last in the programmed pattern) determines the "high pause". > + * > + * SDAM devices supporting LUT do not support "low pause", "high pause" > + * or "ping pong" > */ > > /* Detect palindromes and use "ping pong" to reduce LUT usage */ > - for (i = 0; i < len / 2; i++) { > - brightness_a = pattern[i].brightness; > - brightness_b = pattern[len - i - 1].brightness; > - > - if (brightness_a != brightness_b) { > - ping_pong = false; > - break; > + if (lpg->lut_base) { > + for (i = 0; i < len / 2; i++) { > + brightness_a = pattern[i].brightness; > + brightness_b = pattern[len - i - 1].brightness; > + > + if (brightness_a != brightness_b) { > + ping_pong = false; > + break; > + } > } > - } > + } else > + ping_pong = false; > > /* The pattern length to be written to the LUT */ > if (ping_pong) > @@ -875,12 +1048,27 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, > if (delta_t >= BIT(9)) > goto out_free_pattern; > > - /* Find "low pause" and "high pause" in the pattern */ > - lo_pause = pattern[0].delta_t; > - hi_pause = pattern[actual_len - 1].delta_t; > + /* > + * Find "low pause" and "high pause" in the pattern in the LUT case. > + * LPGs using SDAM for pattern require equal duration of all steps > + */ > + if (lpg->lut_base) { > + lo_pause = pattern[0].delta_t; > + hi_pause = pattern[actual_len - 1].delta_t; > + } else { > + if (delta_t != pattern[0].delta_t || > + delta_t != pattern[actual_len - 1].delta_t) > + goto out_free_pattern; > + } > + > > mutex_lock(&lpg->lock); > - ret = lpg_lut_store(lpg, pattern, actual_len, &lo_idx, &hi_idx); > + > + if (lpg->lpg_chan_sdam) > + ret = lpg_lut_store_sdam(lpg, pattern, actual_len, &lo_idx, &hi_idx); > + else > + ret = lpg_lut_store(lpg, pattern, actual_len, &lo_idx, &hi_idx); > + > if (ret < 0) > goto out_unlock; > > @@ -896,6 +1084,8 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern, > > chan->pattern_lo_idx = lo_idx; > chan->pattern_hi_idx = hi_idx; > + Why are we spacing them out? They look related? > + chan->pattern_set = true; > } > > out_unlock: > @@ -953,6 +1143,7 @@ static int lpg_pattern_clear(struct lpg_led *led) > > for (i = 0; i < led->num_channels; i++) { > chan = led->channels[i]; > + lpg_sdam_configure_triggers(chan, false); > chan->pattern_lo_idx = 0; > chan->pattern_hi_idx = 0; > } > @@ -1188,8 +1379,8 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np) > cdev->brightness_set_blocking = lpg_brightness_mc_set; > cdev->blink_set = lpg_blink_mc_set; > > - /* Register pattern accessors only if we have a LUT block */ > - if (lpg->lut_base) { > + /* Register pattern accessors if we have a LUT block or when using PPG */ > + if (lpg->lut_base || lpg->lpg_chan_sdam) { > cdev->pattern_set = lpg_pattern_mc_set; > cdev->pattern_clear = lpg_pattern_mc_clear; > } > @@ -1202,15 +1393,19 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np) > cdev->brightness_set_blocking = lpg_brightness_single_set; > cdev->blink_set = lpg_blink_single_set; > > - /* Register pattern accessors only if we have a LUT block */ > - if (lpg->lut_base) { > + /* Register pattern accessors if we have a LUT block or when using PPG */ > + if (lpg->lut_base || lpg->lpg_chan_sdam) { > cdev->pattern_set = lpg_pattern_single_set; > cdev->pattern_clear = lpg_pattern_single_clear; > } > } > > cdev->default_trigger = of_get_property(np, "linux,default-trigger", NULL); > - cdev->max_brightness = LPG_RESOLUTION_9BIT - 1; > + > + if (lpg->lpg_chan_sdam) > + cdev->max_brightness = PPG_MAX_LED_BRIGHTNESS; > + else > + cdev->max_brightness = LPG_RESOLUTION_9BIT - 1; Are these not both 0xff (255)? > if (!of_property_read_string(np, "default-state", &state) && > !strcmp(state, "on")) > @@ -1251,6 +1446,8 @@ static int lpg_init_channels(struct lpg *lpg) > chan->base = data->channels[i].base; > chan->triled_mask = data->channels[i].triled_mask; > chan->lut_mask = BIT(i); > + chan->sdam_offset = data->channels[i].sdam_offset; > + chan->lpg_idx = i; > > regmap_read(lpg->map, chan->base + LPG_SUBTYPE_REG, &chan->subtype); > } > @@ -1297,11 +1494,12 @@ static int lpg_init_lut(struct lpg *lpg) > { > const struct lpg_data *data = lpg->data; > > - if (!data->lut_base) > + if (!data->lut_size) > return 0; > > - lpg->lut_base = data->lut_base; > lpg->lut_size = data->lut_size; > + if (data->lut_base) > + lpg->lut_base = data->lut_base; > > lpg->lut_bitmap = devm_bitmap_zalloc(lpg->dev, lpg->lut_size, GFP_KERNEL); > if (!lpg->lut_bitmap) > @@ -1310,6 +1508,44 @@ static int lpg_init_lut(struct lpg *lpg) > return 0; > } > > +static int lpg_init_sdam(struct lpg *lpg) > +{ > + struct lpg_channel *chan; > + int i, sdam_count, rc; > + u8 val = 0; > + > + sdam_count = of_property_count_strings(lpg->dev->of_node, "nvmem-names"); > + if (sdam_count <= 0) > + return 0; > + > + /* get the SDAM device for LPG/LUT config */ Nit: Start comments with an uppercase char. > + lpg->lpg_chan_sdam = devm_nvmem_device_get(lpg->dev, "lpg_chan_sdam"); > + if (IS_ERR(lpg->lpg_chan_sdam)) > + return dev_err_probe(lpg->dev, PTR_ERR(lpg->lpg_chan_sdam), > + "Failed to get lpg sdam device\n"); Should these abbreviations be capitalised? > + lpg->pbs_dev = get_pbs_client_device(lpg->dev); > + if (IS_ERR(lpg->pbs_dev)) > + return dev_err_probe(lpg->dev, PTR_ERR(lpg->pbs_dev), > + "Failed to get PBS client device\n"); > + > + for (i = 0; i < lpg->num_channels; i++) { > + chan = &lpg->channels[i]; Is 'chan' used outside of this for loop? If not, move the declaration into here. > + if (chan->sdam_offset) { > + rc = nvmem_device_write(lpg->lpg_chan_sdam, > + SDAM_PBS_SCRATCH_LUT_COUNTER_OFFSET + chan->sdam_offset, 1, &val); > + if (rc < 0) > + return rc; > + > + rc = lpg_sdam_configure_triggers(chan, false); > + if (rc < 0) > + return rc; > + } > + } > + > + return 0; > +} > + > static int lpg_probe(struct platform_device *pdev) > { > struct device_node *np; > @@ -1346,6 +1582,10 @@ static int lpg_probe(struct platform_device *pdev) > if (ret < 0) > return ret; > > + ret = lpg_init_sdam(lpg); > + if (ret < 0) > + return ret; > + > ret = lpg_init_lut(lpg); > if (ret < 0) > return ret; > -- > 2.41.0 > -- Lee Jones [李琼斯]