From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755761AbbI1DOK (ORCPT ); Sun, 27 Sep 2015 23:14:10 -0400 Received: from eso.teric.us ([69.164.192.171]:48585 "EHLO eso.teric.us" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755639AbbI1DOI (ORCPT ); Sun, 27 Sep 2015 23:14:08 -0400 X-Greylist: delayed 574 seconds by postgrey-1.27 at vger.kernel.org; Sun, 27 Sep 2015 23:14:08 EDT Date: Sun, 27 Sep 2015 22:04:34 -0500 From: Josh Cartwright To: Alexandre Belloni Cc: Alessandro Zummo , rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] rtc: Add a driver for Micro Crystal RV8803 Message-ID: <20150928030434.GJ3543@kryptos> References: <1443275679-21459-1-git-send-email-alexandre.belloni@free-electrons.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1443275679-21459-1-git-send-email-alexandre.belloni@free-electrons.com> User-Agent: Mutt/1.5.23+102 (2ca89bed6448) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- [..] > +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