lkml.org 
[lkml]   [2014]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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.


    \
     
     \ /
      Last update: 2014-02-27 10:41    [W:2.502 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site