Re: [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux