On 4/29/21 12:39 AM, Bjorn Andersson wrote:
On Sun 18 Apr 16:54 CDT 2021, Marijn Suijten wrote:
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)?
Thanks for testing, let's try to land this first iteration first and
then we can add PM660l and PM8150* definitions/support on top.
I'll keep an eye out for when these patches land and send them on top.
Feel free to add me to the CC list for future revisions.
+/**
+ * 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?
I don't see any obvious solution to the latter, so perhaps we should
just stick with defining this per compatible? Or am I reading your
suggestion wrong?
Assuming these devices have a different "LPG subtype" you should be able
to read their value and add it to the list as third option.
Alternatively, can you point out the driver this `3<<4` mask was based
on? With all the information available it should be possible to derive
this from hardware for every channel instead of hardcoding it.
+
+ 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.
The intention is that each code path through these functions will either
pass or spit out an error in the log. I looked through them again and
think I cover all paths...
That is true, all the errors not covered are extremely unlikely like
-ENOMEM. I vaguely recall having to insert extra logging to get through
initial probe, but that might have been something inside lpg_add_led as
well. Fine to leave this as it is.
- Marijn
