Hello,

On Thu, Jul 23, 2020 at 03:44:18PM +0800, Rahul Tanwar wrote:

+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;
+	int ret;
+
+	if (!state->enabled) {
+		ret = lgm_pwm_enable(chip, 0);
+		return ret;
+	}
+
+	/*
+	 * HW only supports NORMAL polarity
+	 * HW supports fixed period which can not be changed/configured by user
+	 */
+	if (state->polarity != PWM_POLARITY_NORMAL ||
+	    state->period != pc->period)
+		return -EINVAL;

At least for state->polarity you have to check before state->enabled, as
the expectation on

        .enabled = false
        .polarity = PWM_POLARITY_INVERSED

is that the output becomes constant high. Also as confirmed at the end
of v4, state->period < pc->period was the right check to do.

+	duty_cycle = min_t(u64, state->duty_cycle, state->period);
+	/* reg_value = duty_ns * LGM_PWM_MAX_DUTY_CYCLE(0xff) / period_ns */
+	val = duty_cycle * LGM_PWM_MAX_DUTY_CYCLE / state->period;

The comment adds only little compared to the code line below. Please
drop it.

Dividing by state->period is wrong. I think you need pc->period here.

+	ret = regmap_update_bits(pc->regmap, LGM_PWM_FAN_CON0, LGM_PWM_FAN_DC_MSK,
+				 FIELD_PREP(LGM_PWM_FAN_DC_MSK, val));
+	if (ret)
+		return ret;
+
+	if (state->enabled)
+		ret = lgm_pwm_enable(chip, 1);
+
+	return ret;
+}
+
+static void lgm_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			      struct pwm_state *state)
+{
+	struct lgm_pwm_chip *pc = to_lgm_pwm_chip(chip);
+	u32 duty, val;
+
+	state->enabled = regmap_test_bits(pc->regmap, LGM_PWM_FAN_CON0,
+					  LGM_PWM_FAN_EN_EN);
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->period = pc->period; /* fixed period */
+
+	regmap_read(pc->regmap, LGM_PWM_FAN_CON0, &val);
+	duty = FIELD_GET(LGM_PWM_FAN_DC_MSK, val);
+	state->duty_cycle = DIV_ROUND_UP(duty * pc->period,
+					 LGM_PWM_MAX_DUTY_CYCLE);
+}
+
+static const struct pwm_ops lgm_pwm_ops = {
+	.get_state = lgm_pwm_get_state,
+	.apply = lgm_pwm_apply,
+	.owner = THIS_MODULE,
+};
+
+static void lgm_pwm_init(struct lgm_pwm_chip *pc)
+{
+	struct device *dev = pc->chip.dev;
+	struct regmap *regmap = pc->regmap;
+	u32 max_rpm, fan_wire, con0_val, con0_mask;
+
+	if (device_property_read_u32(dev, "intel,fan-wire", &fan_wire))
+		fan_wire = 2; /* default is 2 wire mode */
+
+	con0_mask = LGM_PWM_FAN_MODE_MSK;
+
+	switch (fan_wire) {
+	case 4:
+		con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK, LGM_PWM_FAN_MODE_4WIRE);
+		pc->period = LGM_PWM_PERIOD_4WIRE_NSECS;
+		break;
+	default:
+		/* default is 2wire mode */
+		con0_val = FIELD_PREP(LGM_PWM_FAN_MODE_MSK, LGM_PWM_FAN_MODE_2WIRE);
+		pc->period = LGM_PWM_PERIOD_2WIRE_NSECS;
+		break;
+	}
+
+	if (device_property_read_u32(dev, "intel,max-rpm", &max_rpm))
+		max_rpm = LGM_PWM_DEFAULT_RPM;

This property isn't in the binding!?

+	max_rpm = min_t(u32, max_rpm, LGM_PWM_MAX_RPM);
+	if (max_rpm == 0)
+		max_rpm = LGM_PWM_DEFAULT_RPM;
+
+	regmap_update_bits(regmap, LGM_PWM_FAN_CON1, LGM_PWM_FAN_MAX_RPM_MSK, max_rpm);
+	regmap_update_bits(regmap, LGM_PWM_FAN_CON0, con0_mask, con0_val);
+}

Best regards
Uwe

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