Re: [PATCH v1 2/2] Add PWM driver for LGM

[Tanwar, Rahul]

Hi Uwe,

Thanks for review.

On 22/5/2020 4:56 pm, Uwe Kleine-KÃnig wrote:
> Hello,
>
> On Fri, May 22, 2020 at 03:41:59PM +0800, Rahul Tanwar wrote:
>> Add PWM controller driver for Intel's Lightning Mountain(LGM) SoC.
>>
>> Signed-off-by: Rahul Tanwar <rahul.tanwar@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/pwm/Kconfig         |   9 ++
>>  drivers/pwm/Makefile        |   1 +
>>  drivers/pwm/pwm-intel-lgm.c | 356 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 366 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-intel-lgm.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index eebbc917ac97..a582214f50b2 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -232,6 +232,15 @@ config PWM_IMX_TPM
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called pwm-imx-tpm.
>>  
>> +config PWM_INTEL_LGM
>> +	tristate "Intel LGM PWM support"
>> +	depends on X86 || COMPILE_TEST
>> +	help
>> +	  Generic PWM framework driver for LGM SoC.
[...]
>> +};
>> +
>> +static void tach_work(struct work_struct *work)
>> +{
>> +	struct intel_pwm_chip *pc = container_of(work, struct intel_pwm_chip,
>> +						 work.work);
>> +	struct regmap *regmap = pc->regmap;
>> +	u32 fan_tach, fan_dc, val;
>> +	s32 diff;
>> +	static u32 fanspeed_err_cnt, time_window, delta_dc;
>> +
>> +	/*
>> +	 * Fan speed is tracked by reading the active duty cycle of PWM output
>> +	 * from the active duty cycle register. Some variance in the duty cycle
>> +	 * register value is expected. So we set a time window of 30 seconds and
>> +	 * if we detect inaccurate fan speed 6 times within 30 seconds then we
>> +	 * mark it as fan speed problem and fix it by readjusting the duty cycle.
>> +	 */
> I'm a unhappy to have this in the PWM driver. The PWM driver is supposed
> to be generic and I think this belongs into a dedicated driver.

Well noted about all other review concerns. I will rework the driver in v2.
However, i am not very sure about the above point - of having a separate
dedicated driver for tach_work because its logic is tightly coupled with
this driver.

Regards,
Rahul