linux-kernel - RE: [PATCH 5/8] pwm: pwm-tiehrpwm: Add device-tree binding support for EHRPWM driver

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: <B5906170F1614E41A8A28DE3B8D121433EC0447E@DBDE01.ent.ti.com>
Date:	Tue, 6 Nov 2012 06:46:13 +0000
From:	"Bedia, Vaibhav" <vaibhav.bedia@...com>
To:	"Philip, Avinash" <avinashphilip@...com>,
	"thierry.reding@...onic-design.de" <thierry.reding@...onic-design.de>,
	"paul@...an.com" <paul@...an.com>,
	"tony@...mide.com" <tony@...mide.com>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"Cousson, Benoit" <b-cousson@...com>
CC:	Rob Landley <rob@...dley.net>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"Nori, Sekhar" <nsekhar@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Hiremath, Vaibhav" <hvaibhav@...com>,
	"Hebbar, Gururaja" <gururaja.hebbar@...com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	"AnilKumar, Chimata" <anilkumar@...com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH 5/8] pwm: pwm-tiehrpwm: Add device-tree binding support
 for EHRPWM driver

On Mon, Nov 05, 2012 at 14:42:26, Philip, Avinash wrote:
[...]

> +#include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>

Pinctrl changes should be separate patch. Morevoer, you don't mention
that you making this change.

> +
> +#include "tipwmss.h"
>  
>  /* EHRPWM registers and bits definitions */
>  
> @@ -107,6 +111,10 @@
>  #define AQCSFRC_CSFA_FRCHIGH	BIT(1)
>  #define AQCSFRC_CSFA_DISSWFRC	(BIT(1) | BIT(0))
>  
> +#define EPWMCLK_EN_SHIFT	8
> +
> +#define PWM_CELL_SIZE		3
> +
>  #define NUM_PWM_CHANNEL		2	/* EHRPWM channels */
>  
>  struct ehrpwm_pwm_chip {
> @@ -392,12 +400,27 @@ static const struct pwm_ops ehrpwm_pwm_ops = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id ehrpwm_of_match[] = {
> +	{
> +		.compatible	= "ti,am33xx-ehrpwm",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ehrpwm_of_match);
> +#endif
> +
>  static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
>  {
>  	int ret;
>  	struct resource *r;
>  	struct clk *clk;
>  	struct ehrpwm_pwm_chip *pc;
> +	struct pinctrl *pinctrl;
> +
> +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);

I didn't see a patch adding the pinctrl entries.

> +	if (IS_ERR(pinctrl))
> +		dev_warn(&pdev->dev, "failed to configure pins from driver\n");
>  
>  	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
>  	if (!pc) {
> @@ -419,6 +442,7 @@ static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
>  
>  	pc->chip.dev = &pdev->dev;
>  	pc->chip.ops = &ehrpwm_pwm_ops;
> +	pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
>  	pc->chip.base = -1;
>  	pc->chip.npwm = NUM_PWM_CHANNEL;
>  
> @@ -437,8 +461,11 @@ static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
>  		return ret;
>  	}
> -
>  	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_get_sync(&pdev->dev);
> +	pwmss_submodule_state_change(pdev->dev.parent, EPWMCLK_EN_SHIFT, true);

I think you should modify this API to return the status for drivers to check.

Another related question, why should this clock be enabled in probe and not only when it
is required? Shouldn't it be disabled in suspend?

Regards,
Vaibhav
--
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/