On Wed, Jan 01, 2014 at 07:43:41AM +0400, Alexander Shiyan wrote:
> Add a new driver for the PWM controllers on the CLPS711X platform
> based on the PWM framework.
I think you can drop the last part ("based on the PWM framework") of
that sentence. Perhaps a good idea would be to mention some of the
peculiarities of this controller (supports two channels, only 4 bits
specifying the duty-cycle, fixed period, ...).
> diff --git a/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
> new file mode 100644
> index 0000000..4caf819
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/cirrus-clps711x-pwm.txt
> @@ -0,0 +1,16 @@
> +* Cirris Logic CLPS711X PWM controller
> +
> +Required properties:
> +- compatible: Should be "cirrus,clps711x-pwm".
> +- reg: Physical base address and length of the controller's registers.
> +- clocks: Phandle to the PWM source clock.
"phandle"
> +- #pwm-cells: Should be 1. See pwm.txt in this directory for a description of
> + the cells format.
Perhaps in this case it would be easier to simply mention that the cell
specifies the index of the channel. pwm.txt isn't explicit about what a
specifier of size 1 looks like (although it is sort of implied).
> diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
[...]
> +struct clps711x_chip {
> + struct pwm_chip chip;
> + struct clk *clk;
> + void __iomem *pmpcon;
> + spinlock_t lock;
> +};
I'd prefer this to not use this artificial alignment using tabs. Simply
a single space after the type is good enough.
> +static int clps711x_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct clps711x_chip *priv =
> + container_of(chip, struct clps711x_chip, chip);
This should be wrapped into a static inline function to make it shorter:
static inline to_clps711x(struct pwm_chip *chip)
{
return container_of(chip, struct clps711x_chip, chip);
}
> + unsigned int period, freq = clk_get_rate(priv->clk);
> +
> + if (!freq)
> + return -EINVAL;
> +
> + /* Calculate and store constant period value */
> + period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, freq);
> + pwm_set_period(pwm, period);
> + pwm_set_chip_data(pwm, (void *)(uintptr_t)period);
Why store this in chip data again if it can be retrieved directly from
the PWM device using pwm_get_period()?
> +static int clps711x_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + /* Do nothing */
> + return 0;
> +}
> +
> +static void clps711x_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + /* Do nothing */
> +}
I think it would be better if this would set the duty field to 0 to stop
any potential toggling of the PWM signal. .enable() can later restore
the proper value.
The reason for this is that pwm_disable() is supposed to stop the PWM
output from toggling and if you simply ignore it you don't conform to
the API specification.
> +static const struct pwm_ops clps711x_pwm_ops = {
> + .request = clps711x_pwm_request,
> + .config = clps711x_pwm_config,
> + .enable = clps711x_pwm_enable,
> + .disable = clps711x_pwm_disable,
> + .owner = THIS_MODULE,
> +};
Please drop the alignment here as well.
> +static const struct of_device_id __maybe_unused clps711x_pwm_dt_ids[] = {
I don't think there's concensus on the proper placement, but I prefer
__maybe_unused to be at the very end of the declaration.
> +static struct platform_driver clps711x_pwm_driver = {
> + .driver = {
> + .name = "clps711x-pwm",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(clps711x_pwm_dt_ids),
> + },
> + .probe = clps711x_pwm_probe,
> + .remove = clps711x_pwm_remove,
> +};
> +module_platform_driver(clps711x_pwm_driver);
And again, no alignment of the fields here, please.
Thierry
Attachment:
pgpu7h2JO262T.pgp
Description: PGP signature
