On 08/10/10 07:55, Mike Frysinger wrote:
> On Sun, Aug 8, 2010 at 11:08, Jonathan Cameron wrote:
>> Here are the 3 abi update patches needed to make the drivers
>> Mike posted to work with the current mainline kernel.
>
> thanks, i merged your changes into our tree and was able to extend
> them to another driver (gyro/adis16261). we did have a tracker item
> open to cull imu/volt.h usage, but it wasnt fixed yet as you noticed
> ;).
>
> question about iio_work_cont though ... we have some drivers using that field.
> static void ad7816_interrupt_bh(struct work_struct *work_s)
> {
> struct iio_work_cont *wc = to_iio_work_cont_no_check(work_s);
> struct ad7816_chip_info *chip = wc->st;
>
> i'm guessing the ad7816_chip_info.iio_work_cont should be converted to
> a work_struct ?
Exactly. It was originally intended for the case where you had an event interrupt,
but the chip did not need it to be acknowledged. Thus if there was only one
event enabled one could save a bus transaction by not checking what it was.
It appears this case is rather less common than I thought. None of the current
mainlined drivers make use of it (all need to read the 'what was the event' register
in order to clear the interrupt). If a part turns up that can make use of this
functionality we can put it back in, but for now it's just useless bloat.
Usual principle of don't support something until you actually have a user applies.
It went in originally because I misread a datasheet.
Whilst I'm here.... (cc'd Sonic for this bit given the log on that driver)
If anyone is looking at the ad7816 driver at the mo, can I just make a few
quick comments. (I was glancing at it anyway given Mike's pointer to it above).
Feel free to ignore these for now but I thought I'd send them as I was reading it anyway!
1) Don't invent new event codes specific to the device. The over temperature indicator
is a simple threshold event. Also never introduce events specific to the device. We have
plenty of space for these, so however obscure these should go in the relevant header.
Basically this needs a bit of datasheet interpretation (admittedly always a fun game!)
but most events map to fairly standard types.
Also I note that as things currently stand, the bh doesn't actually clear the interrupt.
(pin function table on page 8 of the data sheet says that any serial read will do the job)
Without that, if anyone is using level interrupts you'll just generate another one whether
or not the temp is still over threshold.
2) No newlines in attribute outputs. (available_modes). Also, can save some code by using
a IIO_CONST_ATTR for that one.
3) Store channel etc. Please do this according to the abi (separate attr per channel).
4) Why a shared event? There is only one type as far as I can tell. (actually this one is
non critical, will just save you some code!) I guess it's plausible that code route is
currently broken as I'm not sure it has any users I've tested recently. Please yell if
it is!
5) Non crit as well. Why not use an enum for the second param in spi_device_id table, then you
can avoid strcmp's on the names elsewhere in the driver.
Anyhow just a few small bits as I was looking at it anyway.
Incidentally I'm happy to give earlier feedback on drivers before you are ready to send
them for a merge if it would smooth things later. Just send them to linux-iio and mark
them as say 'not ready for merge' so no one picks them up!
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
