[i2c] [PATCH 1/5] rtc: RTC class driver for the ds1374

[David Brownell]

On Monday 21 May 2007, Scott Wood wrote:
> David Brownell wrote:
> >>+struct ds1374 {
> >>+	struct i2c_client *client;
> >>+	struct rtc_device *rtc;
> >>+	struct work_struct work;
> >>+	struct mutex mutex;
> >>+	spinlock_t lock;
> > 
> > 
> > Both spinlock and mutex?  That looks wrong.  Previously
> > I've written I2C drivers that accept IRQs and only need
> > the mutex (well, it was a semaphore back then).
> 
> The spinlock is just for synchronization during remove().  The main 
> problem in removing it (by freeing the IRQ before calling 
> flush_scheduled_work) is calling enable_irq() after it's been freed.

You could use atomic bitflags.  Having two kinds of lock is
error prone; worth avoiding, just to prevent the confusion.


> >>+static u8 read_reg(struct i2c_client *client, int reg, int *err)
> >
> >>+static void write_reg(struct i2c_client *client, int reg, u8 val, int *err)
> > 
> > 
> > The usual convention is to return negative errno ... for the read()
> > case you can use that too, since positive values can only mean "u8"
> > values.  (Same notion as ssize_t...)
> 
> Sure...  the intent was to simplify making multiple calls, allowing 
> error checking to be deferred to the end.  I can make it more normal if 
> that's what y'all want, though.

I guess I believe in "fail-fast" design inside the kernel.
As well as avoiding strange and/or error-prone idioms.


> > treat it as an error if
> > somehow a time-in-the-past gets to here.
> 
> I'd rather have the alarm go off as soon as possible, since it's also 
> possible that it was in the future when userspace chose the time, but is 
> now in the past.  It'd suck to not wake from suspend because of that. :-)
> 
> Of course, the caller could check for an error and decide what it wants 
> to do, but is that what most users are going to do?

Good programmers check for faults.  My rule of thumb is
that 1/3 of any program checks and handles faults (not
all of which are errors).

Are "most" users using programs written by good programmers
(and not on a bad day)?  You may have a point.  ;)


> >>+		disable_irq_nosync(irq);
> > 
> > ... and it seems here you're assuming this IRQ is level
> > triggered?
> 
> I'm assuming it's at least a possibility.

I suppose the issue of losing an edge trigger isn't going to
be very important with an unshared alarm IRQ.  But disable_irq()
calls always worry me.


> > It'd make a lot more sense to trigger on the
> > falling (?) edge; if you can do that, it'd even be possible
> > to put this on a shared IRQ line.
> 
> Yes, but hardware people don't always do things that make the most 
> sense. :-)

Sometimes it's quite the opposite, in fact!

Regardless:  IRQ setup isn't done by the hardware folks.
I suppose you could expect platform setup to do that.


> > As noted above, "have_irq" is not needed.  And you should
> > probably try to use an edge trigger mode here ...
> 
> Is the driver really the place that should be messing with that sort of 
> thing?  I'd rather leave it up to the platform to configure interrupt 
> triggers (on powerpc, this information is expressed in the device tree).

I guess I'm used to seeing platforms *not* set that stuff up;
it's probably cleaner when they do.


>   The driver should check it though, and allow IRQ sharing if edge.

How could it check, though?  That seems to be missing
functionality in genirq.  When drivers need to behave
differently based on trigger type, the only answer for
now is that _they_ must set the trigger type, not any
platform code.

- Dave