linux-leds.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers
@ 2020-07-24 21:36 Martin Botka
  2020-07-24 21:36 ` [PATCH RCC 1/6] pwm: Add different PWM output types support Martin Botka
                   ` (6 more replies)
  0 siblings, 7 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-24 21:36 UTC (permalink / raw)
  Cc: Martin Botka, Jacek Anaszewski, Pavel Machek, Dan Murphy,
	Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones,
	linux-leds, devicetree, linux-kernel, linux-pwm

Hello,
This series brings QCOM pwm-lpg and tri-led drivers from 4.14 that is required to support pmic-connected notification LED.
This comes straight from downstream and I'm ready for your comments.

Fenglin Wu (6):
  pwm: Add different PWM output types support
  pwm: core: Add option to config PWM duty/period with u64 data length
  pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module
  leds: leds-qti-tri-led: Add LED driver for QTI TRI_LED module
  Documentation: Add binding for qti-tri-led
  Documentation: Add binding for pwm-qti-lpg

 .../bindings/leds/leds-qti-tri-led.txt        |   72 +
 .../devicetree/bindings/pwm/pwm-qti-lpg.txt   |  163 +++
 drivers/leds/Kconfig                          |    9 +
 drivers/leds/Makefile                         |    1 +
 drivers/leds/leds-qti-tri-led.c               |  640 ++++++++
 drivers/pwm/Kconfig                           |   10 +
 drivers/pwm/Makefile                          |    1 +
 drivers/pwm/core.c                            |   56 +-
 drivers/pwm/pwm-qti-lpg.c                     | 1284 +++++++++++++++++
 drivers/pwm/sysfs.c                           |   56 +-
 include/linux/pwm.h                           |  144 +-
 11 files changed, 2418 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-qti-tri-led.txt
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-qti-lpg.txt
 create mode 100644 drivers/leds/leds-qti-tri-led.c
 create mode 100644 drivers/pwm/pwm-qti-lpg.c

