On Tue, May 31, 2016 at 09:08:27AM +0200, Pascal Sachs wrote:
> Hi Guenter
>
> I want to thank you again for the fast and detailed code review.
> I really appreciate it.
>
Sorry it took so long.
[ ... ]
> >>+static struct sht3x_data *sht3x_update_client(struct device *dev)
> >>+{
> >>+ struct sht3x_data *data = dev_get_drvdata(dev);
> >>+ struct i2c_client *client = data->client;
> >>+ u16 interval_ms = mode_to_update_interval[data->mode];
> >
> >If data->mode == 0, interval_ms == 0,
> >
> >>+ unsigned long interval_jiffies = msecs_to_jiffies(interval_ms);
> >
> >... interval_jiffies == 1,
> >
> >>+ unsigned char buf[SHT3X_RESPONSE_LENGTH];
> >>+ u16 val;
> >>+ int ret = 0;
> >>+
> >>+ mutex_lock(&data->data_lock);
> >>+ /* only update once per interval in periodic mode */
> >>+ if (time_after(jiffies, data->last_update + interval_jiffies)) {
> >
> >... and the command is executed every other timer tick. Is that
> >intentional ?
> Yes, this is how it should behave. In single shot mode the sensor measures
> values on demand, which means every time the sysfs interface is called, a
> measurement is triggered. In periodic mode however the measurement process
> is handled internally by the sensor and read out only makes sense if a new
> reading is available.
Please add the explanation as comment into the code.
[ ... ]
> >>+ commands = &limit_commands[index];
> >>+
> >>+ memcpy(position, commands->write_command, SHT3X_CMD_LENGTH);
> >>+ position += SHT3X_CMD_LENGTH;
> >>+ /*
> >>+ * ST = (T + 45) / 175 * 2^16
> >>+ * SRH = RH / 100 * 2^16
> >>+ * adapted for fixed point arithmetic and packed the same as
> >>+ * in limit_show()
> >>+ */
> >>+ raw = ((u32)(temperature + 45000) * 24543) >> (16 + 7);
> >>+ raw |= ((humidity * 42950) >> 16) & 0xfe00;
> >>+
> >>+ *((__be16 *)position) = cpu_to_be16(raw);
> >>+ position += SHT3X_WORD_LEN;
> >>+ *position = crc8(sht3x_crc8_table,
> >>+ position - SHT3X_WORD_LEN,
> >>+ SHT3X_WORD_LEN,
> >>+ SHT3X_CRC8_INIT);
> >>+
> >>+ mutex_lock(&data->data_lock);
> >>+ mutex_lock(&data->i2c_lock);
> >>+ ret = i2c_master_send(client, buffer, sizeof(buffer));
> >>+ mutex_unlock(&data->i2c_lock);
> >>+
> >>+ if (ret != sizeof(buffer))
> >>+ goto out;
> >>+
> >>+ data->temperature_limits[index] = temperature;
> >>+ data->humidity_limits[index] = humidity;
> >>+
> >>+out:
> >>+ mutex_unlock(&data->data_lock);
> >>+ if (ret != sizeof(buffer))
> >>+ return ret < 0 ? ret : -EIO;
> >>+
> >>+ return count;
> >
> >I would prefer something like
> >
> > if (ret != sizeof(buffer)) {
> > if (ret >= 0)
> > ret = -EIO;
> > goto out;
> > }
> >
> > ret = count;
> > data->temperature_limits[index] = temperature;
> > data->humidity_limits[index] = humidity;
> >out:
> > mutex_unlock(&data->data_lock);
> > return ret;
> >
> >which I think would be cleaner. See below though for locking issues.
> OK, wasn't sure if it is appropriate to handle error and valid return in the
> same variable
Done all over the place.
[ ... ]
> >
> >>+ mutex_lock(&data->i2c_lock);
> >>+ mutex_lock(&data->data_lock);
> >>+ /* abort periodic measure */
> >>+ ret = i2c_master_send(client, sht3x_cmd_break, SHT3X_CMD_LENGTH);
> >>+ if (ret != SHT3X_CMD_LENGTH)
> >>+ goto out;
> >>+ data->mode = 0;
> >>+
> >
> >Can you explain why you set data->mode = 0 here ?
> To do any changes to the configuration while in periodic mode we have to
> send a break command to the sensor, which then falls back to single shot
> mode (mode = 0). Therefore for consistency reasons we set the mode to
> the value it actually is at this point.
> If setting the new mode fails, we are still in single shot mode. Like this
> we
> ensure that the data->mode is always correct.
Ok; please add the explanation as comment into the code.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
