Re: [PATCH v3 2/2] iio:proximity:hx9023s: Add TYHX HX9023S sensor driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, May 29, 2024 at 7:58 AM Yasin Lee <yasin.lee.x@xxxxxxxxxxx> wrote:
>
> From: Yasin Lee <yasin.lee.x@xxxxxxxxx>
>
> A SAR sensor from NanjingTianyihexin Electronics Ltd.
>
> The device has the following entry points:
>
> Usual frequency:
> - sampling_frequency
>
> Instant reading of current values for different sensors:
> - in_proximity0_raw
> - in_proximity1_raw
> - in_proximity2_raw
> - in_proximity3_raw
> - in_proximity4_raw
> and associated events in events/

...

> @@ -21,4 +22,3 @@ obj-$(CONFIG_SX_COMMON)       += sx_common.o
>  obj-$(CONFIG_SX9500)           += sx9500.o
>  obj-$(CONFIG_VCNL3020)         += vcnl3020.o
>  obj-$(CONFIG_VL53L0X_I2C)      += vl53l0x-i2c.o
> -

Stray change.

...

> +#include <linux/i2c.h>

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>

Move this group (linux/iio/*) headers...

> +#include <linux/regmap.h>
>
> +#include <asm-generic/unaligned.h>

...here.

Also the rest (only two inclusions?!) is too little to this big
driver. Follow the IWYU principle ("include what you use").

...

> +#define TYHX_DELAY_MS(x) msleep(x)

This is misleading and actually useless. Use msleep() directly (btw,
you missed delay.h to be included).

...

> +#define HX9023S_RAW_BL_CH1_2                   0xED
> +#define HX9023S_RAW_BL_CH2_0                   0xEE
> +#define HX9023S_RAW_BL_CH2_1                   0xEF
> +#define HX9023S_RAW_BL_CH2_2                   0xF0
> +#define HX9023S_RAW_BL_CH3_0                   0xF1
> +#define HX9023S_RAW_BL_CH3_1                   0xF2
> +#define HX9023S_RAW_BL_CH3_2                   0xF3
> +#define HX9023S_RAW_BL_CH4_0                   0xB5
> +#define HX9023S_RAW_BL_CH4_1                   0xB6
> +#define HX9023S_RAW_BL_CH4_2                   0xB7
> +#define HX9023S_LP_DIFF_CH0_0                  0xF4
> +#define HX9023S_LP_DIFF_CH0_1                  0xF5
> +#define HX9023S_LP_DIFF_CH0_2                  0xF6
> +#define HX9023S_LP_DIFF_CH1_0                  0xF7
> +#define HX9023S_LP_DIFF_CH1_1                  0xF8
> +#define HX9023S_LP_DIFF_CH1_2                  0xF9
> +#define HX9023S_LP_DIFF_CH2_0                  0xFA
> +#define HX9023S_LP_DIFF_CH2_1                  0xFB
> +#define HX9023S_LP_DIFF_CH2_2                  0xFC
> +#define HX9023S_LP_DIFF_CH3_0                  0xFD
> +#define HX9023S_LP_DIFF_CH3_1                  0xFE
> +#define HX9023S_LP_DIFF_CH3_2                  0xFF
> +#define HX9023S_LP_DIFF_CH4_0                  0xB8
> +#define HX9023S_LP_DIFF_CH4_1                  0xB9
> +#define HX9023S_LP_DIFF_CH4_2                  0xBA

Please, jeep sorted by the register offset.

...

> +#define HX9023S_DATA_LOCK_MASK BIT(4)
> +#define HX9023S_INTERRUPT_MASK GENMASK(9, 0)
> +#define HX9023S_PROX_DEBOUNCE_MASK GENMASK(3, 0)

bits.h is missing above.

...

> +struct hx9023s_addr_val_pair {
> +       uint8_t addr;
> +       uint8_t val;
> +};

Can you use regular in-kernel types, i.e. u8?

> +struct hx9023s_channel_info {
> +       bool enabled;
> +       bool used;

Despite the above, you missed types.h to be included.

> +       int state;

Have you run `pahole` to check if it would be better to have this field first?

> +};

...

> +static struct hx9023s_addr_val_pair hx9023s_reg_init_list[] = {

I would like to see a comment along each initialisation value to
explain what it does. Otherwise it looks like a magic blob.

Also make comments, if needed, about the ordering of this list. I.o.w.
does it have dependencies or all registers can be initialised in
arbitrary order?

> +};

...

> +struct hx9023s_data {
> +       struct mutex mutex;

> +       struct i2c_client *client;

For what purpose?

> +       struct iio_trigger *trig;
> +       struct regmap *regmap;
> +       unsigned long chan_prox_stat;
> +       bool trigger_enabled;
> +       struct {
> +               __be16 channels[HX9023S_CH_NUM];
> +

Redundant blank line.

> +               s64 ts __aligned(8);
> +

Ditto.

> +       } buffer;
> +       unsigned long chan_read;
> +       unsigned long chan_event;
> +
> +       struct hx9023s_threshold thres[HX9023S_CH_NUM];
> +       struct hx9023s_channel_info *chs_info;
> +       unsigned long ch_en_stat;
> +       unsigned int prox_state_reg;
> +       unsigned int accuracy;
> +       unsigned long channel_used_flag;
> +       unsigned int cs_position[HX9023S_CH_NUM];
> +       unsigned int channel_positive[HX9023S_CH_NUM];
> +       unsigned int channel_negative[HX9023S_CH_NUM];
> +       int raw[HX9023S_CH_NUM];
> +       int lp[HX9023S_CH_NUM]; /*low pass*/
> +       int bl[HX9023S_CH_NUM]; /*base line*/
> +       int diff[HX9023S_CH_NUM]; /*lp - bl*/

Mind spaces in the comments.

> +       uint16_t dac[HX9023S_CH_NUM];

u16

> +       bool sel_bl[HX9023S_CH_NUM];
> +       bool sel_raw[HX9023S_CH_NUM];
> +       bool sel_diff[HX9023S_CH_NUM];
> +       bool sel_lp[HX9023S_CH_NUM];
> +       unsigned int odr;
> +       unsigned int integration_sample;
> +       unsigned int osr[HX9023S_CH_NUM];
> +       unsigned int avg[HX9023S_CH_NUM];
> +       unsigned int lp_alpha[HX9023S_CH_NUM];


Can you rather make a per-channel structure and then have only one array here

  struct foo_channel chan_context[_CH_NUM];

?

> +};

