Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 25 Nov 2022 12:59:00 +0200
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 v3 3/3] iio: magnetometer: add ti tmag5273 driver
Message-ID: <Y4Cf9MvNRcfy+7jf@smile.fi.intel.com>
References: <20221125083526.2422900-1-gerald.loacker@wolfvision.net>
 <20221125083526.2422900-4-gerald.loacker@wolfvision.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221125083526.2422900-4-gerald.loacker@wolfvision.net>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Fri, Nov 25, 2022 at 09:35:26AM +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.

Much better now, my comments below.

...

> +static int tmag5273_write_scale(struct tmag5273_data *data, int scale_micro)
> +{

What about

	u32 mask;

> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tmag5273_scale[0]); i++) {
> +		if (tmag5273_scale[data->version][i].val_micro == scale_micro)
> +			break;
> +	}
> +	if (i == ARRAY_SIZE(tmag5273_scale[0]))
> +		return -EINVAL;
> +	data->scale_index = i;

	if (data->scale_index == MAGN_RANGE_LOW)
		mask = 0;
	else
		mask = TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK;

> +	return regmap_update_bits(data->map,
> +		TMAG5273_SENSOR_CONFIG_2,
> +		TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK,

		mask);

> +		data->scale_index == MAGN_RANGE_LOW ? 0 :
> +			TMAG5273_Z_RANGE_MASK |
> +			TMAG5273_X_Y_RANGE_MASK);

?

> +}

...

> +		switch (chan->type) {
> +		case IIO_MAGN:

> +			if (val != 0)

			if (val)

> +				return -EINVAL;
> +			return tmag5273_write_scale(data, val2);
> +		default:
> +			return -EINVAL;
> +		}

...

> +static const struct regmap_config tmag5273_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,

> +	.max_register = 0xff,

Does it indeed have 256 registers?

> +	.volatile_reg = tmag5273_volatile_reg,
> +};

...

> +static void tmag5273_read_device_property(struct tmag5273_data *data)
> +{

	struct device *dev = data->dev;

> +	const char *str;
> +	int ret;
> +
> +	data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;

	ret = device_property_read_string(dev, "ti,angle-measurement", &str);
	if (ret)
		return;

> +	if (!device_property_read_string(data->dev, "ti,angle-measurement", &str)) {
> +		ret = match_string(tmag5273_angle_names,
> +				   ARRAY_SIZE(tmag5273_angle_names), str);
> +		if (ret < 0)
> +			dev_warn(data->dev,
> +				 "unexpected read angle-measurement property: %s\n", str);
> +		else
> +			data->angle_measurement = ret;
> +	}

	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);
	else
		data->angle_measurement = ret;

> +}

...

> +		return dev_err_probe(data->dev, ret,
> +				     "failed to power on device\n");

I would leave on one line (only 84 characters long).

...

> +		return dev_err_probe(data->dev, ret,
> +				     "failed to read device ID\n");

Ditto.

...

> +	switch (data->devid) {
> +	case TMAG5273_MANUFACTURER_ID:
> +		snprintf(data->name, sizeof(data->name), "tmag5273x%1u",
> +			 data->version);
> +		if (data->version < 1 || data->version > 2)
> +			dev_warn(data->dev, "Unsupported device %s\n",
> +				 data->name);
> +		break;
> +	default:
> +		dev_warn(data->dev, "Unknown device ID 0x%x\n", data->devid);
> +		break;
> +	}
> +
> +	return 0;

'break;':s above can be replaced by direct 'return 0;':s. It's up to you.

...

> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "IIO device allocation failed\n");

We don't print ENOMEM error messages in the drivers, core does this for us.
Otherwise you have to explain why this message is so important.

...

> +	/*
> +	 * Register powerdown deferred callback which suspends the chip
> +	 * after module unloaded.
> +	 *
> +	 * TMAG5273 should be in SUSPEND mode in the two cases:
> +	 * 1) When driver is loaded, but we do not have any data or
> +	 *    configuration requests to it (we are solving it using
> +	 *    autosuspend feature).
> +	 * 2) When driver is unloaded and device is not used (devm action is

Something with indentation of this or other lines.

> +	 *    used in this case).
> +	 */

...

> +		return dev_err_probe(dev, ret,
> +				     "failed to add powerdown action\n");

One line?

...

> +		dev_err(dev, "failed to power off device (%pe)\n",
> +			ERR_PTR(ret));

Ditto.

-- 
With Best Regards,
Andy Shevchenko