Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1233826

Re: [PATCH] rtc: Add a driver for Micro Crystal RV8803

Show all headers | View raw

Hello Alexandre-

Few comments below.

On Sat, Sep 26, 2015 at 03:54:39PM +0200, Alexandre Belloni wrote:
> This driver supports the following functions:
>  - reading and settings time
>  - alarms when connected to an IRQ
>  - reading and clearing the voltage low flags
>  - nvram
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
[..]
> +static irqreturn_t rv8803_handle_irq(int irq, void *dev_id)
> +{
> +	struct i2c_client *client = dev_id;
> +	struct rv8803_data *rv8803 = i2c_get_clientdata(client);
> +	unsigned long events = 0;
> +	u8 flags;
> +
> +	flags = i2c_smbus_read_byte_data(client, RV8803_FLAG);
> +	if (flags <= 0)
> +		return IRQ_HANDLED;

Returning IRQ_HANDLED when no interrupt condition is met.  That seems
like a bad idea.

> +	if (flags & RV8803_FLAG_V1F)
> +		dev_warn(&client->dev, "Voltage low, temperature compensation stopped.\n");
> +
> +	if (flags & RV8803_FLAG_V2F)
> +		dev_warn(&client->dev, "Voltage low, data loss detected.\n");
> +
> +	if (flags & RV8803_FLAG_TF) {
> +		flags &= ~RV8803_FLAG_TF;
> +		rv8803->ctrl &= ~RV8803_CTRL_TIE;
> +		events |= RTC_PF;
> +	}
> +
> +	if (flags & RV8803_FLAG_AF) {
> +		flags &= ~RV8803_FLAG_AF;
> +		rv8803->ctrl &= ~RV8803_CTRL_AIE;
> +		events |= RTC_AF;
> +	}
> +
> +	if (flags & RV8803_FLAG_UF) {
> +		flags &= ~RV8803_FLAG_UF;
> +		rv8803->ctrl &= ~RV8803_CTRL_UIE;
> +		events |= RTC_UF;
> +	}
> +
> +	if (events) {
> +		rtc_update_irq(rv8803->rtc, 1, events);
> +		i2c_smbus_write_byte_data(client, RV8803_FLAG, flags);

How are the many read-modify-write cycles for flags safe without any
form of synchronization?  (Especially given the interrupt handler isn't
under ops_lock).

> +		i2c_smbus_write_byte_data(rv8803->client, RV8803_CTRL,
> +					  rv8803->ctrl);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
[..]
> +static int rv8803_get_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rv8803_data *rv8803 = dev_get_drvdata(dev);
> +	struct i2c_client *client = rv8803->client;
> +	u8 alarmvals[3];
> +	int flags, ret;
> +
> +	if (client->irq <= 0)
> +		return -EINVAL;

It'd be cleaner just to have a second set of rtc_class_ops that can be
switched between based on whether a valid interrupt is specified.

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] rtc: Add a driver for Micro Crystal RV8803 Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2015-09-26 16:00 +0200
  [PATCH] rtc: fix platform_no_drv_owner.cocci warnings kbuild test robot <lkp@intel.com> - 2015-09-26 16:50 +0200
  drivers/rtc/rtc-rv8803.c:494:3-8: No need to set .owner here. The  core will do it. kbuild test robot <lkp@intel.com> - 2015-09-26 16:50 +0200
  Re: [PATCH] rtc: Add a driver for Micro Crystal RV8803 Josh Cartwright <joshc@eso.teric.us> - 2015-09-28 05:20 +0200
  Re: [PATCH] rtc: Add a driver for Micro Crystal RV8803 kbuild test robot <lkp@intel.com> - 2015-09-28 08:20 +0200
  [PATCH] rtc: fix platform_no_drv_owner.cocci warnings kbuild test robot <lkp@intel.com> - 2015-09-28 08:20 +0200

csiph-web