...

> +static const unsigned int hx9023s_samp_freq_table[] = {
> +       2, 2, 4, 6, 8, 10, 14, 18, 22, 26,
> +       30, 34, 38, 42, 46, 50, 56, 62, 68, 74,
> +       80, 90, 100, 200, 300, 400, 600, 800, 1000, 2000,
> +       3000, 4000

Keep trailing comma.

> +};

...

> +static const struct regmap_config hx9023s_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .cache_type = REGCACHE_NONE,

Why no cache?
Do you need a regmap lock on top of what you have already?

> +};

...

> +                       dev_err(&data->client->dev, "i2c read failed\n");

> +                       dev_err(&data->client->dev, "i2c read failed\n");

  struct device *dev = regmap_get_dev(...);

  dev_err(dev, ...);

here and everywhere else.

...

> +static int hx9023s_data_lock(struct hx9023s_data *data, bool locked)
> +{
> +       int ret;
> +
> +       if (locked) {
> +               ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1,
> +                                       HX9023S_DATA_LOCK_MASK, HX9023S_DATA_LOCK_MASK);
> +               if (ret < 0) {

Why ' < 0' ?

> +                       dev_err(&data->client->dev, "i2c read failed\n");
> +                       return ret;
> +               }

  if (ret)
    dev_err();
  return ret;


> +       } else {

Redundant. see above.

> +               ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1,
> +                                       GENMASK(4, 3), 0x00);

With 0x00 --> 0 and above this will be one line.

> +               if (ret < 0) {
> +                       dev_err(&data->client->dev, "i2c read failed\n");
> +                       return ret;
> +               }
> +       }
> +
> +       return ret;

Too many unneeded LoCs, see above how to optimise.

> +}

...

> +static int hx9023s_get_id(struct hx9023s_data *data)
> +{
> +       int ret;
> +       unsigned int rxbuf[1];
> +
> +       ret = regmap_read(data->regmap, HX9023S_DEVICE_ID, rxbuf);
> +       if (ret < 0) {
> +               dev_err(&data->client->dev, "i2c read failed\n");
> +               return ret;
> +       }
> +
> +       return 0;

Same optimisation as per above function applicable here. Do it
everywhere to remove a few LoCs here and there.

> +}

...

> +static int hx9023s_para_cfg(struct hx9023s_data *data)
> +{
> +       int ret;
> +       uint8_t buf[3];
> +
> +       ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &data->odr, 1);
> +       if (ret) {
> +               dev_err(&data->client->dev, "i2c write failed\n");
> +               return ret;
> +       }

> +       buf[0] = data->integration_sample & 0xFF;
> +       buf[1] = data->integration_sample >> 8;

put_unaligned_le16()

> +       ret = regmap_bulk_write(data->regmap, HX9023S_SAMPLE_NUM_7_0, buf, 2);
> +       if (ret) {

> +               dev_err(&data->client->dev, "i2c write failed\n");

This is a very repetitive and useless message AFAICS.

> +               return ret;
> +       }
> +
> +       ret = regmap_bulk_write(data->regmap, HX9023S_INTEGRATION_NUM_7_0, buf, 2);
> +       if (ret) {
> +               dev_err(&data->client->dev, "i2c write failed\n");
> +               return ret;
> +       }
> +
> +       buf[0] = (data->avg[2] << 4) | data->avg[1];
> +       buf[1] = (data->avg[4] << 4) | data->avg[3];

I believe this also can be optimised, esp. if you reconsider the avg[]
data type.

> +       ret = regmap_bulk_write(data->regmap, HX9023S_AVG12_CFG, buf, 2);
> +       if (ret) {
> +               dev_err(&data->client->dev, "i2c write failed\n");
> +               return ret;
> +       }
> +
> +       buf[0] = (data->osr[2] << 4) | data->osr[1];
> +       buf[1] = (data->osr[4] << 4) | data->osr[3];

Ditto.

> +       ret = regmap_bulk_write(data->regmap, HX9023S_NOSR12_CFG, buf, 2);
> +       if (ret) {
> +               dev_err(&data->client->dev, "i2c write failed\n");
> +               return ret;
> +       }
> +
> +       ret = regmap_update_bits(data->regmap, HX9023S_AVG0_NOSR0_CFG, GENMASK(7, 2),
> +                               ((data->avg[0] << 5) | (data->osr[0] << 2)));

Too many parentheses.

> +       if (ret < 0) {
> +               dev_err(&data->client->dev, "i2c read failed\n");
> +               return ret;
> +       }

> +       buf[0] = data->lp_alpha[4];
> +       buf[1] = (data->lp_alpha[1] << 4) | data->lp_alpha[0];
> +       buf[2] = (data->lp_alpha[3] << 4) | data->lp_alpha[2];

Also sounds like put_unaligned_be24() with a properly cooked argument.

> +       ret = regmap_bulk_write(data->regmap, HX9023S_LP_ALP_4_CFG, buf, 3);
> +       if (ret) {
> +               dev_err(&data->client->dev, "i2c write failed\n");
> +               return ret;
> +       }
> +
> +       return ret;
> +}

I stopped here as most of your functions have the same problems and
can be shrinked with a few % gain of the overall number of LoCs.

...

> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
> +               value = get_unaligned_le16(&rx_buf[i * offset_data_size]);

> +               value = value & 0xFFF;
> +               data->dac[i] = value;

Just

  ->dac = value & GENMASK();

> +       }

...

> +static int hx9023s_dts_phase(struct hx9023s_data *data)
> +{

> +       struct device_node *np = data->client->dev.of_node;

No for at least two reasons:
- for the sensors we do not accept new code that is OF-centric, make
use of the agnostic device property APIs
- it's bad to dereference of_node/fwnode as it adds unneeded churn in the future

You will need property.h to be included.

> +       return 0;
> +}

> +       if ((data->chan_read | data->chan_event) != channels) {
> +               for (i = 0; i < HX9023S_CH_NUM; i++) {
> +                       if (test_bit(i, &data->channel_used_flag) && test_bit(i, &channels))

Make it
  for_each_set_bit(i, &channels, ...) {
    if (test_bit(..., _is_used)) // rename _used_flag to _is_used or
even _in_use
  }

(Replace bits.h with bitops.h in the inclusion block for these)

> +                               hx9023s_ch_en_hal(data, i, true);
> +                       else
> +                               hx9023s_ch_en_hal(data, i, false);
> +               }
> +       }
> +
> +       data->chan_read = chan_read;
> +       data->chan_event = chan_event;
> +       return 0;
> +}

...

> +       odr = hx9023s_samp_freq_table[buf[0]];
> +       *val = 1000 / odr;
> +       *val2 = ((1000 % odr) * 1000000ULL) / odr;

Include units.h and use the proper definitions from there.

...

> +       period_ms = div_u64(1000000000ULL, (val * 1000000ULL + val2));

math.h is missing.
Also consider using proper time constants live NSEC_PER_SEC or so.

...

> +       for (i = 0; i < ARRAY_SIZE(hx9023s_samp_freq_table); i++) {

array_size.h is missing.

> +               if (period_ms == hx9023s_samp_freq_table[i])
> +                       break;
> +       }
> +       if (i == ARRAY_SIZE(hx9023s_samp_freq_table)) {
> +               dev_err(&data->client->dev, "Period:%dms NOT found!\n", period_ms);

dev_printk.h

> +               return -EINVAL;

errno.h

> +       }

...

> +       ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &buf[0], 1);

, buf, sizeof(buf) ?

> +       if (ret)

You are not even consistent with the checks in one file!

> +               dev_err(&data->client->dev, "i2c read failed\n");

> +       return ret;

Can it not be 0 here?

...

> +               if (data->chs_info[chan->channel].enabled)
> +                       set_bit(chan->channel, &data->chan_event);
> +               else
> +                       clear_bit(chan->channel, &data->chan_event);

Why atomic?
In any case, use assign_bit() / __assign_bit().

...

> +       int i = 0;

Why signed?

> +       guard(mutex)(&data->mutex);
> +       hx9023s_sample(data);
> +       hx9023s_get_prox_state(data);
> +
> +       for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength)
> +               data->buffer.channels[i++] = data->diff[indio_dev->channels[bit].channel];

...

> +static int hx9023s_probe(struct i2c_client *client)
> +{
> +       int ret;
> +       int i;
> +       struct device *dev = &client->dev;
> +       struct iio_dev *indio_dev;
> +       struct hx9023s_data *data;
> +
> +       indio_dev = devm_iio_device_alloc(dev, sizeof(struct hx9023s_data));
> +       if (!indio_dev)
> +               return dev_err_probe(&client->dev, -ENOMEM, "device alloc failed\n");

You are even inconsistent in the use of dev in a single function! Why
not dev here?

> +       data = iio_priv(indio_dev);
> +       data->client = client;

> +       mutex_init(&data->mutex);

mutex.h

> +       ret = hx9023s_dts_phase(data);
> +       if (ret)
> +               return dev_err_probe(&data->client->dev, ret, "dts phase failed\n");
> +
> +       data->chs_info = devm_kzalloc(&data->client->dev,
> +                               sizeof(struct hx9023s_channel_info) * HX9023S_CH_NUM, GFP_KERNEL);

Okay, you need to replace dev_printk.h I mentioned above by device.h,
but on top of that this should be devm_kcalloc().

> +       if (data->chs_info == NULL)

Pattern is if (!...)

> +               return dev_err_probe(&data->client->dev, -ENOMEM, "channel info alloc failed\n");

Ouch, as I said, this is the third variant of dev to be used. Use dev
everywhere.

> +       for (i = 0; i < HX9023S_CH_NUM; i++)
> +               if (test_bit(i, &data->channel_used_flag))

for_each_set_bit()

> +                       data->chs_info[i].used = true;

Interesting why you need this.

> +       data->regmap = devm_regmap_init_i2c(client, &hx9023s_regmap_config);
> +       if (IS_ERR(data->regmap)) {

> +               ret = PTR_ERR(data->regmap);
> +               return dev_err_probe(&data->client->dev, ret, "regmap init failed\n");

Use dev and move PTR_ERR() to be in the parameter for dev_err_probe().

> +       }
> +
> +       ret = devm_regulator_get_enable(&data->client->dev, "vdd");
> +       if (ret)
> +               return dev_err_probe(&data->client->dev, ret, "regulator get failed\n");

> +       usleep_range(1000, 1100);

fsleep()

> +       ret = hx9023s_get_id(data);
> +       if (ret)
> +               return dev_err_probe(&data->client->dev, ret, "id check failed\n");
> +
> +       indio_dev->channels = hx9023s_channels;
> +       indio_dev->num_channels = ARRAY_SIZE(hx9023s_channels);
> +       indio_dev->info = &hx9023s_info;
> +       indio_dev->modes = INDIO_DIRECT_MODE;
> +       indio_dev->name = "hx9023s";
> +       i2c_set_clientdata(client, indio_dev);
> +
> +       ret = hx9023s_reg_init(data);
> +       if (ret)
> +               return dev_err_probe(&data->client->dev, ret, "device init failed\n");
> +
> +       ret = hx9023s_ch_cfg(data);
> +       if (ret)
> +               return dev_err_probe(&data->client->dev, ret, "channel config failed\n");
> +
> +       ret = hx9023s_para_cfg(data);
> +       if (ret)
> +               return dev_err_probe(&data->client->dev, ret, "parameter config failed\n");
> +
> +       if (client->irq) {
> +               ret = devm_request_threaded_irq(dev, client->irq, hx9023s_irq_handler,
> +                                               hx9023s_irq_thread_handler, IRQF_ONESHOT,
> +                                               "hx9023s_event", indio_dev);
> +               if (ret)
> +                       return dev_err_probe(&data->client->dev, ret, "irq request failed\n");
> +
> +               data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> +                                               iio_device_id(indio_dev));

I'm wondering if there is a default naming in that API... Would be
nice to have it for cases like this.

> +               if (!data->trig)
> +                       return dev_err_probe(&data->client->dev, -ENOMEM,
> +                                       "iio trigger alloc failed\n");
> +
> +               data->trig->ops = &hx9023s_trigger_ops;
> +               iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> +               ret = devm_iio_trigger_register(dev, data->trig);
> +               if (ret)
> +                       return dev_err_probe(&data->client->dev, ret,
> +                                       "iio trigger register failed\n");
> +       }
> +
> +       ret = devm_iio_triggered_buffer_setup(dev, indio_dev, iio_pollfunc_store_time,
> +                                       hx9023s_trigger_handler, &hx9023s_buffer_setup_ops);
> +       if (ret)
> +               return dev_err_probe(&data->client->dev, ret,
> +                               "iio triggered buffer setup failed\n");
> +
> +       ret = devm_iio_device_register(dev, indio_dev);
> +       if (ret)
> +               return dev_err_probe(&data->client->dev, ret, "iio device register failed\n");
> +
> +       return 0;
> +}

...

> +static const struct acpi_device_id hx9023s_acpi_match[] = {
> +       { .id = "TYHX9023", .driver_data = HX9023S_CHIP_ID },

We don't use C99 for ACPI ID tables, moreover you need mod_devicetable.h.
See also below.

> +       {}
> +};
> +MODULE_DEVICE_TABLE(acpi, hx9023s_acpi_match);

> +static const struct of_device_id hx9023s_of_match[] = {
> +       { .compatible = "tyhx,hx9023s", (void *)HX9023S_CHIP_ID },

No, use a proper pointer that will give a chip info like structure.
Same for above and below ID tables.

> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, hx9023s_of_match);
> +
> +static const struct i2c_device_id hx9023s_id[] = {
> +       { .name = "hx9023s", .driver_data = HX9023S_CHIP_ID },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(i2c, hx9023s_id);

...

Also, for better review experience, can you split this to a few patches, like
1. main functionality
2. trigger support
3. ACPI support (ID table)
?

Reviewing 1.5 kLoCs at once is kinda big load.

-- 
With Best Regards,
Andy Shevchenko





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux