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

["Tanwar, Rahul" <rahul.tanwar@linux.intel.com>]

Hi Uwe,

On 24/7/2020 12:15 am, Uwe Kleine-König wrote:
> 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.

For below case:

.enabled = false
.polarity = PWM_POLARITY_INVERSED

Since our HW does not support inversed polarity, the output for above case
is expected to be constant low. And if we disable PWM before checking for
polarity, the output becomes constant low. The code just does that. Sorry,
i could not understand what is wrong with the code. It looks correct to me.

Given the fact that we support fixed period, if we allow
state->period < pc->period case then the duty cycle will be evaluated as
higher than the requested one because the state->period is lesser than
the actual fixed period supported by the HW. Can you please elaborate
on why you think we should allow state->period < pc->period case?

Thanks,

Regards,
Rahul