Messages in this thread | | | Date | Thu, 27 Feb 2014 11:01:52 +0200 | From | Mika Westerberg <> | 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?
> > + 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.
| |