| Message ID | 20230814235918.10396-1-quic_amelende@quicinc.com |
|---|---|
| Headers | show |
| Series | Add support for LUT PPG | expand |
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@quicinc.com> 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@quicinc.com> > --- > 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
On 15/08/2023 01:59, Anjelique Melendez wrote: > Add binding for the Qualcomm Programmable Boot Sequencer device. > > Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > --- > .../bindings/soc/qcom/qcom,pbs.yaml | 46 +++++++++++++++++++ > 1 file changed, 46 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml > new file mode 100644 > index 000000000000..b502ca72266a > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml > @@ -0,0 +1,46 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/soc/qcom/qcom,pbs.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Technologies, Inc. Programmable Boot Sequencer > + > +maintainers: > + - Anjelique Melendez <quic_amelende@quicinc.com> > + > +description: | > + The Qualcomm Technologies, Inc. Programmable Boot Sequencer (PBS) > + supports triggering power up and power down sequences for clients > + upon request. > + > +properties: > + compatible: > + items: > + - enum: > + - qcom,pmi632-pbs > + - const: qcom,pbs > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/spmi/spmi.h> > + > + pmic@0 { This should be rather just "pmic", because the examples have address-cells=1 and size-cells=1, which is not correct in this context. Anyway: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Tue, Aug 15, 2023 at 10:36:39PM +0200, Krzysztof Kozlowski wrote: > On 15/08/2023 01:59, Anjelique Melendez wrote: > > Add binding for the Qualcomm Programmable Boot Sequencer device. > > > > Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > > --- > > .../bindings/soc/qcom/qcom,pbs.yaml | 46 +++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml > > new file mode 100644 > > index 000000000000..b502ca72266a > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml > > @@ -0,0 +1,46 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/soc/qcom/qcom,pbs.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Qualcomm Technologies, Inc. Programmable Boot Sequencer > > + > > +maintainers: > > + - Anjelique Melendez <quic_amelende@quicinc.com> > > + > > +description: | > > + The Qualcomm Technologies, Inc. Programmable Boot Sequencer (PBS) > > + supports triggering power up and power down sequences for clients > > + upon request. > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - qcom,pmi632-pbs > > + - const: qcom,pbs > > + > > + reg: > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/spmi/spmi.h> > > + > > + pmic@0 { > > This should be rather just "pmic", because the examples have > address-cells=1 and size-cells=1, which is not correct in this context. But there's a 'reg' property so you can't drop the unit-address. The bus node (spmi) needs to be added. Rob
On 17/08/2023 18:26, Rob Herring wrote: > On Tue, Aug 15, 2023 at 10:36:39PM +0200, Krzysztof Kozlowski wrote: >> On 15/08/2023 01:59, Anjelique Melendez wrote: >>> Add binding for the Qualcomm Programmable Boot Sequencer device. >>> >>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> >>> --- >>> .../bindings/soc/qcom/qcom,pbs.yaml | 46 +++++++++++++++++++ >>> 1 file changed, 46 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml >>> new file mode 100644 >>> index 000000000000..b502ca72266a >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml >>> @@ -0,0 +1,46 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/soc/qcom/qcom,pbs.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Qualcomm Technologies, Inc. Programmable Boot Sequencer >>> + >>> +maintainers: >>> + - Anjelique Melendez <quic_amelende@quicinc.com> >>> + >>> +description: | >>> + The Qualcomm Technologies, Inc. Programmable Boot Sequencer (PBS) >>> + supports triggering power up and power down sequences for clients >>> + upon request. >>> + >>> +properties: >>> + compatible: >>> + items: >>> + - enum: >>> + - qcom,pmi632-pbs >>> + - const: qcom,pbs >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/spmi/spmi.h> >>> + >>> + pmic@0 { >> >> This should be rather just "pmic", because the examples have >> address-cells=1 and size-cells=1, which is not correct in this context. > > But there's a 'reg' property so you can't drop the unit-address. The bus > node (spmi) needs to be added. reg also can be dropped. I am not sure whether parent PMIC bus is important here. Best regards, Krzysztof
In certain PMICs, LUT pattern and LPG configuration can be stored in SDAM modules instead of LUT peripheral. This feature is called PPG. This change series adds support for PPG. Thanks! Changes since v2: - Patch 1/7 - Fix dt_binding_check error - Rename binding file to match compatible - Iclude SoC specific comptaibles - Patch 2/7 - Update nvmem-names list - Patch 3/7 - Update EXPORT_SYMBOL to EXPORT_SYMBOL_GPL - Fix return/break logic in qcom_pbs_wait_for_ack() - Update iterators to be int - Add constants - Fix function calls in qcom_pbs_trigger_event() - Remove unnessary comments - Return -EPROBE_DEFER from get_pbs_client_device() Changes since v1: - Patch 1/7 - Fix dt_binding_check errors - Update binding description - Path 2/7 - Fix dt_binding_check errors - Update per variant constraints - Update nvmem description - Patch 3/7 - Update get_pbs_client_device() - Drop use of printk - Remove unused function Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sdm632-fairphone-fp3 (pmi632) Anjelique Melendez (7): dt-bindings: soc: qcom: Add qcom,pbs bindings dt-bindings: leds: leds-qcom-lpg: Add support for LPG PPG soc: qcom: add QCOM PBS driver leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM leds: rgb: leds-qcom-lpg: Update PMI632 lpg_data to support PPG leds: rgb: leds-qcom-lpg: Support two-nvmem PPG Scheme leds: rgb: Update PM8350C lpg_data to support two-nvmem PPG Scheme .../bindings/leds/leds-qcom-lpg.yaml | 89 +++- .../bindings/soc/qcom/qcom,pbs.yaml | 46 ++ drivers/leds/rgb/leds-qcom-lpg.c | 395 ++++++++++++++++-- drivers/soc/qcom/Kconfig | 9 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/qcom-pbs.c | 286 +++++++++++++ include/linux/soc/qcom/qcom-pbs.h | 30 ++ 7 files changed, 823 insertions(+), 33 deletions(-) create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,pbs.yaml create mode 100644 drivers/soc/qcom/qcom-pbs.c create mode 100644 include/linux/soc/qcom/qcom-pbs.h