Messages in this thread |  | | Date | Tue, 3 Dec 2013 11:09:12 +0800 | From | Bo Shen <> | Subject | Re: [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
|  |