Re: [PATCH v4 3/3] iio: magnetometer: add ti tmag5273 driver

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Gerald Loacker <gerald.loacker@wolfvision.net>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jakob Hauser <jahau@rocketmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
	Michael Riesch <michael.riesch@wolfvision.net>
Subject: Re: [PATCH v4 3/3] iio: magnetometer: add ti tmag5273 driver
Date: Tue, 29 Nov 2022 18:37:42 +0200	[thread overview]
Message-ID: <Y4Y1VqvYS0XsWi1r@smile.fi.intel.com> (raw)
In-Reply-To: <20221129064540.3218982-4-gerald.loacker@wolfvision.net>

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,
> +			       &reg_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