On Tue, Apr 23, 2013 at 10:23:54PM -0400, Chao Xie wrote: > Add the deice tree support for pwm-pxa.
Instead of repeating the patch subject here, maybe you could be more verbose about the changes you make, like splitting off the parsing of OF and platform data into separate functions. > diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c > index aa4bea7..45d9047 100644 > --- a/drivers/pwm/pwm-pxa.c > +++ b/drivers/pwm/pwm-pxa.c > @@ -9,6 +9,8 @@ > * > * 2008-02-13 initial version > * eric miao <[email protected]> > + * 2013-04-24 add device tree support > + * Chao Xie <[email protected]> > */ > > #include <linux/module.h> > @@ -18,6 +20,8 @@ > #include <linux/err.h> > #include <linux/clk.h> > #include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/pwm.h> > > #include <asm/div64.h> > @@ -124,9 +128,52 @@ static struct pwm_ops pxa_pwm_ops = { > .owner = THIS_MODULE, > }; > > -static int pwm_probe(struct platform_device *pdev) > +static struct of_device_id pxa_pwm_of_match[] = { > + { > + .compatible = "marvell,pxa25x-pwm", > + }, { > + .compatible = "marvell,pxa27x-pwm", > + .data = (void *)HAS_SECONDARY_PWM > + }, { > + .compatible = "marvell,pxa168-pwm", > + }, { > + .compatible = "marvell,pxa910-pwm", > + }, > + {} > +}; > + > +MODULE_DEVICE_TABLE(of, pxa_pwm_of_match); > + > +#ifdef CONFIG_OF > +static int pxa_pwm_parse_data(struct platform_device *pdev, > + struct pxa_pwm_chip *pwm) > +{ > + const struct of_device_id *of_id = > + of_match_device(pxa_pwm_of_match, &pdev->dev); > + unsigned int npwm; > + > + if (!of_id) > + return -ENODEV; > + > + npwm = (unsigned int)(of_id->data); > + pwm->chip.npwm = (npwm & HAS_SECONDARY_PWM) ? 2 : 1; > + > + return 0; > +} > +#else > +static int pxa_pwm_parse_data(struct platform_device *pdev, > + struct pxa_pwm_chip *pwm) > { > const struct platform_device_id *id = platform_get_device_id(pdev); > + > + pwm->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1; > + > + return 0; > +} > +#endif One thing that occurred to me just now is that this might be a potential problem. Suppose you want to build a kernel which has support for using legacy board support (platform data) and DT. If you enable CONFIG_OF you will no longer be able to support boards that use platform data. A common solution to this problem is to prefer platform over DT data, so typically you'd do something like this: if (!pdata) { if (IS_ENABLED(CONFIG_OF)) err = parse_dt(); else err = -ENODEV; } else { err = parse_pdata(); } if (err < 0) return err; Given that in your case you don't have platform data but rather the platform device ID entry, the same scheme should work, since in the OF case the id_entry of the platform device won't be set. Thierry
pgp0WNDg_T6Nb.pgp
Description: PGP signature
