linux-kernel - Re: [PWM PATCH 2/7] Emulates PWM hardware using a high-resolution timer and a GPIO pin

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>] [day] [month] [year] [list]

Date:	Wed, 10 Feb 2010 07:42:57 -0600
From:	Bill Gatliff <bgat@...lgatliff.com>
To:	"Stanislav O. Bezzubtsev" <stas@....cs.msu.su>
CC:	linux-embedded@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PWM PATCH 2/7] Emulates PWM hardware using a high-resolution
 timer and a GPIO pin

Stanislav O. Bezzubtsev wrote:
>> +
>> +struct gpio_pwm {
>> +	struct pwm_device pwm;
>> +	struct hrtimer t;
>>     
>
> Wouldn't a little bit longer name "timer" instead of simple "t" increase readability?
>   

Couldn't hurt.  Done.

>> +static void
>> +gpio_pwm_work (struct work_struct *work)
>> +{
>> +	struct gpio_pwm *gp = container_of(work, struct gpio_pwm, work);
>> +
>> +	if (gp->active)
>> +		gpio_direction_output(gp->gpio, gp->polarity ? 1 : 0);
>> +	else
>> +		gpio_direction_output(gp->gpio, gp->polarity ? 0 : 1);
>>     
>
> Maybe the following would be better:
> gpio_direction_output(gp->gpio, gp->polarity ^ gp->active)
> Instead of doing several comparisons.
>   

... except that I'm trying to guarantee that only the values '1' or '0'
get sent to gpio_direction_output.  There's nothing in the spec that
says other values are legal, although I'll admit that any nonzero value
is unlikely to cause problems.  Should I be pedantic here?

>> +
>> +	if (gp->active)
>> +		hrtimer_start(&gp->t,
>> +			      ktime_set(0, gp->pwm.channels[0].duty_ticks),
>> +			      HRTIMER_MODE_REL);
>> +	else
>> +		hrtimer_start(&gp->t,
>> +			      ktime_set(0,gp->pwm.channels[0].period_ticks
>> +					- gp->pwm.channels[0].duty_ticks),
>> +			      HRTIMER_MODE_REL);
>>     
>
> if (gp->active)
> 	t =  ktime_set(0, gp->pwm.channels[0].duty_ticks));
> else
> 	t = ktime_set(0, gp->pwm.channels[0].period_ticks - gp->pwm.channels[0].duty_ticks));
>
> htimer_start(&gp->t, t, HRTIMER_MODE_REL);
>   

Excellent.


>> +
>> +	ret = pwm_register(&gp->pwm);
>> +	if (ret)
>> +		goto err_pwm_register;
>> +
>> +	return 0;
>> +
>> +err_pwm_register:
>>     
>
> platform_set_drvdata(pdev, 0);
>   

Good catch!


>> +static int __devexit
>> +gpio_pwm_remove(struct platform_device *pdev)
>> +{
>> +	struct gpio_pwm *gp = platform_get_drvdata(pdev);
>> +	int ret;
>> +
>> +	ret = pwm_unregister(&gp->pwm);
>> +	hrtimer_cancel(&gp->t);
>> +	cancel_work_sync(&gp->work);
>>     
>
> platform_set_drvdata(pdev, 0);
>   

Ditto.

> And there are too much pr_debug & dev_dbg calls. Several of them are inside critical sections or in functions called from critical sections (inside spin_lock_irqsave - spin_lock_irqrestore block I mean). Don't think it is good.
>   

Ok.  Now that the code is relatively mature, they're unnecessary anyway.


b.g.

-- 
Bill Gatliff
Embedded systems training and consulting
http://billgatliff.com
bgat@...lgatliff.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/