On 05/27/2011 10:09 AM, Michael Hennerich wrote:
On 05/26/2011 04:58 PM, Jonathan Cameron wrote:
...
What would happen if this driver used any other trigger? Would everything work?
No. But other drivers can you the trigger. It's not really an trigger it's a data ready.
Most are. As you say, it is useful to trigger other reads from this, but not to trigger
this to read from other sources...
I think it would do an immediate read which is going to be a problem. Perhaps
we need a way of restricting triggers. This one can be used by anyone, but the
part can only use it's own trigger (I think).
Having the ability to reject alien triggers are nice to have.
True enough. I guess the easiest is some sort of 'filter' callback on trigger connect.
Then drivers that care, can reject devices that don't match what they need. Would
probably want one in each direction. Trigggers can reject devices and devices can
reject triggers.
Looked is a bool really, might as well make it explicit. Reg can only be a couple
of bytes, so maybe a u8? Doesn't really matter though.
+static int __ad7793_write_reg(struct ad7793_state *st, unsigned locked,
+ unsigned cs_change, unsigned reg,
+ unsigned size, unsigned val)
+{
+ u8 data[4];
Worth putting in board state?
I'll add data to the state structure.
+ struct spi_transfer t = {
+ .tx_buf = data,
+ .len = size + 1,
+ .cs_change = cs_change,
+ };
+ struct spi_message m;
+
+ data[0] = AD7793_COMM_WRITE | AD7793_COMM_ADDR(reg);
+
+ switch (size) {
+ case 3:
+ data[1] = val>> 16;
+ data[2] = val>> 8;
+ data[3] = val;
+ break;
+ case 2:
+ data[1] = val>> 8;
+ data[2] = val;
+ break;
+ case 1:
+ data[1] = val;
+ break;
This is a bit nasty, but I can see why you did it. Though it would give
longer code, I'd be inclined to move the data[0] assignment for all of the
above cases into the switch statement. Then this last element fits
in better with the rest.
Actually I don't use this function with size=0, so I remove this part completely.
Originally it was intended to allow access to the COMM register in order to enable CSREAD.
Good, that's even better.
...
+ ret = ad7793_write_reg(st, AD7793_REG_MODE, sizeof(st->mode), st->mode);
+ if (ret)
+ goto out;
+ /* write/read test for device presence */
Hmm.. this sort of test is always pretty hit and miss. I'd just assume the
board config is correct and not bother with the test when there isn't a who_am_I
register available...
Actually there is an id register that we can query.
Yeah, I noticed that when looking at the datasheet later in the review.
Much better idea ;)
+static int ad7793_ring_postdisable(struct iio_dev *indio_dev)
+{
+ struct ad7793_state *st = iio_priv(indio_dev);
+
+ st->mode = (st->mode& ~AD7793_MODE_SEL(-1)) |
+ AD7793_MODE_SEL(AD7793_MODE_IDLE);
+
+ st->done = false;
+ wait_event_interruptible(st->wq_data_avail, st->done);
So basically this is waiting for one last wakeup to occur before
disabling the irq?
Yes - for CREAD mode is is mandatory that the device is RDY, when
exiting the continuous conversion mode. For continuous conversion mode
not using CREAD we can write to the mode register anytime.
+
+ if (!st->irq_dis)
+ disable_irq_nosync(st->spi->irq);
+
+ __ad7793_write_reg(st, 1, 0, AD7793_REG_MODE,
+ sizeof(st->mode), st->mode);
+
+
+ return spi_bus_unlock(st->spi->master);
+}
+
+/**
+ * ad7793_trigger_handler() bh of trigger launched polling to ring buffer
+ **/
+
+static irqreturn_t ad7793_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->private_data;
+ struct iio_ring_buffer *ring = indio_dev->ring;
+ struct ad7793_state *st = iio_priv(indio_dev);
I like this approach to alignment, nice and tidy ;)
+ s64 dat64[2];
+ s32 *dat32 = (s32 *)dat64;
+
On this front, is it not worth using CREAD bit of the communications register?
Then if I understand correctly, there is no need to do the write element
of this read?
Originally - I thought to use the CREAD, but exiting this mode is not 100% error prone.
See my comment above.
Hmm.. That is somewhat awkward, so I guess what you have is pretty much the only option.
...
+&iio_dev_attr_sampling_frequency.dev_attr.attr,
+&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+&iio_dev_attr_range.dev_attr.attr,
+&iio_dev_attr_range_available.dev_attr.attr,
hmm. I've always been keener on controlling range via 'scale' parameters.
Or does this mean something else for this part?
Well - range implies the maximum input voltage that can be applied.
Scale means something different for me - but I might be wrong.
They tend to be closely connected. So many bits exist, and they apply over
a certain range. That means the scale factor to be applied also changes
as one changes the range. Often it's just a matter of picking one of
scale and range to make controllable, and having the other change
explicitly. We have to have scale available for raw attributes, whereas
range is optional, so I'd generally advocate changing scale to change
the range rather than the other way around..
Hi Jonathan,
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> ls
device0:buffer0 power
in-in_scale range
in0-in0_raw range_available
in1-in1_raw sampling_frequency
in2-in2_raw sampling_frequency_available
in3_raw subsystem
in4_supply_raw temp0_raw
in4_supply_scale temp_scale
in_scale trigger
name uevent
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> cat in_scale
0.000140
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> cat range_available
2500 1250 625 312 156 78 39 19
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> echo 312> range
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> cat in_scale
0.000010
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> echo 78> range
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0> cat in_scale
0.000000
root:/sys/devices/platform/bfin-spi.0/spi0.18/device0>
with these 24-bit converters and input AMPs we are already exhausted
the number of available digits we have for scale.
What shall we do?
I think everything atof() or scanf() eats, should be fine.
Let's introduce an exponent?
int (*read_raw)(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val,
int *val2,
int exp,
long mask);
Also how would you name AIN1(-) - AIN1(-)?
#define AD7793_CH_AIN1P_AIN1M 0 /* AIN1(+) - AIN1(-) */
#define AD7793_CH_AIN2P_AIN2M 1 /* AIN2(+) - AIN2(-) */
#define AD7793_CH_AIN3P_AIN3M 2 /* AIN3(+) - AIN3(-) */
#define AD7793_CH_AIN1M_AIN1M 3 /* AIN1(-) - AIN1(-) */
in0-in0_zerooffset_raw ?
--
Greetings,
Michael
--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif
_______________________________________________
Device-drivers-devel mailing list
Device-drivers-devel@xxxxxxxxxxxxxxxxxxxx
https://blackfin.uclinux.org/mailman/listinfo/device-drivers-devel
--
Greetings,
Michael
--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif
--
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
