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
Date: Fri, 25 Nov 2022 12:59:00 +0200 [thread overview]
Message-ID: <Y4Cf9MvNRcfy+7jf@smile.fi.intel.com> (raw)
In-Reply-To: <20221125083526.2422900-4-gerald.loacker@wolfvision.net>
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
next prev parent reply other threads:[~2022-11-25 10:59 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-25 8:35 [PATCH v2 0/3] add ti tmag5273 driver Gerald Loacker
2022-11-25 8:35 ` [PATCH v3 1/3] iio: add struct declarations for iio types Gerald Loacker
2022-11-25 10:45 ` Andy Shevchenko
2022-11-25 11:01 ` Andy Shevchenko
2022-11-28 12:18 ` Gerald Loacker
2022-11-28 13:27 ` Andy Shevchenko
2022-11-28 13:48 ` Michael Riesch
2022-11-28 14:05 ` Andy Shevchenko
2022-11-28 14:26 ` Michael Riesch
2022-12-03 17:11 ` Jonathan Cameron
2022-11-25 8:35 ` [PATCH v3 2/3] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file Gerald Loacker
2022-11-25 8:35 ` [PATCH v3 3/3] iio: magnetometer: add ti tmag5273 driver Gerald Loacker
2022-11-25 10:59 ` Andy Shevchenko [this message]
2022-11-25 11:00 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y4Cf9MvNRcfy+7jf@smile.fi.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=devicetree@vger.kernel.org \
--cc=gerald.loacker@wolfvision.net \
--cc=jahau@rocketmail.com \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=linus.walleij@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=michael.riesch@wolfvision.net \
--cc=nikita.yoush@cogentembedded.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).