Re: [PATCH v3 2/3] pwm: New driver to support PWMs on TWL4030/6030 series of PMICs

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 8404 bytes --]

On Tue, Nov 20, 2012 at 10:56:21AM +0100, Peter Ujfalusi wrote:
> The driver supports the following PWM outputs:
> TWL4030 PWM0 and PWM1
> TWL6030 PWM1 and PWM2
> 
> On TWL4030 the PWM signals are muxed. Upon requesting the PWM the driver
> will select the correct mux so the PWM can be used. When the PWM has been
> freed the original configuration going to be restored.

"configuration is going to be"

> +config PWM_TWL
> +	tristate "TWL4030/6030 PWM support"
> +	select HAVE_PWM

Why do you select HAVE_PWM?

> +static int twl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      int duty_ns, int period_ns)
> +{
> +	int duty_cycle = DIV_ROUND_UP(duty_ns * TWL_PWM_MAX, period_ns);
> +	u8 pwm_config[2] = {1, 0};
> +	int base, ret;
> +
> +	/*
> +	 * To configure the duty period:
> +	 * On-cycle is set to 1 (the minimum allowed value)
> +	 * The off time of 0 is not configurable, so the mapping is:
> +	 * 0 -> off cycle = 2,
> +	 * 1 -> off cycle = 2,
> +	 * 2 -> off cycle = 3,
> +	 * 126 - > off cycle 127,
> +	 * 127 - > off cycle 1
> +	 * When on cycle == off cycle the PWM will be always on
> +	 */
> +	duty_cycle += 1;

The canonical form to write this would be "duty_cycle++", but maybe it
would even be better to add it to the expression that defines
duty_cycle?

> +static int twl4030_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, &val, TWL4030_GPBR1_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to read GPBR1\n", pwm->label);
> +		return ret;
> +	}
> +
> +	val |= TWL4030_PWM_TOGGLE(pwm->hwpwm, TWL4030_PWMXCLK_ENABLE);
> +
> +	ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
> +	if (ret < 0)
> +		dev_err(chip->dev, "%s: Failed to enable PWM\n", pwm->label);
> +
> +	val |= TWL4030_PWM_TOGGLE(pwm->hwpwm, TWL4030_PWMX_ENABLE);
> +
> +	ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
> +	if (ret < 0)
> +		dev_err(chip->dev, "%s: Failed to enable PWM\n", pwm->label);
> +
> +	return ret;
> +}

Does this perhaps need some locking to make sure the read-modify-write
sequence isn't interrupted?

> +static void twl4030_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, &val, TWL4030_GPBR1_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to read GPBR1\n", pwm->label);
> +		return;
> +	}
> +
> +	val &= ~TWL4030_PWM_TOGGLE(pwm->hwpwm, TWL4030_PWMX_ENABLE);
> +
> +	ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
> +	if (ret < 0)
> +		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
> +
> +	val &= ~TWL4030_PWM_TOGGLE(pwm->hwpwm, TWL4030_PWMXCLK_ENABLE);
> +
> +	ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_GPBR1_REG);
> +	if (ret < 0)
> +		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
> +}

Same here.

> +static int twl4030_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
> +						chip);

This could use a macro like to_twl(chip).

> +	int ret;
> +	u8 val, mask, bits;
> +
> +	ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, &val, TWL4030_PMBR1_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to read PMBR1\n", pwm->label);
> +		return ret;
> +	}
> +
> +	if (pwm->hwpwm) {
> +		/* PWM 1 */
> +		mask = TWL4030_GPIO7_VIBRASYNC_PWM1_MASK;
> +		bits = TWL4030_GPIO7_VIBRASYNC_PWM1_PWM1;
> +	} else {
> +		/* PWM 0 */
> +		mask = TWL4030_GPIO6_PWM0_MUTE_MASK;
> +		bits = TWL4030_GPIO6_PWM0_MUTE_PWM0;
> +	}
> +
> +	/* Save the current MUX configuration for the PWM */
> +	twl->twl4030_pwm_mux &= ~mask;
> +	twl->twl4030_pwm_mux |= (val & mask);
> +
> +	/* Select PWM functionality */
> +	val &= ~mask;
> +	val |= bits;
> +
> +	ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
> +	if (ret < 0)
> +		dev_err(chip->dev, "%s: Failed to request PWM\n", pwm->label);
> +
> +	return ret;
> +}

