Hi!

> > Dalsi cech co hackuje LEDky?
>
> Nie. Slovak.

Vitej do klubu :-).

> > Bindings should go first, they may need to be converted to yml
> > (devicetree people will know).
>
> OK

I'm not 100% sure, please double check.

> > Can generic pwm driver be used here?
>
> I have not tried to do that. But considering it's custom chip from Qualcomm
> then it's unlikely.

Please double check. Actually, is the series for one chip or for two
of them? LED framework should is happy to talk to generic pwm driver...

> >This is for RGB modules, right? It will need to use multicolor
> >framework.
>
> This is for RGB LEDs in phones that are connected via pmic.

Ok. Check multicolor framework in -next.

> > Is this for phone, btw? If so, which one?
>
> Yes it is. And it's for many many phones actually. I have done this mainly
> for SONY phones (Xperia 10, 10 Plus, XA2, XA2 Plus, XA2 Ultra).
> All of them use these drivers for generating the PWM and controlling the
> LEDs.

Aha, ok. I assume they are still quite far away from being usable with
mainline kernels.

Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (1.25 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2020-07-24 22:33:38

Hello Martin,

On Mon, Jul 27, 2020 at 09:58:01AM +0200, Martin Botka wrote:
> > I hit "reply-to-all" and the mail only was sent to you because you wrote
> > to only me.
>
> Yes my reply was only to you. But your original message was sent only to me too.
> So when i clicked reply to all it was only you as you sent it only to me.

Oh indeed. Bummer, and I was so sure to always reply to all :-|

Best regards
Uwe

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

Attachments:

(No filename) (587.00 B)
signature.asc (499.00 B)
Download all attachments

2020-07-27 20:10:51

by Uwe Kleine-König

[permalink] [raw]

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 <[email protected]>
>
> Add pwm_chip to support QTI LPG module and export LPG channels as
> PWM devices for consumer drivers' usage.
>
> Signed-off-by: Fenglin Wu <[email protected]>
> [[email protected]: Fast-forward the driver from kernel 4.14 to 5.8]
> Signed-off-by: Martin Botka <[email protected]>
> ---
> 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/ |

Attachments:

(No filename) (17.37 kB)
signature.asc (499.00 B)
Download all attachments

2020-07-27 20:12:34

Hello Martin,

On Mon, Jul 27, 2020 at 11:16:57PM +0200, Martin Botka wrote:
> Mo 27. 7. 2020 at 22:10 Uwe Kleine-K?nig <[email protected]> wrote:
> >
> > On Fri, Jul 24, 2020 at 11:36:53PM +0200, Martin Botka wrote:
> > > From: Fenglin Wu <[email protected]>
> > >
> > > Add pwm_chip to support QTI LPG module and export LPG channels as
> > > PWM devices for consumer drivers' usage.
> > >
> > > Signed-off-by: Fenglin Wu <[email protected]>
> > > [[email protected]: Fast-forward the driver from kernel 4.14 to 5.8]
> > > Signed-off-by: Martin Botka <[email protected]>
> > > ---
> > > 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 ?

Either drop pr_fmt or at least don't have __func__ in it. This doesn't
belong into the kernel log (in the non-debug case at least).
(For debugging I like:

#define pr_fmt(fmt) "%s:%d: " fmt, __func__, __LINE__

which helps finding the right printk line in the code from a given
output in functions with many printks.)

I don't mind the REG definitions, though aligning the values vertically
is common.

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

ack

Best regards
Uwe

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

Attachments:

(No filename) (4.57 kB)
signature.asc (499.00 B)
Download all attachments

2020-07-28 07:55:06

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [PATCH RCC 1/6] pwm: Add different PWM output types support

Hello Martin,

On Mon, Jul 27, 2020 at 10:56:31PM +0200, Martin Botka wrote:
> Mo 27. 7. 2020 at 22:10 Uwe Kleine-K?nig <[email protected]> wrote:
> > On Fri, Jul 24, 2020 at 11:36:51PM +0200, Martin Botka wrote:
> > > +/*
> > > + * pwm_output_type_support()
> > > + * @pwm: PWM device
> > > + *
> > > + * Returns: output types supported by the PWM device
> > > + */
> > > +static inline int pwm_get_output_type_supported(struct pwm_device *pwm)
> > > +{
> > > + if (pwm->chip->ops->get_output_type_supported != NULL)
> > > + return pwm->chip->ops->get_output_type_supported(pwm->chip, pwm);
> > > + else
> > > + return PWM_OUTPUT_FIXED;
> > > +}
> >
> > I don't like this "advertising" for specific functions. I'd prefer to
> > handle this in .apply(), fix all drivers to return -ESOMETHING when the
> > request cannot be fulfilled.
>
> I will have to disagree on this one. As the functions are called in
> multiple places it would just make mess in the driver.

Note this is something where (I think) I don't agree with Thierry
either. This popped up just yesterday, see

https://www.spinics.net/lists/linux-pwm/msg13290.html

For sure I want at most one such function per driver, so if we really
want to go this path and introduce a capability indicator, this should
be named differently and have a different prototype.

> As the driver is even now not exactly the definition of clean driver i
> would not like to make it even more messy.

> > Having said that I wonder if this output pattern is a common enough
> > property to add support for it in the PWM framework.
> >
>
> I have gotten an email from Guru Das Srinagesh regarding this exact
> issue you are pointing to. Yes the output pattern will be dropped in
> V2.

That's good.

Best regards
Uwe

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

Attachments:

(No filename) (1.97 kB)
signature.asc (499.00 B)
Download all attachments