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.westerb...@linux.intel.com>
> > + * Author: Chew Kean Ho <kean.ho.c...@intel.com>
> > + * Author: Chang Rebecca Swee Fun <rebecca.swee.fun.ch...@intel.com>
> > + * Author: Chew Chiau Ee <chiau.ee.c...@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.westerb...@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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
