Answers inline
On 08/25/2016 08:34 AM, Peter Meerwald-Stadler wrote:
+#define ZPA_CONVERSION_TIMEOUT (HZ / 5)
ZPA2326_ prefix would be expected by convention
It seems such a long prefix to me. I would prefer "zpa" but it's not
distinctive enough.
Anyway, convention prevails.
+
+/* Registers map. */
Register map
+#define ZPA_REF_P_XL_REG ((u8)0x8)
do we really need u8 here?
nop.
why?
Just to inform reader addresses are encoded onto 8 bits. Removed.
+/*
+ * Enable device, i.e. get out of low power mode.
+ *
+ * Required to access complete register space and to perform any sampling
+ * or control operations.
+ */
+static int zpa_enable_device(const struct iio_dev *indio_dev)
+{
+ int err = zpa_write_byte(indio_dev, ZPA_CTRL_REG0_REG,
+ ZPA_CTRL_REG0_ENABLE);
+ if (err) {
+ zpa_err(indio_dev, "failed to enable device (%d)", err);
I think messages should end with \n
Why ? Enforce flushing ? dev_err adds an implicit "\n". It saves a few
characters and
sometimes makes strings fit into 80 columns :)
+/* Enqueue new channel samples to IIO buffer. */
+static int zpa_fill_sample_buffer(struct iio_dev *indio_dev,
+ const struct zpa_private *private)
+{
+ struct {
+ u32 pressure;
+ u16 temperature;
timestamp should be 8-byte aligned, padding needed
This one surprises me. I may completly miss the point but unless using
the packed attribute, compiler implicitly aligns "timestamp" upon 8 bytes
boundary in this case.
+ u64 timestamp;
+ } sample;
extra spaces before 'sample'
text-aligned onto the below variable declaration.
+ int err;
+
+ if (test_bit(0, indio_dev->active_scan_mask)) {
+ /* Get current pressure from hardware fifo. */
+ err = zpa_dequeue_pressure(indio_dev, &sample.pressure);
pressure is 4 bytes, but only 3 bytes are read; one byte uninitialized?
arrrgh! Many thanks for this. My mind must have been floating around while
coding this.
+ if (err) {
+ zpa_warn(indio_dev, "failed to fetch pressure (%d)",
here and elsewhere: why zpa_warn() on error?
Because this error does not put kernel and/or driver functional
stability in danger.
Maybe we can recover from it at next sampling round. It just requires user
attention.
I've always been a bit confused as to which log level to use and from
which context.
Any rule of thumb ?
+/* Retrieve a raw sample and convert it to CPU endianness. */
+static int zpa_fetch_raw_sample(const struct iio_dev *indio_dev,
+ enum iio_chan_type type,
+ int *value)
+{
+ int err;
+
+ switch (type) {
+ case IIO_PRESSURE:
+ zpa_dbg(indio_dev, "fetching raw pressure sample");
+
+ err = zpa_read_block(indio_dev, ZPA_PRESS_OUT_XL_REG, 3,
+ (u8 *)value);
+ if (err) {
+ zpa_warn(indio_dev, "failed to fetch pressure (%d)",
+ err);
+ return err;
+ }
+
+ /* Pressure is a 24 bits wide little-endian unsigned int. */
+ *value = (((u8 *)value)[2] << 16) | (((u8 *)value)[1] << 8) |
+ ((u8 *)value)[0];
+
+ return IIO_VAL_INT;
+
+ case IIO_TEMP:
+ zpa_dbg(indio_dev, "fetching raw temperature sample");
+
+ err = zpa_read_block(indio_dev, ZPA_TEMP_OUT_L_REG, 2,
+ (u8 *)value);
+ if (err) {
+ zpa_warn(indio_dev, "failed to fetch temperature (%d)",
+ err);
+ return err;
+ }
+
+ /* Temperature is a 16 bits wide little-endian signed int. */
+ *value = (int)le16_to_cpup((__le16 *)value);
+
+ return IIO_VAL_INT;
+
+ default:
+ BUG();
return -EINVAL
This case is clearly a coding bug since if channels are properly defined
this will never happen unless IIO layer is completly screwed up.
Why returning an error then ? This is a situation that requires debug /
code rework.
Same for other BUG() invocations.
I'm not reluctant to change anything here, I just want to properly
understand
the rationales.
+static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
+ zpa_show_frequency,
+ zpa_store_frequency);
+
+/* Expose supported hardware sampling frequencies (Hz) through sysfs. */
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("1 5 11 23");
+
+static struct attribute *zpa_attributes[] = {
+ &iio_dev_attr_sampling_frequency.dev_attr.attr,
+ &iio_const_attr_sampling_frequency_available.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group zpa_attribute_group = {
+ .attrs = zpa_attributes,
+};
+
+static const struct iio_chan_spec zpa_channels[] = {
+ [0] = {
+ .type = IIO_PRESSURE,
+ .channel = 0,
.channel is not needed since channels are not indexed
ack'ed
+
+ zpa_init_runtime(slave);
+
+ err = devm_iio_device_register(slave, indio_dev);
can't use devm_ register if there is a non-empty remove()
Removal hooks are registered onto bus specific devices not onto
IIO ones. Enabling devm debug outputs a removal flow trace that
seems correct. Is this really wrong ?
Many thanks for the review. Regards,
Greg.
--
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
