linux-kernel - Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <409ee148-a6a6-327f-e4d7-455f98ef4c66@linux.intel.com>
Date:   Thu, 23 Jul 2020 12:16:18 +0800
From:   "Tanwar, Rahul" <rahul.tanwar@...ux.intel.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     linux-pwm@...r.kernel.org, thierry.reding@...il.com,
        p.zabel@...gutronix.de, robh+dt@...nel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        andriy.shevchenko@...el.com, songjun.Wu@...el.com,
        cheol.yong.kim@...el.com, qi-ming.wu@...el.com,
        rahul.tanwar.linux@...il.com
Subject: Re: [PATCH v4 2/2] Add PWM fan controller driver for LGM SoC


Hi Uwe,

Thanks for the feedback.

On 14/7/2020 3:10 am, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Jun 30, 2020 at 03:55:32PM +0800, Rahul Tanwar wrote:
>> 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 <rahul.tanwar@...ux.intel.com>
>> ---
>>  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

[...]

>> +
>> +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.

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.

[...]
>> +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()."

Regards,
Rahul