Re: [PATCH 1/2] pwm: pwm-tiecap: Add device-tree binding support forAPWM driver

[Thierry Reding]

On Wed, Sep 26, 2012 at 04:57:42PM +0530, Philip, Avinash wrote:
[...]
> +#include <linux/platform_data/ti-pwmss.h>
[...]
> +static struct pwmss_platform_data am33xx_data = {
> +	.has_configspace	= true,
> +};

This structure is defined in a public header. I don't see why it has to,
given that it's only used to associate some data with an of_device_id
entry below. Since AM33xx never had anything but OF support in the
mainline kernel I don't think we should add platform data.

> +#ifdef CONFIG_OF
> +static const struct of_device_id ecap_of_match[] = {
> +	{
> +		.compatible	= "ti,am33xx-ecap",
> +		.data		= &am33xx_data,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ecap_of_match);
> +#endif
> +

I don't quite see why we need to pass this platform data to the device
at all since there is no other variant anyway. I think this should only
be added if this turns out to be required at some point.

>  static int __devinit ecap_pwm_probe(struct platform_device *pdev)
>  {
>  	int ret;
>  	struct resource *r;
>  	struct clk *clk;
>  	struct ecap_pwm_chip *pc;
> +	struct pwmss_platform_data *pdata = pdev->dev.platform_data;
> +	const struct of_device_id *match;
> +	u16 regval;
> +	struct pinctrl *pinctrl;
> +
> +	match = of_match_device(of_match_ptr(ecap_of_match), &pdev->dev);

I've never seen of_match_ptr() used this way. Maybe you should think
about not #ifdef'ing OF specific code at all. That way ecap_of_match
will always exist and you could use the IS_ENABLED() macro to
conditionalize at compile time. The compiler will probably be clever
enough to optimize the ecap_of_match away if OF is not enabled.

Given my comment earlier, since AM33xx is OF only you might just as well
add a depends on OF to this driver and things will become a lot easier.

> +
> +	if (match)
> +		pdata = (struct pwmss_platform_data *)match->data;
> +
> +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> +	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) {
> @@ -211,6 +268,8 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
>  
>  	pc->chip.dev = &pdev->dev;
>  	pc->chip.ops = &ecap_pwm_ops;
> +	pc->chip.of_xlate = of_ecap_xlate;
> +	pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
>  	pc->chip.base = -1;
>  	pc->chip.npwm = 1;
>  
> @@ -231,13 +290,56 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev)
>  	}
>  
>  	pm_runtime_enable(&pdev->dev);
> +
> +	/*
> +	 * Some platform has extra PWM-subsystem common config space
> +	 * and requires special handling of clock gating.
> +	 */
> +	if (pdata && pdata->has_configspace) {
> +		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		if (!r) {
> +			dev_err(&pdev->dev, "no memory resource defined\n");
> +			ret = -ENODEV;
> +			goto err_disable_clock;
> +		}
> +
> +		pc->config_base = devm_ioremap(&pdev->dev, r->start,
> +				resource_size(r));
> +		if (!pc->config_base) {
> +			dev_err(&pdev->dev, "failed to ioremap() registers\n");
> +			ret = -EADDRNOTAVAIL;
> +			goto err_disable_clock;
> +		}

Isn't this missing a request_mem_region()? I assume you don't do that
here because you need the same region in the EHRPWM driver, right? This
should be indication enough that the design is not right here. I think
we need a proper abstraction here. Can this not be done via PM runtime
support? If not, maybe this should be represented by clock objects since
the bit obviously enables a clock.

> +
> +		/* Enable ECAP clock gating at PWM-subsystem common config */
> +		pm_runtime_get_sync(&pdev->dev);
> +		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
> +		regval |= PWMSS_ECAP_CLK_EN;
> +		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
> +		pm_runtime_put_sync(&pdev->dev);
> +	}
> +
>  	platform_set_drvdata(pdev, pc);
>  	return 0;
> +
> +err_disable_clock:
> +	pm_runtime_disable(&pdev->dev);
> +	return ret;
>  }
>  
>  static int __devexit ecap_pwm_remove(struct platform_device *pdev)
>  {
>  	struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
> +	u16 regval;
> +
> +	if (pc->config_base) {
> +		/* Disable ECAP clock gating at PWM-subsystem common config */
> +		pm_runtime_get_sync(&pdev->dev);
> +		regval = readw(pc->config_base + PWMSS_CLKCONFIG);
> +		regval &= ~PWMSS_ECAP_CLK_EN;
> +		writew(regval, pc->config_base + PWMSS_CLKCONFIG);
> +		pm_runtime_put_sync(&pdev->dev);
> +	}
>  
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> @@ -247,6 +349,9 @@ static int __devexit ecap_pwm_remove(struct platform_device *pdev)
>  static struct platform_driver ecap_pwm_driver = {
>  	.driver = {
>  		.name = "ecap",
> +#ifdef CONFIG_OF
> +		.of_match_table = of_match_ptr(ecap_of_match),
> +#endif

If you use of_match_ptr() you don't need the additional #ifdef
CONFIG_OF. But as I already said I don't think you need this at all
anyway. You should really just depend on OF and be done with it.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature