Re: [PATCH-V2 1/3] RTC:s35390a: Add Alarm interrupt support

[Felipe Balbi]

Hi,

On Sat, Aug 21, 2010 at 02:30:27PM +0200, ext hvaibhav@xxxxxx wrote:
> +static irqreturn_t s35390a_irq_thread(int irq, void *handle)
> +{
> +	char buf[1];
> +	struct s35390a *s35390a = handle;
> +	struct i2c_client *client = s35390a->client[0];

don't you need some locking on the irq handler ? a mutex maybe ? Just 
wondering...

> @@ -261,15 +424,30 @@ static int s35390a_probe(struct i2c_client *client,
> 	if (s35390a_get_datetime(client, &tm) < 0)
> 		dev_warn(&client->dev, "clock needs to be set\n");
>
> +	if (client->irq >= 0) {
> +		err = request_threaded_irq(client->irq, NULL,
> +				s35390a_irq_thread,
> +				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +				client->name, s35390a);
> +		if (err) {
> +			dev_err(&client->dev, "unable to request IRQ\n");
> +			goto exit_dummy;
> +		}
> +	}
> +
> 	s35390a->rtc = rtc_device_register(s35390a_driver.driver.name,
> 				&client->dev, &s35390a_rtc_ops, THIS_MODULE);
>
> 	if (IS_ERR(s35390a->rtc)) {
> 		err = PTR_ERR(s35390a->rtc);
> -		goto exit_dummy;
> +		goto exit_intr;
> 	}
> +
> 	return 0;
>
> +exit_intr:
> +	free_irq(client->irq, client);

free_irq() won't behave correctly, I believe since you're passing 
different dev_id parameters. If you look at the implementation of 
free_irq() you'll see it uses dev_id to find the correct struct 
irqaction pointer.

--
balbi

DefectiveByDesign.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/