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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
