Re: [PATCH] mfd: Add PWM1 and PWM2 support to twl6030-pwm

[Samuel Ortiz <sameo@linux.intel.com>]

Hi Hemanth,

On Tue, Aug 30, 2011 at 12:27:17PM +0530, Hemanth V wrote:
> From: Hemanth V <hemanthv@ti.com>
> Date: Fri, 26 Aug 2011 10:49:29 +0530
> Subject: [PATCH] Add PWM1 and PWM2 support to twl6030-pwm driver
> 
> This patch adds support for PWM1/PWM2. TWL6030 PWM driver also
> supports Indicator LED PWM. Function pointers are defined for
> for init, enable, disable and configuration for both Indicator LED
> PWM (led_pwm) and PWM1/PWM2 (std_pwm)
Some comments on this code:


> +/* PWMs supported by driver */
> +#define PWM_ID_LED		1
> +#define PWM_ID_PWM1		2
> +#define PWM_ID_PWM2		3
I wish we could use enums here, but that's not what the PWM API is expecting.



> +int led_pwm_enable(struct pwm_device *pwm)
All your pwm_ops should be static now.


>  {
>  	u8 val;
>  	int ret;
> @@ -95,9 +140,8 @@ int pwm_enable(struct pwm_device *pwm)
>  	twl_i2c_read_u8(TWL6030_MODULE_ID1, &val, LED_PWM_CTRL2);
>  	return 0;
>  }
> -EXPORT_SYMBOL(pwm_enable);
> 
> -void pwm_disable(struct pwm_device *pwm)
> +void led_pwm_disable(struct pwm_device *pwm)
>  {
>  	u8 val;
>  	int ret;
> @@ -120,37 +164,284 @@ void pwm_disable(struct pwm_device *pwm)
>  	}
>  	return;
>  }
> -EXPORT_SYMBOL(pwm_disable);
> 
> -struct pwm_device *pwm_request(int pwm_id, const char *label)
> +int led_pwm_init(struct pwm_device *pwm)
>  {
>  	u8 val;
>  	int ret;
> +
> +	val = PWM_CTRL2_DIS_PD | PWM_CTRL2_CURR_02 | PWM_CTRL2_SRC_VBUS |
> +		PWM_CTRL2_MODE_HW;
> +
> +	ret = twl_i2c_write_u8(TWL6030_MODULE_ID1, val, LED_PWM_CTRL2);
> +
> +	return ret;
> +}
> +
> +static struct pwm_ops pwm_led = {
> +	.config = led_pwm_config,
> +	.enable = led_pwm_enable,
> +	.disable = led_pwm_disable,
> +	.init = led_pwm_init,
> +};
> +
> +int std_pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> +{
> +	int ret = 0, level, pwm_id, reg;
> +
> +	level = (duty_ns * PWM_CTRL1_MAX) / period_ns;
> +	pwm_id = pwm->pwm_id;
> +
> +	if (pwm_id == PWM_ID_PWM1)
> +		reg = LED_PWM1ON;
> +	else
> +		reg = LED_PWM2ON;
This is not consistent with your:

if (PWM1)
else if (PWM2)
else
	error

logic below.

Moreover, I'd rather use switch() here but that's more of a personal taste
than anything else.


> +struct pwm_device *pwm_request(int pwm_id, const char *label)
> +{
> +	int ret, found = 0;
>  	struct pwm_device *pwm;
> 
> +	mutex_lock(&pwm_lock);
> +
> +	list_for_each_entry(pwm, &pwm_list, node) {
> +		if (pwm->pwm_id == pwm_id) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (found) {
> +		if (pwm->use_count == 0) {
> +			pwm->use_count++;
> +			pwm->label = label;
> +		} else {
> +			pwm = ERR_PTR(-EBUSY);
> +		}
I failed to understand the logic here. How can you have found == TRUE, and
use_count being 0 ? Also, don't you want to track the pwm users and disable it
when user_count is reaching 0 ? You're not doing that from pwm_free().



> +		goto out;
You're leaving with the pwm_lock locked.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/