On Mon, Jul 23, 2012 at 13:12:21, Thierry Reding wrote:
> On Fri, Jul 13, 2012 at 03:05:02PM +0530, Philip, Avinash wrote:
> > Enhanced high resolution PWM module (EHRPWM) hardware can be used to
> > generate PWM output over 2 channels. This commit adds PWM driver support
> > for EHRPWM device present on AM33XX SOC. Current implementation supports
> > simple PWM functionality.
> >
> > Signed-off-by: Philip, Avinash <avinashphilip@xxxxxx>
> > Reviewed-by: Vaibhav Bedia <vaibhav.bedia@xxxxxx>
>
> So this driver is very similar to the ECAP one and pretty much all the
> comments apply to this as well. Some additional comments below.
Ok I will correct it for all the comments.
>
> > ---
[snip]
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-ehrpwm.
> > +
>
> Maybe it would be useful to prefix the names with AM33XX here. ECAP and
> EHRPWM are sort of generic and may have name clashes in the future.
Ok, I will make us TI prefix as davinci platform also uses same modules.
>
> > +#define PWM_CHANNEL 2 /* EHRPWM channels */
>
> I'd say you can just replace the one occurrence of this with the literal
> 2. If you still want to have the symbolic name, then I'd suggest to call
> it something like NUM_PWM_CHANNELS to make its meaning more obvious.
I will correct it as NUM_PWM_CHANNELS.
>
> > +static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev)
> > +{
> > + struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev);
> > +
> > + if (WARN_ON(!pc))
> > + return -ENODEV;
> > +
> > + pm_runtime_disable(&pdev->dev);
> > + pwmchip_remove(&pc->chip);
> > + return 0;
> > +}
>
> I forgot to mention this for ECAP, but you need to check the return
> value of pwmchip_remove() because there are situations where it can
> actually fail.
I will correct it.
Thanks
Avinash
>
> Thierry
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
