Messages in this thread | | | Date | Tue, 29 Nov 2022 18:37:42 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v4 3/3] iio: magnetometer: add ti tmag5273 driver |
| |
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
| |