Hello,

On Thu, Jul 23, 2020 at 12:16:18PM +0800, Tanwar, Rahul wrote:

On 14/7/2020 3:10 am, Uwe Kleine-König wrote:

quoted

Hello,

On Tue, Jun 30, 2020 at 03:55:32PM +0800, Rahul Tanwar wrote:

quoted

Intel Lightning Mountain(LGM) SoC contains a PWM fan controller.
This PWM controller does not have any other consumer, it is a
dedicated PWM controller for fan attached to the system. Add
driver for this PWM fan controller.

Signed-off-by: Rahul Tanwar <redacted>
---
 drivers/pwm/Kconfig         |  11 ++
 drivers/pwm/Makefile        |   1 +
 drivers/pwm/pwm-intel-lgm.c | 266 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 278 insertions(+)
 create mode 100644 drivers/pwm/pwm-intel-lgm.c

[...]

quoted

quoted

+
+static int lgm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
+	u32 duty_cycle, val;
+	unsigned int period;
+
+	if (!state->enabled) {
+		lgm_pwm_enable(chip, 0);
+		return 0;
+	}
+
+	period = min_t(u64, state->period, pc->period);
+
+	if (state->polarity != PWM_POLARITY_NORMAL ||
+	    period < pc->period)
+		return -EINVAL;

This check looks wrong. If you refuse period < pc->period there isn't
much configuration possible.

I am kind of stuck here. I made this change of adding a check
period < pc->period based on your feedback on v2 patch.
In fact, you had specified this code in v2 review feedback
and i used the same exact code.

My feedback was nonsense, sorry. Now I don't understand anymore what I
thought. The check you proposed in v2 is perfectly fine.

How should we handle it when the hardware supports fixed period.
We don't want user to change period and allow just changing
duty_cycle. With that intention, i had first added a strict check
which refused configuration if period != pc->period. Period is
intended to be a read only value.

How do you suggest we handle the fixed period hardware support?
Would you have any reference example of other drivers which also
supports fixed period? Thanks.

[...]

quoted

quoted

+static int lgm_pwm_remove(struct platform_device *pdev)
+{
+	struct lgm_pwm_chip *pc = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&pc->chip);
+	if (ret < 0)
+		return ret;
+
+	clk_disable_unprepare(pc->clk);
+	reset_control_assert(pc->rst);

Please swap the two previous lines to match the error patch of .probe.

Again, i had made this change based on your below review feedback
for v1. IMO, reverse of probe makes more sense.

"In .probe() you first release reset and then enable the clock. It's good
style to do it the other way round in .remove()."

Again my comment was nonsense, I'm sorry. I think I was irritated by the
fact that reset handling happens in between the clk stuff in probe. You
do there:

	devm_clk_get
	devm_reset_control_get_exclusive
	reset_control_deassert
	clk_prepare_enable

and I noticed that as "in probe clk handling is done first".

Looking at the other feedback I think my other comments are not BS.

Best regards and sorry again,
Uwe

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