Message-ID: <20200727200954.pzw6swdboa65474i@pengutronix.de>
Date:   Mon, 27 Jul 2020 22:09:54 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Martin Botka <martin.botka1@...il.com>
Cc:     Fenglin Wu <fenglinw@...eaurora.org>,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Pavel Machek <pavel@....cz>, Dan Murphy <dmurphy@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Lee Jones <lee.jones@...aro.org>, linux-leds@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pwm@...r.kernel.org
Subject: Re: [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG
 module

On Fri, Jul 24, 2020 at 11:36:53PM +0200, Martin Botka wrote:
> From: Fenglin Wu <fenglinw@...eaurora.org>
> 
> 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@...eaurora.org>
> [martin.botka1@...il.com: Fast-forward the driver from kernel 4.14 to 5.8]
> Signed-off-by: Martin Botka <martin.botka1@...il.com>
> ---
>  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.

>  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.

> +#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.

> +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.

> +
> +	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().

> +	.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.

> +	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.

> +
> +	mutex_destroy(&chip->bus_lock);
> +	dev_set_drvdata(chip->dev, NULL);

dev_set_drvdata(..., NULL) is not needed.

> +
> +	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
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists