On Thu, Oct 20, 2022 at 02:37:15PM +0300, Matti Vaittinen wrote:
> KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor features
> include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ,
> tap/motion detection, wake-up & back-to-sleep events, four acceleration
> ranges (2, 4, 8 and 16g) and probably some other cool features.
>
> Add support for the basic accelerometer features such as getting the
> acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> using the WMI IRQ).
>
> Important things to be added include the double-tap, motion
> detection and wake-up as well as the runtime power management.
...
> + if (!i2c->irq) {
> + dev_err(dev, "No IRQ configured\n");
> + return -EINVAL;
At least
return dev_err_probe(...);
for know error codes (or when we know that there won't be EPROBE_DEFER), takes
less LoCs in the source file.
> + }
...
> + regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
> + if (IS_ERR(regmap)) {
> + dev_err(dev, "Failed to initialize Regmap\n");
> + return PTR_ERR(regmap);
Ditto here and anywhere else for the similar cases.
> + }
...
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *vals = (const int *)kx022a_accel_samp_freq_table;
> + *length = ARRAY_SIZE(kx022a_accel_samp_freq_table) * 2;
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + return IIO_AVAIL_LIST;
> + case IIO_CHAN_INFO_SCALE:
> + *vals = (const int *)kx022a_scale_table;
> + *length = ARRAY_SIZE(kx022a_scale_table) * 2;
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + return IIO_AVAIL_LIST;
These ' * 2' can be replaced with respective ARRAY_SIZE() of nested element
for robustness, but I don't think it worth it. What we need is to provide
IIO specific type for these tables and use it.
...
> +static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
> +{
> + int ret;
> +
> + if (on)
> + ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> + KX022A_MASK_PC1);
> + else
> + ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
> + KX022A_MASK_PC1);
> +
> + if (ret)
> + dev_err(data->dev, "Turn %s fail %d\n", (on) ? "ON" : "OFF",
> + ret);
str_on_off() ?
> + return ret;
> +
> +}
...
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
> +
> + while (n--)
> + if (val == kx022a_accel_samp_freq_table[n][0] &&
> + kx022a_accel_samp_freq_table[n][1] == val2)
Why not to use the same kind of l and r arguments in == lines?
In current form it's a bit harder to see what the catch here.
> + break;
> + if (n < 0) {
> + ret = -EINVAL;
> + goto unlock_out;
> + }
> + ret = kx022a_turn_off_lock(data);
> + if (ret)
> + break;
> +
> + ret = regmap_update_bits(data->regmap,
> + KX022A_REG_ODCNTL,
> + KX022A_MASK_ODR, n);
> + data->odr_ns = kx022a_odrs[n];
> + kx022a_turn_on_unlock(data);
> + break;
> + case IIO_CHAN_INFO_SCALE:
> + n = ARRAY_SIZE(kx022a_scale_table);
> +
> + while (n-- > 0)
> + if (val == kx022a_scale_table[n][0] &&
> + kx022a_scale_table[n][1] == val2)
Ditto.
> + break;
> + if (n < 0) {
> + ret = -EINVAL;
> + goto unlock_out;
> + }
> +
> + ret = kx022a_turn_off_lock(data);
> + if (ret)
> + break;
> +
> + ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
> + KX022A_MASK_GSEL,
> + n << KX022A_GSEL_SHIFT);
> + kx022a_turn_on_unlock(data);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
...
> +static int kx022a_get_axis(struct kx022a_data *data,
> + struct iio_chan_spec const *chan,
> + int *val)
> +{
> + int ret;
> +
> + ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
> + sizeof(__le16));
> + if (ret)
> + return ret;
> +
> + *val = le16_to_cpu(data->buffer[0]);
'p'-variant of the above would look better
*val = le16_to_cpup(data->buffer);
since it will be the same as above address without any additional arithmetics.
> + return IIO_VAL_INT;
> +}
...
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, ®val);
> + if (ret)
> + return ret;
> +
> + if ((regval & KX022A_MASK_ODR) >
> + ARRAY_SIZE(kx022a_accel_samp_freq_table)) {
> + dev_err(data->dev, "Invalid ODR\n");
> + return -EINVAL;
> + }
> +
> + kx022a_reg2freq(regval, val, val2);
> + ret = IIO_VAL_INT_PLUS_MICRO;
> +
> + break;
return IIO_VAL_INT_PLUS_MICRO;
> +
> + case IIO_CHAN_INFO_SCALE:
> + ret = regmap_read(data->regmap, KX022A_REG_CNTL, ®val);
> + if (ret < 0)
> + return ret;
> +
> + kx022a_reg2scale(regval, val, val2);
> +
> + ret = IIO_VAL_INT_PLUS_MICRO;
> + break;
Ditto.
...
> + return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
Would simple '0' suffice?
...
> + for (i = 0; i < count; i++) {
> + int bit;
> + u16 *samples = &buffer[i * 3];
I would put it as
u16 *samples = &buffer[i * 3];
int bit;
> + for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
> + memcpy(&data->scan.channels[bit], &samples[bit],
> + sizeof(data->scan.channels[0]));
Why not use bit instead of 0 for the sake of consistency?
Also might be good to have a temporary for channels:
... *chs = data->scan.channels;
for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
memcpy(&chs[bit], &samples[bit], sizeof(chs[bit]));
> + iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
> +
> + tstamp += sample_period;
> + }
...
> + ret = regmap_clear_bits(data->regmap, data->ien_reg,
> + KX022A_MASK_WMI);
I don't see why it's not on a single line. Even if you are a conservative
adept of 80.
Maybe other lines also need to be revised?
> + if (ret)
> + goto unlock_out;
...
> + int ret = IRQ_NONE;
> +
> + mutex_lock(&data->mutex);
> +
> + if (data->trigger_enabled) {
> + iio_trigger_poll_chained(data->trig);
> + ret = IRQ_HANDLED;
> + }
> +
> + if (data->state & KX022A_STATE_FIFO) {
> + ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
> + if (ret > 0)
> + ret = IRQ_HANDLED;
I don't like it. Perhaps
bool handled = false;
int ret;
...
ret = ...
if (ret > 0)
handled = true;
...
return IRQ_RETVAL(handled);
> + }
> +
> + mutex_unlock(&data->mutex);
> +
> + return ret;
...
> + if (!dev)
> + return -ENODEV;
Do you really need this check?
...
> + fw = dev_fwnode(dev);
> + if (!fw)
> + return -ENODEV;
You may combine these two in one.
struct fwnode_handle *fwnode;
fwnode = dev ? dev_fwnode(dev) : NULL;
if (!fwnode)
return -ENODEV;
And please, call it fwnode.
...
> + irq = fwnode_irq_get_byname(fw, "INT1");
> + if (irq > 0) {
> + data->inc_reg = KX022A_REG_INC1;
> + data->ien_reg = KX022A_REG_INC4;
> +
> + if (fwnode_irq_get_byname(dev_fwnode(dev), "INT2") > 0)
Why not use fwnode again
> + dev_warn(dev, "Only one IRQ supported\n");
> + } else {
> + irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
Ditto.
> + if (irq <= 0)
> + return dev_err_probe(dev, irq, "No suitable IRQ\n");
> +
> + data->inc_reg = KX022A_REG_INC5;
> + data->ien_reg = KX022A_REG_INC6;
> + }
...
> + if (ret)
> + return dev_err_probe(data->dev, ret,
> + "iio_triggered_buffer_setup_ext FAIL %d\n",
> + ret);
Drop dup ret at the end, dev_err_probe() has been adding it to each message.
...
> + /*
> + * No need to check for NULL. request_threadedI_irq() defaults to
> + * dev_name() should the alloc fail.
> + */
> + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
> + dev_name(data->dev));
It's not clear why do you need a suffix here.
--
With Best Regards,
Andy Shevchenko
