lkml.org 
[lkml]   [2013]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v8 1/2] PWM: atmel-pwm: add PWM controller driver
    Hi Thierry,

    On 12/02/2013 06:59 PM, Thierry Reding wrote:
    > On Mon, Nov 18, 2013 at 05:13:21PM +0800, Bo Shen wrote:
    > [...]
    >> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
    > [...]
    >> +/* Max value for duty and period
    >
    > Block comments should be of this form:
    >
    > /*
    > * Max value ...
    > * ...
    > */

    OK, I will use this style.

    >> +static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
    >> + int duty_ns, int period_ns)
    >> +{
    >> + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
    >> + unsigned long clk_rate, prd, dty;
    >> + unsigned long long div;
    >> + int ret, pres = 0;
    >> +
    >> + clk_rate = clk_get_rate(atmel_pwm->clk);
    >> + div = clk_rate;
    >> +
    >> + /* Calculate the period cycles */
    >> + while (div > PWM_MAX_PRD) {
    >> + div = clk_rate / (1 << pres);
    >> + div = div * period_ns;
    >> + /* 1/Hz = 100000000 ns */
    >
    > I don't think that comment is needed.

    This is asked to be added.
    And, I think keep it and it won't hurt, what do you think?

    >> + do_div(div, 1000000000);
    >> +
    >> + if (pres++ > PRD_MAX_PRES) {
    >> + dev_err(chip->dev, "pres exceed the maximum value\n");
    >
    > "exceeds"

    Thanks for correct it.

    >> + return -EINVAL;
    >> + }
    >> + }
    >> +
    >> + /* Calculate the duty cycles */
    >> + prd = div;
    >> + div *= duty_ns;
    >> + do_div(div, period_ns);
    >> + dty = div;
    >> +
    >> + ret = clk_enable(atmel_pwm->clk);
    >> + if (ret) {
    >> + dev_err(chip->dev, "failed to enable pwm clock\n");
    >
    > "PWM clock"

    OK, I will change all low case pwm to upper case PWM.

    >> +static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device *pwm,
    >> + int dty, int prd)
    >> +{
    >> + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
    >> + unsigned int val;
    >> +
    >> + /*
    >> + * If the PWM channel is disabled, write value to duty and period
    >> + * registers directly.
    >> + * If the PWM channel is enabled, using the update register, it needs
    >> + * to set bit 10 of CMR to 0
    >> + */
    >
    > I think it would make sense to split this comment and move each part
    > into the respective conditional branch.

    OK, I will split them.

    >> + if (test_bit(PWMF_ENABLED, &pwm->flags)) {
    >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
    >> +
    >> + val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
    >> + val &= ~PWM_CMR_UPD_CDTY;
    >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
    >> + } else {
    >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CDTY, dty);
    >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CPRD, prd);
    >> + }
    >> +}
    >> +
    >> +static void atmel_pwm_config_v2(struct pwm_chip *chip, struct pwm_device *pwm,
    >> + int dty, int prd)
    >> +{
    >> + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
    >> +
    >> + /*
    >> + * If the PWM channel is disabled, write value to duty and period
    >> + * registers directly.
    >> + * If the PWM channel is enabled, using the duty update register to
    >> + * update the value.
    >> + */
    >
    > Same here.
    >
    >> + if (test_bit(PWMF_ENABLED, &pwm->flags)) {
    >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTYUPD, dty);
    >> + } else {
    >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CDTY, dty);
    >> + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV2_CPRD, prd);
    >> + }
    >> +}
    >
    > Neither version 1 nor version 2 seem to be able to change the period
    > while the channel is enabled. Perhaps that should be checked for in
    > atmel_pwm_config() and an error (-EBUSY) returned?

    The period is configured in dt in device tree, or platform data in non
    device tree. Nowhere will update period. So, not code to update period.
    Am I right? If not, please figure out.

    >> +
    >> +static int atmel_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
    >> + enum pwm_polarity polarity)
    >> +{
    >> + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
    >> + u32 val = 0;
    >> + int ret;
    >> +
    >> + if (polarity == PWM_POLARITY_NORMAL)
    >> + val &= ~PWM_CMR_CPOL;
    >> + else
    >> + val |= PWM_CMR_CPOL;
    >
    > I think I've mentioned this before, but val is always assigned to 0, so
    > clearing a bit is a superfluous. Perhaps you need to readl the CMR
    > register first before toggling the bit here?

    Thanks, we should read CMR, and set the CPOL accordingly.

    >> +
    >> + ret = clk_enable(atmel_pwm->clk);
    >> + if (ret) {
    >> + dev_err(chip->dev, "failed to enable pwm clock\n");
    >
    > "PWM clock"
    >
    >> +#ifdef CONFIG_OF
    >> +static const struct of_device_id atmel_pwm_dt_ids[] = {
    >> + {
    >> + .compatible = "atmel,at91sam9rl-pwm",
    >> + .data = &atmel_pwm_data_v1,
    >> + }, {
    >> + .compatible = "atmel,sama5d3-pwm",
    >> + .data = &atmel_pwm_data_v2,
    >> + }, {
    >> + /* sentinel */
    >> + },
    >> +};
    >> +MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
    >> +#endif
    >
    > I don't think you can do this. You use this table in a call to
    > of_match_device() later on, in code which isn't protected by a
    > corresponding #ifdef.

    I will remove #ifdef.

    >> +static inline const struct atmel_pwm_data * __init
    >> + atmel_pwm_get_driver_data(struct platform_device *pdev)
    >
    > I don't think __init is warranted here. In fact I think this will give
    > you a build warning, because this code is called from atmel_pwm_probe(),
    > which in turn isn't marked __init.

    OK, I will remove __init.

    > Also it's probably not worth marking this inline explicitly. It isn't
    > all that short, and the compiler will likely inline it anyway since it's
    > only called once.

    It only called one, so, it can be inline.

    >> +{
    >> + if (pdev->dev.of_node) {
    >> + const struct of_device_id *match;
    >> + match = of_match_device(atmel_pwm_dt_ids, &pdev->dev);
    >
    > Blank line between the above two for readability.

    OK, I will add one blank line.

    >> + if (match == NULL)
    >> + return NULL;
    >> + return match->data;
    >
    > Same here. And "if (!match)" rather than "if (match == NULL)".

    OK, I will change like this.

    >> + }
    >> +
    >> + return (struct atmel_pwm_data *)
    >> + platform_get_device_id(pdev)->driver_data;
    >
    > Please use a temporary variable here to make this more readable, like
    > so:
    >
    > struct platform_device_id *id = platform_get_device_id(pdev);
    >
    > ...
    >
    > return (struct atmel_pwm_data *)id->driver;
    >
    >> +}

    OK, I will change like this.

    >> +static int atmel_pwm_probe(struct platform_device *pdev)
    >> +{
    >> + const struct atmel_pwm_data *data;
    >> + struct atmel_pwm_chip *atmel_pwm;
    >> + struct resource *res;
    >> + int ret;
    >> +
    >> + atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL);
    >> + if (!atmel_pwm)
    >> + return -ENOMEM;
    >
    > You could move this further down, so that memory doesn't get allocated
    > if atmel_pwm_get_driver_data() or platform_get_resource() fails.

    OK, I will move this down.

    >> +
    >> + data = atmel_pwm_get_driver_data(pdev);
    >> + if (!data)
    >> + return -ENODEV;
    >> +
    >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    >> + if (!res)
    >> + return -ENODEV;
    >
    > No need to check the return value here. devm_ioremap_resource() checks
    > it for you.

    OK, I will remove this check.

    >> +
    >> + atmel_pwm->base = devm_ioremap_resource(&pdev->dev, res);
    >> + if (IS_ERR(atmel_pwm->base))
    >> + return PTR_ERR(atmel_pwm->base);
    >> +
    >> + atmel_pwm->clk = devm_clk_get(&pdev->dev, NULL);
    >> + if (IS_ERR(atmel_pwm->clk))
    >> + return PTR_ERR(atmel_pwm->clk);
    >> +
    >> + ret = clk_prepare(atmel_pwm->clk);
    >> + if (ret) {
    >> + dev_err(&pdev->dev, "failed to prepare pwm clock\n");
    >
    > "PWM clock"
    >
    >> + return ret;
    >> + }
    >> +
    >> + atmel_pwm->chip.dev = &pdev->dev;
    >> + atmel_pwm->chip.ops = &atmel_pwm_ops;
    >> + if (pdev->dev.of_node) {
    >
    > Blank line between the above two for readability.

    OK, I will add blank line.

    >> + atmel_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
    >> + atmel_pwm->chip.of_pwm_n_cells = 3;
    >> + atmel_pwm->chip.base = -1;
    >> + } else {
    >> + atmel_pwm->chip.base = pdev->id;
    >
    > That's not correct. The chip cannot be tied to pdev->id, because that ID
    > is the instance number of the device. So typically you would have
    > devices name like this:
    >
    > atmel-pwm.0
    > atmel-pwm.1
    > ...
    >
    > Now, if you have that, then you won't be able to register the second
    > instance because the first instance will already have requested PWMs
    > 0-3, and setting .base to 1 will cause PWMs 1-4 to be requested, which
    > intersects with the range of the first instance.
    >
    > The same applies of course if you have other PWM controllers in the
    > system which have similar instance names.
    >
    > So the right thing to do here is to provide that number via platform
    > data so that platform code can define it, knowing in advance all ranges
    > for all other PWM controllers and thereby make sure there's no
    > intersection.

    OK, I will fix this.

    >> + }
    >> + atmel_pwm->chip.npwm = 4;
    >
    > Blank line between the above two for readability.

    OK, I will add blank line.

    >> + atmel_pwm->config = data->config;
    >> +
    >> + ret = pwmchip_add(&atmel_pwm->chip);
    >> + if (ret < 0) {
    >> + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
    >
    > "PWM chip"
    >
    > Thierry

    Best Regards,
    Bo Shen



    \
     
     \ /
      Last update: 2013-12-03 04:41    [W:2.635 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site