Re: [PATCH 1/1] iio: adc: tlc4541: add support for TI tlc4541 adc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




G'day Peter,

Thanks for the review.

On 5/01/2017 17:21, Peter Meerwald-Stadler wrote:
<snip>
+
+#define TLC4541_V_CHAN(index, bits) {                                 \
+		.type = IIO_VOLTAGE,                                  \
+		.indexed = 1,                                         \

no need if there is just one channel

+		.channel = index,                                     \
+		.info_mask_separate       = BIT(IIO_CHAN_INFO_RAW),   \
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+		.address = index,                                     \

.address not needed

+		.scan_index = index,                                  \
+		.scan_type = {                                        \
+			.sign = 'u',                                  \
+			.realbits = (bits),                           \
+			.storagebits = 16,                            \
+			.endianness = IIO_BE,                         \
+		},                                                    \
+	}
+
+#define DECLARE_TLC4541_CHANNELS(name, bits) \

this flexibility is only needed when further chips are added; maybe start
simple and only implement what is needed at first
I'll add the spec for to the TLC3541 which is a 14-bit version of this chip.
I don't have one to test, but it looks pretty straight forward.


+const struct iio_chan_spec name ## _channels[] = { \
+	TLC4541_V_CHAN(0, bits), \
+	IIO_CHAN_SOFT_TIMESTAMP(1), \
+}
+
+static DECLARE_TLC4541_CHANNELS(tlc4541, 16);
+
+static const struct tlc4541_chip_info tlc4541_chip_info[] = {
+	[TLC4541] = {
+		.channels = tlc4541_channels,
+		.num_channels = ARRAY_SIZE(tlc4541_channels),
+	},
+};
+
+static irqreturn_t tlc4541_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct tlc4541_state *st = iio_priv(indio_dev);
+	u16 buf[8]; /* 2 bytes data + 6 bytes padding + 8 bytes timestamp */
+	int ret;
+
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	if (ret < 0)
+		goto done;
+
+	buf[0] = be16_to_cpu(st->rx_buf[0]);

endianness is set to IIO_BE in scan_type, so this conversion is not needed
and maybe also buf and the copy can be avoided if rx_buf is large enough

I was doing the conversion in IIO_CHAN_INFO_RAW as well.
Is it required there? I'm guessing yes.


+	iio_push_to_buffers_with_timestamp(indio_dev, buf,
+					   iio_get_time_ns(indio_dev));
+
+done:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static int tlc4541_get_range(struct tlc4541_state *st)
+{
+	int vref;
+
+	vref = regulator_get_voltage(st->reg);
+	if (vref < 0)
+		return vref;
+
+	vref /= 1000;
+
+	return vref;
+}
+
+static int tlc4541_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val,
+			    int *val2,
+			    long m)
+{
+	int ret = 0;
+	struct tlc4541_state *st = iio_priv(indio_dev);
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+		ret = spi_sync(st->spi, &st->scan_single_msg);
+		iio_device_release_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+		*val = be16_to_cpu(st->rx_buf[0]);

on page 12 of the datasheet, the conversion results is in two registers?
and rx_buf has two elements?

haven't investigated in detail -- maybe a comment would be good to detail
operation?
I set that to 4 bytes because I also use that for the dummy 24 clock cycles
at the beginning as well. I think that documentation is a bit misleading.
Possible due to the tlc3451 datasheet being  very similar. Those two registers
are 14 bits and 2 bits wide respectively. The SPI cycle time shows data is
available in the first 16 bits.



+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		ret = tlc4541_get_range(st);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	}
+	return -EINVAL;
+}
<snip>


--
Regards
Phil Reid

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux