linux-kernel - Re: [PATCH V2 1/3] drivers/pwm st_pwm: Add support for ST's Pulse Width Modulator

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: <4DEDA12D.6080406@st.com>
Date:	Tue, 7 Jun 2011 09:25:25 +0530
From:	viresh kumar <viresh.kumar@...com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"arnd@...db.de" <arnd@...db.de>,
	Armando VISCONTI <armando.visconti@...com>,
	Shiraz HASHIM <shiraz.hashim@...com>,
	Vipin KUMAR <vipin.kumar@...com>,
	Rajeev KUMAR <rajeev-dlh.kumar@...com>,
	Deepak SIKRI <deepak.sikri@...com>,
	Vipul Kumar SAMAR <vipulkumar.samar@...com>,
	Amit VIRDI <Amit.VIRDI@...com>,
	Pratyush ANAND <pratyush.anand@...com>,
	Bhupesh SHARMA <bhupesh.sharma@...com>,
	"viresh.linux@...il.com" <viresh.linux@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 1/3] drivers/pwm st_pwm: Add support for ST's Pulse
 Width Modulator

On 06/07/2011 06:03 AM, Andrew Morton wrote:
> On Tue, 31 May 2011 14:21:51 +0530
> Viresh Kumar <viresh.kumar@...com> wrote:

>> + * lock: lock specific to a pwm device
> 
> More specificity here would be helpful.  Precisely which data does the
> lock protect?
> 

>> + * lock: lock specific to current pwm ip
> 
> Ditto.
> 

Ok.

>> +int pwm_config(struct pwm_device *pwmd, int duty_ns, int period_ns)
>> +{
>> +	u64 val, div, clk_rate;
>> +	unsigned long prescale = MIN_PRESCALE, pv, dc;
>> +	int ret = 0;
>> +
>> +	if (!pwmd) {
>> +		pr_err("pwm: config - NULL pwm device pointer\n");
>> +		return -EFAULT;
>> +	}
>> +
>> +	if (period_ns == 0 || duty_ns > period_ns) {
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	/* TODO: Need to optimize this loop */
>> +	while (1) {
>> +		div = 1000000000;
>> +		div *= 1 + prescale;
>> +		clk_rate = clk_get_rate(pwmd->pwm->clk);
>> +		val = clk_rate * period_ns;
>> +		pv = div64_u64(val, div);
>> +		val = clk_rate * duty_ns;
>> +		dc = div64_u64(val, div);
>> +
>> +		if ((pv == 0) || (dc == 0)) {
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>> +		if ((pv > MAX_PERIOD) || (dc > MAX_DUTY)) {
>> +			prescale++;
>> +			if (prescale > MAX_PRESCALE) {
>> +				ret = -EINVAL;
>> +				goto err;
>> +			}
>> +			continue;
>> +		}
>> +		if ((pv < MIN_PERIOD) || (dc < MIN_DUTY)) {
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>> +		break;
>> +	}
> 
> gee, is this some sort of puzzle?  So human-readable description of
> what this code is doing would be an improvement.
> 

Sure. Will add that.

>> +	spin_lock(&pwmd->pwm->lock);
>> +	ret = clk_enable(pwmd->pwm->clk);
>> +	if (ret) {
>> +		spin_unlock(&pwmd->pwm->lock);
>> +		goto err;
>> +	}
>> +
>> +	spin_lock(&pwmd->lock);
>> +	writel(prescale << PRESCALE_SHIFT, pwmd->pwm->mmio_base +
>> +			pwmd->offset + PWMCR);
>> +	writel(dc, pwmd->pwm->mmio_base + pwmd->offset + PWMDCR);
>> +	writel(pv, pwmd->pwm->mmio_base + pwmd->offset + PWMPCR);
>> +	spin_unlock(&pwmd->lock);
>> +	clk_disable(pwmd->pwm->clk);
>> +	spin_unlock(&pwmd->pwm->lock);
> 
> The nesting rules for these two locks seems sensible and obvious, but I
> guess documenting the rule wouldn't hurt.
> 

Ok.

>> +	return 0;
>> +err:
>> +	dev_err(&pwmd->pwm->pdev->dev, "pwm config fail\n");
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(pwm_config);
>> +
>>
>> ...
>>
>> +static int __devinit st_pwm_probe(struct platform_device *pdev)
> 
> And here things get rather odd.
> 
> Most of this file is a generic, non-device specific PWM layer, exported
> to other modules.  But then we get into driver bits which are specific
> to one paritular type of device.  Confused - this is like putting the
> e100 driver inside net/ipv4/tcp.c?
> 

Sorry but i couldn't get this one completely. :(
Driver is specific to pwm peripheral by ST. This driver can be used for
SPEAr or may be other SoC or Devices, and is not at all dependent on SPEAr.

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