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:0.143 / U:0.244 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site