-- 
2.27.0


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH RCC 1/6] pwm: Add different PWM output types support
  2020-07-24 21:36 [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Martin Botka
@ 2020-07-24 21:36 ` Martin Botka
  2020-07-27 20:10   ` Uwe Kleine-König
  2020-07-27 21:46   ` Guru Das Srinagesh
  2020-07-24 21:36 ` [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length Martin Botka
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-24 21:36 UTC (permalink / raw)
  Cc: Fenglin Wu, Konrad Dybcio, Martin Botka, Jacek Anaszewski,
	Pavel Machek, Dan Murphy, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Lee Jones, linux-leds, devicetree,
	linux-kernel, linux-pwm

From: Fenglin Wu <fenglinw@codeaurora.org>

Normally, PWM channel has fixed output until software request to change
its settings. There are some PWM devices which their outputs could be
changed autonomously according to a predefined pattern programmed in
hardware. Add pwm_output_type enum type to identify these two different
PWM types and add relevant helper functions to set and get PWM output
types and pattern.

Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
[konradybcio@gmail.com: Fast-forward from kernel 4.14 to 5.8]
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
Signed-off-by: Martin Botka <martin.botka1@gmail.com>
---
 drivers/pwm/core.c  | 26 +++++++++++++++++
 drivers/pwm/sysfs.c | 50 ++++++++++++++++++++++++++++++++
 include/linux/pwm.h | 69 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 004b2ea9b5fd..f3aa44106962 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -304,6 +304,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
 		pwm->state.polarity = polarity;
+		pwm->state.output_type = PWM_OUTPUT_FIXED;
 
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
@@ -627,6 +628,31 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 			pwm->state.polarity = state->polarity;
 		}
 
+		if (state->output_type != pwm->state.output_type) {
+			if (!pwm->chip->ops->set_output_type)
+				return -ENOTSUPP;
+
+			err = pwm->chip->ops->set_output_type(pwm->chip, pwm,
+										state->output_type);
+			if (err)
+				return err;
+
+			pwm->state.output_type = state->output_type;
+		}
+
+		if (state->output_pattern != pwm->state.output_pattern &&
+						state->output_pattern != NULL) {
+			if (!pwm->chip->ops->set_output_pattern)
+				return -ENOTSUPP;
+
+			err = pwm->chip->ops->set_output_pattern(pwm->chip,
+								pwm, state->output_pattern);
+			if (err)
+				return err;
+
+			pwm->state.output_pattern = state->output_pattern;
+		}
+
 		if (state->period != pwm->state.period ||
 		    state->duty_cycle != pwm->state.duty_cycle) {
 			err = chip->ops->config(pwm->chip, pwm,
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 2389b8669846..4ee1e81db0bc 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -215,11 +215,60 @@ static ssize_t capture_show(struct device *child,
 	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
 }
 
+static ssize_t output_type_show(struct device *child,
+							struct device_attribute *attr,
+							char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+	const char *output_type = "unknown";
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+	switch (state.output_type) {
+	case PWM_OUTPUT_FIXED:
+		output_type = "fixed";
+		break;
+	case PWM_OUTPUT_MODULATED:
+		output_type = "modulated";
+		break;
+	default:
+		break;
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", output_type);
+}
+
+static ssize_t output_type_store(struct device *child,
+								struct device_attribute *attr,
+								const char *buf, size_t size)
+{
+	struct pwm_export *export = child_to_pwm_export(child);
+	struct pwm_device *pwm = export->pwm;
+	struct pwm_state state;
+	int ret = -EINVAL;
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, &state);
+	if (sysfs_streq(buf, "fixed"))
+		state.output_type = PWM_OUTPUT_FIXED;
+	else if (sysfs_streq(buf, "modulated"))
+		state.output_type = PWM_OUTPUT_MODULATED;
+	else
+		goto unlock;
+
+	ret = pwm_apply_state(pwm, &state);
+unlock:
+	mutex_unlock(&export->lock);
+
+	return ret ? : size;
+}
+
 static DEVICE_ATTR_RW(period);
 static DEVICE_ATTR_RW(duty_cycle);
 static DEVICE_ATTR_RW(enable);
 static DEVICE_ATTR_RW(polarity);
 static DEVICE_ATTR_RO(capture);
+static DEVICE_ATTR_RW(output_type);
 
 static struct attribute *pwm_attrs[] = {
 	&dev_attr_period.attr,
@@ -227,6 +276,7 @@ static struct attribute *pwm_attrs[] = {
 	&dev_attr_enable.attr,
 	&dev_attr_polarity.attr,
 	&dev_attr_capture.attr,
+	&dev_attr_output_type.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(pwm);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 2635b2a55090..10a102efadc4 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -48,6 +48,29 @@ enum {
 	PWMF_EXPORTED = 1 << 1,
 };
 
+/*
+ * enum pwm_output_type - output type of the PWM signal
+ * @PWM_OUTPUT_FIXED: PWM output is fixed until a change request
+ * @PWM_OUTPUT_MODULATED: PWM output is modulated in hardware
+ * autonomously with a predefined pattern
+ */
+enum pwm_output_type {
+	PWM_OUTPUT_FIXED = 1 << 0,
+	PWM_OUTPUT_MODULATED = 1 << 1,
+};
+
+/*
+ * struct pwm_output_pattern - PWM duty pattern for MODULATED duty type
+ * @duty_pattern: PWM duty cycles in the pattern for duty modulation
+ * @num_entries: number of entries in the pattern
+ * @cycles_per_duty: number of PWM period cycles an entry stays at
+ */
+struct pwm_output_pattern {
+	unsigned int *duty_pattern;
+	unsigned int num_entries;
+	unsigned int cycles_per_duty;
+};
+
 /*
  * struct pwm_state - state of a PWM channel
  * @period: PWM period (in nanoseconds)
@@ -59,6 +82,8 @@ struct pwm_state {
 	unsigned int period;
 	unsigned int duty_cycle;
 	enum pwm_polarity polarity;
+	enum pwm_output_type output_type;
+	struct pwm_output_pattern *output_pattern;
 	bool enabled;
 };
 
@@ -146,6 +171,26 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 	return state.polarity;
 }
 
+static inline enum pwm_output_type pwm_get_output_type(
+				const struct pwm_device *pwm)
+{
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return state.output_type;
+}
+
+static inline struct pwm_output_pattern *pwm_get_output_pattern(
+				struct pwm_device *pwm)
+{
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return pwm->state.output_pattern ?: NULL;
+}
+
 static inline void pwm_get_args(const struct pwm_device *pwm,
 				struct pwm_args *args)
 {
@@ -254,6 +299,9 @@ pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int duty_cycle,
  * @set_polarity: configure the polarity of this PWM
  * @enable: enable PWM output toggling
  * @disable: disable PWM output toggling
+ * @get_output_type_supported: get the supported output type
+ * @set_output_type: set PWM output type
+ * @set_output_pattern: set the pattern for the modulated output
  */
 struct pwm_ops {
 	int (*request)(struct pwm_chip *chip, struct pwm_device *pwm);
@@ -273,6 +321,13 @@ struct pwm_ops {
 			    enum pwm_polarity polarity);
 	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
 	void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
+	int (*get_output_type_supported)(struct pwm_chip *chip,
+			    struct pwm_device *pwm);
+	int (*set_output_type)(struct pwm_chip *chip, struct pwm_device *pwm,
+			    enum pwm_output_type output_type);
+	int (*set_output_pattern)(struct pwm_chip *chip,
+			    struct pwm_device *pwm,
+			    struct pwm_output_pattern *output_pattern);
 };
 
 /**
@@ -318,6 +373,20 @@ void pwm_free(struct pwm_device *pwm);
 int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
 int pwm_adjust_config(struct pwm_device *pwm);
 
+/*
+ * 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;
+}
+
 /**
  * pwm_config() - change a PWM device configuration
  * @pwm: PWM device
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-24 21:36 [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Martin Botka
  2020-07-24 21:36 ` [PATCH RCC 1/6] pwm: Add different PWM output types support Martin Botka
@ 2020-07-24 21:36 ` Martin Botka
  2020-07-25 14:55   ` Martin Botka
  2020-07-26  9:04   ` Andy Shevchenko
  2020-07-24 21:36 ` [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module Martin Botka
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-24 21:36 UTC (permalink / raw)
  Cc: Fenglin Wu, Konrad Dybcio, Martin Botka, Jacek Anaszewski,
	Pavel Machek, Dan Murphy, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Lee Jones, linux-leds, devicetree,
	linux-kernel, linux-pwm

From: Fenglin Wu <fenglinw@codeaurora.org>

Currently, PWM core driver provides interfaces for configuring PWM
period and duty length in nanoseconds with an integer data type, so
the max period can be only set to ~2.147 seconds. Add interfaces which
can set PWM period and duty with u64 data type to remove this
limitation.

Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
[konradybcio@gmail.com: Fast-forward from kernel 4.14 to 5.8]
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
Signed-off-by: Martin Botka <martin.botka1@gmail.com>
---
 drivers/pwm/core.c  | 30 +++++++++++------
 drivers/pwm/sysfs.c |  6 ++--
 include/linux/pwm.h | 79 +++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 95 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index f3aa44106962..82411e3ccbbb 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -511,12 +511,12 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
 	    last->period > s2.period &&
 	    last->period <= state->period)
 		dev_warn(chip->dev,
-			 ".apply didn't pick the best available period (requested: %u, applied: %u, possible: %u)\n",
+			 ".apply didn't pick the best available period (requested: %llu, applied: %llu, possible: %llu)\n",
 			 state->period, s2.period, last->period);
 
 	if (state->enabled && state->period < s2.period)
 		dev_warn(chip->dev,
-			 ".apply is supposed to round down period (requested: %u, applied: %u)\n",
+			 ".apply is supposed to round down period (requested: %llu, applied: %llu)\n",
 			 state->period, s2.period);
 
 	if (state->enabled &&
@@ -525,14 +525,14 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
 	    last->duty_cycle > s2.duty_cycle &&
 	    last->duty_cycle <= state->duty_cycle)
 		dev_warn(chip->dev,
-			 ".apply didn't pick the best available duty cycle (requested: %u/%u, applied: %u/%u, possible: %u/%u)\n",
+			 ".apply didn't pick the best available duty cycle (requested: %llu/%llu, applied: %llu/%llu, possible: %llu/%llu)\n",
 			 state->duty_cycle, state->period,
 			 s2.duty_cycle, s2.period,
 			 last->duty_cycle, last->period);
 
 	if (state->enabled && state->duty_cycle < s2.duty_cycle)
 		dev_warn(chip->dev,
-			 ".apply is supposed to round down duty_cycle (requested: %u/%u, applied: %u/%u)\n",
+			 ".apply is supposed to round down duty_cycle (requested: %llu/%llu, applied: %llu/%llu)\n",
 			 state->duty_cycle, state->period,
 			 s2.duty_cycle, s2.period);
 
@@ -559,7 +559,7 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
 	    (s1.enabled && s1.period != last->period) ||
 	    (s1.enabled && s1.duty_cycle != last->duty_cycle)) {
 		dev_err(chip->dev,
-			".apply is not idempotent (ena=%d pol=%d %u/%u) -> (ena=%d pol=%d %u/%u)\n",
+			".apply is not idempotent (ena=%d pol=%d %llu/%llu) -> (ena=%d pol=%d %llu/%llu)\n",
 			s1.enabled, s1.polarity, s1.duty_cycle, s1.period,
 			last->enabled, last->polarity, last->duty_cycle,
 			last->period);
@@ -655,9 +655,19 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 
 		if (state->period != pwm->state.period ||
 		    state->duty_cycle != pwm->state.duty_cycle) {
-			err = chip->ops->config(pwm->chip, pwm,
-						state->duty_cycle,
-						state->period);
+			if (pwm->chip->ops->config_extend) {
+					err = pwm->chip->ops->config_extend(pwm->chip,
+								pwm, state->duty_cycle,
+								state->period);
+			} else {
+				if (state->period > UINT_MAX)
+					pr_warn("period %llu duty_cycle %llu will be truncated\n",
+								state->period,
+								state->duty_cycle);
+				err = pwm->chip->ops->config(pwm->chip, pwm,
+								state->duty_cycle,
+								state->period);
+			}
 			if (err)
 				return err;
 
@@ -1310,8 +1320,8 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 		if (state.enabled)
 			seq_puts(s, " enabled");
 
-		seq_printf(s, " period: %u ns", state.period);
-		seq_printf(s, " duty: %u ns", state.duty_cycle);
+		seq_printf(s, " period: %llu ns", state.period);
+		seq_printf(s, " duty: %llu ns", state.duty_cycle);
 		seq_printf(s, " polarity: %s",
 			   state.polarity ? "inverse" : "normal");
 
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 4ee1e81db0bc..f9d7ebfb48f4 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -42,7 +42,7 @@ static ssize_t period_show(struct device *child,
 
 	pwm_get_state(pwm, &state);
 
-	return sprintf(buf, "%u\n", state.period);
+	return sprintf(buf, "%llu\n", state.period);
 }
 
 static ssize_t period_store(struct device *child,
@@ -77,7 +77,7 @@ static ssize_t duty_cycle_show(struct device *child,
 
 	pwm_get_state(pwm, &state);
 
-	return sprintf(buf, "%u\n", state.duty_cycle);
+	return sprintf(buf, "%llu\n", state.duty_cycle);
 }
 
 static ssize_t duty_cycle_store(struct device *child,
@@ -212,7 +212,7 @@ static ssize_t capture_show(struct device *child,
 	if (ret)
 		return ret;
 
-	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
+	return sprintf(buf, "%llu %llu\n", result.period, result.duty_cycle);
 }
 
 static ssize_t output_type_show(struct device *child,
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 10a102efadc4..ab235007287f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -39,7 +39,7 @@ enum pwm_polarity {
  * current PWM hardware state.
  */
 struct pwm_args {
-	unsigned int period;
+	u64 period;
 	enum pwm_polarity polarity;
 };
 
@@ -66,9 +66,9 @@ enum pwm_output_type {
  * @cycles_per_duty: number of PWM period cycles an entry stays at
  */
 struct pwm_output_pattern {
-	unsigned int *duty_pattern;
+	u64 *duty_pattern;
 	unsigned int num_entries;
-	unsigned int cycles_per_duty;
+	u64 cycles_per_duty;
 };
 
 /*
@@ -79,8 +79,8 @@ struct pwm_output_pattern {
  * @enabled: PWM enabled status
  */
 struct pwm_state {
-	unsigned int period;
-	unsigned int duty_cycle;
+	u64 period;
+	u64 duty_cycle;
 	enum pwm_polarity polarity;
 	enum pwm_output_type output_type;
 	struct pwm_output_pattern *output_pattern;
@@ -138,12 +138,30 @@ static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
 		pwm->state.period = period;
 }
 
+static inline void pwm_set_period_extend(struct pwm_device *pwm, u64 period)
+{
+	if (pwm)
+		pwm->state.period = period;
+}
+
 static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
 {
 	struct pwm_state state;
 
 	pwm_get_state(pwm, &state);
 
+	if (state.period > UINT_MAX)
+		pr_warn("PWM period %llu is truncated\n", state.period);
+
+	return (unsigned int)state.period;
+}
+
+static inline u64 pwm_get_period_extend(const struct pwm_device *pwm)
+{
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
 	return state.period;
 }
 
@@ -153,12 +171,30 @@ static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
 		pwm->state.duty_cycle = duty;
 }
 
+static inline void pwm_set_duty_cycle_extend(struct pwm_device *pwm, u64 duty)
+{
+	if (pwm)
+		pwm->state.duty_cycle = duty;
+}
+
 static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm)
 {
 	struct pwm_state state;
 
 	pwm_get_state(pwm, &state);
 
+	if (state.duty_cycle > UINT_MAX)
+		pr_warn("PWM duty cycle %llu is truncated\n", state.duty_cycle);
+
+	return (unsigned int)state.duty_cycle;
+}
+
+static inline u64 pwm_get_duty_cycle_extend(const struct pwm_device *pwm)
+{
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
 	return state.duty_cycle;
 }
 
@@ -296,6 +332,8 @@ pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int duty_cycle,
  *	       registered.
  * @owner: helps prevent removal of modules exporting active PWMs
  * @config: configure duty cycles and period length for this PWM
+ * @config_extend: configure duty cycles and period length for this
+ *	       PWM with u64 data type
  * @set_polarity: configure the polarity of this PWM
  * @enable: enable PWM output toggling
  * @disable: disable PWM output toggling
@@ -317,6 +355,8 @@ struct pwm_ops {
 	/* Only used by legacy drivers */
 	int (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
 		      int duty_ns, int period_ns);
+	int (*config_extend)(struct pwm_chip *chip, struct pwm_device *pwm,
+		      u64 duty_ns, u64 period_ns);
 	int (*set_polarity)(struct pwm_chip *chip, struct pwm_device *pwm,
 			    enum pwm_polarity polarity);
 	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
@@ -362,8 +402,8 @@ struct pwm_chip {
  * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
  */
 struct pwm_capture {
-	unsigned int period;
-	unsigned int duty_cycle;
+	u64 period;
+	u64 duty_cycle;
 };
 
 #if IS_ENABLED(CONFIG_PWM)
@@ -415,6 +455,31 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
 	return pwm_apply_state(pwm, &state);
 }
 
+/*
+ * pwm_config_extend() - change PWM period and duty length with u64 data type
+ * @pwm: PWM device
+ * @duty_ns: "on" time (in nanoseconds)
+ * @period_ns: duration (in nanoseconds) of one cycle
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+static inline int pwm_config_extend(struct pwm_device *pwm, u64 duty_ns,
+			     u64 period_ns)
+{
+	struct pwm_state state;
+
+	if (!pwm)
+		return -EINVAL;
+
+	pwm_get_state(pwm, &state);
+	if (state.duty_cycle == duty_ns && state.period == period_ns)
+		return 0;
+
+	state.duty_cycle = duty_ns;
+	state.period = period_ns;
+	return pwm_apply_state(pwm, &state);
+}
+
 /**
  * pwm_enable() - start a PWM output toggling
  * @pwm: PWM device
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module
  2020-07-24 21:36 [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Martin Botka
  2020-07-24 21:36 ` [PATCH RCC 1/6] pwm: Add different PWM output types support Martin Botka
  2020-07-24 21:36 ` [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length Martin Botka
@ 2020-07-24 21:36 ` Martin Botka
  2020-07-27 20:09   ` Uwe Kleine-König
  2020-07-24 21:36 ` [PATCH RFC 4/6] leds: leds-qti-tri-led: Add LED driver for QTI TRI_LED module Martin Botka
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 31+ messages in thread
From: Martin Botka @ 2020-07-24 21:36 UTC (permalink / raw)
  Cc: Fenglin Wu, Martin Botka, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Lee Jones, linux-leds, devicetree, linux-kernel, linux-pwm

From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.org>
[martin.botka1@gmail.com: Fast-forward the driver from kernel 4.14 to 5.8]
Signed-off-by: Martin Botka <martin.botka1@gmail.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
 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__
+
+#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 };
+
+struct lpg_ramp_config {
+	u16 step_ms;
+	u8 pause_hi_count;
+	u8 pause_lo_count;
+	u8 hi_idx;
+	u8 lo_idx;
+	bool ramp_dir_low_to_hi;
+	bool pattern_repeat;
+	bool toggle;
+	u32 *pattern;
+	u32 pattern_length;
+};
+
+struct lpg_pwm_config {
+	u32 pwm_size;
+	u32 pwm_clk;
+	u32 prediv;
+	u32 clk_exp;
+	u16 pwm_value;
+	u64 best_period_ns;
+};
+
+struct qpnp_lpg_lut {
+	struct qpnp_lpg_chip *chip;
+	struct mutex lock;
+	u32 reg_base;
+	u32 *pattern; /* patterns in percentage */
+};
+
+struct qpnp_lpg_channel {
+	struct qpnp_lpg_chip *chip;
+	struct lpg_pwm_config pwm_config;
+	struct lpg_ramp_config ramp_config;
+	u32 lpg_idx;
+	u32 reg_base;
+	u32 max_pattern_length;
+	u8 src_sel;
+	u8 subtype;
+	bool lut_written;
+	u64 current_period_ns;
+	u64 current_duty_ns;
+};
+
+struct qpnp_lpg_chip {
+	struct pwm_chip pwm_chip;
+	struct regmap *regmap;
+	struct device *dev;
+	struct qpnp_lpg_channel *lpgs;
+	struct qpnp_lpg_lut *lut;
+	struct mutex bus_lock;
+	u32 *lpg_group;
+	u32 num_lpgs;
+};
+
+static int qpnp_lpg_read(struct qpnp_lpg_channel *lpg, u16 addr, u8 *val)
+{
+	int rc;
+	unsigned int tmp;
+
+	mutex_lock(&lpg->chip->bus_lock);
+	rc = regmap_read(lpg->chip->regmap, lpg->reg_base + addr, &tmp);
+	if (rc < 0)
+		dev_err(lpg->chip->dev, "Read addr 0x%x failed, rc=%d\n",
+			lpg->reg_base + addr, rc);
+	else
+		*val = (u8)tmp;
+	mutex_unlock(&lpg->chip->bus_lock);
+
+	return rc;
+}
+
+static int qpnp_lpg_write(struct qpnp_lpg_channel *lpg, u16 addr, u8 val)
+{
+	int rc;
+
+	mutex_lock(&lpg->chip->bus_lock);
+	rc = regmap_write(lpg->chip->regmap, lpg->reg_base + addr, val);
+	if (rc < 0)
+		dev_err(lpg->chip->dev,
+			"Write addr 0x%x with value %d failed, rc=%d\n",
+			lpg->reg_base + addr, val, rc);
+	mutex_unlock(&lpg->chip->bus_lock);
+
+	return rc;
+}
+
+static int qpnp_lpg_masked_write(struct qpnp_lpg_channel *lpg, u16 addr,
+				 u8 mask, u8 val)
+{
+	int rc;
+
+	mutex_lock(&lpg->chip->bus_lock);
+	rc = regmap_update_bits(lpg->chip->regmap, lpg->reg_base + addr, mask,
+				val);
+	if (rc < 0)
+		dev_err(lpg->chip->dev,
+			"Update addr 0x%x to val 0x%x with mask 0x%x failed, rc=%d\n",
+			lpg->reg_base + addr, val, mask, rc);
+	mutex_unlock(&lpg->chip->bus_lock);
+
+	return rc;
+}
+
+static int qpnp_lut_write(struct qpnp_lpg_lut *lut, u16 addr, u8 val)
+{
+	int rc;
+
+	mutex_lock(&lut->chip->bus_lock);
+	rc = regmap_write(lut->chip->regmap, lut->reg_base + addr, val);
+	if (rc < 0)
+		dev_err(lut->chip->dev,
+			"Write addr 0x%x with value %d failed, rc=%d\n",
+			lut->reg_base + addr, val, rc);
+	mutex_unlock(&lut->chip->bus_lock);
+
+	return rc;
+}
+
+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);
+
+	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);
+	return rc;
+}
+
+static int qpnp_lpg_set_pwm_config(struct qpnp_lpg_channel *lpg)
+{
+	int rc;
+	u8 val, mask, shift;
+	int pwm_size_idx, pwm_clk_idx, prediv_idx, clk_exp_idx;
+
+	pwm_size_idx = __find_index_in_array(lpg->pwm_config.pwm_size, pwm_size,
+					     ARRAY_SIZE(pwm_size));
+	pwm_clk_idx = __find_index_in_array(
+		lpg->pwm_config.pwm_clk, clk_freq_hz, ARRAY_SIZE(clk_freq_hz));
+	prediv_idx = __find_index_in_array(lpg->pwm_config.prediv, clk_prediv,
+					   ARRAY_SIZE(clk_prediv));
+	clk_exp_idx =
+		__find_index_in_array(lpg->pwm_config.clk_exp, pwm_exponent,
+				      ARRAY_SIZE(pwm_exponent));
+
+	if (pwm_size_idx < 0 || pwm_clk_idx < 0 || prediv_idx < 0 ||
+	    clk_exp_idx < 0)
+		return -EINVAL;
+
+	/* pwm_clk_idx is 1 bit lower than the register value */
+	pwm_clk_idx += 1;
+	if (lpg->subtype == SUBTYPE_PWM) {
+		shift = LPG_PWM_SIZE_PWM_SHIFT;
+		mask = LPG_PWM_SIZE_PWM_MASK;
+	} else {
+		shift = LPG_PWM_SIZE_LPG_SHIFT;
+		mask = LPG_PWM_SIZE_LPG_MASK;
+	}
+
+	val = pwm_size_idx << shift | pwm_clk_idx;
+	mask |= LPG_PWM_CLK_FREQ_SEL_MASK;
+	rc = qpnp_lpg_masked_write(lpg, REG_LPG_PWM_SIZE_CLK, mask, val);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Write LPG_PWM_SIZE_CLK failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	val = prediv_idx << LPG_PWM_FREQ_PREDIV_SHIFT | clk_exp_idx;
+	mask = LPG_PWM_FREQ_PREDIV_MASK | LPG_PWM_FREQ_EXPONENT_MASK;
+	rc = qpnp_lpg_masked_write(lpg, REG_LPG_PWM_FREQ_PREDIV_CLK, mask, val);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Write LPG_PWM_FREQ_PREDIV_CLK failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	if (lpg->src_sel == LUT_PATTERN)
+		return 0;
+
+	val = lpg->pwm_config.pwm_value >> 8;
+	mask = LPG_PWM_VALUE_MSB_MASK;
+	rc = qpnp_lpg_masked_write(lpg, REG_LPG_PWM_VALUE_MSB, mask, val);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Write LPG_PWM_VALUE_MSB failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	val = lpg->pwm_config.pwm_value & LPG_PWM_VALUE_LSB_MASK;
+	rc = qpnp_lpg_write(lpg, REG_LPG_PWM_VALUE_LSB, val);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Write LPG_PWM_VALUE_LSB failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	val = LPG_PWM_VALUE_SYNC;
+	rc = qpnp_lpg_write(lpg, REG_LPG_PWM_SYNC, val);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev, "Write LPG_PWM_SYNC failed, rc=%d\n",
+			rc);
+		return rc;
+	}
+
+	return rc;
+}
+
+static int qpnp_lpg_set_lut_pattern(struct qpnp_lpg_channel *lpg,
+				    unsigned int *pattern, unsigned int length)
+{
+	struct qpnp_lpg_lut *lut = lpg->chip->lut;
+	int i, rc = 0;
+	u16 full_duty_value, pwm_values[LPG_LUT_COUNT_MAX + 1] = { 0 };
+	u8 lsb, msb, addr;
+
+	if (length > lpg->max_pattern_length) {
+		dev_err(lpg->chip->dev,
+			"new pattern length (%d) larger than predefined (%d)\n",
+			length, lpg->max_pattern_length);
+		return -EINVAL;
+	}
+
+	/* Program LUT pattern */
+	mutex_lock(&lut->lock);
+	addr = REG_LPG_LUT_1_LSB + lpg->ramp_config.lo_idx * 2;
+	for (i = 0; i < length; i++) {
+		full_duty_value = 1 << lpg->pwm_config.pwm_size;
+		pwm_values[i] = pattern[i] * full_duty_value / 100;
+
+		if (unlikely(pwm_values[i] > full_duty_value)) {
+			dev_err(lpg->chip->dev,
+				"PWM value %d exceed the max %d\n",
+				pwm_values[i], full_duty_value);
+			rc = -EINVAL;
+			goto unlock;
+		}
+
+		if (pwm_values[i] == full_duty_value)
+			pwm_values[i] = full_duty_value - 1;
+
+		lsb = pwm_values[i] & 0xff;
+		msb = pwm_values[i] >> 8;
+		rc = qpnp_lut_write(lut, addr++, lsb);
+		if (rc < 0) {
+			dev_err(lpg->chip->dev,
+				"Write NO.%d LUT pattern LSB (%d) failed, rc=%d",
+				i, lsb, rc);
+			goto unlock;
+		}
+
+		rc = qpnp_lut_masked_write(lut, addr++, LPG_LUT_VALUE_MSB_MASK,
+					   msb);
+		if (rc < 0) {
+			dev_err(lpg->chip->dev,
+				"Write NO.%d LUT pattern MSB (%d) failed, rc=%d",
+				i, msb, rc);
+			goto unlock;
+		}
+	}
+	lpg->ramp_config.pattern_length = length;
+unlock:
+	mutex_unlock(&lut->lock);
+
+	return rc;
+}
+
+static int qpnp_lpg_set_ramp_config(struct qpnp_lpg_channel *lpg)
+{
+	struct lpg_ramp_config *ramp = &lpg->ramp_config;
+	u8 lsb, msb, addr, mask, val;
+	int rc = 0;
+
+	/* Set ramp step duration */
+	lsb = ramp->step_ms & 0xff;
+	msb = ramp->step_ms >> 8;
+	addr = REG_LPG_RAMP_STEP_DURATION_LSB;
+	rc = qpnp_lpg_write(lpg, addr, lsb);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Write RAMP_STEP_DURATION_LSB failed, rc=%d\n", rc);
+		return rc;
+	}
+	rc = qpnp_lpg_write(lpg, addr + 1, msb);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Write RAMP_STEP_DURATION_MSB failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	/* Set hi_idx and lo_idx */
+	rc = qpnp_lpg_masked_write(lpg, REG_LPG_HI_INDEX, LPG_HI_LO_IDX_MASK,
+				   ramp->hi_idx);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev, "Write LPG_HI_IDX failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	rc = qpnp_lpg_masked_write(lpg, REG_LPG_LO_INDEX, LPG_HI_LO_IDX_MASK,
+				   ramp->lo_idx);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev, "Write LPG_LO_IDX failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	/* Set pause_hi/lo_count */
+	rc = qpnp_lpg_write(lpg, REG_LPG_PAUSE_HI_MULTIPLIER,
+			    ramp->pause_hi_count);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Write LPG_PAUSE_HI_MULTIPLIER failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	rc = qpnp_lpg_write(lpg, REG_LPG_PAUSE_LO_MULTIPLIER,
+			    ramp->pause_lo_count);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Write LPG_PAUSE_LO_MULTIPLIER failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	/* Set LPG_PATTERN_CONFIG */
+	addr = REG_LPG_PATTERN_CONFIG;
+	mask = LPG_PATTERN_EN_PAUSE_LO | LPG_PATTERN_EN_PAUSE_HI |
+	       LPG_PATTERN_RAMP_TOGGLE | LPG_PATTERN_REPEAT |
+	       LPG_PATTERN_RAMP_LO_TO_HI;
+	val = 0;
+	if (ramp->pause_lo_count != 0)
+		val |= LPG_PATTERN_EN_PAUSE_LO;
+	if (ramp->pause_hi_count != 0)
+		val |= LPG_PATTERN_EN_PAUSE_HI;
+	if (ramp->ramp_dir_low_to_hi)
+		val |= LPG_PATTERN_RAMP_LO_TO_HI;
+	if (ramp->pattern_repeat)
+		val |= LPG_PATTERN_REPEAT;
+	if (ramp->toggle)
+		val |= LPG_PATTERN_RAMP_TOGGLE;
+
+	rc = qpnp_lpg_masked_write(lpg, addr, mask, val);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Write LPG_PATTERN_CONFIG failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	return rc;
+}
+
+static void __qpnp_lpg_calc_pwm_period(u64 period_ns,
+				       struct lpg_pwm_config *pwm_config)
+{
+	struct lpg_pwm_config configs[NUM_PWM_SIZE];
+	int i, j, m, n;
+	u64 tmp1, tmp2;
+	u64 clk_period_ns = 0, pwm_clk_period_ns;
+	u64 clk_delta_ns = U64_MAX, min_clk_delta_ns = U64_MAX;
+	u64 pwm_period_delta = U64_MAX, min_pwm_period_delta = U64_MAX;
+	int pwm_size_step;
+
+	/*
+	 *              (2^pwm_size) * (2^pwm_exp) * prediv * NSEC_PER_SEC
+	 * pwm_period = ---------------------------------------------------
+	 *                               clk_freq_hz
+	 *
+	 * Searching the closest settings for the requested PWM period.
+	 */
+	for (n = 0; n < ARRAY_SIZE(pwm_size); n++) {
+		pwm_clk_period_ns = period_ns >> pwm_size[n];
+		for (i = ARRAY_SIZE(clk_freq_hz) - 1; i >= 0; i--) {
+			for (j = 0; j < ARRAY_SIZE(clk_prediv); j++) {
+				for (m = 0; m < ARRAY_SIZE(pwm_exponent); m++) {
+					tmp1 = 1 << pwm_exponent[m];
+					tmp1 *= clk_prediv[j];
+					tmp2 = NSEC_PER_SEC;
+					do_div(tmp2, clk_freq_hz[i]);
+
+					clk_period_ns = tmp1 * tmp2;
+
+					clk_delta_ns = abs(pwm_clk_period_ns -
+							   clk_period_ns);
+					/*
+					 * Find the closest setting for
+					 * PWM frequency predivide value
+					 */
+					if (clk_delta_ns < min_clk_delta_ns) {
+						min_clk_delta_ns = clk_delta_ns;
+						configs[n].pwm_clk =
+							clk_freq_hz[i];
+						configs[n].prediv =
+							clk_prediv[j];
+						configs[n].clk_exp =
+							pwm_exponent[m];
+						configs[n].pwm_size =
+							pwm_size[n];
+						configs[n].best_period_ns =
+							clk_period_ns;
+					}
+				}
+			}
+		}
+
+		configs[n].best_period_ns *= 1 << pwm_size[n];
+		/* Find the closest setting for PWM period */
+		pwm_period_delta = min_clk_delta_ns << pwm_size[n];
+		if (pwm_period_delta < min_pwm_period_delta) {
+			min_pwm_period_delta = pwm_period_delta;
+			memcpy(pwm_config, &configs[n],
+			       sizeof(struct lpg_pwm_config));
+		}
+	}
+
+	/* Larger PWM size can achieve better resolution for PWM duty */
+	for (n = ARRAY_SIZE(pwm_size) - 1; n > 0; n--) {
+		if (pwm_config->pwm_size >= pwm_size[n])
+			break;
+		pwm_size_step = pwm_size[n] - pwm_config->pwm_size;
+		if (pwm_config->clk_exp >= pwm_size_step) {
+			pwm_config->pwm_size = pwm_size[n];
+			pwm_config->clk_exp -= pwm_size_step;
+		}
+	}
+	pr_debug(
+		"PWM setting for period_ns %llu: pwm_clk = %dHZ, prediv = %d, exponent = %d, pwm_size = %d\n",
+		period_ns, pwm_config->pwm_clk, pwm_config->prediv,
+		pwm_config->clk_exp, pwm_config->pwm_size);
+	pr_debug("Actual period: %lluns\n", pwm_config->best_period_ns);
+}
+
+static void __qpnp_lpg_calc_pwm_duty(u64 period_ns, u64 duty_ns,
+				     struct lpg_pwm_config *pwm_config)
+{
+	u16 pwm_value, max_pwm_value;
+	u64 tmp;
+
+	tmp = (u64)duty_ns << pwm_config->pwm_size;
+	pwm_value = (u16)div64_u64(tmp, period_ns);
+
+	max_pwm_value = (1 << pwm_config->pwm_size) - 1;
+	if (pwm_value > max_pwm_value)
+		pwm_value = max_pwm_value;
+	pwm_config->pwm_value = pwm_value;
+}
+
+static int qpnp_lpg_config(struct qpnp_lpg_channel *lpg, u64 duty_ns,
+			   u64 period_ns)
+{
+	int rc;
+
+	if (duty_ns > period_ns) {
+		dev_err(lpg->chip->dev,
+			"Duty %lluns is larger than period %lluns\n", duty_ns,
+			period_ns);
+		return -EINVAL;
+	}
+
+	if (period_ns != lpg->current_period_ns) {
+		__qpnp_lpg_calc_pwm_period(period_ns, &lpg->pwm_config);
+
+		/* program LUT if PWM period is changed */
+		if (lpg->src_sel == LUT_PATTERN) {
+			rc = qpnp_lpg_set_lut_pattern(
+				lpg, lpg->ramp_config.pattern,
+				lpg->ramp_config.pattern_length);
+			if (rc < 0) {
+				dev_err(lpg->chip->dev,
+					"set LUT pattern failed for LPG%d, rc=%d\n",
+					lpg->lpg_idx, rc);
+				return rc;
+			}
+			lpg->lut_written = true;
+		}
+	}
+
+	if (period_ns != lpg->current_period_ns ||
+	    duty_ns != lpg->current_duty_ns)
+		__qpnp_lpg_calc_pwm_duty(period_ns, duty_ns, &lpg->pwm_config);
+
+	rc = qpnp_lpg_set_pwm_config(lpg);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev,
+			"Config PWM failed for channel %d, rc=%d\n",
+			lpg->lpg_idx, rc);
+		return rc;
+	}
+
+	lpg->current_period_ns = period_ns;
+	lpg->current_duty_ns = duty_ns;
+
+	return rc;
+}
+
+static int qpnp_lpg_pwm_config(struct pwm_chip *pwm_chip,
+			       struct pwm_device *pwm, int duty_ns,
+			       int period_ns)
+{
+	struct qpnp_lpg_channel *lpg;
+
+	lpg = pwm_dev_to_qpnp_lpg(pwm_chip, pwm);
+	if (lpg == NULL) {
+		dev_err(pwm_chip->dev, "lpg not found\n");
+		return -ENODEV;
+	}
+
+	return qpnp_lpg_config(lpg, (u64)duty_ns, (u64)period_ns);
+}
+
+static int qpnp_lpg_pwm_config_extend(struct pwm_chip *pwm_chip,
+		struct pwm_device *pwm, u64 duty_ns, u64 period_ns)
+{
+	struct qpnp_lpg_channel *lpg;
+
+	lpg = pwm_dev_to_qpnp_lpg(pwm_chip, pwm);
+	if (lpg == NULL) {
+		dev_err(pwm_chip->dev, "lpg not found\n");
+		return -ENODEV;
+	}
+
+	return qpnp_lpg_config(lpg, duty_ns, period_ns);
+}
+
+static int qpnp_lpg_pwm_src_enable(struct qpnp_lpg_channel *lpg, bool en)
+{
+	struct qpnp_lpg_chip *chip = lpg->chip;
+	struct qpnp_lpg_lut *lut = chip->lut;
+	struct pwm_device *pwm;
+	u8 mask, val;
+	int i, lpg_idx, rc;
+
+	mask = LPG_PWM_SRC_SELECT_MASK | LPG_EN_LPG_OUT_BIT |
+	       LPG_EN_RAMP_GEN_MASK;
+	val = lpg->src_sel << LPG_PWM_SRC_SELECT_SHIFT;
+
+	if (lpg->src_sel == LUT_PATTERN)
+		val |= 1 << LPG_EN_RAMP_GEN_SHIFT;
+
+	if (en)
+		val |= 1 << LPG_EN_LPG_OUT_SHIFT;
+
+	rc = qpnp_lpg_masked_write(lpg, REG_LPG_ENABLE_CONTROL, mask, val);
+	if (rc < 0) {
+		dev_err(chip->dev, "Write LPG_ENABLE_CONTROL failed, rc=%d\n",
+			rc);
+		return rc;
+	}
+
+	if (lpg->src_sel == LUT_PATTERN && en) {
+		val = 1 << lpg->lpg_idx;
+		for (i = 0; i < chip->num_lpgs; i++) {
+			if (chip->lpg_group == NULL)
+				break;
+			if (chip->lpg_group[i] == 0)
+				break;
+			lpg_idx = chip->lpg_group[i] - 1;
+			pwm = &chip->pwm_chip.pwms[lpg_idx];
+			if ((pwm_get_output_type(pwm) == PWM_OUTPUT_MODULATED)
+						&& pwm_is_enabled(pwm)) {
+				rc = qpnp_lpg_masked_write(&chip->lpgs[lpg_idx],
+						REG_LPG_ENABLE_CONTROL,
+						LPG_EN_LPG_OUT_BIT, 0);
+				if (rc < 0)
+					break;
+				rc = qpnp_lpg_masked_write(&chip->lpgs[lpg_idx],
+						REG_LPG_ENABLE_CONTROL,
+						LPG_EN_LPG_OUT_BIT,
+						LPG_EN_LPG_OUT_BIT);
+				if (rc < 0)
+					break;
+				val |= 1 << lpg_idx;
+			}
+		}
+		mutex_lock(&lut->lock);
+		rc = qpnp_lut_write(lut, REG_LPG_LUT_RAMP_CONTROL, val);
+		if (rc < 0)
+			dev_err(chip->dev,
+				"Write LPG_LUT_RAMP_CONTROL failed, rc=%d\n",
+				rc);
+		mutex_unlock(&lut->lock);
+	}
+
+	return rc;
+}
+
+static int qpnp_lpg_pwm_set_output_type(struct pwm_chip *pwm_chip,
+		struct pwm_device *pwm, enum pwm_output_type output_type)
+{
+	struct qpnp_lpg_channel *lpg;
+	enum lpg_src src_sel;
+	int rc;
+
+	lpg = pwm_dev_to_qpnp_lpg(pwm_chip, pwm);
+	if (lpg == NULL) {
+		dev_err(pwm_chip->dev, "lpg not found\n");
+		return -ENODEV;
+	}
+
+	if (lpg->chip->lut == NULL) {
+		pr_debug("lpg%d only support PWM mode\n", lpg->lpg_idx);
+		return 0;
+	}
+
+	src_sel = (output_type == PWM_OUTPUT_MODULATED) ?
+				LUT_PATTERN : PWM_VALUE;
+	if (src_sel == lpg->src_sel)
+		return 0;
+
+	if (src_sel == LUT_PATTERN) {
+		/* program LUT if it's never been programmed */
+		if (!lpg->lut_written) {
+			rc = qpnp_lpg_set_lut_pattern(lpg,
+					lpg->ramp_config.pattern,
+					lpg->ramp_config.pattern_length);
+			if (rc < 0) {
+				dev_err(lpg->chip->dev, "set LUT pattern failed for LPG%d, rc=%d\n",
+						lpg->lpg_idx, rc);
+				return rc;
+			}
+			lpg->lut_written = true;
+		}
+
+		rc = qpnp_lpg_set_ramp_config(lpg);
+		if (rc < 0) {
+			dev_err(pwm_chip->dev, "Config LPG%d ramping failed, rc=%d\n",
+					lpg->lpg_idx, rc);
+			return rc;
+		}
+	}
+
+	lpg->src_sel = src_sel;
+
+	if (pwm_is_enabled(pwm)) {
+		rc = qpnp_lpg_pwm_src_enable(lpg, true);
+		if (rc < 0) {
+			dev_err(pwm_chip->dev, "Enable PWM output failed for channel %d, rc=%d\n",
+					lpg->lpg_idx, rc);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static int qpnp_lpg_pwm_set_output_pattern(struct pwm_chip *pwm_chip,
+	struct pwm_device *pwm, struct pwm_output_pattern *output_pattern)
+{
+	struct qpnp_lpg_channel *lpg;
+	u64 period_ns, duty_ns, tmp;
+	u32 *percentages;
+	int rc = 0, i;
+
+	lpg = pwm_dev_to_qpnp_lpg(pwm_chip, pwm);
+	if (lpg == NULL) {
+		dev_err(pwm_chip->dev, "lpg not found\n");
+		return -ENODEV;
+	}
+
+	if (output_pattern->num_entries > lpg->max_pattern_length) {
+		dev_err(lpg->chip->dev, "pattern length %d shouldn't exceed %d\n",
+				output_pattern->num_entries,
+				lpg->max_pattern_length);
+		return -EINVAL;
+	}
+
+	percentages = kcalloc(output_pattern->num_entries,
+				sizeof(u32), GFP_KERNEL);
+	if (!percentages)
+		return -ENOMEM;
+
+	period_ns = pwm_get_period_extend(pwm);
+	for (i = 0; i < output_pattern->num_entries; i++) {
+		duty_ns = output_pattern->duty_pattern[i];
+		if (duty_ns > period_ns) {
+			dev_err(lpg->chip->dev, "duty %lluns is larger than period %lluns\n",
+					duty_ns, period_ns);
+			goto err;
+		}
+		/* Translate the pattern in duty_ns to percentage */
+		tmp = (u64)duty_ns * 100;
+		percentages[i] = (u32)div64_u64(tmp, period_ns);
+	}
+
+	rc = qpnp_lpg_set_lut_pattern(lpg, percentages,
+			output_pattern->num_entries);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev, "Set LUT pattern failed for LPG%d, rc=%d\n",
+				lpg->lpg_idx, rc);
+		goto err;
+	}
+
+	lpg->lut_written = true;
+	memcpy(lpg->ramp_config.pattern, percentages,
+			output_pattern->num_entries);
+	lpg->ramp_config.hi_idx = lpg->ramp_config.lo_idx +
+				output_pattern->num_entries - 1;
+
+	tmp = (u64)output_pattern->cycles_per_duty * period_ns;
+	do_div(tmp, NSEC_PER_MSEC);
+	lpg->ramp_config.step_ms = (u16)tmp;
+
+	rc = qpnp_lpg_set_ramp_config(lpg);
+	if (rc < 0)
+		dev_err(pwm_chip->dev, "Config LPG%d ramping failed, rc=%d\n",
+				lpg->lpg_idx, rc);
+err:
+	kfree(percentages);
+
+	return rc;
+}
+
+
+static int qpnp_lpg_pwm_enable(struct pwm_chip *pwm_chip,
+			       struct pwm_device *pwm)
+{
+	struct qpnp_lpg_channel *lpg;
+	int rc = 0;
+
+	lpg = pwm_dev_to_qpnp_lpg(pwm_chip, pwm);
+	if (lpg == NULL) {
+		dev_err(pwm_chip->dev, "lpg not found\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Update PWM_VALUE_SYNC to make sure PWM_VALUE
+	 * will be updated everytime before enabling.
+	 */
+	if (lpg->src_sel == PWM_VALUE) {
+		rc = qpnp_lpg_write(lpg, REG_LPG_PWM_SYNC, LPG_PWM_VALUE_SYNC);
+		if (rc < 0) {
+			dev_err(lpg->chip->dev,
+				"Write LPG_PWM_SYNC failed, rc=%d\n", rc);
+			return rc;
+		}
+	}
+
+	rc = qpnp_lpg_set_glitch_removal(lpg, true);
+	if (rc < 0) {
+		dev_err(lpg->chip->dev, "Enable glitch-removal failed, rc=%d\n",
+			rc);
+		return rc;
+	}
+
+	rc = qpnp_lpg_pwm_src_enable(lpg, true);
+	if (rc < 0)
+		dev_err(pwm_chip->dev,
+			"Enable PWM output failed for channel %d, rc=%d\n",
+			lpg->lpg_idx, rc);
+
+	return rc;
+}
+
+static void qpnp_lpg_pwm_disable(struct pwm_chip *pwm_chip,
+				 struct pwm_device *pwm)
+{
+	struct qpnp_lpg_channel *lpg;
+	int rc;
+
+	lpg = pwm_dev_to_qpnp_lpg(pwm_chip, pwm);
+	if (lpg == NULL) {
+		dev_err(pwm_chip->dev, "lpg not found\n");
+		return;
+	}
+
+	rc = qpnp_lpg_pwm_src_enable(lpg, false);
+	if (rc < 0) {
+		dev_err(pwm_chip->dev,
+			"Disable PWM output failed for channel %d, rc=%d\n",
+			lpg->lpg_idx, rc);
+		return;
+	}
+
+	rc = qpnp_lpg_set_glitch_removal(lpg, false);
+	if (rc < 0)
+		dev_err(lpg->chip->dev,
+			"Disable glitch-removal failed, rc=%d\n", rc);
+}
+
+static int qpnp_lpg_pwm_output_types_supported(struct pwm_chip *pwm_chip,
+				struct pwm_device *pwm)
+{
+	enum pwm_output_type type = PWM_OUTPUT_FIXED;
+	struct qpnp_lpg_channel *lpg;
+
+	lpg = pwm_dev_to_qpnp_lpg(pwm_chip, pwm);
+	if (lpg == NULL) {
+		dev_err(pwm_chip->dev, "lpg not found\n");
+		return type;
+	}
+
+	if (lpg->chip->lut != NULL)
+		type |= PWM_OUTPUT_MODULATED;
+
+	return type;
+}
+
+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,
+	.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);
+	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);
+
+	mutex_destroy(&chip->bus_lock);
+	dev_set_drvdata(chip->dev, NULL);
+
+	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


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH RFC 4/6] leds: leds-qti-tri-led: Add LED driver for QTI TRI_LED module
  2020-07-24 21:36 [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Martin Botka
                   ` (2 preceding siblings ...)
  2020-07-24 21:36 ` [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module Martin Botka
@ 2020-07-24 21:36 ` Martin Botka
  2020-07-26 17:25   ` Martin Botka
  2020-07-24 21:36 ` [PATCH RFC 5/6] Documentation: Add binding for qti-tri-led Martin Botka
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 31+ messages in thread
From: Martin Botka @ 2020-07-24 21:36 UTC (permalink / raw)
  Cc: Fenglin Wu, Martin Botka, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Lee Jones, linux-leds, devicetree, linux-kernel, linux-pwm

From: Fenglin Wu <fenglinw@codeaurora.org>

QTI TRI_LED module has 3 current sinks at max for LED driver and
each is controlled by a PWM channel used for LED dimming or blinking.
Add the driver to support it.

Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
[martin.botka1@@gmail.com: Fast-forward the driver from kernel 4.14 to 5.8]
Signed-off-by: Martin Botka <martin.botka1@gmail.com>
---
 drivers/leds/Kconfig            |   9 +
 drivers/leds/Makefile           |   1 +
 drivers/leds/leds-qti-tri-led.c | 640 ++++++++++++++++++++++++++++++++
 3 files changed, 650 insertions(+)
 create mode 100644 drivers/leds/leds-qti-tri-led.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed943140e1fd..37beff922945 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -768,6 +768,15 @@ config LEDS_POWERNV
 	  To compile this driver as a module, choose 'm' here: the module
 	  will be called leds-powernv.
 
+config LEDS_QTI_TRI_LED
+	tristate "LED support for Qualcomm Technologies, Inc. TRI_LED"
+	depends on LEDS_CLASS && MFD_SPMI_PMIC && PWM && OF
+	help
+	  This driver supports the TRI_LED module found in Qualcomm
+	  Technologies, Inc. PMIC chips. TRI_LED supports 3 LED drivers
+	  at max and each is controlled by a PWM channel used for dimming
+	  or blinking.
+
 config LEDS_SYSCON
 	bool "LED support for LEDs on system controllers"
 	depends on LEDS_CLASS=y
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d6b8a792c936..16a5be936178 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
 obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
+obj-$(CONFIG_LEDS_QTI_TRI_LED)		+= leds-qti-tri-led.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
 obj-$(CONFIG_LEDS_COBALT_QUBE)		+= leds-cobalt-qube.o
 obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
diff --git a/drivers/leds/leds-qti-tri-led.c b/drivers/leds/leds-qti-tri-led.c
new file mode 100644
index 000000000000..ea5069e22662
--- /dev/null
+++ b/drivers/leds/leds-qti-tri-led.c
@@ -0,0 +1,640 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/nvmem-consumer.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/types.h>
+
+#define TRILED_REG_TYPE 0x04
+#define TRILED_REG_SUBTYPE 0x05
+#define TRILED_REG_EN_CTL 0x46
+
+/* TRILED_REG_EN_CTL */
+#define TRILED_EN_CTL_MASK GENMASK(7, 5)
+#define TRILED_EN_CTL_MAX_BIT 7
+
+#define TRILED_TYPE 0x19
+#define TRILED_SUBTYPE_LED3H0L12 0x02
+#define TRILED_SUBTYPE_LED2H0L12 0x03
+#define TRILED_SUBTYPE_LED1H2L12 0x04
+
+#define TRILED_NUM_MAX 3
+
+#define PWM_PERIOD_DEFAULT_NS 1000000
+
+struct pwm_setting {
+	u64 pre_period_ns;
+	u64 period_ns;
+	u64 duty_ns;
+};
+
+struct led_setting {
+	u64 on_ms;
+	u64 off_ms;
+	enum led_brightness brightness;
+	bool blink;
+	bool breath;
+};
+
+struct qpnp_led_dev {
+	struct led_classdev cdev;
+	struct pwm_device *pwm_dev;
+	struct pwm_setting pwm_setting;
+	struct led_setting led_setting;
+	struct qpnp_tri_led_chip *chip;
+	struct mutex lock;
+	const char *label;
+	const char *default_trigger;
+	u8 id;
+	bool blinking;
+	bool breathing;
+};
+
+struct qpnp_tri_led_chip {
+	struct device *dev;
+	struct regmap *regmap;
+	struct qpnp_led_dev *leds;
+	struct nvmem_device *pbs_nvmem;
+	struct mutex bus_lock;
+	int num_leds;
+	u16 reg_base;
+	u8 subtype;
+	u8 bitmap;
+};
+
+static int qpnp_tri_led_read(struct qpnp_tri_led_chip *chip, u16 addr, u8 *val)
+{
+	int rc;
+	unsigned int tmp;
+
+	mutex_lock(&chip->bus_lock);
+	rc = regmap_read(chip->regmap, chip->reg_base + addr, &tmp);
+	if (rc < 0)
+		dev_err(chip->dev, "Read addr 0x%x failed, rc=%d\n", addr, rc);
+	else
+		*val = (u8)tmp;
+	mutex_unlock(&chip->bus_lock);
+
+	return rc;
+}
+
+static int qpnp_tri_led_masked_write(struct qpnp_tri_led_chip *chip, u16 addr,
+				     u8 mask, u8 val)
+{
+	int rc;
+
+	mutex_lock(&chip->bus_lock);
+	rc = regmap_update_bits(chip->regmap, chip->reg_base + addr, mask, val);
+	if (rc < 0)
+		dev_err(chip->dev,
+			"Update addr 0x%x to val 0x%x with mask 0x%x failed, rc=%d\n",
+			addr, val, mask, rc);
+	mutex_unlock(&chip->bus_lock);
+
+	return rc;
+}
+
+static int __tri_led_config_pwm(struct qpnp_led_dev *led,
+				struct pwm_setting *pwm)
+{
+	struct pwm_state pstate;
+	int rc;
+
+	pwm_get_state(led->pwm_dev, &pstate);
+	pstate.enabled = !!(pwm->duty_ns != 0);
+	pstate.period = pwm->period_ns;
+	pstate.duty_cycle = pwm->duty_ns;
+	rc = pwm_apply_state(led->pwm_dev, &pstate);
+
+	if (rc < 0)
+		dev_err(led->chip->dev,
+			"Apply PWM state for %s led failed, rc=%d\n",
+			led->cdev.name, rc);
+
+	return rc;
+}
+
+#define PBS_ENABLE 1
+#define PBS_DISABLE 2
+#define PBS_ARG 0x42
+#define PBS_TRIG_CLR 0xE6
+#define PBS_TRIG_SET 0xE5
+static int __tri_led_set(struct qpnp_led_dev *led)
+{
+	int rc = 0;
+	u8 val = 0, mask = 0, pbs_val;
+	u8 prev_bitmap;
+
+	rc = __tri_led_config_pwm(led, &led->pwm_setting);
+	if (rc < 0) {
+		dev_err(led->chip->dev,
+			"Configure PWM for %s led failed, rc=%d\n",
+			led->cdev.name, rc);
+		return rc;
+	}
+
+	mask |= 1 << (TRILED_EN_CTL_MAX_BIT - led->id);
+
+	if (led->pwm_setting.duty_ns == 0)
+		val = 0;
+	else
+		val = mask;
+
+	if (led->chip->subtype == TRILED_SUBTYPE_LED2H0L12 &&
+	    led->chip->pbs_nvmem) {
+		/*
+		 * Control BOB_CONFIG_EXT_CTRL2_FORCE_EN for HR_LED through
+		 * PBS trigger. PBS trigger for enable happens if any one of
+		 * LEDs are turned on. PBS trigger for disable happens only
+		 * if both LEDs are turned off.
+		 */
+
+		prev_bitmap = led->chip->bitmap;
+		if (val)
+			led->chip->bitmap |= (1 << led->id);
+		else
+			led->chip->bitmap &= ~(1 << led->id);
+
+		if (!(led->chip->bitmap & prev_bitmap)) {
+			pbs_val = led->chip->bitmap ? PBS_ENABLE : PBS_DISABLE;
+			rc = nvmem_device_write(led->chip->pbs_nvmem, PBS_ARG,
+						1, &pbs_val);
+			if (rc < 0) {
+				dev_err(led->chip->dev,
+					"Couldn't set PBS_ARG, rc=%d\n", rc);
+				return rc;
+			}
+
+			pbs_val = 1;
+			rc = nvmem_device_write(led->chip->pbs_nvmem,
+						PBS_TRIG_CLR, 1, &pbs_val);
+			if (rc < 0) {
+				dev_err(led->chip->dev,
+					"Couldn't set PBS_TRIG_CLR, rc=%d\n",
+					rc);
+				return rc;
+			}
+
+			pbs_val = 1;
+			rc = nvmem_device_write(led->chip->pbs_nvmem,
+						PBS_TRIG_SET, 1, &pbs_val);
+			if (rc < 0) {
+				dev_err(led->chip->dev,
+					"Couldn't set PBS_TRIG_SET, rc=%d\n",
+					rc);
+				return rc;
+			}
+		}
+	}
+
+	rc = qpnp_tri_led_masked_write(led->chip, TRILED_REG_EN_CTL, mask, val);
+	if (rc < 0)
+		dev_err(led->chip->dev, "Update addr 0x%x failed, rc=%d\n",
+			TRILED_REG_EN_CTL, rc);
+
+	return rc;
+}
+
+static int qpnp_tri_led_set(struct qpnp_led_dev *led)
+{
+	u64 on_ms, off_ms, period_ns, duty_ns;
+	enum led_brightness brightness = led->led_setting.brightness;
+	int rc = 0;
+
+	if (led->led_setting.blink) {
+		on_ms = led->led_setting.on_ms;
+		off_ms = led->led_setting.off_ms;
+
+		duty_ns = on_ms * NSEC_PER_MSEC;
+		period_ns = (on_ms + off_ms) * NSEC_PER_MSEC;
+
+		if (period_ns < duty_ns && duty_ns != 0)
+			period_ns = duty_ns + 1;
+	} else {
+		/* Use initial period if no blinking is required */
+		period_ns = led->pwm_setting.pre_period_ns;
+
+		if (brightness == LED_OFF)
+			duty_ns = 0;
+
+		duty_ns = period_ns * brightness;
+		do_div(duty_ns, LED_FULL);
+
+		if (period_ns < duty_ns && duty_ns != 0)
+			period_ns = duty_ns + 1;
+	}
+	dev_dbg(led->chip->dev,
+		"PWM settings for %s led: period = %lluns, duty = %lluns\n",
+		led->cdev.name, period_ns, duty_ns);
+
+	led->pwm_setting.duty_ns = duty_ns;
+	led->pwm_setting.period_ns = period_ns;
+
+	rc = __tri_led_set(led);
+	if (rc < 0) {
+		dev_err(led->chip->dev, "__tri_led_set %s failed, rc=%d\n",
+			led->cdev.name, rc);
+		return rc;
+	}
+
+	if (led->led_setting.blink) {
+		led->cdev.brightness = LED_FULL;
+		led->blinking = true;
+		led->breathing = false;
+	} else if (led->led_setting.breath) {
+		led->cdev.brightness = LED_FULL;
+		led->blinking = false;
+		led->breathing = true;
+	} else {
+		led->cdev.brightness = led->led_setting.brightness;
+		led->blinking = false;
+		led->breathing = false;
+	}
+
+	return rc;
+}
+
+static int qpnp_tri_led_set_brightness(struct led_classdev *led_cdev,
+				       enum led_brightness brightness)
+{
+	struct qpnp_led_dev *led =
+		container_of(led_cdev, struct qpnp_led_dev, cdev);
+	int rc = 0;
+
+	mutex_lock(&led->lock);
+	if (brightness > LED_FULL)
+		brightness = LED_FULL;
+
+	if (brightness == led->led_setting.brightness && !led->blinking &&
+	    !led->breathing) {
+		mutex_unlock(&led->lock);
+		return 0;
+	}
+
+	led->led_setting.brightness = brightness;
+	if (!!brightness)
+		led->led_setting.off_ms = 0;
+	else
+		led->led_setting.on_ms = 0;
+	led->led_setting.blink = false;
+	led->led_setting.breath = false;
+
+	rc = qpnp_tri_led_set(led);
+	if (rc)
+		dev_err(led->chip->dev, "Set led failed for %s, rc=%d\n",
+			led->label, rc);
+
+	mutex_unlock(&led->lock);
+
+	return rc;
+}
+
+static enum led_brightness
+qpnp_tri_led_get_brightness(struct led_classdev *led_cdev)
+{
+	return led_cdev->brightness;
+}
+
+static int qpnp_tri_led_set_blink(struct led_classdev *led_cdev,
+				  unsigned long *on_ms, unsigned long *off_ms)
+{
+	struct qpnp_led_dev *led =
+		container_of(led_cdev, struct qpnp_led_dev, cdev);
+	int rc = 0;
+
+	mutex_lock(&led->lock);
+	if (led->blinking && *on_ms == led->led_setting.on_ms &&
+	    *off_ms == led->led_setting.off_ms) {
+		dev_dbg(led_cdev->dev,
+			"Ignore, on/off setting is not changed: on %lums, off %lums\n",
+			*on_ms, *off_ms);
+		mutex_unlock(&led->lock);
+		return 0;
+	}
+
+	if (*on_ms == 0) {
+		led->led_setting.blink = false;
+		led->led_setting.breath = false;
+		led->led_setting.brightness = LED_OFF;
+	} else if (*off_ms == 0) {
+		led->led_setting.blink = false;
+		led->led_setting.breath = false;
+		led->led_setting.brightness = led->cdev.brightness;
+	} else {
+		led->led_setting.on_ms = *on_ms;
+		led->led_setting.off_ms = *off_ms;
+		led->led_setting.blink = true;
+		led->led_setting.breath = false;
+	}
+
+	rc = qpnp_tri_led_set(led);
+	if (rc)
+		dev_err(led->chip->dev, "Set led failed for %s, rc=%d\n",
+			led->label, rc);
+
+	mutex_unlock(&led->lock);
+	return rc;
+}
+
+static ssize_t breath_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct qpnp_led_dev *led =
+		container_of(led_cdev, struct qpnp_led_dev, cdev);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", led->led_setting.breath);
+}
+
+static ssize_t breath_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	int rc;
+	bool breath;
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct qpnp_led_dev *led =
+		container_of(led_cdev, struct qpnp_led_dev, cdev);
+
+	rc = kstrtobool(buf, &breath);
+	if (rc < 0)
+		return rc;
+
+	cancel_work_sync(&led_cdev->set_brightness_work);
+
+	mutex_lock(&led->lock);
+	if (led->breathing == breath)
+		goto unlock;
+
+	led->led_setting.blink = false;
+	led->led_setting.breath = breath;
+	led->led_setting.brightness = breath ? LED_FULL : LED_OFF;
+	rc = qpnp_tri_led_set(led);
+	if (rc < 0)
+		dev_err(led->chip->dev, "Set led failed for %s, rc=%d\n",
+			led->label, rc);
+
+unlock:
+	mutex_unlock(&led->lock);
+	return (rc < 0) ? rc : count;
+}
+
+static DEVICE_ATTR(breath, 0644, breath_show, breath_store);
+static const struct attribute *breath_attrs[] = { &dev_attr_breath.attr, NULL };
+
+static int qpnp_tri_led_register(struct qpnp_tri_led_chip *chip)
+{
+	struct qpnp_led_dev *led;
+	int rc, i, j;
+
+	for (i = 0; i < chip->num_leds; i++) {
+		led = &chip->leds[i];
+		mutex_init(&led->lock);
+		led->cdev.name = led->label;
+		led->cdev.max_brightness = LED_FULL;
+		led->cdev.brightness_set_blocking = qpnp_tri_led_set_brightness;
+		led->cdev.brightness_get = qpnp_tri_led_get_brightness;
+		led->cdev.blink_set = qpnp_tri_led_set_blink;
+		led->cdev.default_trigger = led->default_trigger;
+		led->cdev.brightness = LED_OFF;
+
+		rc = devm_led_classdev_register(chip->dev, &led->cdev);
+		if (rc < 0) {
+			dev_err(chip->dev,
+				"%s led class device registering failed, rc=%d\n",
+				led->label, rc);
+			goto err_out;
+		}
+
+		/* Make sure to initialize the LEDs to known values */
+		rc = qpnp_tri_led_set(led);
+		if (rc)
+			dev_warn(chip->dev,
+				 "Cannot initialize %s LED to OFF value: %d\n",
+				 led->label, rc);
+	}
+
+	return 0;
+
+err_out:
+	for (j = 0; j <= i; j++) {
+		if (j < i)
+			sysfs_remove_files(&chip->leds[j].cdev.dev->kobj,
+					   breath_attrs);
+		mutex_destroy(&chip->leds[j].lock);
+	}
+	return rc;
+}
+
+static int qpnp_tri_led_hw_init(struct qpnp_tri_led_chip *chip)
+{
+	int rc = 0;
+	u8 val;
+
+	rc = qpnp_tri_led_read(chip, TRILED_REG_TYPE, &val);
+	if (rc < 0) {
+		dev_err(chip->dev, "Read REG_TYPE failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	if (val != TRILED_TYPE) {
+		dev_err(chip->dev, "invalid subtype(%d)\n", val);
+		return -ENODEV;
+	}
+
+	rc = qpnp_tri_led_read(chip, TRILED_REG_SUBTYPE, &val);
+	if (rc < 0) {
+		dev_err(chip->dev, "Read REG_SUBTYPE failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	chip->subtype = val;
+
+	return 0;
+}
+
+static int qpnp_tri_led_parse_dt(struct qpnp_tri_led_chip *chip)
+{
+	struct device_node *node = chip->dev->of_node, *child_node;
+	struct qpnp_led_dev *led;
+	struct pwm_args pargs;
+	const __be32 *addr;
+	int rc = 0, id, i = 0;
+
+	addr = of_get_address(chip->dev->of_node, 0, NULL, NULL);
+	if (!addr) {
+		dev_err(chip->dev, "Getting address failed\n");
+		return -EINVAL;
+	}
+	chip->reg_base = be32_to_cpu(addr[0]);
+
+	chip->num_leds = of_get_available_child_count(node);
+	if (chip->num_leds == 0) {
+		dev_err(chip->dev, "No led child node defined\n");
+		return -ENODEV;
+	}
+
+	if (chip->num_leds > TRILED_NUM_MAX) {
+		dev_err(chip->dev, "can't support %d leds(max %d)\n",
+			chip->num_leds, TRILED_NUM_MAX);
+		return -EINVAL;
+	}
+
+	if (of_find_property(chip->dev->of_node, "nvmem", NULL)) {
+		chip->pbs_nvmem = devm_nvmem_device_get(chip->dev, "pbs_sdam");
+		if (IS_ERR_OR_NULL(chip->pbs_nvmem)) {
+			rc = PTR_ERR(chip->pbs_nvmem);
+			if (rc != -EPROBE_DEFER) {
+				dev_err(chip->dev,
+					"Couldn't get nvmem device, rc=%d\n",
+					rc);
+				return -ENODEV;
+			}
+			chip->pbs_nvmem = NULL;
+			return rc;
+		}
+	}
+
+	chip->leds = devm_kcalloc(chip->dev, chip->num_leds,
+				  sizeof(struct qpnp_led_dev), GFP_KERNEL);
+	if (!chip->leds)
+		return -ENOMEM;
+
+	for_each_available_child_of_node (node, child_node) {
+		rc = of_property_read_u32(child_node, "led-sources", &id);
+		if (rc) {
+			dev_err(chip->dev, "Get led-sources failed, rc=%d\n",
+				rc);
+			return rc;
+		}
+
+		if (id >= TRILED_NUM_MAX) {
+			dev_err(chip->dev, "only support 0~%d current source\n",
+				TRILED_NUM_MAX - 1);
+			return -EINVAL;
+		}
+
+		led = &chip->leds[i++];
+		led->chip = chip;
+		led->id = id;
+		led->label = of_get_property(child_node, "label", NULL) ?:
+				     child_node->name;
+
+		led->pwm_dev = devm_of_pwm_get(chip->dev, child_node, NULL);
+		if (IS_ERR(led->pwm_dev)) {
+			rc = PTR_ERR(led->pwm_dev);
+			if (rc != -EPROBE_DEFER)
+				dev_err(chip->dev,
+					"Get pwm device for %s led failed, rc=%d\n",
+					led->label, rc);
+			return rc;
+		}
+
+		pwm_get_args(led->pwm_dev, &pargs);
+		if (pargs.period == 0)
+			led->pwm_setting.pre_period_ns = PWM_PERIOD_DEFAULT_NS;
+		else
+			led->pwm_setting.pre_period_ns = pargs.period;
+
+		led->default_trigger = of_get_property(
+			child_node, "linux,default-trigger", NULL);
+	}
+
+	return rc;
+}
+
+static int qpnp_tri_led_probe(struct platform_device *pdev)
+{
+	struct qpnp_tri_led_chip *chip;
+	int rc = 0;
+
+	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;
+	}
+
+	rc = qpnp_tri_led_parse_dt(chip);
+	if (rc < 0) {
+		dev_err(chip->dev,
+			"Devicetree properties parsing failed, rc=%d\n", rc);
+		return rc;
+	}
+
+	mutex_init(&chip->bus_lock);
+
+	rc = qpnp_tri_led_hw_init(chip);
+	if (rc) {
+		dev_err(chip->dev, "HW initialization failed, rc=%d\n", rc);
+		goto destroy;
+	}
+
+	dev_set_drvdata(chip->dev, chip);
+	rc = qpnp_tri_led_register(chip);
+	if (rc < 0) {
+		dev_err(chip->dev,
+			"Registering LED class devices failed, rc=%d\n", rc);
+		goto destroy;
+	}
+
+	dev_dbg(chip->dev, "Tri-led module with subtype 0x%x is detected\n",
+		chip->subtype);
+	return 0;
+destroy:
+	mutex_destroy(&chip->bus_lock);
+	dev_set_drvdata(chip->dev, NULL);
+
+	return rc;
+}
+
+static int qpnp_tri_led_remove(struct platform_device *pdev)
+{
+	int i;
+	struct qpnp_tri_led_chip *chip = dev_get_drvdata(&pdev->dev);
+
+	mutex_destroy(&chip->bus_lock);
+	for (i = 0; i < chip->num_leds; i++) {
+		sysfs_remove_files(&chip->leds[i].cdev.dev->kobj, breath_attrs);
+		mutex_destroy(&chip->leds[i].lock);
+	}
+	dev_set_drvdata(chip->dev, NULL);
+	return 0;
+}
+
+static const struct of_device_id qpnp_tri_led_of_match[] = {
+	{
+		.compatible = "qcom,tri-led",
+	},
+	{},
+};
+
+static struct platform_driver qpnp_tri_led_driver = {
+	.driver		= {
+		.name		= "qcom,tri-led",
+		.of_match_table	= qpnp_tri_led_of_match,
+	},
+	.probe		= qpnp_tri_led_probe,
+	.remove		= qpnp_tri_led_remove,
+};
+module_platform_driver(qpnp_tri_led_driver);
+
+MODULE_DESCRIPTION("QTI TRI_LED driver");
+MODULE_LICENSE("GPL v2");
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH RFC 5/6] Documentation: Add binding for qti-tri-led
  2020-07-24 21:36 [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Martin Botka
                   ` (3 preceding siblings ...)
  2020-07-24 21:36 ` [PATCH RFC 4/6] leds: leds-qti-tri-led: Add LED driver for QTI TRI_LED module Martin Botka
@ 2020-07-24 21:36 ` Martin Botka
  2020-07-24 21:36 ` [PATCH RFC 6/6] Documentation: Add binding for pwm-qti-lpg Martin Botka
  2020-07-24 22:05 ` [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Pavel Machek
  6 siblings, 0 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-24 21:36 UTC (permalink / raw)
  Cc: Fenglin Wu, Martin Botka, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Lee Jones, linux-leds, devicetree, linux-kernel, linux-pwm

From: Fenglin Wu <fenglinw@codeaurora.org>

Add documentation for qti-tri-led

Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
Signed-off-by: Martin Botka <martin.botka1@gmail.com>
---
 .../bindings/leds/leds-qti-tri-led.txt        | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-qti-tri-led.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-qti-tri-led.txt b/Documentation/devicetree/bindings/leds/leds-qti-tri-led.txt
new file mode 100644
index 000000000000..e179f4222739
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-qti-tri-led.txt
@@ -0,0 +1,72 @@
+Qualcomm Technologies, Inc. TRI_LED driver specific bindings
+
+This binding document describes the properties of TRI_LED module in
+Qualcomm Technologies, Inc. PMIC chips.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Must be "qcom,tri-led".
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: Register base of the TRI_LED module and length.
+
+- nvmem-names:
+	Usage: optional
+	Value type: <string>
+	Definition: Nvmem device name for SDAM to do PBS trigger. It must be
+		defined as "pbs_sdam". This is required only for HR_LEDs.
+
+- nvmem:
+	Usage: optional
+	Value type: <phandle>
+	Definition: Phandle of the nvmem device name to access SDAM to do PBS
+		trigger. This is required only for HR_LEDs.
+
+Properties for child nodes:
+- pwms:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: The PWM device (phandle) used for controlling LED.
+
+- led-sources:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: see Documentation/devicetree/bindings/leds/common.txt;
+		Device current output identifiers are: 0 - LED1_EN,
+		1 - LED2_EN, 2 - LED3_EN.
+
+- label:
+	Usage: optional
+	Value type: <string>
+	Definition: see Documentation/devicetree/bindings/leds/common.txt;
+
+- linux,default-trigger:
+	Usage: optional
+	Value_type: <string>
+	Definition: see Documentation/devicetree/bindings/leds/common.txt;
+
+Example:
+
+	pmi8998_rgb: tri-led@d000{
+		compatible = "qcom,tri-led";
+		reg = <0xd000 0x100>;
+
+		red {
+			label = "red";
+			pwms = <&pmi8998_lpg 4 1000000>;
+			led-sources = <0>;
+		};
+		green {
+			label = "green";
+			pwms = <&pmi8998_lpg 3 1000000>;
+			led-sources = <1>;
+		};
+		blue {
+			label = "blue";
+			pwms = <&pmi8998_lpg 2 1000000>;
+			led-sources = <2>;
+		};
+	};
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH RFC 6/6] Documentation: Add binding for pwm-qti-lpg
  2020-07-24 21:36 [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Martin Botka
                   ` (4 preceding siblings ...)
  2020-07-24 21:36 ` [PATCH RFC 5/6] Documentation: Add binding for qti-tri-led Martin Botka
@ 2020-07-24 21:36 ` Martin Botka
  2020-07-24 22:05 ` [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Pavel Machek
  6 siblings, 0 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-24 21:36 UTC (permalink / raw)
  Cc: Fenglin Wu, Martin Botka, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Lee Jones, linux-leds, devicetree, linux-kernel, linux-pwm

From: Fenglin Wu <fenglinw@codeaurora.org>

Add documentation for pwm-qti-lpg

Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
Signed-off-by: Martin Botka <martin.botka1@gmail.com>
---
 .../devicetree/bindings/pwm/pwm-qti-lpg.txt   | 163 ++++++++++++++++++
 1 file changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-qti-lpg.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-qti-lpg.txt b/Documentation/devicetree/bindings/pwm/pwm-qti-lpg.txt
new file mode 100644
index 000000000000..df2810626da4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-qti-lpg.txt
@@ -0,0 +1,163 @@
+Qualcomm Technologies, Inc. LPG driver specific bindings
+
+This binding document describes the properties of LPG (Light Pulse Generator)
+device module in Qualcomm Technologies, Inc. PMIC chips.
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Must be "qcom,pwm-lpg".
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: Register base for LPG and LUT modules.
+
+- reg-names:
+	Usage: required
+	Value type: <string>
+	Definition: The name of the register defined in the reg property.
+		      It must have "lpg-base", "lut-base" is optional but
+		      it's required if any LPG channels support LUT mode.
+
+- #pwm-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: The number of cells in "pwms" property specified in
+		      PWM user nodes. It should be 2. The first cell is
+		      the PWM channel ID indexed from 0, and the second
+		      cell is the PWM default period in nanoseconds.
+
+- qcom,num-lpg-channels:
+	Usage: required
+	Value type: <u32>
+	Definition: The number of the consecutive LPG/PWM channels in the chip.
+
+- qcom,lut-patterns:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: Duty ratios in percentages for LPG working at LUT mode.
+		      These duty ratios will be translated into PWM values
+		      and stored in LUT module. The LUT module has resource
+		      to store 47 PWM values at max and shared for all LPG
+		      channels. This property is required if any LPG channels
+		      support LUT mode.
+
+- qcom,sync-channel-ids:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: The hardware IDs of the LPG channel that required be
+		      grouped together. These channels will share the same LUT
+		      ramping configuration so that they will be enabled with a
+		      synchronized pattern. If the LUT ramping configuration
+		      differs for the channels grouped for synchronization,
+		      configuration of the first channel will be applied for
+		      all others.
+
+Subnode is optional if LUT mode is not required, it's required if any LPG
+channels expected to be supported in LUT mode.
+
+Subnode properties:
+Subnodes for each LPG channel (lpg@X) can be defined if any of the following
+parameters needs to be configured for that channel.
+
+- qcom,lpg-chan-id:
+	Usage: required
+	Value type: <u32>
+	Definition: The LPG channel's hardware ID indexed from 1. Allowed
+		      range is 1 - 8. Maximum value depends on the number of
+		      channels supported on PMIC.
+
+- qcom,ramp-step-ms:
+	Usage: required
+	Value type: <u32>
+	Definition: The step duration in milliseconds for LPG staying at each
+		      duty specified in the LUT pattern. Allowed range is
+		      1 - 511.
+
+- qcom,ramp-high-index:
+	Usage: required
+	Value type: <u32>
+	Definition: The high index of the LUT pattern where LPG ends up
+		      ramping to. Allowed range is 1 - 47.
+
+- qcom,ramp-low-index:
+	Usage: required
+	Value type: <u32>
+	Definition: The low index of the LUT pattern from where LPG begins
+		      ramping from. Allowed range is 0 - 46.
+
+- qcom,ramp-from-low-to-high:
+	Usage: optional
+	Value type: <empty>
+	Definition: The flag to specify the LPG ramping direction. The ramping
+		      direction is from low index to high index of the LUT
+		      pattern if it's specified.
+
+- qcom,ramp-pattern-repeat:
+	Usage: optional
+	Value type: <empty>
+	Definition: The flag to specify if LPG would be ramping with the LUT
+		      pattern repeatedly.
+
+- qcom,ramp-toggle:
+	Usage: optional
+	Value type: <empty>
+	Definition: The flag to specify if LPG would toggle the LUT pattern
+		      in ramping. If toggling enabled, LPG would return to the
+		      low index when high index is reached, or return to the high
+		      index when low index is reached.
+
+- qcom,ramp-pause-hi-count:
+	Usage: optional
+	Value type: <u32>
+	Definition: The step count that LPG stop the output when it ramped up
+		      to the high index of the LUT.
+
+- qcom,ramp-pause-lo-count:
+	Usage: optional
+	Value type: <u32>
+	Definition: The step count that LPG stop the output when it ramped up
+		      to the low index of the LUT.
+Example:
+
+	pmi8998_lpg: lpg@b100 {
+		compatible = "qcom,pwm-lpg";
+		reg = <0xb100 0x600>, <0xb000 0x100>;
+		reg-names = "lpg-base", "lut-base";
+		qcom,num-lpg-channels = <6>;
+		#pwm-cells = <2>;
+		qcom,lut-patterns = <0 14 28 42 56 70 84 100
+					100 84 70 56 42 28 14 0>;
+		qcom,sync-channel-ids = <3 4 5>;
+		lpg@3 {
+			qcom,lpg-chan-id = <3>;
+			qcom,ramp-step-ms = <200>;
+			qcom,ramp-pause-hi-count = <10>;
+			qcom,ramp-pause-lo-count = <10>;
+			qcom,ramp-low-index = <0>;
+			qcom,ramp-high-index = <15>;
+			qcom,ramp-from-low-to-high;
+			qcom,ramp-pattern-repeat;
+		};
+		lpg@4 {
+			qcom,lpg-chan-id = <4>;
+			qcom,ramp-step-ms = <200>;
+			qcom,ramp-pause-hi-count = <10>;
+			qcom,ramp-pause-lo-count = <10>;
+			qcom,ramp-low-index = <0>;
+			qcom,ramp-high-index = <15>;
+			qcom,ramp-from-low-to-high;
+			qcom,ramp-pattern-repeat;
+		};
+		lpg@5 {
+			qcom,lpg-chan-id = <5>;
+			qcom,ramp-step-ms = <200>;
+			qcom,ramp-pause-hi-count = <10>;
+			qcom,ramp-pause-lo-count = <10>;
+			qcom,ramp-low-index = <0>;
+			qcom,ramp-high-index = <15>;
+			qcom,ramp-from-low-to-high;
+			qcom,ramp-pattern-repeat;
+		};
+	};
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers
  2020-07-24 21:36 [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Martin Botka
                   ` (5 preceding siblings ...)
  2020-07-24 21:36 ` [PATCH RFC 6/6] Documentation: Add binding for pwm-qti-lpg Martin Botka
@ 2020-07-24 22:05 ` Pavel Machek
  2020-07-24 22:26   ` Martin Botka
       [not found]   ` <CADQ2G_HbysJbiQKSRmA6HDRfjPYPiDjbZfeuUjM1mNV-BBBC-A@mail.gmail.com>
  6 siblings, 2 replies; 31+ messages in thread
From: Pavel Machek @ 2020-07-24 22:05 UTC (permalink / raw)
  To: Martin Botka
  Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Lee Jones, linux-leds, devicetree,
	linux-kernel, linux-pwm

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

Hi!

Dalsi cech co hackuje LEDky?

> This series brings QCOM pwm-lpg and tri-led drivers from 4.14 that is required to support pmic-connected notification LED.
> This comes straight from downstream and I'm ready for your comments.

Yeah, so...

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

Can generic pwm driver be used here?

This is for RGB modules, right? It will need to use multicolor
framework.

"Breathing" will need to be figured out. It should become a trigger.

Is this for phone, btw? If so, which one?

Best regards,
								Pavel

>  .../bindings/leds/leds-qti-tri-led.txt        |   72 +
>  .../devicetree/bindings/pwm/pwm-qti-lpg.txt   |  163 +++
>  drivers/leds/Kconfig                          |    9 +
>  drivers/leds/Makefile                         |    1 +
>  drivers/leds/leds-qti-tri-led.c               |  640 ++++++++
>  drivers/pwm/Kconfig                           |   10 +
>  drivers/pwm/Makefile                          |    1 +
>  drivers/pwm/core.c                            |   56 +-
>  drivers/pwm/pwm-qti-lpg.c                     | 1284 +++++++++++++++++
>  drivers/pwm/sysfs.c                           |   56 +-
>  include/linux/pwm.h                           |  144 +-
>  11 files changed, 2418 insertions(+), 18 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-qti-tri-led.txt
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-qti-lpg.txt
>  create mode 100644 drivers/leds/leds-qti-tri-led.c
>  create mode 100644 drivers/pwm/pwm-qti-lpg.c
> 

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers
  2020-07-24 22:05 ` [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Pavel Machek
@ 2020-07-24 22:26   ` Martin Botka
  2020-07-24 22:32     ` Pavel Machek
       [not found]   ` <CADQ2G_HbysJbiQKSRmA6HDRfjPYPiDjbZfeuUjM1mNV-BBBC-A@mail.gmail.com>
  1 sibling, 1 reply; 31+ messages in thread
From: Martin Botka @ 2020-07-24 22:26 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Lee Jones, linux-leds, devicetree,
	linux-kernel, linux-pwm

Hi,

Dalsi cech co hackuje LEDky?

Nie. Slovak.

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

OK

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.

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.

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.

P.S resending because i sent it as HTML apparently.
Best Regards,
Martin

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers
       [not found]   ` <CADQ2G_HbysJbiQKSRmA6HDRfjPYPiDjbZfeuUjM1mNV-BBBC-A@mail.gmail.com>
@ 2020-07-24 22:31     ` Pavel Machek
  2020-07-24 22:46       ` Martin Botka
  0 siblings, 1 reply; 31+ messages in thread
From: Pavel Machek @ 2020-07-24 22:31 UTC (permalink / raw)
  To: Martin Botka
  Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Lee Jones, linux-leds, devicetree,
	linux-kernel, linux-pwm

[-- Attachment #1: Type: text/plain, Size: 1281 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers
  2020-07-24 22:26   ` Martin Botka
@ 2020-07-24 22:32     ` Pavel Machek
  2020-07-24 22:48       ` Martin Botka
  0 siblings, 1 reply; 31+ messages in thread
From: Pavel Machek @ 2020-07-24 22:32 UTC (permalink / raw)
  To: Martin Botka
  Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Lee Jones, linux-leds, devicetree,
	linux-kernel, linux-pwm

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

Hi!


> 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.
> 
> P.S resending because i sent it as HTML apparently.

First copy was ok. Second had broken formatting. Both were plaintext
AFAICT.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers
  2020-07-24 22:31     ` Pavel Machek
@ 2020-07-24 22:46       ` Martin Botka
  0 siblings, 0 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-24 22:46 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Lee Jones, linux-leds, devicetree,
	linux-kernel, linux-pwm

> Vitej do klubu :-).

Dakujem

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

I'm not sure on that one. I will have to talk to some people about that.
I will get back to you on that when i find out how it actually is.
But i think its one single chip. Not sure tho.

> Ok. Check multicolor framework in -next.

I will look into it. Thank you

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

Actually no.
While Konrad didn't send the patches yet we have some stuff working as:
Panel
Touchscreen
LEDs with this series
Flash light

You can check https://github.com/konradybcio/linux/commits/ninges_labs
for the current progress on those phones.

Best Regards,
Martin

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers
  2020-07-24 22:32     ` Pavel Machek
@ 2020-07-24 22:48       ` Martin Botka
  0 siblings, 0 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-24 22:48 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, Thierry Reding,
	Uwe Kleine-König, Lee Jones, linux-leds, devicetree,
	linux-kernel, linux-pwm

> First copy was ok. Second had broken formatting. Both were plaintext
> AFAICT.

It's just that gmail was telling me that it didn't send it to some of
the email addresses because i forgot to check the "Use plain text" in
Gmail.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-24 21:36 ` [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length Martin Botka
@ 2020-07-25 14:55   ` Martin Botka
  2020-07-25 15:24     ` Pavel Machek
  2020-07-26  9:04   ` Andy Shevchenko
  1 sibling, 1 reply; 31+ messages in thread
From: Martin Botka @ 2020-07-25 14:55 UTC (permalink / raw)
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Lee Jones, linux-leds, devicetree, linux-kernel, linux-pwm

Hello,

As can be seen this divides llu by llu in few warnings and error.
At the time of sending i didn't realize it but this fails on 32 bit
architectures.

So i would like to ask how would you like this fixed ?
Using macro or some other way ?

Thank you.

Best regards,
Martin

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-25 14:55   ` Martin Botka
@ 2020-07-25 15:24     ` Pavel Machek
  2020-07-25 15:30       ` Martin Botka
  0 siblings, 1 reply; 31+ messages in thread
From: Pavel Machek @ 2020-07-25 15:24 UTC (permalink / raw)
  To: Martin Botka
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Dan Murphy,
	Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones,
	linux-leds, devicetree, linux-kernel, linux-pwm

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

Hi!

> As can be seen this divides llu by llu in few warnings and error.
> At the time of sending i didn't realize it but this fails on 32 bit
> architectures.
> 
> So i would like to ask how would you like this fixed ?
> Using macro or some other way ?

+#include <linux/math64.h>
-               gain_q23 = (gain_q23 * dmic->boost_gain) / 100;
+               gain_q23 = div_u64(gain_q23 * dmic->boost_gain, 100);

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-25 15:24     ` Pavel Machek
@ 2020-07-25 15:30       ` Martin Botka
  0 siblings, 0 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-25 15:30 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Dan Murphy,
	Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones,
	linux-leds, devicetree, linux-kernel, linux-pwm

> +#include <linux/math64.h>
> -               gain_q23 = (gain_q23 * dmic->boost_gain) / 100;
> +               gain_q23 = div_u64(gain_q23 * dmic->boost_gain, 100);

Ok so using a macro.

Thank you.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-24 21:36 ` [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length Martin Botka
  2020-07-25 14:55   ` Martin Botka
@ 2020-07-26  9:04   ` Andy Shevchenko
  2020-07-26  9:12     ` Martin Botka
  1 sibling, 1 reply; 31+ messages in thread
From: Andy Shevchenko @ 2020-07-26  9:04 UTC (permalink / raw)
  To: Martin Botka
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Lee Jones, Linux LED Subsystem, devicetree,
	Linux Kernel Mailing List, linux-pwm

On Sat, Jul 25, 2020 at 12:40 AM Martin Botka <martin.botka1@gmail.com> wrote:
>
> From: Fenglin Wu <fenglinw@codeaurora.org>
>
> Currently, PWM core driver provides interfaces for configuring PWM
> period and duty length in nanoseconds with an integer data type, so
> the max period can be only set to ~2.147 seconds. Add interfaces which
> can set PWM period and duty with u64 data type to remove this
> limitation.

And all divisions go mad on 32-bit CPU, right?
Please, if you thought about it carefully, update a commit message to
clarify that.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-26  9:04   ` Andy Shevchenko
@ 2020-07-26  9:12     ` Martin Botka
  2020-07-27  7:29       ` Martin Botka
  0 siblings, 1 reply; 31+ messages in thread
From: Martin Botka @ 2020-07-26  9:12 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Lee Jones, Linux LED Subsystem, devicetree,
	Linux Kernel Mailing List, linux-pwm

> And all divisions go mad on 32-bit CPU, right?
> Please, if you thought about it carefully, update a commit message to
> clarify that.

Hello,
This patch will be dropped in V2 since another series already made these u64.
See a9d887dc1c60ed67f2271d66560cdcf864c4a578 in linux-next.
I have not tested compiling that commit in linux-next on 32 bit arch
but if it fails i can replace this commit with fix for that.

Also  I'm not the author of this commit.
Konrad Dybcio fast forwarded it to 5.8 from 4.14.
Fenglin Wu is the author and also created that commit message.

Thank you.

Best regards
Martin

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 4/6] leds: leds-qti-tri-led: Add LED driver for QTI TRI_LED module
  2020-07-24 21:36 ` [PATCH RFC 4/6] leds: leds-qti-tri-led: Add LED driver for QTI TRI_LED module Martin Botka
@ 2020-07-26 17:25   ` Martin Botka
  0 siblings, 0 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-26 17:25 UTC (permalink / raw)
  To: Konrad Dybcio
  Cc: Fenglin Wu, Jacek Anaszewski, Pavel Machek, Dan Murphy,
	Rob Herring, Thierry Reding, Uwe Kleine-König, Lee Jones,
	Linux LED Subsystem, devicetree, Linux Kernel Mailing List,
	linux-pwm

Hello.

This driver has the breath feature of the driver broken as I sent a
slightly modified version of it from when I was testing it.
The proper version will come when i will be sending out V2 which will
be hopefully soon as I'm little busy.

Best regards
Martin

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-26  9:12     ` Martin Botka
@ 2020-07-27  7:29       ` Martin Botka
  2020-07-27  7:32         ` Martin Botka
  2020-07-27  7:52         ` Uwe Kleine-König
  0 siblings, 2 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-27  7:29 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Lee Jones,
	Linux LED Subsystem, devicetree, Linux Kernel Mailing List,
	linux-pwm, Andy Shevchenko

Hello Uwe,

On Sat, Jul 25, 2020 at 09:12:23PM +0200, Martin Botka wrote:
>> > Note there is already a series that changes these values to u64. See
>> > a9d887dc1c60ed67f2271d66560cdcf864c4a578 in linux-next.
>>
>> Amazing. But isn't there the same issue with it as this one where this
>> would fail to build on 32 bit architecture?
>
> In theory all these cases are coped for. I didn't see any problems yet,
> so I still assume also the 32 bit archs are fine.

OK then all is fine. I will drop the patch in V2.

Also Uwe i just realized that you sent the original message and also
this reply only to me and not to anyone else.
Could you please send the messages also to everyone else ?

Thank you.

Best regards,
Martin

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-27  7:29       ` Martin Botka
@ 2020-07-27  7:32         ` Martin Botka
  2020-07-27  7:52         ` Uwe Kleine-König
  1 sibling, 0 replies; 31+ messages in thread
From: Martin Botka @ 2020-07-27  7:32 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Lee Jones,
	Linux LED Subsystem, devicetree, Linux Kernel Mailing List,
	linux-pwm, Andy Shevchenko

> Could you please send the messages also to everyone else ?

Next time of course.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-27  7:29       ` Martin Botka
  2020-07-27  7:32         ` Martin Botka
@ 2020-07-27  7:52         ` Uwe Kleine-König
  2020-07-27  7:58           ` Martin Botka
  1 sibling, 1 reply; 31+ messages in thread
From: Uwe Kleine-König @ 2020-07-27  7:52 UTC (permalink / raw)
  To: Martin Botka
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Lee Jones,
	Linux LED Subsystem, devicetree, Linux Kernel Mailing List,
	linux-pwm, Andy Shevchenko

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

Hello Martin,

On Mon, Jul 27, 2020 at 09:29:19AM +0200, Martin Botka wrote:
> On Sat, Jul 25, 2020 at 09:12:23PM +0200, Martin Botka wrote:
> >> > Note there is already a series that changes these values to u64. See
> >> > a9d887dc1c60ed67f2271d66560cdcf864c4a578 in linux-next.
> >>
> >> Amazing. But isn't there the same issue with it as this one where this
> >> would fail to build on 32 bit architecture?
> >
> > In theory all these cases are coped for. I didn't see any problems yet,
> > so I still assume also the 32 bit archs are fine.
> 
> OK then all is fine. I will drop the patch in V2.
> 
> Also Uwe i just realized that you sent the original message and also
> this reply only to me and not to anyone else.
> Could you please send the messages also to everyone else ?

I hit "reply-to-all" and the mail only was sent to you because you wrote
to only me.

Also threading is somehow strange because your reply to my mail (with

	Message-Id: 20200727070411.ovkuwm76vuw3heo7@pengutronix.de

) has

	In-Reply-To: <CADQ2G_HkiAZx8OhfQ_jeizveMaB-QN9dfN6Tcwfk9XuF97rmOg@mail.gmail.com>

. So I assume all the strange things happened on your side until proved
otherwise. :-)

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-27  7:52         ` Uwe Kleine-König
@ 2020-07-27  7:58           ` Martin Botka
  2020-07-27  8:34             ` Uwe Kleine-König
  0 siblings, 1 reply; 31+ messages in thread
From: Martin Botka @ 2020-07-27  7:58 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Lee Jones,
	Linux LED Subsystem, devicetree, Linux Kernel Mailing List,
	linux-pwm, Andy Shevchenko

Hello,

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

> Also threading is somehow strange because your reply to my mail

Yes Gmail would not allow me to reply to your message and also send it
to everyone so i had to reply to Andy's email which is why the
threading is broken there. Sorry for that.

> So I assume all the strange things happened on your side until proved
> otherwise. :-)

I think i just proved otherwise :)

Best Regards,
Martin

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length
  2020-07-27  7:58           ` Martin Botka
@ 2020-07-27  8:34             ` Uwe Kleine-König
  0 siblings, 0 replies; 31+ messages in thread
From: Uwe Kleine-König @ 2020-07-27  8:34 UTC (permalink / raw)
  To: Martin Botka
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Lee Jones,
	Linux LED Subsystem, devicetree, Linux Kernel Mailing List,
	linux-pwm, Andy Shevchenko

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module
  2020-07-24 21:36 ` [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module Martin Botka
@ 2020-07-27 20:09   ` Uwe Kleine-König
  2020-07-27 21:16     ` Martin Botka
  0 siblings, 1 reply; 31+ messages in thread
From: Uwe Kleine-König @ 2020-07-27 20:09 UTC (permalink / raw)
  To: Martin Botka
  Cc: Fenglin Wu, Jacek Anaszewski, Pavel Machek, Dan Murphy,
	Rob Herring, Thierry Reding, Lee Jones, linux-leds, devicetree,
	linux-kernel, linux-pwm

[-- Attachment #1: Type: text/plain, Size: 17785 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RCC 1/6] pwm: Add different PWM output types support
  2020-07-24 21:36 ` [PATCH RCC 1/6] pwm: Add different PWM output types support Martin Botka
@ 2020-07-27 20:10   ` Uwe Kleine-König
  2020-07-27 20:56     ` Martin Botka
  2020-07-27 21:46   ` Guru Das Srinagesh
  1 sibling, 1 reply; 31+ messages in thread
From: Uwe Kleine-König @ 2020-07-27 20:10 UTC (permalink / raw)
  To: Martin Botka
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Lee Jones, linux-leds,
	devicetree, linux-kernel, linux-pwm

[-- Attachment #1: Type: text/plain, Size: 7779 bytes --]

Hello,

On Fri, Jul 24, 2020 at 11:36:51PM +0200, Martin Botka wrote:
> From: Fenglin Wu <fenglinw@codeaurora.org>
> 
> Normally, PWM channel has fixed output until software request to change

"fixed" in the sense of "periodic", not "constant", right?

> its settings. There are some PWM devices which their outputs could be
> changed autonomously according to a predefined pattern programmed in
> hardware. Add pwm_output_type enum type to identify these two different
> PWM types and add relevant helper functions to set and get PWM output
> types and pattern.

You write "some devices". Which do you have in mind?

> [...]
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 2389b8669846..4ee1e81db0bc 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c

adapting the sysfs stuff should be done in a separate step.

> @@ -215,11 +215,60 @@ static ssize_t capture_show(struct device *child,
>  	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
>  }
>  
> +static ssize_t output_type_show(struct device *child,
> +							struct device_attribute *attr,
> +							char *buf)
> +{
> +	const struct pwm_device *pwm = child_to_pwm_device(child);
> +	const char *output_type = "unknown";
> +	struct pwm_state state;
> +
> +	pwm_get_state(pwm, &state);
> +	switch (state.output_type) {
> +	case PWM_OUTPUT_FIXED:
> +		output_type = "fixed";
> +		break;
> +	case PWM_OUTPUT_MODULATED:
> +		output_type = "modulated";
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", output_type);
> +}
> +
> +static ssize_t output_type_store(struct device *child,
> +								struct device_attribute *attr,
> +								const char *buf, size_t size)

Indention is broken here. Please align to the opening (.

> +{
> +	struct pwm_export *export = child_to_pwm_export(child);
> +	struct pwm_device *pwm = export->pwm;
> +	struct pwm_state state;
> +	int ret = -EINVAL;
> +
> +	mutex_lock(&export->lock);
> +	pwm_get_state(pwm, &state);
> +	if (sysfs_streq(buf, "fixed"))
> +		state.output_type = PWM_OUTPUT_FIXED;
> +	else if (sysfs_streq(buf, "modulated"))
> +		state.output_type = PWM_OUTPUT_MODULATED;
> +	else
> +		goto unlock;
> +
> +	ret = pwm_apply_state(pwm, &state);
> +unlock:
> +	mutex_unlock(&export->lock);
> +
> +	return ret ? : size;
> +}
> +
>  static DEVICE_ATTR_RW(period);
>  static DEVICE_ATTR_RW(duty_cycle);
>  static DEVICE_ATTR_RW(enable);
>  static DEVICE_ATTR_RW(polarity);
>  static DEVICE_ATTR_RO(capture);
> +static DEVICE_ATTR_RW(output_type);
>  
>  static struct attribute *pwm_attrs[] = {
>  	&dev_attr_period.attr,
> @@ -227,6 +276,7 @@ static struct attribute *pwm_attrs[] = {
>  	&dev_attr_enable.attr,
>  	&dev_attr_polarity.attr,
>  	&dev_attr_capture.attr,
> +	&dev_attr_output_type.attr,
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(pwm);
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 2635b2a55090..10a102efadc4 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -48,6 +48,29 @@ enum {
>  	PWMF_EXPORTED = 1 << 1,
>  };
>  
> +/*
> + * enum pwm_output_type - output type of the PWM signal
> + * @PWM_OUTPUT_FIXED: PWM output is fixed until a change request
> + * @PWM_OUTPUT_MODULATED: PWM output is modulated in hardware
> + * autonomously with a predefined pattern
> + */
> +enum pwm_output_type {
> +	PWM_OUTPUT_FIXED = 1 << 0,
> +	PWM_OUTPUT_MODULATED = 1 << 1,
> +};
> +
> +/*
> + * struct pwm_output_pattern - PWM duty pattern for MODULATED duty type
> + * @duty_pattern: PWM duty cycles in the pattern for duty modulation
> + * @num_entries: number of entries in the pattern
> + * @cycles_per_duty: number of PWM period cycles an entry stays at
> + */
> +struct pwm_output_pattern {
> +	unsigned int *duty_pattern;
> +	unsigned int num_entries;
> +	unsigned int cycles_per_duty;
> +};

I don't understand the semantics here. (i.e. how does a given
pwm_output_pattern map to the intended wave form?)

> +
>  /*
>   * struct pwm_state - state of a PWM channel
>   * @period: PWM period (in nanoseconds)
> @@ -59,6 +82,8 @@ struct pwm_state {
>  	unsigned int period;
>  	unsigned int duty_cycle;
>  	enum pwm_polarity polarity;
> +	enum pwm_output_type output_type;
> +	struct pwm_output_pattern *output_pattern;
>  	bool enabled;
>  };
>  
> @@ -146,6 +171,26 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
>  	return state.polarity;
>  }
>  
> +static inline enum pwm_output_type pwm_get_output_type(
> +				const struct pwm_device *pwm)
> +{
> +	struct pwm_state state;
> +
> +	pwm_get_state(pwm, &state);
> +
> +	return state.output_type;
> +}
> +
> +static inline struct pwm_output_pattern *pwm_get_output_pattern(
> +				struct pwm_device *pwm)
> +{
> +	struct pwm_state state;
> +
> +	pwm_get_state(pwm, &state);
> +
> +	return pwm->state.output_pattern ?: NULL;

Who is the owner of the data behind this pointer? Is it expected to be
valid only until the next call to change the output? What happens if the
caller modifies the data returned?

> +}
> +
>  static inline void pwm_get_args(const struct pwm_device *pwm,
>  				struct pwm_args *args)
>  {
> @@ -254,6 +299,9 @@ pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int duty_cycle,
>   * @set_polarity: configure the polarity of this PWM
>   * @enable: enable PWM output toggling
>   * @disable: disable PWM output toggling
> + * @get_output_type_supported: get the supported output type
> + * @set_output_type: set PWM output type
> + * @set_output_pattern: set the pattern for the modulated output
>   */
>  struct pwm_ops {
>  	int (*request)(struct pwm_chip *chip, struct pwm_device *pwm);
> @@ -273,6 +321,13 @@ struct pwm_ops {
>  			    enum pwm_polarity polarity);
>  	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
>  	void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
> +	int (*get_output_type_supported)(struct pwm_chip *chip,
> +			    struct pwm_device *pwm);
> +	int (*set_output_type)(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    enum pwm_output_type output_type);
> +	int (*set_output_pattern)(struct pwm_chip *chip,
> +			    struct pwm_device *pwm,
> +			    struct pwm_output_pattern *output_pattern);

This doesn't match the atomic approach that we're following since the
introduction of .apply. Please don't add new non-atomic callbacks.

>  };
>  
>  /**
> @@ -318,6 +373,20 @@ void pwm_free(struct pwm_device *pwm);
>  int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
>  int pwm_adjust_config(struct pwm_device *pwm);
>  
> +/*
> + * 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.

Having said that I wonder if this output pattern is a common enough
property to add support for it in the PWM framework.

If the only usage is to drive a LED this might maybe better be a
dedicated LED driver? (Though I'm not sure this is a good idea either.)

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RCC 1/6] pwm: Add different PWM output types support
  2020-07-27 20:10   ` Uwe Kleine-König
@ 2020-07-27 20:56     ` Martin Botka
  2020-07-28  7:52       ` Uwe Kleine-König
  0 siblings, 1 reply; 31+ messages in thread
From: Martin Botka @ 2020-07-27 20:56 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Lee Jones,
	Linux LED Subsystem, devicetree, Linux Kernel Mailing List,
	linux-pwm

Mo 27. 7. 2020 at 22:10 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Fri, Jul 24, 2020 at 11:36:51PM +0200, Martin Botka wrote:
> > From: Fenglin Wu <fenglinw@codeaurora.org>
> >
> > Normally, PWM channel has fixed output until software request to change
>
> "fixed" in the sense of "periodic", not "constant", right?

Correct.

>
> > its settings. There are some PWM devices which their outputs could be
> > changed autonomously according to a predefined pattern programmed in
> > hardware. Add pwm_output_type enum type to identify these two different
> > PWM types and add relevant helper functions to set and get PWM output
> > types and pattern.
>
> You write "some devices". Which do you have in mind?

As can be seen this commit is not authored by me so the commit message
is from the original author.
So i don't know what the original author meant here.
I can only guess that s/he meant all the PWM chips made by Qcom or
other companies that also have hardware based support for patterns.

>
> > [...]
> > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > index 2389b8669846..4ee1e81db0bc 100644
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
>
> adapting the sysfs stuff should be done in a separate step.

By that you mean in separate commit right ?

>
> > @@ -215,11 +215,60 @@ static ssize_t capture_show(struct device *child,
> >       return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
> >  }
> >
> > +static ssize_t output_type_show(struct device *child,
> > +                                                     struct device_attribute *attr,
> > +                                                     char *buf)
> > +{
> > +     const struct pwm_device *pwm = child_to_pwm_device(child);
> > +     const char *output_type = "unknown";
> > +     struct pwm_state state;
> > +
> > +     pwm_get_state(pwm, &state);
> > +     switch (state.output_type) {
> > +     case PWM_OUTPUT_FIXED:
> > +             output_type = "fixed";
> > +             break;
> > +     case PWM_OUTPUT_MODULATED:
> > +             output_type = "modulated";
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return snprintf(buf, PAGE_SIZE, "%s\n", output_type);
> > +}
> > +
> > +static ssize_t output_type_store(struct device *child,
> > +                                                             struct device_attribute *attr,
> > +                                                             const char *buf, size_t size)
>
> Indention is broken here. Please align to the opening (.

OK. Will do.

>
> > +{
> > +     struct pwm_export *export = child_to_pwm_export(child);
> > +     struct pwm_device *pwm = export->pwm;
> > +     struct pwm_state state;
> > +     int ret = -EINVAL;
> > +
> > +     mutex_lock(&export->lock);
> > +     pwm_get_state(pwm, &state);
> > +     if (sysfs_streq(buf, "fixed"))
> > +             state.output_type = PWM_OUTPUT_FIXED;
> > +     else if (sysfs_streq(buf, "modulated"))
> > +             state.output_type = PWM_OUTPUT_MODULATED;
> > +     else
> > +             goto unlock;
> > +
> > +     ret = pwm_apply_state(pwm, &state);
> > +unlock:
> > +     mutex_unlock(&export->lock);
> > +
> > +     return ret ? : size;
> > +}
> > +
> >  static DEVICE_ATTR_RW(period);
> >  static DEVICE_ATTR_RW(duty_cycle);
> >  static DEVICE_ATTR_RW(enable);
> >  static DEVICE_ATTR_RW(polarity);
> >  static DEVICE_ATTR_RO(capture);
> > +static DEVICE_ATTR_RW(output_type);
> >
> >  static struct attribute *pwm_attrs[] = {
> >       &dev_attr_period.attr,
> > @@ -227,6 +276,7 @@ static struct attribute *pwm_attrs[] = {
> >       &dev_attr_enable.attr,
> >       &dev_attr_polarity.attr,
> >       &dev_attr_capture.attr,
> > +     &dev_attr_output_type.attr,
> >       NULL
> >  };
> >  ATTRIBUTE_GROUPS(pwm);
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 2635b2a55090..10a102efadc4 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -48,6 +48,29 @@ enum {
> >       PWMF_EXPORTED = 1 << 1,
> >  };
> >
> > +/*
> > + * enum pwm_output_type - output type of the PWM signal
> > + * @PWM_OUTPUT_FIXED: PWM output is fixed until a change request
> > + * @PWM_OUTPUT_MODULATED: PWM output is modulated in hardware
> > + * autonomously with a predefined pattern
> > + */
> > +enum pwm_output_type {
> > +     PWM_OUTPUT_FIXED = 1 << 0,
> > +     PWM_OUTPUT_MODULATED = 1 << 1,
> > +};
> > +
> > +/*
> > + * struct pwm_output_pattern - PWM duty pattern for MODULATED duty type
> > + * @duty_pattern: PWM duty cycles in the pattern for duty modulation
> > + * @num_entries: number of entries in the pattern
> > + * @cycles_per_duty: number of PWM period cycles an entry stays at
> > + */
> > +struct pwm_output_pattern {
> > +     unsigned int *duty_pattern;
> > +     unsigned int num_entries;
> > +     unsigned int cycles_per_duty;
> > +};
>
> I don't understand the semantics here. (i.e. how does a given
> pwm_output_pattern map to the intended wave form?)

I did not understand that part either.
It's not uncommon for Qcom to have some stuff hidden like this in
other drivers that we have in downstream (Tho they mainly hide that in
pre-compiled binary blobs).
Here it's either the missing PBS driver from downstream that i want to
send later in separate series.
Or that it's fully done in hardware. Unlikely but you never know with Qcom.


>
> > +
> >  /*
> >   * struct pwm_state - state of a PWM channel
> >   * @period: PWM period (in nanoseconds)
> > @@ -59,6 +82,8 @@ struct pwm_state {
> >       unsigned int period;
> >       unsigned int duty_cycle;
> >       enum pwm_polarity polarity;
> > +     enum pwm_output_type output_type;
> > +     struct pwm_output_pattern *output_pattern;
> >       bool enabled;
> >  };
> >
> > @@ -146,6 +171,26 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
> >       return state.polarity;
> >  }
> >
> > +static inline enum pwm_output_type pwm_get_output_type(
> > +                             const struct pwm_device *pwm)
> > +{
> > +     struct pwm_state state;
> > +
> > +     pwm_get_state(pwm, &state);
> > +
> > +     return state.output_type;
> > +}
> > +
> > +static inline struct pwm_output_pattern *pwm_get_output_pattern(
> > +                             struct pwm_device *pwm)
> > +{
> > +     struct pwm_state state;
> > +
> > +     pwm_get_state(pwm, &state);
> > +
> > +     return pwm->state.output_pattern ?: NULL;
>
> Who is the owner of the data behind this pointer? Is it expected to be
> valid only until the next call to change the output? What happens if the
> caller modifies the data returned?
>
> > +}
> > +
> >  static inline void pwm_get_args(const struct pwm_device *pwm,
> >                               struct pwm_args *args)
> >  {
> > @@ -254,6 +299,9 @@ pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int duty_cycle,
> >   * @set_polarity: configure the polarity of this PWM
> >   * @enable: enable PWM output toggling
> >   * @disable: disable PWM output toggling
> > + * @get_output_type_supported: get the supported output type
> > + * @set_output_type: set PWM output type
> > + * @set_output_pattern: set the pattern for the modulated output
> >   */
> >  struct pwm_ops {
> >       int (*request)(struct pwm_chip *chip, struct pwm_device *pwm);
> > @@ -273,6 +321,13 @@ struct pwm_ops {
> >                           enum pwm_polarity polarity);
> >       int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
> >       void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
> > +     int (*get_output_type_supported)(struct pwm_chip *chip,
> > +                         struct pwm_device *pwm);
> > +     int (*set_output_type)(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                         enum pwm_output_type output_type);
> > +     int (*set_output_pattern)(struct pwm_chip *chip,
> > +                         struct pwm_device *pwm,
> > +                         struct pwm_output_pattern *output_pattern);
>
> This doesn't match the atomic approach that we're following since the
> introduction of .apply. Please don't add new non-atomic callbacks.
>
> >  };
> >
> >  /**
> > @@ -318,6 +373,20 @@ void pwm_free(struct pwm_device *pwm);
> >  int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
> >  int pwm_adjust_config(struct pwm_device *pwm);
> >
> > +/*
> > + * 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.
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.

Best Regards,
Martin

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module
  2020-07-27 20:09   ` Uwe Kleine-König
@ 2020-07-27 21:16     ` Martin Botka
  2020-07-28  7:08       ` Uwe Kleine-König
  0 siblings, 1 reply; 31+ messages in thread
From: Martin Botka @ 2020-07-27 21:16 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Fenglin Wu, Jacek Anaszewski, Pavel Machek, Dan Murphy,
	Rob Herring, Thierry Reding, Lee Jones, Linux LED Subsystem,
	devicetree, Linux Kernel Mailing List, linux-pwm

Hello,

Mo 27. 7. 2020 at 22:10 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
>
> On Fri, Jul 24, 2020 at 11:36:53PM +0200, Martin Botka wrote:
> > From: Fenglin Wu <fenglinw@codeaurora.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@codeaurora.org>
> > [martin.botka1@gmail.com: Fast-forward the driver from kernel 4.14 to 5.8]
> > Signed-off-by: Martin Botka <martin.botka1@gmail.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.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RCC 1/6] pwm: Add different PWM output types support
  2020-07-24 21:36 ` [PATCH RCC 1/6] pwm: Add different PWM output types support Martin Botka
  2020-07-27 20:10   ` Uwe Kleine-König
@ 2020-07-27 21:46   ` Guru Das Srinagesh
  1 sibling, 0 replies; 31+ messages in thread
From: Guru Das Srinagesh @ 2020-07-27 21:46 UTC (permalink / raw)
  To: Martin Botka
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Uwe Kleine-König,
	Lee Jones, linux-leds, devicetree, linux-kernel, linux-pwm

On Fri, Jul 24, 2020 at 11:36:51PM +0200, Martin Botka wrote:
> From: Fenglin Wu <fenglinw@codeaurora.org>
> 
> Normally, PWM channel has fixed output until software request to change
> its settings. There are some PWM devices which their outputs could be
> changed autonomously according to a predefined pattern programmed in
> hardware. Add pwm_output_type enum type to identify these two different
> PWM types and add relevant helper functions to set and get PWM output
> types and pattern.
> 
> Signed-off-by: Fenglin Wu <fenglinw@codeaurora.org>
> [konradybcio@gmail.com: Fast-forward from kernel 4.14 to 5.8]
> Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
> Signed-off-by: Martin Botka <martin.botka1@gmail.com>

Hello,

Re-sending my reply as somehow the cc-fields got dropped when I posted
earlier.

This was the feedback received from the maintainers when I posted this
patch last year [1]. Accordingly, we updated the patch to drop
"output_pattern" altogether and made "output_type" read-only [2] -
haven't attempted upstreaming this version yet.

[1]: https://lore.kernel.org/lkml/20190916140146.GC7488@ulmo/
[2]: https://android-review.googlesource.com/c/kernel/common/+/1170135

Thank you.

Guru Das.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module
  2020-07-27 21:16     ` Martin Botka
@ 2020-07-28  7:08       ` Uwe Kleine-König
  0 siblings, 0 replies; 31+ messages in thread
From: Uwe Kleine-König @ 2020-07-28  7:08 UTC (permalink / raw)
  To: Martin Botka
  Cc: Fenglin Wu, Jacek Anaszewski, Pavel Machek, Dan Murphy,
	Rob Herring, Thierry Reding, Lee Jones, Linux LED Subsystem,
	devicetree, Linux Kernel Mailing List, linux-pwm

[-- Attachment #1: Type: text/plain, Size: 4680 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH RCC 1/6] pwm: Add different PWM output types support
  2020-07-27 20:56     ` Martin Botka
@ 2020-07-28  7:52       ` Uwe Kleine-König
  0 siblings, 0 replies; 31+ messages in thread
From: Uwe Kleine-König @ 2020-07-28  7:52 UTC (permalink / raw)
  To: Martin Botka
  Cc: Fenglin Wu, Konrad Dybcio, Jacek Anaszewski, Pavel Machek,
	Dan Murphy, Rob Herring, Thierry Reding, Lee Jones,
	Linux LED Subsystem, devicetree, Linux Kernel Mailing List,
	linux-pwm

[-- Attachment #1: Type: text/plain, Size: 2015 bytes --]

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 <u.kleine-koenig@pengutronix.de> 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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2020-07-28  7:53 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 21:36 [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Martin Botka
2020-07-24 21:36 ` [PATCH RCC 1/6] pwm: Add different PWM output types support Martin Botka
2020-07-27 20:10   ` Uwe Kleine-König
2020-07-27 20:56     ` Martin Botka
2020-07-28  7:52       ` Uwe Kleine-König
2020-07-27 21:46   ` Guru Das Srinagesh
2020-07-24 21:36 ` [PATCH RFC 2/6] pwm: core: Add option to config PWM duty/period with u64 data length Martin Botka
2020-07-25 14:55   ` Martin Botka
2020-07-25 15:24     ` Pavel Machek
2020-07-25 15:30       ` Martin Botka
2020-07-26  9:04   ` Andy Shevchenko
2020-07-26  9:12     ` Martin Botka
2020-07-27  7:29       ` Martin Botka
2020-07-27  7:32         ` Martin Botka
2020-07-27  7:52         ` Uwe Kleine-König
2020-07-27  7:58           ` Martin Botka
2020-07-27  8:34             ` Uwe Kleine-König
2020-07-24 21:36 ` [PATCH RFC 3/6] pwm: pwm-qti-lpg: Add PWM driver for QTI LPG module Martin Botka
2020-07-27 20:09   ` Uwe Kleine-König
2020-07-27 21:16     ` Martin Botka
2020-07-28  7:08       ` Uwe Kleine-König
2020-07-24 21:36 ` [PATCH RFC 4/6] leds: leds-qti-tri-led: Add LED driver for QTI TRI_LED module Martin Botka
2020-07-26 17:25   ` Martin Botka
2020-07-24 21:36 ` [PATCH RFC 5/6] Documentation: Add binding for qti-tri-led Martin Botka
2020-07-24 21:36 ` [PATCH RFC 6/6] Documentation: Add binding for pwm-qti-lpg Martin Botka
2020-07-24 22:05 ` [PATCH RFC 0/6] Add QCOM pwm-lpg and tri-led drivers Pavel Machek
2020-07-24 22:26   ` Martin Botka
2020-07-24 22:32     ` Pavel Machek
2020-07-24 22:48       ` Martin Botka
     [not found]   ` <CADQ2G_HbysJbiQKSRmA6HDRfjPYPiDjbZfeuUjM1mNV-BBBC-A@mail.gmail.com>
2020-07-24 22:31     ` Pavel Machek
2020-07-24 22:46       ` Martin Botka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).