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:
pgpuDjvnjNDFI.pgp
Description: PGP signature
