| Message ID | 1397311131-13371-1-git-send-email-chiau.ee.chew@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On 2014/4/12 21:58, Chew Chiau Ee wrote: > From: Alan Cox <alan@linux.intel.com> > > Not all systems enumerate the PWM devices via ACPI. They can also be exposed > via the PCI interface. > > Signed-off-by: Alan Cox <alan@linux.intel.com> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> > --- > drivers/pwm/pwm-lpss.c | 160 ++++++++++++++++++++++++++++++++++++++--------- > 1 files changed, 129 insertions(+), 31 deletions(-) > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > index 449e372..6f79bf8 100644 > --- a/drivers/pwm/pwm-lpss.c > +++ b/drivers/pwm/pwm-lpss.c > @@ -6,6 +6,7 @@ > * Author: Chew Kean Ho <kean.ho.chew@intel.com> > * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> > * Author: Chew Chiau Ee <chiau.ee.chew@intel.com> > + * Author: Alan Cox <alan@linux.intel.com> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -19,6 +20,9 @@ > #include <linux/module.h> > #include <linux/pwm.h> > #include <linux/platform_device.h> > +#include <linux/pci.h> > + > +static int pci_drv, plat_drv; /* So we know which drivers registered */ > > #define PWM 0x00000000 > #define PWM_ENABLE BIT(31) > @@ -34,6 +38,15 @@ struct pwm_lpss_chip { > struct pwm_chip chip; > void __iomem *regs; > struct clk *clk; > + unsigned long clk_rate; > +}; > + > +struct pwm_lpss_boardinfo { > + unsigned long clk_rate; > +}; > + > +static const struct pwm_lpss_boardinfo byt_info = { > + 25000000 > }; > > static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) > @@ -55,7 +68,7 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > /* The equation is: base_unit = ((freq / c) * 65536) + correction */ > base_unit = freq * 65536; > > - c = clk_get_rate(lpwm->clk); > + c = lpwm->clk_rate; > if (!c) > return -EINVAL; > > @@ -113,52 +126,47 @@ static const struct pwm_ops pwm_lpss_ops = { > .owner = THIS_MODULE, > }; > > -static const struct acpi_device_id pwm_lpss_acpi_match[] = { > - { "80860F09", 0 }, > - { }, > -}; > -MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); > - > -static int pwm_lpss_probe(struct platform_device *pdev) > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, > + struct resource *r, struct pwm_lpss_boardinfo *info) > { > struct pwm_lpss_chip *lpwm; > - struct resource *r; > int ret; > > - lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); > + lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL); > if (!lpwm) > - return -ENOMEM; > - > - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + return ERR_PTR(-ENOMEM); > > - lpwm->regs = devm_ioremap_resource(&pdev->dev, r); > + lpwm->regs = devm_ioremap_resource(dev, r); > if (IS_ERR(lpwm->regs)) > - return PTR_ERR(lpwm->regs); > - > - lpwm->clk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(lpwm->clk)) { > - dev_err(&pdev->dev, "failed to get PWM clock\n"); > - return PTR_ERR(lpwm->clk); > + return lpwm->regs; > + > + if (info) { > + lpwm->clk_rate = info->clk_rate; > + } else { > + lpwm->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(lpwm->clk)) { > + dev_err(dev, "failed to get PWM clock\n"); > + return (void *)lpwm->clk; > + } > + lpwm->clk_rate = clk_get_rate(lpwm->clk); > } > > - lpwm->chip.dev = &pdev->dev; > + lpwm->chip.dev = dev; > lpwm->chip.ops = &pwm_lpss_ops; > lpwm->chip.base = -1; > lpwm->chip.npwm = 1; > > ret = pwmchip_add(&lpwm->chip); > if (ret) { > - dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret); > - return ret; > + dev_err(dev, "failed to add PWM chip: %d\n", ret); > + return ERR_PTR(ret); > } > > - platform_set_drvdata(pdev, lpwm); > - return 0; > + return lpwm; > } > > -static int pwm_lpss_remove(struct platform_device *pdev) > +static int pwm_lpss_remove(struct pwm_lpss_chip *lpwm) > { > - struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev); > u32 ctrl; > > ctrl = readl(lpwm->regs + PWM); > @@ -167,17 +175,107 @@ static int pwm_lpss_remove(struct platform_device *pdev) > return pwmchip_remove(&lpwm->chip); > } > > -static struct platform_driver pwm_lpss_driver = { > +static int pwm_lpss_probe_pci(struct pci_dev *pdev, > + const struct pci_device_id *id) > +{ > + struct pwm_lpss_boardinfo *info; > + struct pwm_lpss_chip *lpwm; > + int err; > + > + err = pci_enable_device(pdev); > + if (err < 0) > + return err; > + > + info = (struct pwm_lpss_boardinfo *)id->driver_data; > + lpwm = pwm_lpss_probe(&pdev->dev, &pdev->resource[0], info); > + if (IS_ERR(lpwm)) > + return PTR_ERR(lpwm); > + > + pci_set_drvdata(pdev, lpwm); > + return 0; > +} > + > +static void pwm_lpss_remove_pci(struct pci_dev *pdev) > +{ > + struct pwm_lpss_chip *lpwm = pci_get_drvdata(pdev); > + > + pwm_lpss_remove(lpwm); > + pci_disable_device(pdev); > +} > + > +static struct pci_device_id pwm_lpss_pci_ids[] = { > + { PCI_VDEVICE(INTEL, 0x0F08), (unsigned long)&byt_info}, > + { PCI_VDEVICE(INTEL, 0x0F09), (unsigned long)&byt_info}, > + { 0,} > +}; > +MODULE_DEVICE_TABLE(pci, pwm_lpss_pci_ids); > + > +static struct pci_driver pwm_lpss_driver_pci = { > + .name = "pwm-lpss", > + .id_table = pwm_lpss_pci_ids, > + .probe = pwm_lpss_probe_pci, > + .remove = pwm_lpss_remove_pci, > +}; > + > +static int pwm_lpss_probe_platform(struct platform_device *pdev) > +{ > + struct pwm_lpss_chip *lpwm; > + struct resource *r; > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + lpwm = pwm_lpss_probe(&pdev->dev, r, NULL); > + if (IS_ERR(lpwm)) > + return PTR_ERR(lpwm); > + > + platform_set_drvdata(pdev, lpwm); > + return 0; > +} > + > +static int pwm_lpss_remove_platform(struct platform_device *pdev) > +{ > + struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev); > + > + return pwm_lpss_remove(lpwm); > +} > + > +static const struct acpi_device_id pwm_lpss_acpi_match[] = { > + { "80860F09", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); > + > +static struct platform_driver pwm_lpss_driver_platform = { > .driver = { > .name = "pwm-lpss", > .acpi_match_table = pwm_lpss_acpi_match, > }, > - .probe = pwm_lpss_probe, > - .remove = pwm_lpss_remove, > + .probe = pwm_lpss_probe_platform, > + .remove = pwm_lpss_remove_platform, > }; > -module_platform_driver(pwm_lpss_driver); > + > +static int __init pwm_init(void) > +{ > + pci_drv = pci_register_driver(&pwm_lpss_driver_pci); > + plat_drv = platform_driver_register(&pwm_lpss_driver_platform); > + if (pci_drv && plat_drv) > + return pci_drv; > + > + return 0; > +} > + > +static void __exit pwm_exit(void) > +{ > + if (!pci_drv) > + pci_unregister_driver(&pwm_lpss_driver_pci); > + if (!plat_drv) > + platform_driver_unregister(&pwm_lpss_driver_platform); > +} > > MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); > MODULE_LICENSE("GPL v2"); > MODULE_ALIAS("platform:pwm-lpss"); Looks a good idea to combine pci and acpi driver together. Since pci driver is added, here the alias need to be refined. Others look good. Thanks, -Aubrey > + > +module_init(pwm_init); > +module_exit(pwm_exit); > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PiA+ICBNT0RVTEVfREVTQ1JJUFRJT04oIlBXTSBkcml2ZXIgZm9yIEludGVsIExQU1MiKTsNCj4g Pk1PRFVMRV9BVVRIT1IoIk1pa2ENCj4gPiBXZXN0ZXJiZXJnIDxtaWthLndlc3RlcmJlcmdAbGlu dXguaW50ZWwuY29tPiIpOw0KPiA+ICBNT0RVTEVfTElDRU5TRSgiR1BMIHYyIik7DQo+ID4gIE1P RFVMRV9BTElBUygicGxhdGZvcm06cHdtLWxwc3MiKTsNCj4gDQo+IExvb2tzIGEgZ29vZCBpZGVh IHRvIGNvbWJpbmUgcGNpIGFuZCBhY3BpIGRyaXZlciB0b2dldGhlci4NCj4gU2luY2UgcGNpIGRy aXZlciBpcyBhZGRlZCwgaGVyZSB0aGUgYWxpYXMgbmVlZCB0byBiZSByZWZpbmVkLg0KPiBPdGhl cnMgbG9vayBnb29kLg0KPiANCj4gVGhhbmtzLA0KPiAtQXVicmV5DQoNCk9rLiBJIHdpbGwgY2hh bmdlIGl0IHRvIE1PRFVMRV9BTElBUygicGNpL3BsYXRmb3JtOnB3bS1scHNzIik7DQoNClRoYW5r cywNCkNoaWF1IEVlDQoNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 12, 2014 at 09:58:51PM +0800, Chew Chiau Ee wrote: > From: Alan Cox <alan@linux.intel.com> > > Not all systems enumerate the PWM devices via ACPI. They can also be exposed > via the PCI interface. > > Signed-off-by: Alan Cox <alan@linux.intel.com> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> > --- > drivers/pwm/pwm-lpss.c | 160 ++++++++++++++++++++++++++++++++++++++--------- > 1 files changed, 129 insertions(+), 31 deletions(-) > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > index 449e372..6f79bf8 100644 > --- a/drivers/pwm/pwm-lpss.c > +++ b/drivers/pwm/pwm-lpss.c > @@ -6,6 +6,7 @@ > * Author: Chew Kean Ho <kean.ho.chew@intel.com> > * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> > * Author: Chew Chiau Ee <chiau.ee.chew@intel.com> > + * Author: Alan Cox <alan@linux.intel.com> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -19,6 +20,9 @@ > #include <linux/module.h> > #include <linux/pwm.h> > #include <linux/platform_device.h> > +#include <linux/pci.h> > + > +static int pci_drv, plat_drv; /* So we know which drivers registered */ > > #define PWM 0x00000000 > #define PWM_ENABLE BIT(31) > @@ -34,6 +38,15 @@ struct pwm_lpss_chip { > struct pwm_chip chip; > void __iomem *regs; > struct clk *clk; > + unsigned long clk_rate; > +}; > + > +struct pwm_lpss_boardinfo { > + unsigned long clk_rate; > +}; > + > +static const struct pwm_lpss_boardinfo byt_info = { > + 25000000 > }; > > static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) > @@ -55,7 +68,7 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > /* The equation is: base_unit = ((freq / c) * 65536) + correction */ > base_unit = freq * 65536; > > - c = clk_get_rate(lpwm->clk); > + c = lpwm->clk_rate; > if (!c) > return -EINVAL; > > @@ -113,52 +126,47 @@ static const struct pwm_ops pwm_lpss_ops = { > .owner = THIS_MODULE, > }; > > -static const struct acpi_device_id pwm_lpss_acpi_match[] = { > - { "80860F09", 0 }, > - { }, > -}; > -MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); > - > -static int pwm_lpss_probe(struct platform_device *pdev) > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, > + struct resource *r, struct pwm_lpss_boardinfo *info) > { > struct pwm_lpss_chip *lpwm; > - struct resource *r; > int ret; > > - lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); > + lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL); > if (!lpwm) > - return -ENOMEM; > - > - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + return ERR_PTR(-ENOMEM); > > - lpwm->regs = devm_ioremap_resource(&pdev->dev, r); > + lpwm->regs = devm_ioremap_resource(dev, r); > if (IS_ERR(lpwm->regs)) > - return PTR_ERR(lpwm->regs); > - > - lpwm->clk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(lpwm->clk)) { > - dev_err(&pdev->dev, "failed to get PWM clock\n"); > - return PTR_ERR(lpwm->clk); > + return lpwm->regs; > + > + if (info) { > + lpwm->clk_rate = info->clk_rate; > + } else { > + lpwm->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(lpwm->clk)) { > + dev_err(dev, "failed to get PWM clock\n"); > + return (void *)lpwm->clk; Why the cast here? > + } > + lpwm->clk_rate = clk_get_rate(lpwm->clk); > } > > - lpwm->chip.dev = &pdev->dev; > + lpwm->chip.dev = dev; > lpwm->chip.ops = &pwm_lpss_ops; > lpwm->chip.base = -1; > lpwm->chip.npwm = 1; > > ret = pwmchip_add(&lpwm->chip); > if (ret) { > - dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret); > - return ret; > + dev_err(dev, "failed to add PWM chip: %d\n", ret); > + return ERR_PTR(ret); > } > > - platform_set_drvdata(pdev, lpwm); > - return 0; > + return lpwm; > } > > -static int pwm_lpss_remove(struct platform_device *pdev) > +static int pwm_lpss_remove(struct pwm_lpss_chip *lpwm) > { > - struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev); > u32 ctrl; > > ctrl = readl(lpwm->regs + PWM); > @@ -167,17 +175,107 @@ static int pwm_lpss_remove(struct platform_device *pdev) > return pwmchip_remove(&lpwm->chip); > } > > -static struct platform_driver pwm_lpss_driver = { > +static int pwm_lpss_probe_pci(struct pci_dev *pdev, > + const struct pci_device_id *id) > +{ > + struct pwm_lpss_boardinfo *info; > + struct pwm_lpss_chip *lpwm; > + int err; > + > + err = pci_enable_device(pdev); > + if (err < 0) > + return err; > + > + info = (struct pwm_lpss_boardinfo *)id->driver_data; > + lpwm = pwm_lpss_probe(&pdev->dev, &pdev->resource[0], info); > + if (IS_ERR(lpwm)) > + return PTR_ERR(lpwm); > + > + pci_set_drvdata(pdev, lpwm); > + return 0; > +} > + > +static void pwm_lpss_remove_pci(struct pci_dev *pdev) > +{ > + struct pwm_lpss_chip *lpwm = pci_get_drvdata(pdev); > + > + pwm_lpss_remove(lpwm); > + pci_disable_device(pdev); > +} > + > +static struct pci_device_id pwm_lpss_pci_ids[] = { > + { PCI_VDEVICE(INTEL, 0x0F08), (unsigned long)&byt_info}, > + { PCI_VDEVICE(INTEL, 0x0F09), (unsigned long)&byt_info}, I think non-capital letters for hex numbers are more consistent. I.e 0x0f08. > + { 0,} { }, as in the platform part looks better, IMO. > +}; > +MODULE_DEVICE_TABLE(pci, pwm_lpss_pci_ids); > + > +static struct pci_driver pwm_lpss_driver_pci = { > + .name = "pwm-lpss", > + .id_table = pwm_lpss_pci_ids, The platform part doesn't use tabs, so I think you should follow the style here. > + .probe = pwm_lpss_probe_pci, > + .remove = pwm_lpss_remove_pci, > +}; > + > +static int pwm_lpss_probe_platform(struct platform_device *pdev) > +{ > + struct pwm_lpss_chip *lpwm; > + struct resource *r; > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + lpwm = pwm_lpss_probe(&pdev->dev, r, NULL); > + if (IS_ERR(lpwm)) > + return PTR_ERR(lpwm); > + > + platform_set_drvdata(pdev, lpwm); > + return 0; > +} > + > +static int pwm_lpss_remove_platform(struct platform_device *pdev) > +{ > + struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev); > + > + return pwm_lpss_remove(lpwm); > +} > + > +static const struct acpi_device_id pwm_lpss_acpi_match[] = { > + { "80860F09", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); > + > +static struct platform_driver pwm_lpss_driver_platform = { > .driver = { > .name = "pwm-lpss", > .acpi_match_table = pwm_lpss_acpi_match, > }, > - .probe = pwm_lpss_probe, > - .remove = pwm_lpss_remove, > + .probe = pwm_lpss_probe_platform, > + .remove = pwm_lpss_remove_platform, > }; > -module_platform_driver(pwm_lpss_driver); > + > +static int __init pwm_init(void) > +{ > + pci_drv = pci_register_driver(&pwm_lpss_driver_pci); > + plat_drv = platform_driver_register(&pwm_lpss_driver_platform); > + if (pci_drv && plat_drv) > + return pci_drv; > + > + return 0; > +} > + > +static void __exit pwm_exit(void) > +{ > + if (!pci_drv) > + pci_unregister_driver(&pwm_lpss_driver_pci); > + if (!plat_drv) > + platform_driver_unregister(&pwm_lpss_driver_platform); > +} > > MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); > MODULE_LICENSE("GPL v2"); > MODULE_ALIAS("platform:pwm-lpss"); > + > +module_init(pwm_init); > +module_exit(pwm_exit); Maybe move these to follow the function defitions, like: pwm_init() { } module_init(pwm_init); and so on. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 14, 2014 at 02:05:25AM +0000, Chew, Chiau Ee wrote: > > > MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > > >MODULE_AUTHOR("Mika > > > Westerberg <mika.westerberg@linux.intel.com>"); > > > MODULE_LICENSE("GPL v2"); > > > MODULE_ALIAS("platform:pwm-lpss"); > > > > Looks a good idea to combine pci and acpi driver together. > > Since pci driver is added, here the alias need to be refined. > > Others look good. > > > > Thanks, > > -Aubrey > > Ok. I will change it to MODULE_ALIAS("pci/platform:pwm-lpss"); Hmm, does that really work like that? For PCI and ACPI you already have tables with MODULE_DEVICE_TABLE(). For pure platform driver (which is probably not going to be used) you nede to have MODULE_ALIAS() if you want modprobe to load it automagically. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, > > + struct resource *r, struct pwm_lpss_boardinfo *info) > > { > > struct pwm_lpss_chip *lpwm; > > - struct resource *r; > > int ret; > > > > - lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); > > + lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL); > > if (!lpwm) > > - return -ENOMEM; > > - > > - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + return ERR_PTR(-ENOMEM); > > > > - lpwm->regs = devm_ioremap_resource(&pdev->dev, r); > > + lpwm->regs = devm_ioremap_resource(dev, r); > > if (IS_ERR(lpwm->regs)) > > - return PTR_ERR(lpwm->regs); > > - > > - lpwm->clk = devm_clk_get(&pdev->dev, NULL); > > - if (IS_ERR(lpwm->clk)) { > > - dev_err(&pdev->dev, "failed to get PWM clock\n"); > > - return PTR_ERR(lpwm->clk); > > + return lpwm->regs; > > + > > + if (info) { > > + lpwm->clk_rate = info->clk_rate; > > + } else { > > + lpwm->clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(lpwm->clk)) { > > + dev_err(dev, "failed to get PWM clock\n"); > > + return (void *)lpwm->clk; > > Why the cast here? Alan, please do correct me if I describe wrongly on the purpose that you add the cast here. As you can see, pwm_lpss_probe() is expected to return a pointer of type struct pwm_lpss_chip. On the other hand, the return of devm_clk_get() would be a pointer of type struct clk. So if were to use lpwm->clk as the return value of pwm_lpss_probe() , a cast would be needed since the pointer type is different from the expected one. The compiler would complain on " warning: return from incompatible pointer type" if without the cast. > > + > > +static struct pci_device_id pwm_lpss_pci_ids[] = { > > + { PCI_VDEVICE(INTEL, 0x0F08), (unsigned long)&byt_info}, > > + { PCI_VDEVICE(INTEL, 0x0F09), (unsigned long)&byt_info}, > > I think non-capital letters for hex numbers are more consistent. I.e 0x0f08. > Ok. Noted. > > + { 0,} > > { }, > > as in the platform part looks better, IMO. Ok. Noted. > > > +}; > > +MODULE_DEVICE_TABLE(pci, pwm_lpss_pci_ids); > > + > > +static struct pci_driver pwm_lpss_driver_pci = { > > + .name = "pwm-lpss", > > + .id_table = pwm_lpss_pci_ids, > > The platform part doesn't use tabs, so I think you should follow the style here. > Ok. Noted > > > > MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > MODULE_AUTHOR("Mika > > Westerberg <mika.westerberg@linux.intel.com>"); > > MODULE_LICENSE("GPL v2"); > > MODULE_ALIAS("platform:pwm-lpss"); > > + > > +module_init(pwm_init); > > +module_exit(pwm_exit); > > Maybe move these to follow the function defitions, like: > > pwm_init() > { > } > module_init(pwm_init); > > and so on. Ok. Noted. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Mon, Apr 14, 2014 at 02:05:25AM +0000, Chew, Chiau Ee wrote: > > > > MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > > > >MODULE_AUTHOR("Mika Westerberg > > > ><mika.westerberg@linux.intel.com>"); > > > > MODULE_LICENSE("GPL v2"); > > > > MODULE_ALIAS("platform:pwm-lpss"); > > > > > > Looks a good idea to combine pci and acpi driver together. > > > Since pci driver is added, here the alias need to be refined. > > > Others look good. > > > > > > Thanks, > > > -Aubrey > > > > Ok. I will change it to MODULE_ALIAS("pci/platform:pwm-lpss"); > > Hmm, does that really work like that? > > For PCI and ACPI you already have tables with MODULE_DEVICE_TABLE(). > > For pure platform driver (which is probably not going to be used) you nede to > have MODULE_ALIAS() if you want modprobe to load it automagically. Ok, looks like MODULE_ALIAS() is a redundant. I will remove it. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 15, 2014 at 08:41:02AM +0000, Chew, Chiau Ee wrote: > > > > > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, > > > + struct resource *r, struct pwm_lpss_boardinfo *info) > > > { > > > struct pwm_lpss_chip *lpwm; > > > - struct resource *r; > > > int ret; > > > > > > - lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); > > > + lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL); > > > if (!lpwm) > > > - return -ENOMEM; > > > - > > > - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + return ERR_PTR(-ENOMEM); > > > > > > - lpwm->regs = devm_ioremap_resource(&pdev->dev, r); > > > + lpwm->regs = devm_ioremap_resource(dev, r); > > > if (IS_ERR(lpwm->regs)) > > > - return PTR_ERR(lpwm->regs); > > > - > > > - lpwm->clk = devm_clk_get(&pdev->dev, NULL); > > > - if (IS_ERR(lpwm->clk)) { > > > - dev_err(&pdev->dev, "failed to get PWM clock\n"); > > > - return PTR_ERR(lpwm->clk); > > > + return lpwm->regs; > > > + > > > + if (info) { > > > + lpwm->clk_rate = info->clk_rate; > > > + } else { > > > + lpwm->clk = devm_clk_get(dev, NULL); > > > + if (IS_ERR(lpwm->clk)) { > > > + dev_err(dev, "failed to get PWM clock\n"); > > > + return (void *)lpwm->clk; > > > > Why the cast here? > > Alan, please do correct me if I describe wrongly on the purpose that you add the cast here. > > As you can see, pwm_lpss_probe() is expected to return a pointer of type struct pwm_lpss_chip. On the other hand, > the return of devm_clk_get() would be a pointer of type struct clk. So if were to use lpwm->clk as the return value of pwm_lpss_probe() , > a cast would be needed since the pointer type is different from the expected one. The compiler would complain on " warning: return from incompatible pointer type" > if without the cast. I think you can use ERR_CAST() here. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Tue, Apr 15, 2014 at 08:41:02AM +0000, Chew, Chiau Ee wrote: > > > > > > > > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, > > > > + struct resource *r, struct pwm_lpss_boardinfo *info) > > > > { > > > > struct pwm_lpss_chip *lpwm; > > > > - struct resource *r; > > > > int ret; > > > > > > > > - lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); > > > > + lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL); > > > > if (!lpwm) > > > > - return -ENOMEM; > > > > - > > > > - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + return ERR_PTR(-ENOMEM); > > > > > > > > - lpwm->regs = devm_ioremap_resource(&pdev->dev, r); > > > > + lpwm->regs = devm_ioremap_resource(dev, r); > > > > if (IS_ERR(lpwm->regs)) > > > > - return PTR_ERR(lpwm->regs); > > > > - > > > > - lpwm->clk = devm_clk_get(&pdev->dev, NULL); > > > > - if (IS_ERR(lpwm->clk)) { > > > > - dev_err(&pdev->dev, "failed to get PWM clock\n"); > > > > - return PTR_ERR(lpwm->clk); > > > > + return lpwm->regs; > > > > + > > > > + if (info) { > > > > + lpwm->clk_rate = info->clk_rate; > > > > + } else { > > > > + lpwm->clk = devm_clk_get(dev, NULL); > > > > + if (IS_ERR(lpwm->clk)) { > > > > + dev_err(dev, "failed to get PWM clock\n"); > > > > + return (void *)lpwm->clk; > > > > > > Why the cast here? > > > > Alan, please do correct me if I describe wrongly on the purpose that you add > the cast here. > > > > As you can see, pwm_lpss_probe() is expected to return a pointer of type > struct pwm_lpss_chip. On the other hand, > > the return of devm_clk_get() would be a pointer of type struct clk. So > > if were to use lpwm->clk as the return value of pwm_lpss_probe() , a cast > would be needed since the pointer type is different from the expected one. The > compiler would complain on " warning: return from incompatible pointer type" > > if without the cast. > > I think you can use ERR_CAST() here. You are right....ERR_CAST() can be used. Ok...i will clean up the patch and resend a v2 version. Thanks for the review and feedback. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 12, 2014 at 09:58:51PM +0800, Chew Chiau Ee wrote: > From: Alan Cox <alan@linux.intel.com> > > Not all systems enumerate the PWM devices via ACPI. They can also be exposed > via the PCI interface. > > Signed-off-by: Alan Cox <alan@linux.intel.com> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> > --- > drivers/pwm/pwm-lpss.c | 160 ++++++++++++++++++++++++++++++++++++++--------- > 1 files changed, 129 insertions(+), 31 deletions(-) > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > index 449e372..6f79bf8 100644 > --- a/drivers/pwm/pwm-lpss.c > +++ b/drivers/pwm/pwm-lpss.c > @@ -6,6 +6,7 @@ > * Author: Chew Kean Ho <kean.ho.chew@intel.com> > * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> > * Author: Chew Chiau Ee <chiau.ee.chew@intel.com> > + * Author: Alan Cox <alan@linux.intel.com> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -19,6 +20,9 @@ > #include <linux/module.h> > #include <linux/pwm.h> > #include <linux/platform_device.h> > +#include <linux/pci.h> > + > +static int pci_drv, plat_drv; /* So we know which drivers registered */ > > #define PWM 0x00000000 > #define PWM_ENABLE BIT(31) > @@ -34,6 +38,15 @@ struct pwm_lpss_chip { > struct pwm_chip chip; > void __iomem *regs; > struct clk *clk; > + unsigned long clk_rate; > +}; > + > +struct pwm_lpss_boardinfo { > + unsigned long clk_rate; > +}; > + > +static const struct pwm_lpss_boardinfo byt_info = { > + 25000000 > }; > > static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) > @@ -55,7 +68,7 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > /* The equation is: base_unit = ((freq / c) * 65536) + correction */ > base_unit = freq * 65536; > > - c = clk_get_rate(lpwm->clk); > + c = lpwm->clk_rate; > if (!c) > return -EINVAL; > > @@ -113,52 +126,47 @@ static const struct pwm_ops pwm_lpss_ops = { > .owner = THIS_MODULE, > }; > > -static const struct acpi_device_id pwm_lpss_acpi_match[] = { > - { "80860F09", 0 }, > - { }, > -}; > -MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); > - > -static int pwm_lpss_probe(struct platform_device *pdev) > +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, > + struct resource *r, struct pwm_lpss_boardinfo *info) Has this landed anywhere? I didn't see it in mainline or next, am I looking in the wrong place? If it's just still pending, I ran into one issue integrating it with 3.14.2: CC [M] drivers/pwm/pwm-lpss.o drivers/pwm/pwm-lpss.c: In function ‘pwm_lpss_probe_pci’: drivers/pwm/pwm-lpss.c:192:2: warning: passing argument 3 of ‘pwm_lpss_probe’ discards ‘const’ qualifier from pointer target type [enabled by default] drivers/pwm/pwm-lpss.c:130:30: note: expected ‘struct pwm_lpss_boardinfo *’ but argument is of type ‘const struct pwm_lpss_boardinfo *’ Can we make the third argument to pwm_lpss_probe a const? The following is working for me: static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r, const struct pwm_lpss_boardinfo *info) Thanks, -- Darren Hart Open Source Technology Center darren.hart@intel.com Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 12, 2014 at 04:18:16PM -0700, Darren Hart wrote: > On Sat, Apr 12, 2014 at 09:58:51PM +0800, Chew Chiau Ee wrote: > > From: Alan Cox <alan@linux.intel.com> > > > > Not all systems enumerate the PWM devices via ACPI. They can also be exposed > > via the PCI interface. > > > > Signed-off-by: Alan Cox <alan@linux.intel.com> > > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > --- > > drivers/pwm/pwm-lpss.c | 160 ++++++++++++++++++++++++++++++++++++++--------- > > 1 files changed, 129 insertions(+), 31 deletions(-) [...] > Has this landed anywhere? I didn't see it in mainline or next, am I looking in > the wrong place? This was applied to the pwm/for-next tree and has been in linux-next since next-20140429. > If it's just still pending, I ran into one issue integrating it > with 3.14.2: > > CC [M] drivers/pwm/pwm-lpss.o > drivers/pwm/pwm-lpss.c: In function ‘pwm_lpss_probe_pci’: > drivers/pwm/pwm-lpss.c:192:2: warning: passing argument 3 of ‘pwm_lpss_probe’ > discards ‘const’ qualifier from pointer target type [enabled by default] > drivers/pwm/pwm-lpss.c:130:30: note: expected ‘struct pwm_lpss_boardinfo *’ > but argument is of type ‘const struct pwm_lpss_boardinfo *’ > > Can we make the third argument to pwm_lpss_probe a const? The following is > working for me: > > static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, > struct resource *r, > const struct pwm_lpss_boardinfo *info) A fix like that was applied last week and is in next-20140512. Thierry
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 449e372..6f79bf8 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -6,6 +6,7 @@ * Author: Chew Kean Ho <kean.ho.chew@intel.com> * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> * Author: Chew Chiau Ee <chiau.ee.chew@intel.com> + * Author: Alan Cox <alan@linux.intel.com> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -19,6 +20,9 @@ #include <linux/module.h> #include <linux/pwm.h> #include <linux/platform_device.h> +#include <linux/pci.h> + +static int pci_drv, plat_drv; /* So we know which drivers registered */ #define PWM 0x00000000 #define PWM_ENABLE BIT(31) @@ -34,6 +38,15 @@ struct pwm_lpss_chip { struct pwm_chip chip; void __iomem *regs; struct clk *clk; + unsigned long clk_rate; +}; + +struct pwm_lpss_boardinfo { + unsigned long clk_rate; +}; + +static const struct pwm_lpss_boardinfo byt_info = { + 25000000 }; static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) @@ -55,7 +68,7 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, /* The equation is: base_unit = ((freq / c) * 65536) + correction */ base_unit = freq * 65536; - c = clk_get_rate(lpwm->clk); + c = lpwm->clk_rate; if (!c) return -EINVAL; @@ -113,52 +126,47 @@ static const struct pwm_ops pwm_lpss_ops = { .owner = THIS_MODULE, }; -static const struct acpi_device_id pwm_lpss_acpi_match[] = { - { "80860F09", 0 }, - { }, -}; -MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); - -static int pwm_lpss_probe(struct platform_device *pdev) +static struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, + struct resource *r, struct pwm_lpss_boardinfo *info) { struct pwm_lpss_chip *lpwm; - struct resource *r; int ret; - lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); + lpwm = devm_kzalloc(dev, sizeof(*lpwm), GFP_KERNEL); if (!lpwm) - return -ENOMEM; - - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + return ERR_PTR(-ENOMEM); - lpwm->regs = devm_ioremap_resource(&pdev->dev, r); + lpwm->regs = devm_ioremap_resource(dev, r); if (IS_ERR(lpwm->regs)) - return PTR_ERR(lpwm->regs); - - lpwm->clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(lpwm->clk)) { - dev_err(&pdev->dev, "failed to get PWM clock\n"); - return PTR_ERR(lpwm->clk); + return lpwm->regs; + + if (info) { + lpwm->clk_rate = info->clk_rate; + } else { + lpwm->clk = devm_clk_get(dev, NULL); + if (IS_ERR(lpwm->clk)) { + dev_err(dev, "failed to get PWM clock\n"); + return (void *)lpwm->clk; + } + lpwm->clk_rate = clk_get_rate(lpwm->clk); } - lpwm->chip.dev = &pdev->dev; + lpwm->chip.dev = dev; lpwm->chip.ops = &pwm_lpss_ops; lpwm->chip.base = -1; lpwm->chip.npwm = 1; ret = pwmchip_add(&lpwm->chip); if (ret) { - dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret); - return ret; + dev_err(dev, "failed to add PWM chip: %d\n", ret); + return ERR_PTR(ret); } - platform_set_drvdata(pdev, lpwm); - return 0; + return lpwm; } -static int pwm_lpss_remove(struct platform_device *pdev) +static int pwm_lpss_remove(struct pwm_lpss_chip *lpwm) { - struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev); u32 ctrl; ctrl = readl(lpwm->regs + PWM); @@ -167,17 +175,107 @@ static int pwm_lpss_remove(struct platform_device *pdev) return pwmchip_remove(&lpwm->chip); } -static struct platform_driver pwm_lpss_driver = { +static int pwm_lpss_probe_pci(struct pci_dev *pdev, + const struct pci_device_id *id) +{ + struct pwm_lpss_boardinfo *info; + struct pwm_lpss_chip *lpwm; + int err; + + err = pci_enable_device(pdev); + if (err < 0) + return err; + + info = (struct pwm_lpss_boardinfo *)id->driver_data; + lpwm = pwm_lpss_probe(&pdev->dev, &pdev->resource[0], info); + if (IS_ERR(lpwm)) + return PTR_ERR(lpwm); + + pci_set_drvdata(pdev, lpwm); + return 0; +} + +static void pwm_lpss_remove_pci(struct pci_dev *pdev) +{ + struct pwm_lpss_chip *lpwm = pci_get_drvdata(pdev); + + pwm_lpss_remove(lpwm); + pci_disable_device(pdev); +} + +static struct pci_device_id pwm_lpss_pci_ids[] = { + { PCI_VDEVICE(INTEL, 0x0F08), (unsigned long)&byt_info}, + { PCI_VDEVICE(INTEL, 0x0F09), (unsigned long)&byt_info}, + { 0,} +}; +MODULE_DEVICE_TABLE(pci, pwm_lpss_pci_ids); + +static struct pci_driver pwm_lpss_driver_pci = { + .name = "pwm-lpss", + .id_table = pwm_lpss_pci_ids, + .probe = pwm_lpss_probe_pci, + .remove = pwm_lpss_remove_pci, +}; + +static int pwm_lpss_probe_platform(struct platform_device *pdev) +{ + struct pwm_lpss_chip *lpwm; + struct resource *r; + + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + lpwm = pwm_lpss_probe(&pdev->dev, r, NULL); + if (IS_ERR(lpwm)) + return PTR_ERR(lpwm); + + platform_set_drvdata(pdev, lpwm); + return 0; +} + +static int pwm_lpss_remove_platform(struct platform_device *pdev) +{ + struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev); + + return pwm_lpss_remove(lpwm); +} + +static const struct acpi_device_id pwm_lpss_acpi_match[] = { + { "80860F09", 0 }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); + +static struct platform_driver pwm_lpss_driver_platform = { .driver = { .name = "pwm-lpss", .acpi_match_table = pwm_lpss_acpi_match, }, - .probe = pwm_lpss_probe, - .remove = pwm_lpss_remove, + .probe = pwm_lpss_probe_platform, + .remove = pwm_lpss_remove_platform, }; -module_platform_driver(pwm_lpss_driver); + +static int __init pwm_init(void) +{ + pci_drv = pci_register_driver(&pwm_lpss_driver_pci); + plat_drv = platform_driver_register(&pwm_lpss_driver_platform); + if (pci_drv && plat_drv) + return pci_drv; + + return 0; +} + +static void __exit pwm_exit(void) +{ + if (!pci_drv) + pci_unregister_driver(&pwm_lpss_driver_pci); + if (!plat_drv) + platform_driver_unregister(&pwm_lpss_driver_platform); +} MODULE_DESCRIPTION("PWM driver for Intel LPSS"); MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); MODULE_LICENSE("GPL v2"); MODULE_ALIAS("platform:pwm-lpss"); + +module_init(pwm_init); +module_exit(pwm_exit);