On Tue, Nov 29, 2022 at 07:45:40AM +0100, Gerald Loacker wrote:
> Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
> Additionally to temperature and magnetic X, Y and Z-axes the angle and
> magnitude are reported.
> The sensor is operating in continuous measurement mode and changes to sleep
> mode if not used for 5 seconds.
...
> +static int tmag5273_get_measure(struct tmag5273_data *data, s16 *t, s16 *x,
> + s16 *y, s16 *z, u16 *angle, u16 *magnitude)
> +{
> + unsigned int status, val;
> + __be16 reg_data[4];
> + int ret;
> +
> + mutex_lock(&data->lock);
> +
> + /*
> + * Max. conversion time is 2425 us in 32x averaging mode for all three
> + * channels. Since we are in continuous measurement mode, a measurement
> + * may already be there, so poll for completed measurement with
> + * timeout.
> + */
> + ret = regmap_read_poll_timeout(data->map, TMAG5273_CONV_STATUS, status,
> + status & TMAG5273_CONV_STATUS_COMPLETE,
> + 100, 10000);
> + if (ret) {
> + dev_err_probe(data->dev, ret,
> + "timeout waiting for measurement\n");
Is it called from ->probe()? I don't think so...
> + goto out_unlock;
> + }
> +
> + ret = regmap_bulk_read(data->map, TMAG5273_T_MSB_RESULT, reg_data,
> + sizeof(reg_data));
> + if (ret)
> + goto out_unlock;
> + *t = be16_to_cpu(reg_data[0]);
> + *x = be16_to_cpu(reg_data[1]);
> + *y = be16_to_cpu(reg_data[2]);
> + *z = be16_to_cpu(reg_data[3]);
> +
> + ret = regmap_bulk_read(data->map, TMAG5273_ANGLE_RESULT_MSB,
> + ®_data[0], sizeof(reg_data[0]));
> + if (ret)
> + goto out_unlock;
> + /*
> + * angle has 9 bits integer value and 4 bits fractional part
> + * 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
> + * 0 0 0 a a a a a a a a a f f f f
> + */
> + *angle = be16_to_cpu(reg_data[0]);
> +
> + ret = regmap_read(data->map, TMAG5273_MAGNITUDE_RESULT, &val);
> + if (ret < 0)
> + goto out_unlock;
> + *magnitude = val;
> +
> +out_unlock:
> + mutex_unlock(&data->lock);
> + return ret;
> +}
...
> +static const struct iio_info tmag5273_info = {
> + .read_avail = &tmag5273_read_avail,
> + .read_raw = &tmag5273_read_raw,
> + .write_raw = &tmag5273_write_raw,
> +};
Functions when being assigned are already pointers, no?
...
> + ret = match_string(tmag5273_angle_names,
> + ARRAY_SIZE(tmag5273_angle_names), str);
> + if (ret < 0)
> + dev_warn(dev, "unexpected read angle-measurement property: %s\n", str);
dev_warn(dev, "unexpected value in angle-measurement property: %s\n", str);
?
> + else
> + data->angle_measurement = ret;
...
> + snprintf(data->name, sizeof(data->name), "tmag5273x%1u", data->version);
Thinking more about this format, perhaps
snprintf(data->name, sizeof(data->name), "tmag5273x-v%1u", data->version);
?
...
> +static int tmag5273_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tmag5273_data *data = iio_priv(indio_dev);
> + int ret;
> + /*
> + * Time to go to stand-by mode from sleep mode is 50us
> + * typically. During this time no I2C access is possible.
> + */
Shouldn't be this comment closer to usleep_range()?
> + tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
> + usleep_range(80, 200);
> + ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
> + if (ret)
> + dev_err(dev, "failed to power on device (%pe)\n", ERR_PTR(ret));
> +
> + return ret;
> +}
...
> +static DEFINE_RUNTIME_DEV_PM_OPS(tmag5273_pm_ops, tmag5273_runtime_suspend,
> + tmag5273_runtime_resume, NULL);
I would logically split it like:
static DEFINE_RUNTIME_DEV_PM_OPS(tmag5273_pm_ops,
tmag5273_runtime_suspend, tmag5273_runtime_resume, NULL);
or like:
static DEFINE_RUNTIME_DEV_PM_OPS(tmag5273_pm_ops,
tmag5273_runtime_suspend, tmag5273_runtime_resume,
NULL);
or like:
static DEFINE_RUNTIME_DEV_PM_OPS(tmag5273_pm_ops,
tmag5273_runtime_suspend,
tmag5273_runtime_resume,
NULL);
Depending on your preferences.
--
With Best Regards,
Andy Shevchenko
