Hi Bjorn,
On 10/21/20 10:12 PM, Bjorn Andersson wrote:
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@xxxxxxxxxx>
Tested-by: Luca Weiss <luca@xxxxxxxxx>
Thanks for these patches. I have tested them successfully on the Sony
Xperia XA2 (Discovery, Nile platform) with the leds on the PM660l - feel
free to add my Tested-by. Should I send the configuration your way for
inclusion in this patch, or submit them separately (either applied
after, or included as separate patches in the next version of this series)?
+/**
+ * struct lpg_data - initialization data
+ * @lut_base: base address of LUT block
+ * @lut_size: number of entries in LUT
+ * @triled_base: base address of TRILED
+ * @pwm_9bit_mask: bitmask for switching from 6bit to 9bit pwm
Our downstream kernel derives this from the "LPG subtype" for each
distinct channel, read from register offset +0x5. A value of 0xb is
subtype "PWM" with a shift of 2, a value of 0x11 is subtype "LPG_LITE"
with a shift of 4. Can we do the same here instead of hardcoding it for
all channels in the LPG at once? How should we determine if the mask is
one or two bits wide, for the 3<<4 case?
+ * @num_channels: number of channels in LPG
+ * @channels: list of channel initialization data
+ */
+ if (ping_pong) {
+ if (len % 2)
+ hi_pause = 0;
+ else
+ hi_pause = pattern[len + 1 / 2].delta_t;
len + 1 should be wrapped in parentheses just like the reassignment to
len= below, otherwise this is always an out of bounds read (at len + 0).
+ lo_pause = pattern[len - 1].delta_t;
+
+ len = (len + 1) / 2;
+ } else {
+ hi_pause = pattern[len - 1].delta_t;
+ lo_pause = 0;
+ }
+
+ ret = lpg_lut_store(lpg, pattern, len, &lo_idx, &hi_idx);
+ if (ret < 0)
+ goto out;
+
+ for (i = 0; i < led->num_channels; i++) {
+ chan = led->channels[i];
+
+ chan->ramp_duration_ms = pattern[0].delta_t * len;
Perhaps this could store the duration of a single step instead, since
the only use in lpg_apply_lut_control divides it by pattern length again?
+ chan->ramp_ping_pong = ping_pong;
+ chan->ramp_oneshot = repeat != -1;
+
+ chan->ramp_lo_pause_ms = lo_pause;
+ chan->ramp_hi_pause_ms = hi_pause;
+
+ chan->pattern_lo_idx = lo_idx;
+ chan->pattern_hi_idx = hi_idx;
+ }
+
+out:
+ return ret;
+}
+static int lpg_init_lut(struct lpg *lpg)
+{
+ const struct lpg_data *data = lpg->data;
+ size_t bitmap_size;
+
+ if (!data->lut_base)
+ return 0;
+
+ lpg->lut_base = data->lut_base;
+ lpg->lut_size = data->lut_size;
+
+ bitmap_size = BITS_TO_LONGS(lpg->lut_size) * sizeof(unsigned long);
+ lpg->lut_bitmap = devm_kzalloc(lpg->dev, bitmap_size, GFP_KERNEL);
Would it be nicer to use BITS_TO_BYTES here, or otherwise
devm_kcalloc(..., bitmap_size, sizeof(long), ...) without mutiplying
with sizeof(unsigned long)?
+
+ bitmap_clear(lpg->lut_bitmap, 0, lpg->lut_size);
+ return lpg->lut_bitmap ? 0 : -ENOMEM;
+}
+
+static int lpg_probe(struct platform_device *pdev)
+{
+ struct device_node *np;
+ struct lpg *lpg;
+ int ret;
+ int i;
+
+ lpg = devm_kzalloc(&pdev->dev, sizeof(*lpg), GFP_KERNEL);
+ if (!lpg)
+ return -ENOMEM;
+
+ lpg->data = of_device_get_match_data(&pdev->dev);
+ if (!lpg->data)
+ return -EINVAL;
+
+ lpg->dev = &pdev->dev;
+
+ lpg->map = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!lpg->map) {
+ dev_err(&pdev->dev, "parent regmap unavailable\n");
+ return -ENXIO;
+ }
+
+ ret = lpg_init_channels(lpg);
+ if (ret < 0)
+ return ret;
+
+ ret = lpg_init_triled(lpg);
+ if (ret < 0)
+ return ret;
+
+ ret = lpg_init_lut(lpg);
+ if (ret < 0)
+ return ret;
How about turning these returns into dev_err_probe? I'm not sure if
that's the expected way to go nowadays, but having some form of logging
when a driver fails to probe is always good to have.
Thanks!
Marijn
+
+ for_each_available_child_of_node(pdev->dev.of_node, np) {
+ ret = lpg_add_led(lpg, np);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < lpg->num_channels; i++)
+ lpg_apply_dtest(&lpg->channels[i]);
+
+ ret = lpg_add_pwm(lpg);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, lpg);
+
+ return 0;
+}
