Re: [PATCH v5 2/2] Add PWM fan controller driver for LGM SoC

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

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

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