Again, more read-modify-write without locking.

> +
> +static void twl4030_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
> +						chip);
> +	int ret;
> +	u8 val, mask;
> +
> +	ret = twl_i2c_read_u8(TWL4030_MODULE_INTBR, &val, TWL4030_PMBR1_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to read PMBR1\n", pwm->label);
> +		return;
> +	}
> +
> +	if (pwm->hwpwm)
> +		/* PWM 1 */
> +		mask = TWL4030_GPIO7_VIBRASYNC_PWM1_MASK;
> +	else
> +		/* PWM 0 */
> +		mask = TWL4030_GPIO6_PWM0_MUTE_MASK;

I'm not sure how useful these comments are. You have both an explicit
check for pwm->hwpwm to make it obvious that it isn't 0 and the mask
macros contain the PWM0 and PWM1 substrings respectively.

You could make it even more explicit perhaps by turning the check into:

	if (pwm->hwpwm == 1)

But either way I think you can drop the /* PWM 1 */ and /* PWM 0 */
comments.

> +
> +	/* Restore the MUX configuration for the PWM */
> +	val &= ~mask;
> +	val |= (twl->twl4030_pwm_mux & mask);
> +
> +	ret = twl_i2c_write_u8(TWL4030_MODULE_INTBR, val, TWL4030_PMBR1_REG);
> +	if (ret < 0)
> +		dev_err(chip->dev, "%s: Failed to free PWM\n", pwm->label);
> +}
> +
> +static int twl6030_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
> +						chip);
> +	int ret;
> +	u8 val;
> +
> +	val = twl->twl6030_toggle3;
> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
> +	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXR);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to enable PWM\n", pwm->label);
> +		return ret;
> +	}
> +
> +	twl->twl6030_toggle3 = val;
> +
> +	return 0;
> +}
> +
> +static void twl6030_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct twl_pwm_chip *twl = container_of(chip, struct twl_pwm_chip,
> +						chip);
> +	int ret;
> +	u8 val;
> +
> +	val = twl->twl6030_toggle3;
> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXR);
> +	val &= ~TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to read TOGGLE3\n", pwm->label);
> +		return;
> +	}
> +
> +	val |= TWL6030_PWM_TOGGLE(pwm->hwpwm, TWL6030_PWMXS | TWL6030_PWMXEN);
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, TWL6030_TOGGLE3_REG);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "%s: Failed to disable PWM\n", pwm->label);
> +		return;
> +	}
> +
> +	twl->twl6030_toggle3 = val;
> +}
> +
> +static struct pwm_ops twl_pwm_ops = {
> +	.config = twl_pwm_config,
> +};

It might actually be worth to split this into two pwm_ops structures,
one for 4030 and one for 6030. In that case you would still be able to
make them const, which probably outweighs the space savings here.

Actually, I think this is even potentially buggy since you may have more
than one instance of this driver. For instance if you have a TWL6030 and
a TWL4030 in a single system this will break horribly since you'll over-
write the pwm_ops of one of them.

> +	if (twl_class_is_4030()) {
> +		twl_pwm_ops.enable = twl4030_pwm_enable;
> +		twl_pwm_ops.disable = twl4030_pwm_disable;
> +		twl_pwm_ops.request = twl4030_pwm_request;
> +		twl_pwm_ops.free = twl4030_pwm_free;

This would become:

		twl->chip.ops = &twl4030_pwm_ops;

> +	} else {
> +		twl_pwm_ops.enable = twl6030_pwm_enable;
> +		twl_pwm_ops.disable = twl6030_pwm_disable;

and:
		twl->chip.ops = &twl6030_pwm_ops;

> +static struct of_device_id twl_pwm_of_match[] = {
> +	{ .compatible = "ti,twl4030-pwm" },
> +	{ .compatible = "ti,twl6030-pwm" },
> +};
> +
> +MODULE_DEVICE_TABLE(of, twl_pwm_of_match);

Nit: I think the blank line between "};" and "MODULE_DEVICE_TABLE(...)"
can go away. And you might want to protect this with an "#ifdef
CONFIG_OF" since you don't depend on OF.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]