| Message ID | 1390240808-18582-1-git-send-email-chiau.ee.chew@intel.com |
|---|---|
| State | Rejected |
| Headers | show |
> +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > + u8 on_time_div; > + unsigned long c = clk_get_rate(lpwm->clk); > + unsigned long long base_unit, hz = 1000000000UL; > + u32 ctrl; > + > + do_div(hz, period_ns); > + > + /* The equation is: base_unit = ((hz / c) * 65536) + correction */ > + base_unit = hz * 65536; > + do_div(base_unit, c); > + base_unit += PWM_DIVISION_CORRECTION; > + if (base_unit > PWM_LIMIT) > + return -EINVAL; > + > + if (duty_ns <= 0) > + duty_ns = 1; > + on_time_div = 255 - (255 * duty_ns / period_ns); > + > + ctrl = readl(lpwm->regs + PWM); > + ctrl &= ~(PWM_BASE_UNIT_MASK | PWM_ON_TIME_DIV_MASK); > + ctrl |= (u16) base_unit << PWM_BASE_UNIT_SHIFT; > + ctrl |= on_time_div; > + /* request PWM to update on next cycle */ > + ctrl |= PWM_SW_UPDATE; > + writel(ctrl, lpwm->regs + PWM); > + Who handles the locking on all these functions. The pwm layer doesn't but simnply exports stuff like pwm_config() directly to other bits of the kernel so you are not guaranteed to be called via sysfs. (This btw looks to be a problem with a pile of the other pwm drivers, and with the pwm core code which doesn't properly lock its own handling of pwm->duty_cycle and pwm->period in pwm_config(), nor pwm->polarity in pwm_set_polarity). I think the core config methods need some kind of locking ? Alan -- 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, Jan 20, 2014 at 01:28:14PM +0000, One Thousand Gnomes wrote: > > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > > + int duty_ns, int period_ns) > > +{ > > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > > + u8 on_time_div; > > + unsigned long c = clk_get_rate(lpwm->clk); > > + unsigned long long base_unit, hz = 1000000000UL; > > + u32 ctrl; > > + > > + do_div(hz, period_ns); > > + > > + /* The equation is: base_unit = ((hz / c) * 65536) + correction */ > > + base_unit = hz * 65536; > > + do_div(base_unit, c); > > + base_unit += PWM_DIVISION_CORRECTION; > > + if (base_unit > PWM_LIMIT) > > + return -EINVAL; > > + > > + if (duty_ns <= 0) > > + duty_ns = 1; > > + on_time_div = 255 - (255 * duty_ns / period_ns); > > + > > + ctrl = readl(lpwm->regs + PWM); > > + ctrl &= ~(PWM_BASE_UNIT_MASK | PWM_ON_TIME_DIV_MASK); > > + ctrl |= (u16) base_unit << PWM_BASE_UNIT_SHIFT; > > + ctrl |= on_time_div; > > + /* request PWM to update on next cycle */ > > + ctrl |= PWM_SW_UPDATE; > > + writel(ctrl, lpwm->regs + PWM); > > + > > Who handles the locking on all these functions. The pwm layer doesn't but > simnply exports stuff like pwm_config() directly to other bits of the > kernel so you are not guaranteed to be called via sysfs. > > (This btw looks to be a problem with a pile of the other pwm drivers, and > with the pwm core code which doesn't properly lock its own handling of > pwm->duty_cycle and pwm->period in pwm_config(), nor pwm->polarity in > pwm_set_polarity). Good point. I checked few PWM drivers as well and none of them (including this one) does any locking around ->config(). > I think the core config methods need some kind of locking ? Thierry, any comments on this? -- 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, Jan 21, 2014 at 10:52:36AM +0200, Mika Westerberg wrote: > On Mon, Jan 20, 2014 at 01:28:14PM +0000, One Thousand Gnomes wrote: > > > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > > > + int duty_ns, int period_ns) > > > +{ > > > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > > > + u8 on_time_div; > > > + unsigned long c = clk_get_rate(lpwm->clk); > > > + unsigned long long base_unit, hz = 1000000000UL; > > > + u32 ctrl; > > > + > > > + do_div(hz, period_ns); > > > + > > > + /* The equation is: base_unit = ((hz / c) * 65536) + correction */ > > > + base_unit = hz * 65536; > > > + do_div(base_unit, c); > > > + base_unit += PWM_DIVISION_CORRECTION; > > > + if (base_unit > PWM_LIMIT) > > > + return -EINVAL; > > > + > > > + if (duty_ns <= 0) > > > + duty_ns = 1; > > > + on_time_div = 255 - (255 * duty_ns / period_ns); > > > + > > > + ctrl = readl(lpwm->regs + PWM); > > > + ctrl &= ~(PWM_BASE_UNIT_MASK | PWM_ON_TIME_DIV_MASK); > > > + ctrl |= (u16) base_unit << PWM_BASE_UNIT_SHIFT; > > > + ctrl |= on_time_div; > > > + /* request PWM to update on next cycle */ > > > + ctrl |= PWM_SW_UPDATE; > > > + writel(ctrl, lpwm->regs + PWM); > > > + > > > > Who handles the locking on all these functions. The pwm layer doesn't but > > simnply exports stuff like pwm_config() directly to other bits of the > > kernel so you are not guaranteed to be called via sysfs. > > > > (This btw looks to be a problem with a pile of the other pwm drivers, and > > with the pwm core code which doesn't properly lock its own handling of > > pwm->duty_cycle and pwm->period in pwm_config(), nor pwm->polarity in > > pwm_set_polarity). > > Good point. I checked few PWM drivers as well and none of them (including > this one) does any locking around ->config(). > > > I think the core config methods need some kind of locking ? > > Thierry, any comments on this? The idea behind this is that only a single user can have access to a given PWM device at a time. The PWM device's PWMF_REQUESTED flag is set (and cleared) under the pwm_lock and any subsequent users will not be able to use that specific device (pwm_request() return -EBUSY). There is obviously an assumption here that each user knows what they are doing and aren't calling any of the public pwm_*() functions concurrently. I haven't come across a situation where this is actually a problem because typically these functions are called either via sysfs or some other higher-level where synchronization is already properly handled. So the only thing that drivers should be taking care of is synchronizing access to registers common to multiple PWM devices. Does that clarify things? Thierry
On Tue, Jan 21, 2014 at 08:43:39PM +0100, Thierry Reding wrote: > The idea behind this is that only a single user can have access to a > given PWM device at a time. The PWM device's PWMF_REQUESTED flag is set > (and cleared) under the pwm_lock and any subsequent users will not be > able to use that specific device (pwm_request() return -EBUSY). > > There is obviously an assumption here that each user knows what they are > doing and aren't calling any of the public pwm_*() functions > concurrently. I haven't come across a situation where this is actually a > problem because typically these functions are called either via sysfs or > some other higher-level where synchronization is already properly > handled. > > So the only thing that drivers should be taking care of is synchronizing > access to registers common to multiple PWM devices. OK, and since LPSS PWM don't share registers we shouldn't need to do anything here. > Does that clarify things? It does for me, thanks for the explanation. -- 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
> Does that clarify things?
Yes
--
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
> -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: Wednesday, January 22, 2014 5:31 PM > To: Thierry Reding > Cc: One Thousand Gnomes; Chew, Chiau Ee; linux-pwm@vger.kernel.org; linux- > kernel@vger.kernel.org; Chew, Kean Ho; Chang, Rebecca Swee Fun > Subject: Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM > > On Tue, Jan 21, 2014 at 08:43:39PM +0100, Thierry Reding wrote: > > The idea behind this is that only a single user can have access to a > > given PWM device at a time. The PWM device's PWMF_REQUESTED flag is > > set (and cleared) under the pwm_lock and any subsequent users will not > > be able to use that specific device (pwm_request() return -EBUSY). > > > > There is obviously an assumption here that each user knows what they > > are doing and aren't calling any of the public pwm_*() functions > > concurrently. I haven't come across a situation where this is actually > > a problem because typically these functions are called either via > > sysfs or some other higher-level where synchronization is already > > properly handled. > > > > So the only thing that drivers should be taking care of is > > synchronizing access to registers common to multiple PWM devices. > > OK, and since LPSS PWM don't share registers we shouldn't need to do anything > here. > > > Does that clarify things? > > It does for me, thanks for the explanation. Hi Thierry, Would like to find out whether this pwm patch ready to get accepted into mainline kernel? Didn't mean to be pushy :). Btw, in order to have the Intel Baytrail PWM controller working, it has dependency on the acpi_lpss.c which I have sent the patch (https://lkml.org/lkml/2014/2/18/127) for upstream as well and cc you in the loop. Thanks, Chiau Ee -- 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, Jan 21, 2014 at 02:00:08AM +0800, Chew Chiau Ee wrote: [...] > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c [...] > +/* > + * Intel Low Power Subsystem PWM controller driver > + * > + * Copyright (C) 2014, Intel Corporation > + * Author: Mika Westerberg <mika.westerberg@linux.intel.com> > + * 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> > + * > + * 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 > + * published by the Free Software Foundation. > + */ > + > +#include <linux/acpi.h> > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/pwm.h> > +#include <linux/platform_device.h> > + > +#define PWM 0x00000000 > +#define PWM_ENABLE BIT(31) > +#define PWM_SW_UPDATE BIT(30) > +#define PWM_BASE_UNIT_SHIFT 8 > +#define PWM_BASE_UNIT_MASK 0x00ffff00 > +#define PWM_ON_TIME_DIV_MASK 0x000000ff Does the documentation really call this "on time"? I've always only seen this called duty-cycle. > +#define PWM_DIVISION_CORRECTION 0x2 > +#define PWM_LIMIT (0x8000 + PWM_DIVISION_CORRECTION) > + > + > +struct pwm_lpss_chip { > + struct pwm_chip chip; > + void __iomem *regs; > + struct clk *clk; > +}; > + > +static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) > +{ > + return container_of(chip, struct pwm_lpss_chip, chip); > +} > + > +static void pwm_lpss_set_state(struct pwm_lpss_chip *lpwm, bool enable) > +{ > + u32 ctrl; > + > + ctrl = readl(lpwm->regs + PWM); > + if (enable) > + ctrl |= PWM_ENABLE; > + else > + ctrl &= ~PWM_ENABLE; > + writel(ctrl, lpwm->regs + PWM); Nit: could use more blank lines around readl() and writel(), but I'm told that not everybody likes it that way, so if you absolutely must keep it this way, that's fine, too. =) Also, is it really necessary to turn this into a function? It's only called in three places, where each call site would only require three lines. This function takes up 12 lines in total, so there's no real gain. > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) Align arguments on subsequent lines with those of the first line, please. > +{ > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > + u8 on_time_div; > + unsigned long c = clk_get_rate(lpwm->clk); > + unsigned long long base_unit, hz = 1000000000UL; "hz" -> "freq"? "1000000000UL" -> "NSECS_PER_SEC"? > +static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > + > + clk_prepare_enable(lpwm->clk); You need to check the return value of clk_prepare_enable(). > +#ifdef CONFIG_ACPI > +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev) > +{ > + struct pwm_lpss_chip *lpwm; > + struct resource *r; > + > + lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); > + if (!lpwm) { > + dev_err(&pdev->dev, "failed to allocate memory for platform data\n"); No need to print this message. You should get one from the allocator itself. > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!r) { > + dev_err(&pdev->dev, "failed to get mmio base\n"); > + return NULL; > + } > + > + lpwm->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(lpwm->clk)) { > + dev_err(&pdev->dev, "failed to get pwm clk\n"); > + return NULL; > + } "pwm" -> "PWM", "clk" -> "clock", please. > + > + lpwm->chip.base = -1; > + > + lpwm->regs = devm_request_and_ioremap(&pdev->dev, r); > + if (!lpwm->regs) devm_ioremap_resource()? If so, it returns an ERR_PTR() encoded error code on failure, so make sure to check with IS_ERR(lpwm->regs) instead. Also you can get rid of the extra error checking after the call to platform_get_resource() because devm_ioremap_resource() checks for it already. Furthermore the ordering of calls is somewhat unusual here. I'd prefer platform_get_resource() followed by devm_ioremap_resource() directly. > + return NULL; > + > + return lpwm; > +} > + > +static const struct acpi_device_id pwm_lpss_acpi_match[] = { > + { "80860F08", 0 }, > + { "80860F09", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); > +#else > +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev) > +{ > + return NULL; > +} > +#endif I think that #else is completely dead code since the driver depends on ACPI and therefore CONFIG_ACPI will always be enabled when building this driver. > +static int pwm_lpss_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct pwm_lpss_chip *lpwm; > + int ret; > + > + lpwm = dev_get_platdata(dev); struct pwm_lpss_chip is defined in this source file, how can anybody else know what to fill platform_data with? > + if (!lpwm) { > + lpwm = pwm_lpss_acpi_get_pdata(pdev); > + if (!lpwm) > + return -ENODEV; > + } [...] > +static struct platform_driver pwm_lpss_driver = { > + .driver = { > + .name = "pwm-lpss", > + .acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match), ACPI_PTR not needed since the table will always be there. > + }, > + .probe = pwm_lpss_probe, > + .remove = pwm_lpss_remove, > +}; > +module_platform_driver(pwm_lpss_driver); > + > +MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); > +MODULE_LICENSE("GPL"); The file header says GPL v2, but "GPL" here means "GPL v2 or later". Thierry
On Wed, Feb 26, 2014 at 03:38:23PM +0100, Thierry Reding wrote: > On Tue, Jan 21, 2014 at 02:00:08AM +0800, Chew Chiau Ee wrote: > [...] > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > [...] > > +/* > > + * Intel Low Power Subsystem PWM controller driver > > + * > > + * Copyright (C) 2014, Intel Corporation > > + * Author: Mika Westerberg <mika.westerberg@linux.intel.com> > > + * 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> > > + * > > + * 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 > > + * published by the Free Software Foundation. > > + */ > > + > > +#include <linux/acpi.h> > > +#include <linux/clk.h> > > +#include <linux/device.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/pwm.h> > > +#include <linux/platform_device.h> > > + > > +#define PWM 0x00000000 > > +#define PWM_ENABLE BIT(31) > > +#define PWM_SW_UPDATE BIT(30) > > +#define PWM_BASE_UNIT_SHIFT 8 > > +#define PWM_BASE_UNIT_MASK 0x00ffff00 > > +#define PWM_ON_TIME_DIV_MASK 0x000000ff > > Does the documentation really call this "on time"? I've always only seen > this called duty-cycle. Yes, it's called like that in the documentation. > > > +#define PWM_DIVISION_CORRECTION 0x2 > > +#define PWM_LIMIT (0x8000 + PWM_DIVISION_CORRECTION) > > + > > + > > +struct pwm_lpss_chip { > > + struct pwm_chip chip; > > + void __iomem *regs; > > + struct clk *clk; > > +}; > > + > > +static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) > > +{ > > + return container_of(chip, struct pwm_lpss_chip, chip); > > +} > > + > > +static void pwm_lpss_set_state(struct pwm_lpss_chip *lpwm, bool enable) > > +{ > > + u32 ctrl; > > + > > + ctrl = readl(lpwm->regs + PWM); > > + if (enable) > > + ctrl |= PWM_ENABLE; > > + else > > + ctrl &= ~PWM_ENABLE; > > + writel(ctrl, lpwm->regs + PWM); > > Nit: could use more blank lines around readl() and writel(), but I'm > told that not everybody likes it that way, so if you absolutely must > keep it this way, that's fine, too. =) > > Also, is it really necessary to turn this into a function? It's only > called in three places, where each call site would only require three > lines. This function takes up 12 lines in total, so there's no real > gain. Good point. > > > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > > + int duty_ns, int period_ns) > > Align arguments on subsequent lines with those of the first line, > please. OK > > > +{ > > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > > + u8 on_time_div; > > + unsigned long c = clk_get_rate(lpwm->clk); > > + unsigned long long base_unit, hz = 1000000000UL; > > "hz" -> "freq"? "1000000000UL" -> "NSECS_PER_SEC"? OK > > > +static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > > + > > + clk_prepare_enable(lpwm->clk); > > You need to check the return value of clk_prepare_enable(). Indeed. Will fix. > > > +#ifdef CONFIG_ACPI > > +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev) > > +{ > > + struct pwm_lpss_chip *lpwm; > > + struct resource *r; > > + > > + lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); > > + if (!lpwm) { > > + dev_err(&pdev->dev, "failed to allocate memory for platform data\n"); > > No need to print this message. You should get one from the allocator > itself. OK > > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!r) { > > + dev_err(&pdev->dev, "failed to get mmio base\n"); > > + return NULL; > > + } > > + > > + lpwm->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(lpwm->clk)) { > > + dev_err(&pdev->dev, "failed to get pwm clk\n"); > > + return NULL; > > + } > > "pwm" -> "PWM", "clk" -> "clock", please. OK > > > + > > + lpwm->chip.base = -1; > > + > > + lpwm->regs = devm_request_and_ioremap(&pdev->dev, r); > > + if (!lpwm->regs) > > devm_ioremap_resource()? If so, it returns an ERR_PTR() encoded error > code on failure, so make sure to check with IS_ERR(lpwm->regs) instead. > Also you can get rid of the extra error checking after the call to > platform_get_resource() because devm_ioremap_resource() checks for it > already. > > Furthermore the ordering of calls is somewhat unusual here. I'd prefer > platform_get_resource() followed by devm_ioremap_resource() directly. Yup, we will change that. > > > + return NULL; > > + > > + return lpwm; > > +} > > + > > +static const struct acpi_device_id pwm_lpss_acpi_match[] = { > > + { "80860F08", 0 }, > > + { "80860F09", 0 }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); > > +#else > > +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev) > > +{ > > + return NULL; > > +} > > +#endif > > I think that #else is completely dead code since the driver depends on > ACPI and therefore CONFIG_ACPI will always be enabled when building this > driver. We are getting PCI enumeration for this device as well but for now we can drop the code and add it back later if needed. > > > +static int pwm_lpss_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct pwm_lpss_chip *lpwm; > > + int ret; > > + > > + lpwm = dev_get_platdata(dev); > > struct pwm_lpss_chip is defined in this source file, how can anybody > else know what to fill platform_data with? Good point. Chiau Ee, do you recall why that thing is there in the first place? > > + if (!lpwm) { > > + lpwm = pwm_lpss_acpi_get_pdata(pdev); > > + if (!lpwm) > > + return -ENODEV; > > + } > [...] > > +static struct platform_driver pwm_lpss_driver = { > > + .driver = { > > + .name = "pwm-lpss", > > + .acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match), > > ACPI_PTR not needed since the table will always be there. OK. > > > + }, > > + .probe = pwm_lpss_probe, > > + .remove = pwm_lpss_remove, > > +}; > > +module_platform_driver(pwm_lpss_driver); > > + > > +MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > > +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); > > +MODULE_LICENSE("GPL"); > > The file header says GPL v2, but "GPL" here means "GPL v2 or later". OK, we will change that to GPLv2. Thanks a lot for your review! We will do the changes and submit v2. -- 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
> -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: Thursday, February 27, 2014 5:02 PM > To: Thierry Reding > Cc: Chew, Chiau Ee; linux-pwm@vger.kernel.org; linux-kernel@vger.kernel.org; > Chew, Kean Ho; Chang, Rebecca Swee Fun > Subject: Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM > > On Wed, Feb 26, 2014 at 03:38:23PM +0100, Thierry Reding wrote: > > On Tue, Jan 21, 2014 at 02:00:08AM +0800, Chew Chiau Ee wrote: > > [...] > > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > > [...] > > > +/* > > > + * Intel Low Power Subsystem PWM controller driver > > > + * > > > + * Copyright (C) 2014, Intel Corporation > > > + * Author: Mika Westerberg <mika.westerberg@linux.intel.com> > > > + * 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> > > > + * > > > + * 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 > > > + * published by the Free Software Foundation. > > > + */ > > > + > > > +#include <linux/acpi.h> > > > +#include <linux/clk.h> > > > +#include <linux/device.h> > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/pwm.h> > > > +#include <linux/platform_device.h> > > > + > > > +#define PWM 0x00000000 > > > +#define PWM_ENABLE BIT(31) > > > +#define PWM_SW_UPDATE BIT(30) > > > +#define PWM_BASE_UNIT_SHIFT 8 > > > +#define PWM_BASE_UNIT_MASK 0x00ffff00 > > > +#define PWM_ON_TIME_DIV_MASK 0x000000ff > > > > Does the documentation really call this "on time"? I've always only > > seen this called duty-cycle. > > Yes, it's called like that in the documentation. > > > > > > +#define PWM_DIVISION_CORRECTION 0x2 > > > +#define PWM_LIMIT (0x8000 + > PWM_DIVISION_CORRECTION) > > > + > > > + > > > +struct pwm_lpss_chip { > > > + struct pwm_chip chip; > > > + void __iomem *regs; > > > + struct clk *clk; > > > +}; > > > + > > > +static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) > > > +{ > > > + return container_of(chip, struct pwm_lpss_chip, chip); } > > > + > > > +static void pwm_lpss_set_state(struct pwm_lpss_chip *lpwm, bool > > > +enable) { > > > + u32 ctrl; > > > + > > > + ctrl = readl(lpwm->regs + PWM); > > > + if (enable) > > > + ctrl |= PWM_ENABLE; > > > + else > > > + ctrl &= ~PWM_ENABLE; > > > + writel(ctrl, lpwm->regs + PWM); > > > > Nit: could use more blank lines around readl() and writel(), but I'm > > told that not everybody likes it that way, so if you absolutely must > > keep it this way, that's fine, too. =) > > > > Also, is it really necessary to turn this into a function? It's only > > called in three places, where each call site would only require three > > lines. This function takes up 12 lines in total, so there's no real > > gain. > > Good point. > > > > > > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device > *pwm, > > > + int duty_ns, int period_ns) > > > > Align arguments on subsequent lines with those of the first line, > > please. > > OK > > > > > > +{ > > > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > > > + u8 on_time_div; > > > + unsigned long c = clk_get_rate(lpwm->clk); > > > + unsigned long long base_unit, hz = 1000000000UL; > > > > "hz" -> "freq"? "1000000000UL" -> "NSECS_PER_SEC"? > > OK > > > > > > +static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device > > > +*pwm) { > > > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > > > + > > > + clk_prepare_enable(lpwm->clk); > > > > You need to check the return value of clk_prepare_enable(). > > Indeed. Will fix. > > > > > > +#ifdef CONFIG_ACPI > > > +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct > > > +platform_device *pdev) { > > > + struct pwm_lpss_chip *lpwm; > > > + struct resource *r; > > > + > > > + lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); > > > + if (!lpwm) { > > > + dev_err(&pdev->dev, "failed to allocate memory for platform > > > +data\n"); > > > > No need to print this message. You should get one from the allocator > > itself. > > OK > > > > > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + if (!r) { > > > + dev_err(&pdev->dev, "failed to get mmio base\n"); > > > + return NULL; > > > + } > > > + > > > + lpwm->clk = devm_clk_get(&pdev->dev, NULL); > > > + if (IS_ERR(lpwm->clk)) { > > > + dev_err(&pdev->dev, "failed to get pwm clk\n"); > > > + return NULL; > > > + } > > > > "pwm" -> "PWM", "clk" -> "clock", please. > > OK > > > > > > + > > > + lpwm->chip.base = -1; > > > + > > > + lpwm->regs = devm_request_and_ioremap(&pdev->dev, r); > > > + if (!lpwm->regs) > > > > devm_ioremap_resource()? If so, it returns an ERR_PTR() encoded error > > code on failure, so make sure to check with IS_ERR(lpwm->regs) instead. > > Also you can get rid of the extra error checking after the call to > > platform_get_resource() because devm_ioremap_resource() checks for it > > already. > > > > Furthermore the ordering of calls is somewhat unusual here. I'd prefer > > platform_get_resource() followed by devm_ioremap_resource() directly. > > Yup, we will change that. > > > > > > + return NULL; > > > + > > > + return lpwm; > > > +} > > > + > > > +static const struct acpi_device_id pwm_lpss_acpi_match[] = { > > > + { "80860F08", 0 }, > > > + { "80860F09", 0 }, > > > + { }, > > > +}; > > > +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); #else struct > > > +pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device > > > +*pdev) { > > > + return NULL; > > > +} > > > +#endif > > > > I think that #else is completely dead code since the driver depends on > > ACPI and therefore CONFIG_ACPI will always be enabled when building > > this driver. > > We are getting PCI enumeration for this device as well but for now we can drop > the code and add it back later if needed. > > > > > > +static int pwm_lpss_probe(struct platform_device *pdev) { > > > + struct device *dev = &pdev->dev; > > > + struct pwm_lpss_chip *lpwm; > > > + int ret; > > > + > > > + lpwm = dev_get_platdata(dev); > > > > struct pwm_lpss_chip is defined in this source file, how can anybody > > else know what to fill platform_data with? > > Good point. Chiau Ee, do you recall why that thing is there in the first place? This is added because we would eventually want to have PCI enumeration support added. I think we shall find a way to better enable both the ACPI enumeration and PCI enumeration support. > > > > + if (!lpwm) { > > > + lpwm = pwm_lpss_acpi_get_pdata(pdev); > > > + if (!lpwm) > > > + return -ENODEV; > > > + } > > [...] > > > +static struct platform_driver pwm_lpss_driver = { > > > + .driver = { > > > + .name = "pwm-lpss", > > > + .acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match), > > > > ACPI_PTR not needed since the table will always be there. > > OK. > > > > > > + }, > > > + .probe = pwm_lpss_probe, > > > + .remove = pwm_lpss_remove, > > > +}; > > > +module_platform_driver(pwm_lpss_driver); > > > + > > > +MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > > > +MODULE_AUTHOR("Mika Westerberg > <mika.westerberg@linux.intel.com>"); > > > +MODULE_LICENSE("GPL"); > > > > The file header says GPL v2, but "GPL" here means "GPL v2 or later". > > OK, we will change that to GPLv2. > > Thanks a lot for your review! We will do the changes and submit v2. Ok. I will make the changes accordingly. -- 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 Thu, Feb 27, 2014 at 09:03:41AM +0000, Chew, Chiau Ee wrote: > > > > +static int pwm_lpss_probe(struct platform_device *pdev) { > > > > + struct device *dev = &pdev->dev; > > > > + struct pwm_lpss_chip *lpwm; > > > > + int ret; > > > > + > > > > + lpwm = dev_get_platdata(dev); > > > > > > struct pwm_lpss_chip is defined in this source file, how can anybody > > > else know what to fill platform_data with? > > > > Good point. Chiau Ee, do you recall why that thing is there in the first place? > > This is added because we would eventually want to have PCI enumeration support > added. I think we shall find a way to better enable both the ACPI enumeration and > PCI enumeration support. Agreed. For now, can you please drop the platform data thing from the driver? Thanks. > > > > + if (!lpwm) { > > > > + lpwm = pwm_lpss_acpi_get_pdata(pdev); > > > > + if (!lpwm) > > > > + return -ENODEV; > > > > + } > > > [...] > > > > +static struct platform_driver pwm_lpss_driver = { > > > > + .driver = { > > > > + .name = "pwm-lpss", > > > > + .acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match), > > > > > > ACPI_PTR not needed since the table will always be there. > > > > OK. > > > > > > > > > + }, > > > > + .probe = pwm_lpss_probe, > > > > + .remove = pwm_lpss_remove, > > > > +}; > > > > +module_platform_driver(pwm_lpss_driver); > > > > + > > > > +MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > > > > +MODULE_AUTHOR("Mika Westerberg > > <mika.westerberg@linux.intel.com>"); > > > > +MODULE_LICENSE("GPL"); > > > > > > The file header says GPL v2, but "GPL" here means "GPL v2 or later". > > > > OK, we will change that to GPLv2. > > > > Thanks a lot for your review! We will do the changes and submit v2. > > Ok. I will make the changes accordingly. Thanks. -- 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
> -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: Thursday, February 27, 2014 9:14 PM > To: Chew, Chiau Ee > Cc: Thierry Reding; linux-pwm@vger.kernel.org; linux-kernel@vger.kernel.org; > Chew, Kean Ho; Chang, Rebecca Swee Fun > Subject: Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM > > On Thu, Feb 27, 2014 at 09:03:41AM +0000, Chew, Chiau Ee wrote: > > > > > +static int pwm_lpss_probe(struct platform_device *pdev) { > > > > > + struct device *dev = &pdev->dev; > > > > > + struct pwm_lpss_chip *lpwm; > > > > > + int ret; > > > > > + > > > > > + lpwm = dev_get_platdata(dev); > > > > > > > > struct pwm_lpss_chip is defined in this source file, how can > > > > anybody else know what to fill platform_data with? > > > > > > Good point. Chiau Ee, do you recall why that thing is there in the first place? > > > > This is added because we would eventually want to have PCI enumeration > > support added. I think we shall find a way to better enable both the > > ACPI enumeration and PCI enumeration support. > > Agreed. For now, can you please drop the platform data thing from the driver? > Thanks. > Ok. Noted. > > > > > + if (!lpwm) { > > > > > + lpwm = pwm_lpss_acpi_get_pdata(pdev); > > > > > + if (!lpwm) > > > > > + return -ENODEV; > > > > > + } > > > > [...] > > > > > +static struct platform_driver pwm_lpss_driver = { > > > > > + .driver = { > > > > > + .name = "pwm-lpss", > > > > > + .acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match), > > > > > > > > ACPI_PTR not needed since the table will always be there. > > > > > > OK. > > > > > > > > > > > > + }, > > > > > + .probe = pwm_lpss_probe, > > > > > + .remove = pwm_lpss_remove, > > > > > +}; > > > > > +module_platform_driver(pwm_lpss_driver); > > > > > + > > > > > +MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > > > > > +MODULE_AUTHOR("Mika Westerberg > > > <mika.westerberg@linux.intel.com>"); > > > > > +MODULE_LICENSE("GPL"); > > > > > > > > The file header says GPL v2, but "GPL" here means "GPL v2 or later". > > > > > > OK, we will change that to GPLv2. > > > > > > Thanks a lot for your review! We will do the changes and submit v2. > > > > Ok. I will make the changes accordingly. > > Thanks. -- 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
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 6a2a1e0a..4f75589 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -109,6 +109,16 @@ config PWM_LPC32XX To compile this driver as a module, choose M here: the module will be called pwm-lpc32xx. +config PWM_LPSS + tristate "Intel LPSS PWM support" + depends on ACPI + help + Generic PWM framework driver for Intel Low Power Subsystem PWM + controller. + + To compile this driver as a module, choose M here: the module + will be called pwm-lpss. + config PWM_MXS tristate "Freescale MXS PWM support" depends on ARCH_MXS && OF diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 1b99cfb..d5b316f 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o obj-$(CONFIG_PWM_IMX) += pwm-imx.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o +obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o obj-$(CONFIG_PWM_MXS) += pwm-mxs.o obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c new file mode 100644 index 0000000..a7418ca --- /dev/null +++ b/drivers/pwm/pwm-lpss.c @@ -0,0 +1,207 @@ +/* + * Intel Low Power Subsystem PWM controller driver + * + * Copyright (C) 2014, Intel Corporation + * Author: Mika Westerberg <mika.westerberg@linux.intel.com> + * 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> + * + * 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 + * published by the Free Software Foundation. + */ + +#include <linux/acpi.h> +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/pwm.h> +#include <linux/platform_device.h> + +#define PWM 0x00000000 +#define PWM_ENABLE BIT(31) +#define PWM_SW_UPDATE BIT(30) +#define PWM_BASE_UNIT_SHIFT 8 +#define PWM_BASE_UNIT_MASK 0x00ffff00 +#define PWM_ON_TIME_DIV_MASK 0x000000ff +#define PWM_DIVISION_CORRECTION 0x2 +#define PWM_LIMIT (0x8000 + PWM_DIVISION_CORRECTION) + + +struct pwm_lpss_chip { + struct pwm_chip chip; + void __iomem *regs; + struct clk *clk; +}; + +static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip) +{ + return container_of(chip, struct pwm_lpss_chip, chip); +} + +static void pwm_lpss_set_state(struct pwm_lpss_chip *lpwm, bool enable) +{ + u32 ctrl; + + ctrl = readl(lpwm->regs + PWM); + if (enable) + ctrl |= PWM_ENABLE; + else + ctrl &= ~PWM_ENABLE; + writel(ctrl, lpwm->regs + PWM); +} + +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct pwm_lpss_chip *lpwm = to_lpwm(chip); + u8 on_time_div; + unsigned long c = clk_get_rate(lpwm->clk); + unsigned long long base_unit, hz = 1000000000UL; + u32 ctrl; + + do_div(hz, period_ns); + + /* The equation is: base_unit = ((hz / c) * 65536) + correction */ + base_unit = hz * 65536; + do_div(base_unit, c); + base_unit += PWM_DIVISION_CORRECTION; + if (base_unit > PWM_LIMIT) + return -EINVAL; + + if (duty_ns <= 0) + duty_ns = 1; + on_time_div = 255 - (255 * duty_ns / period_ns); + + ctrl = readl(lpwm->regs + PWM); + ctrl &= ~(PWM_BASE_UNIT_MASK | PWM_ON_TIME_DIV_MASK); + ctrl |= (u16) base_unit << PWM_BASE_UNIT_SHIFT; + ctrl |= on_time_div; + /* request PWM to update on next cycle */ + ctrl |= PWM_SW_UPDATE; + writel(ctrl, lpwm->regs + PWM); + + return 0; +} + +static int pwm_lpss_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct pwm_lpss_chip *lpwm = to_lpwm(chip); + + clk_prepare_enable(lpwm->clk); + pwm_lpss_set_state(lpwm, true); + return 0; +} + +static void pwm_lpss_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct pwm_lpss_chip *lpwm = to_lpwm(chip); + + pwm_lpss_set_state(lpwm, false); + clk_disable_unprepare(lpwm->clk); +} + +static const struct pwm_ops pwm_lpss_ops = { + .config = pwm_lpss_config, + .enable = pwm_lpss_enable, + .disable = pwm_lpss_disable, + .owner = THIS_MODULE, +}; + +#ifdef CONFIG_ACPI +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev) +{ + struct pwm_lpss_chip *lpwm; + struct resource *r; + + lpwm = devm_kzalloc(&pdev->dev, sizeof(*lpwm), GFP_KERNEL); + if (!lpwm) { + dev_err(&pdev->dev, "failed to allocate memory for platform data\n"); + return NULL; + } + + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!r) { + dev_err(&pdev->dev, "failed to get mmio base\n"); + return NULL; + } + + lpwm->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(lpwm->clk)) { + dev_err(&pdev->dev, "failed to get pwm clk\n"); + return NULL; + } + + lpwm->chip.base = -1; + + lpwm->regs = devm_request_and_ioremap(&pdev->dev, r); + if (!lpwm->regs) + return NULL; + + return lpwm; +} + +static const struct acpi_device_id pwm_lpss_acpi_match[] = { + { "80860F08", 0 }, + { "80860F09", 0 }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, pwm_lpss_acpi_match); +#else +struct pwm_lpss_chip *pwm_lpss_acpi_get_pdata(struct platform_device *pdev) +{ + return NULL; +} +#endif + +static int pwm_lpss_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct pwm_lpss_chip *lpwm; + int ret; + + lpwm = dev_get_platdata(dev); + if (!lpwm) { + lpwm = pwm_lpss_acpi_get_pdata(pdev); + if (!lpwm) + return -ENODEV; + } + + lpwm->chip.dev = &pdev->dev; + lpwm->chip.ops = &pwm_lpss_ops; + 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; + } + + platform_set_drvdata(pdev, lpwm); + return 0; +} + +static int pwm_lpss_remove(struct platform_device *pdev) +{ + struct pwm_lpss_chip *lpwm = platform_get_drvdata(pdev); + + pwm_lpss_set_state(lpwm, false); + return pwmchip_remove(&lpwm->chip); +} + +static struct platform_driver pwm_lpss_driver = { + .driver = { + .name = "pwm-lpss", + .acpi_match_table = ACPI_PTR(pwm_lpss_acpi_match), + }, + .probe = pwm_lpss_probe, + .remove = pwm_lpss_remove, +}; +module_platform_driver(pwm_lpss_driver); + +MODULE_DESCRIPTION("PWM driver for Intel LPSS"); +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:pwm-lpss");