[PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM.

[Philip, Avinash]

On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote:
> On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote:
[snip]
> >  obj-$(CONFIG_PWM_VT8500)	+= pwm-vt8500.o
> > +obj-$(CONFIG_PWM_ECAP)		+= pwm-ecap.o
> 
> Both the Kconfig and Makefile should have the entries ordered
> alphabetically.

Ok. I will correct it.

> 
[snip]
> > +/* ECAP registers and bits definitions */
> > +#define TSCTR			0x00
> > +#define CTRPHS			0x04
> > +#define CAP1			0x08
> > +#define CAP2			0x0C
> > +#define CAP3			0x10
> > +#define CAP4			0x14
> > +#define ECCTL1			0x28
> 
> These registers are not used. I guess there is some use to list all
> registers here but on the other hand the majority are unused so they
> just clutter the driver.


> 
> > +#define ECCTL2_APWM_POL_LOW	BIT(10)
> 
> This bit is never used.

> 
> > +#define ECEINT			0x2C
> > +#define ECFLG			0x2E
> > +#define ECCLR			0x30
> > +#define REVID			0x5c
> 
> These are unused as well.

Ok. I will remove it. These are been placed in future enhancement.

> 
> > +
> > +#define DRIVER_NAME	"ecap"
> 
> You only use this once when defining the struct platform_driver, so
> maybe you can just drop it.

In the v2 patch, I am planning to use same in
platform_get_resource_byname(). Here I will use this define to search
resources.

> 
> > +
> > +struct ecap_pwm_chip {
> > +	struct pwm_chip	chip;
> > +	unsigned int	clk_rate;
> > +	void __iomem	*mmio_base;
> > +	int		pwm_period_ns;
> > +	int		pwm_duty_ns;
> > +};
> 
> The pwm_period_ns and pwm_duty_ns don't seem to be used at all. Can they
> be dropped?

Ok. I will remove it. 

> 
> > +	/*
> > +	 * Enable 'Free run Time stamp counter mode' to start counter
> > +	 * and  'APWM mode' to enable APWM output
> > +	 */
> > +	reg_val = readw(pc->mmio_base + ECCTL2);
> > +	reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE;
> 
> You already set the APWM_MODE bit in .config(), why is it needed here
> again? Seeing that .disable() clears the bit as well, perhaps leaving it
> clear in .config() is the better option.

Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low 
in idle state).

The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2)
During PWM configuration. To enable loading from Shadow registers, APWM mode 
should be set.
The same is done in .config().


> 
> > +}
> > +
> > +static void ecap_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> > +		dev_warn(chip->dev, "Removing PWM device without disabling\n");
> > +		pm_runtime_put_sync(chip->dev);
> > +	}
> > +}
> 
> I wonder whether we want to have something like this in the PWM core at
> some point. Maybe we should call .disable() automatically when they are
> freed, or alternatively return -EBUSY if a PWM is freed but still
> enabled. I think I prefer the latter. For now we can leave this in,
> because I don't want to make any such core changes for 3.6 now that the
> merge window is open.

OK Thanks.

> 
> > +
> > +static struct pwm_ops ecap_pwm_ops = {
> > +	.free		= ecap_pwm_free,
> > +	.config		= ecap_pwm_config,
> > +	.enable		= ecap_pwm_enable,
> > +	.disable	= ecap_pwm_disable,
> > +	.owner		= THIS_MODULE,
> > +};
> 
> This should be "static const pwm_ops ...".

Ok I will correct it.

> 
> > +
> > +static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> > +{
> > +	int ret;
> > +	struct resource *r;
> > +	struct clk *clk;
> > +	struct ecap_pwm_chip *pc;
> > +
> > +	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> > +	if (!pc) {
> > +		dev_err(&pdev->dev, "failed to allocate memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	clk = devm_clk_get(&pdev->dev, "fck");
> > +	if (IS_ERR(clk)) {
> > +		dev_err(&pdev->dev, "failed to get clock\n");
> > +		return PTR_ERR(clk);
> > +	}
> > +
> > +	pc->clk_rate = clk_get_rate(clk);
> > +	if (!pc->clk_rate) {
> > +		dev_err(&pdev->dev, "failed to get clock rate\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	pc->chip.dev = &pdev->dev;
> > +	pc->chip.ops = &ecap_pwm_ops;
> > +	pc->chip.base = -1;
> > +	pc->chip.npwm = 1;
> 
> The cover letter mentions that the AM335x has 3 instances of the ECAP.
> Is there any chance that they can be unified in one driver instance
> (i.e. .npwm = 3?).

No. All instances have separate resources (clocks, memory ..).

> 
> > +
> > +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg");
> > +	if (r == NULL) {
> 
> You use "if (!ptr)" everywhere else, maybe you should make this
> consistent?
Ok
> Also the platform_get_resource_byname() is really only
> useful for devices that have several register regions associated with
> them. I don't know your hardware in detail, but I doubt that a PWM
> device has more than one register region.

In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP & 
Common Config space). So I need to use platform_get_resource_byname()

> 
> > +		dev_err(&pdev->dev, "no memory resource defined\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> > +	if (!pc->mmio_base) {
> > +		dev_err(&pdev->dev, "failed to ioremap() registers\n");
> > +		return -EADDRNOTAVAIL;
> > +	}
> > +
> > +	ret = pwmchip_add(&pc->chip);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	pm_runtime_enable(&pdev->dev);
> > +	platform_set_drvdata(pdev, pc);
> > +	dev_info(&pdev->dev, "PWM device initialized\n");
> 
> I think this can go away. If none of the above errors is shown you can
> just assume that the PWM was initialized.

Ok. I will remove.

> 
> > +	return 0;
> > +}
> > +
> > +static int __devexit ecap_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
> > +	struct pwm_device *pwm;
> > +
> > +	if (WARN_ON(!pc))
> > +		return -ENODEV;
> 
> Do you really want to be this verbose? This kind of check is very rare
> anyway, but explicitly warning about it doesn't seems overkill. I think
> the check can even be left out, since every possible error that would
> lead to the driver data not being checked would already cause .probe()
> to return < 0 and therefore .remove() won't ever be called anyway.

Point taken, I will remove.

> 
> > +
> > +	pwm = &pc->chip.pwms[0];
> 
> You don't use this variable.
Ok

Thanks
Avinash

> 
> Thierry
>