Hello,
Mo 27. 7. 2020 at 22:10 Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> On Fri, Jul 24, 2020 at 11:36:53PM +0200, Martin Botka wrote:
> > From: Fenglin Wu <fenglinw@xxxxxxxxxxxxxx>
> >
> > Add pwm_chip to support QTI LPG module and export LPG channels as
> > PWM devices for consumer drivers' usage.
> >
> > Signed-off-by: Fenglin Wu <fenglinw@xxxxxxxxxxxxxx>
> > [martin.botka1@xxxxxxxxx: Fast-forward the driver from kernel 4.14 to 5.8]
> > Signed-off-by: Martin Botka <martin.botka1@xxxxxxxxx>
> > ---
> > drivers/pwm/Kconfig | 10 +
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-qti-lpg.c | 1284 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 1295 insertions(+)
> > create mode 100644 drivers/pwm/pwm-qti-lpg.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index cb8d739067d2..8a52d6884a9a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -399,6 +399,16 @@ config PWM_RCAR
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-rcar.
> >
> > +config PWM_QTI_LPG
> > + tristate "Qualcomm Technologies, Inc. LPG driver"
> > + depends on MFD_SPMI_PMIC && OF
> > + help
> > + This driver supports the LPG (Light Pulse Generator) module found in
> > + Qualcomm Technologies, Inc. PMIC chips. Each LPG channel can be
> > + configured to operate in PWM mode to output a fixed amplitude with
> > + variable duty cycle or in LUT (Look up table) mode to output PWM
> > + signal with a modulated amplitude.
> > +
> > config PWM_RENESAS_TPU
> > tristate "Renesas TPU PWM support"
> > depends on ARCH_RENESAS || COMPILE_TEST
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index a59c710e98c7..3555a6aa3f33 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o
> > obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> > obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o
> > +obj-$(CONFIG_PWM_QTI_LPG) += pwm-qti-lpg.o
>
> Please keep this list alphabetically sorted.
OK
>
> > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
> > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
> > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> > diff --git a/drivers/pwm/pwm-qti-lpg.c b/drivers/pwm/pwm-qti-lpg.c
> > new file mode 100644
> > index 000000000000..d24c3b3a3d8c
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-qti-lpg.c
> > @@ -0,0 +1,1284 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#define pr_fmt(fmt) "%s: " fmt, __func__
>
> This smells like debug stuff. Please drop this.
What do you mean ?
The #define pr_fmt(fmt) or the tons of REG definitions ?
>
> > +#include <linux/bitops.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#define REG_SIZE_PER_LPG 0x100
> > +#define LPG_BASE "lpg-base"
> > +#define LUT_BASE "lut-base"
> > +
> > +/* LPG module registers */
> > +#define REG_LPG_PERPH_SUBTYPE 0x05
> > +#define REG_LPG_PATTERN_CONFIG 0x40
> > +#define REG_LPG_PWM_SIZE_CLK 0x41
> > +#define REG_LPG_PWM_FREQ_PREDIV_CLK 0x42
> > +#define REG_LPG_PWM_TYPE_CONFIG 0x43
> > +#define REG_LPG_PWM_VALUE_LSB 0x44
> > +#define REG_LPG_PWM_VALUE_MSB 0x45
> > +#define REG_LPG_ENABLE_CONTROL 0x46
> > +#define REG_LPG_PWM_SYNC 0x47
> > +#define REG_LPG_RAMP_STEP_DURATION_LSB 0x50
> > +#define REG_LPG_RAMP_STEP_DURATION_MSB 0x51
> > +#define REG_LPG_PAUSE_HI_MULTIPLIER 0x52
> > +#define REG_LPG_PAUSE_LO_MULTIPLIER 0x54
> > +#define REG_LPG_HI_INDEX 0x56
> > +#define REG_LPG_LO_INDEX 0x57
> > +
> > +/* REG_LPG_PATTERN_CONFIG */
> > +#define LPG_PATTERN_EN_PAUSE_LO BIT(0)
> > +#define LPG_PATTERN_EN_PAUSE_HI BIT(1)
> > +#define LPG_PATTERN_RAMP_TOGGLE BIT(2)
> > +#define LPG_PATTERN_REPEAT BIT(3)
> > +#define LPG_PATTERN_RAMP_LO_TO_HI BIT(4)
> > +
> > +/* REG_LPG_PERPH_SUBTYPE */
> > +#define SUBTYPE_PWM 0x0b
> > +#define SUBTYPE_LPG_LITE 0x11
> > +
> > +/* REG_LPG_PWM_SIZE_CLK */
> > +#define LPG_PWM_SIZE_LPG_MASK BIT(4)
> > +#define LPG_PWM_SIZE_PWM_MASK BIT(2)
> > +#define LPG_PWM_SIZE_LPG_SHIFT 4
> > +#define LPG_PWM_SIZE_PWM_SHIFT 2
> > +#define LPG_PWM_CLK_FREQ_SEL_MASK GENMASK(1, 0)
> > +
> > +/* REG_LPG_PWM_FREQ_PREDIV_CLK */
> > +#define LPG_PWM_FREQ_PREDIV_MASK GENMASK(6, 5)
> > +#define LPG_PWM_FREQ_PREDIV_SHIFT 5
> > +#define LPG_PWM_FREQ_EXPONENT_MASK GENMASK(2, 0)
> > +
> > +/* REG_LPG_PWM_TYPE_CONFIG */
> > +#define LPG_PWM_EN_GLITCH_REMOVAL_MASK BIT(5)
> > +
> > +/* REG_LPG_PWM_VALUE_LSB */
> > +#define LPG_PWM_VALUE_LSB_MASK GENMASK(7, 0)
> > +
> > +/* REG_LPG_PWM_VALUE_MSB */
> > +#define LPG_PWM_VALUE_MSB_MASK BIT(0)
> > +
> > +/* REG_LPG_ENABLE_CONTROL */
> > +#define LPG_EN_LPG_OUT_BIT BIT(7)
> > +#define LPG_EN_LPG_OUT_SHIFT 7
> > +#define LPG_PWM_SRC_SELECT_MASK BIT(2)
> > +#define LPG_PWM_SRC_SELECT_SHIFT 2
> > +#define LPG_EN_RAMP_GEN_MASK BIT(1)
> > +#define LPG_EN_RAMP_GEN_SHIFT 1
> > +
> > +/* REG_LPG_PWM_SYNC */
> > +#define LPG_PWM_VALUE_SYNC BIT(0)
> > +
> > +#define NUM_PWM_SIZE 2
> > +#define NUM_PWM_CLK 3
> > +#define NUM_CLK_PREDIV 4
> > +#define NUM_PWM_EXP 8
> > +
> > +#define LPG_HI_LO_IDX_MASK GENMASK(5, 0)
> > +
> > +/* LUT module registers */
> > +#define REG_LPG_LUT_1_LSB 0x42
> > +#define REG_LPG_LUT_RAMP_CONTROL 0xc8
> > +
> > +#define LPG_LUT_VALUE_MSB_MASK BIT(0)
> > +#define LPG_LUT_COUNT_MAX 47
> > +
> > +enum lpg_src {
> > + LUT_PATTERN = 0,
> > + PWM_VALUE,
> > +};
> > +
> > +static const int pwm_size[NUM_PWM_SIZE] = { 6, 9 };
> > +static const int clk_freq_hz[NUM_PWM_CLK] = { 1024, 32768, 19200000 };
> > +static const int clk_prediv[NUM_CLK_PREDIV] = { 1, 3, 5, 6 };
> > +static const int pwm_exponent[NUM_PWM_EXP] = { 0, 1, 2, 3, 4, 5, 6, 7 };
>
> I don't know what the compiler makes of these arrays, but the last one
> at least can be replaces by some simple math.
Yup.
>
> > +static int qpnp_lut_masked_write(struct qpnp_lpg_lut *lut, u16 addr, u8 mask,
> > + u8 val)
> > +{
> > + int rc;
> > +
> > + mutex_lock(&lut->chip->bus_lock);
> > + rc = regmap_update_bits(lut->chip->regmap, lut->reg_base + addr, mask,
> > + val);
> > + if (rc < 0)
> > + dev_err(lut->chip->dev,
> > + "Update addr 0x%x to val 0x%x with mask 0x%x failed, rc=%d\n",
> > + lut->reg_base + addr, val, mask, rc);
> > + mutex_unlock(&lut->chip->bus_lock);
>
> The error logging can be moved out of the lock.
Will do.
>
> > +
> > + return rc;
> > +}
> > +
> > +static struct qpnp_lpg_channel *pwm_dev_to_qpnp_lpg(struct pwm_chip *pwm_chip,
> > + struct pwm_device *pwm)
> > +{
> > + struct qpnp_lpg_chip *chip =
> > + container_of(pwm_chip, struct qpnp_lpg_chip, pwm_chip);
> > + u32 hw_idx = pwm->hwpwm;
> > +
> > + if (hw_idx >= chip->num_lpgs) {
> > + dev_err(chip->dev, "hw index %d out of range [0-%d]\n", hw_idx,
> > + chip->num_lpgs - 1);
> > + return NULL;
> > + }
> > +
> > + return &chip->lpgs[hw_idx];
> > +}
> > +
> > +static int __find_index_in_array(int member, const int array[], int length)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < length; i++) {
> > + if (member == array[i])
> > + return i;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int qpnp_lpg_set_glitch_removal(struct qpnp_lpg_channel *lpg, bool en)
> > +{
> > + int rc;
> > + u8 mask, val;
> > +
> > + val = en ? LPG_PWM_EN_GLITCH_REMOVAL_MASK : 0;
> > + mask = LPG_PWM_EN_GLITCH_REMOVAL_MASK;
> > + rc = qpnp_lpg_masked_write(lpg, REG_LPG_PWM_TYPE_CONFIG, mask, val);
> > + if (rc < 0)
> > + dev_err(lpg->chip->dev,
> > + "Write LPG_PWM_TYPE_CONFIG failed, rc=%d\n", rc);
>
> qpnp_lpg_masked_write already issues a warning.
>
> > + return rc;
> > +}
> > +
> > [...]
> > +static const struct pwm_ops qpnp_lpg_pwm_ops = {
> > + .config = qpnp_lpg_pwm_config,
> > + .config_extend = qpnp_lpg_pwm_config_extend,
> > + .get_output_type_supported = qpnp_lpg_pwm_output_types_supported,
> > + .set_output_type = qpnp_lpg_pwm_set_output_type,
> > + .set_output_pattern = qpnp_lpg_pwm_set_output_pattern,
> > + .enable = qpnp_lpg_pwm_enable,
> > + .disable = qpnp_lpg_pwm_disable,
>
> As already noted in the former patch: Please implement .apply() and
> .get_state().
So drop:
.get_output_type_supported = qpnp_lpg_pwm_output_types_supported,
.set_output_type = qpnp_lpg_pwm_set_output_type,
.set_output_pattern = qpnp_lpg_pwm_set_output_pattern,
Ad implement implement .apply and .get_state if i understood you correctly.
Right ?
>
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int qpnp_lpg_parse_dt(struct qpnp_lpg_chip *chip)
> > +{
> > + struct device_node *child;
> > + struct qpnp_lpg_channel *lpg;
> > + struct lpg_ramp_config *ramp;
> > + int rc = 0, i;
> > + u32 base, length, lpg_chan_id, tmp;
> > + const __be32 *addr;
> > +
> > + addr = of_get_address(chip->dev->of_node, 0, NULL, NULL);
> > + if (!addr) {
> > + dev_err(chip->dev, "Get %s address failed\n", LPG_BASE);
> > + return -EINVAL;
> > + }
> > +
> > + base = be32_to_cpu(addr[0]);
> > + rc = of_property_read_u32(chip->dev->of_node, "qcom,num-lpg-channels",
> > + &chip->num_lpgs);
> > + if (rc < 0) {
> > + dev_err(chip->dev,
> > + "Failed to get qcom,num-lpg-channels, rc=%d\n", rc);
> > + return rc;
> > + }
> > +
> > + if (chip->num_lpgs == 0) {
> > + dev_err(chip->dev, "No LPG channels specified\n");
> > + return -EINVAL;
> > + }
> > +
> > + chip->lpgs = devm_kcalloc(chip->dev, chip->num_lpgs,
> > + sizeof(*chip->lpgs), GFP_KERNEL);
>
> Would be great to lower the number of allocations - ideally to one at
> probe time.
Hmmmm. Okay.
>
> > + if (!chip->lpgs)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < chip->num_lpgs; i++) {
> > + chip->lpgs[i].chip = chip;
> > + chip->lpgs[i].lpg_idx = i;
> > + chip->lpgs[i].reg_base = base + i * REG_SIZE_PER_LPG;
> > + chip->lpgs[i].src_sel = PWM_VALUE;
> > + rc = qpnp_lpg_read(&chip->lpgs[i], REG_LPG_PERPH_SUBTYPE,
> > + &chip->lpgs[i].subtype);
> > + if (rc < 0) {
> > + dev_err(chip->dev, "Read subtype failed, rc=%d\n", rc);
> > + return rc;
> > + }
> > + }
> > +
> > + addr = of_get_address(chip->dev->of_node, 1, NULL, NULL);
> > + if (!addr) {
> > + pr_debug("NO LUT address assigned\n");
> > + return 0;
> > + }
> > +
> > + chip->lut = devm_kmalloc(chip->dev, sizeof(*chip->lut), GFP_KERNEL);
> > + if (!chip->lut)
> > + return -ENOMEM;
> > +
> > + chip->lut->chip = chip;
> > + chip->lut->reg_base = be32_to_cpu(*addr);
> > + mutex_init(&chip->lut->lock);
> > +
> > + rc = of_property_count_elems_of_size(chip->dev->of_node,
> > + "qcom,lut-patterns", sizeof(u32));
> > + if (rc < 0) {
> > + dev_err(chip->dev, "Read qcom,lut-patterns failed, rc=%d\n",
> > + rc);
> > + return rc;
> > + }
> > +
> > + length = rc;
> > + if (length > LPG_LUT_COUNT_MAX) {
> > + dev_err(chip->dev,
> > + "qcom,lut-patterns length %d exceed max %d\n", length,
> > + LPG_LUT_COUNT_MAX);
> > + return -EINVAL;
> > + }
> > +
> > + chip->lut->pattern =
> > + devm_kcalloc(chip->dev, LPG_LUT_COUNT_MAX,
> > + sizeof(*chip->lut->pattern), GFP_KERNEL);
> > + if (!chip->lut->pattern)
> > + return -ENOMEM;
> > +
> > + rc = of_property_read_u32_array(chip->dev->of_node, "qcom,lut-patterns",
> > + chip->lut->pattern, length);
> > + if (rc < 0) {
> > + dev_err(chip->dev, "Get qcom,lut-patterns failed, rc=%d\n", rc);
> > + return rc;
> > + }
> > +
> > + if (of_get_available_child_count(chip->dev->of_node) == 0) {
> > + dev_err(chip->dev, "No ramp configuration for any LPG\n");
> > + return -EINVAL;
> > + }
> > +
> > + for_each_available_child_of_node (chip->dev->of_node, child) {
> > + rc = of_property_read_u32(child, "qcom,lpg-chan-id",
> > + &lpg_chan_id);
> > + if (rc < 0) {
> > + dev_err(chip->dev,
> > + "Get qcom,lpg-chan-id failed for node %s, rc=%d\n",
> > + child->name, rc);
> > + return rc;
> > + }
> > +
> > + if (lpg_chan_id > chip->num_lpgs) {
> > + dev_err(chip->dev,
> > + "lpg-chann-id %d is out of range 1~%d\n",
> > + lpg_chan_id, chip->num_lpgs);
> > + return -EINVAL;
> > + }
> > +
> > + /* lpg channel id is indexed from 1 in hardware */
> > + lpg = &chip->lpgs[lpg_chan_id - 1];
> > + ramp = &lpg->ramp_config;
> > +
> > + rc = of_property_read_u32(child, "qcom,ramp-step-ms", &tmp);
> > + if (rc < 0) {
> > + dev_err(chip->dev,
> > + "get qcom,ramp-step-ms failed for lpg%d, rc=%d\n",
> > + lpg_chan_id, rc);
> > + return rc;
> > + }
> > + ramp->step_ms = (u16)tmp;
> > +
> > + rc = of_property_read_u32(child, "qcom,ramp-low-index", &tmp);
> > + if (rc < 0) {
> > + dev_err(chip->dev,
> > + "get qcom,ramp-low-index failed for lpg%d, rc=%d\n",
> > + lpg_chan_id, rc);
> > + return rc;
> > + }
> > + ramp->lo_idx = (u8)tmp;
> > + if (ramp->lo_idx >= LPG_LUT_COUNT_MAX) {
> > + dev_err(chip->dev,
> > + "qcom,ramp-low-index should less than max %d\n",
> > + LPG_LUT_COUNT_MAX);
> > + return -EINVAL;
> > + }
> > +
> > + rc = of_property_read_u32(child, "qcom,ramp-high-index", &tmp);
> > + if (rc < 0) {
> > + dev_err(chip->dev,
> > + "get qcom,ramp-high-index failed for lpg%d, rc=%d\n",
> > + lpg_chan_id, rc);
> > + return rc;
> > + }
> > + ramp->hi_idx = (u8)tmp;
> > +
> > + if (ramp->hi_idx > LPG_LUT_COUNT_MAX) {
> > + dev_err(chip->dev,
> > + "qcom,ramp-high-index shouldn't exceed max %d\n",
> > + LPG_LUT_COUNT_MAX);
> > + return -EINVAL;
> > + }
> > +
> > + if (ramp->hi_idx <= ramp->lo_idx) {
> > + dev_err(chip->dev,
> > + "high-index(%d) should be larger than low-index(%d)\n",
> > + ramp->hi_idx, ramp->lo_idx);
> > + return -EINVAL;
> > + }
> > +
> > + ramp->pattern_length = ramp->hi_idx - ramp->lo_idx + 1;
> > + ramp->pattern = &chip->lut->pattern[ramp->lo_idx];
> > + lpg->max_pattern_length = ramp->pattern_length;
> > +
> > + rc = of_property_read_u32(child, "qcom,ramp-pause-hi-count",
> > + &tmp);
> > + if (rc < 0)
> > + ramp->pause_hi_count = 0;
> > + else
> > + ramp->pause_hi_count = (u8)tmp;
> > +
> > + rc = of_property_read_u32(child, "qcom,ramp-pause-lo-count",
> > + &tmp);
> > + if (rc < 0)
> > + ramp->pause_lo_count = 0;
> > + else
> > + ramp->pause_lo_count = (u8)tmp;
> > +
> > + ramp->ramp_dir_low_to_hi = of_property_read_bool(
> > + child, "qcom,ramp-from-low-to-high");
> > +
> > + ramp->pattern_repeat = of_property_read_bool(
> > + child, "qcom,ramp-pattern-repeat");
> > +
> > + ramp->toggle = of_property_read_bool(child, "qcom,ramp-toggle");
> > + }
> > +
> > + rc = of_property_count_elems_of_size(
> > + chip->dev->of_node, "qcom,sync-channel-ids", sizeof(u32));
> > + if (rc < 0)
> > + return 0;
> > +
> > + length = rc;
> > + if (length > chip->num_lpgs) {
> > + dev_err(chip->dev,
> > + "qcom,sync-channel-ids has too many channels: %d\n",
> > + length);
> > + return -EINVAL;
> > + }
> > +
> > + chip->lpg_group = devm_kcalloc(chip->dev, chip->num_lpgs, sizeof(u32),
> > + GFP_KERNEL);
> > + if (!chip->lpg_group)
> > + return -ENOMEM;
> > +
> > + rc = of_property_read_u32_array(chip->dev->of_node,
> > + "qcom,sync-channel-ids",
> > + chip->lpg_group, length);
> > + if (rc < 0) {
> > + dev_err(chip->dev, "Get qcom,sync-channel-ids failed, rc=%d\n",
> > + rc);
> > + return rc;
> > + }
> > +
> > + for (i = 0; i < length; i++) {
> > + if (chip->lpg_group[i] <= 0 ||
> > + chip->lpg_group[i] > chip->num_lpgs) {
> > + dev_err(chip->dev,
> > + "lpg_group[%d]: %d is not a valid channel\n", i,
> > + chip->lpg_group[i]);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + /*
> > + * The LPG channel in the same group should have the same ramping
> > + * configuration, so force to use the ramping configuration of the
> > + * 1st LPG channel in the group for sychronization.
> > + */
> > + lpg = &chip->lpgs[chip->lpg_group[0] - 1];
> > + ramp = &lpg->ramp_config;
> > +
> > + for (i = 1; i < length; i++) {
> > + lpg = &chip->lpgs[chip->lpg_group[i] - 1];
> > + memcpy(&lpg->ramp_config, ramp, sizeof(struct lpg_ramp_config));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int qpnp_lpg_probe(struct platform_device *pdev)
> > +{
> > + int rc;
> > + struct qpnp_lpg_chip *chip;
> > +
> > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > + if (!chip)
> > + return -ENOMEM;
> > +
> > + chip->dev = &pdev->dev;
> > + chip->regmap = dev_get_regmap(chip->dev->parent, NULL);
> > + if (!chip->regmap) {
> > + dev_err(chip->dev, "Getting regmap failed\n");
> > + return -EINVAL;
> > + }
> > +
> > + mutex_init(&chip->bus_lock);
> > + rc = qpnp_lpg_parse_dt(chip);
> > + if (rc < 0) {
> > + dev_err(chip->dev,
> > + "Devicetree properties parsing failed, rc=%d\n", rc);
> > + goto err_out;
> > + }
> > +
> > + dev_set_drvdata(chip->dev, chip);
> > + chip->pwm_chip.dev = chip->dev;
> > + chip->pwm_chip.base = -1;
> > + chip->pwm_chip.npwm = chip->num_lpgs;
> > + chip->pwm_chip.ops = &qpnp_lpg_pwm_ops;
> > +
> > + rc = pwmchip_add(&chip->pwm_chip);
> > + if (rc < 0) {
> > + dev_err(chip->dev, "Add pwmchip failed, rc=%d\n", rc);
> > + goto err_out;
> > + }
> > +
> > + return 0;
> > +err_out:
> > + mutex_destroy(&chip->bus_lock);
> > + return rc;
> > +}
> > +
> > +static int qpnp_lpg_remove(struct platform_device *pdev)
> > +{
> > + struct qpnp_lpg_chip *chip = dev_get_drvdata(&pdev->dev);
> > + int rc = 0;
> > +
> > + rc = pwmchip_remove(&chip->pwm_chip);
> > + if (rc < 0)
> > + dev_err(chip->dev, "Remove pwmchip failed, rc=%d\n", rc);
>
> Please use %pe instead of %d for error codes.
OK.
>
> > +
> > + mutex_destroy(&chip->bus_lock);
> > + dev_set_drvdata(chip->dev, NULL);
>
> dev_set_drvdata(..., NULL) is not needed.
Okay.
>
> > +
> > + return rc;
> > +}
> > +
> > +static const struct of_device_id qpnp_lpg_of_match[] = {
> > + {
> > + .compatible = "qcom,pwm-lpg",
> > + },
> > + {},
> > +};
> > +
> > +static struct platform_driver qpnp_lpg_driver = {
> > + .driver = {
> > + .name = "qcom,pwm-lpg",
> > + .of_match_table = qpnp_lpg_of_match,
> > + },
> > + .probe = qpnp_lpg_probe,
> > + .remove = qpnp_lpg_remove,
> > +};
> > +module_platform_driver(qpnp_lpg_driver);
> > +
> > +MODULE_DESCRIPTION("QTI LPG driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.27.0
Thank you.
Best Regards,
Martin
