Messages in this thread | | | Date | Fri, 25 Nov 2022 12:59:00 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 3/3] iio: magnetometer: add ti tmag5273 driver |
| |
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
| |