[v4,2/2] iio:proximity:hx9023s: Add TYHX HX9023S sensor driver

Commit Message

Yasin Lee June 7, 2024, 11:41 a.m. UTC

From: Yasin Lee <[e-mail]>

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/

Signed-off-by: Yasin Lee <[e-mail]>
---
 drivers/iio/proximity/Kconfig   |   14 +
 drivers/iio/proximity/Makefile  |    1 +
 drivers/iio/proximity/hx9023s.c | 1162 +++++++++++++++++++++++++++++++
 3 files changed, 1177 insertions(+)
 create mode 100644 drivers/iio/proximity/hx9023s.c

Comments

Andy Shevchenko June 7, 2024, 7:40 p.m. UTC | #1

On Fri, Jun 7, 2024 at 2:42 PM Yasin Lee <[e-mail]> wrote:
>
> From: Yasin Lee <[e-mail]>
>
> 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/

...

> +#include <linux/acpi.h>

Unused.

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>

> +#include <linux/byteorder/generic.h>

It should be asm/byteorder.h after linux/* (you have already
unaligned.h there, move this one there)

> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqreturn.h>

> +#include <linux/kernel.h>

Why do you need this?

> +#include <linux/math.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/types.h>

Do you really use all of these iio/*?

...

> +struct hx9023s_ch_data {
> +       int raw;
> +       int lp; /* low pass */
> +       int bl; /* base line */

> +       int diff; /* lp-bl */

Decrypt the comment.

> +       struct {
> +               int near;
> +               int far;
> +       } thres;

Do all of the above have to be signed? Why?

> +       u16 dac;
> +       u8 cs_position;
> +       u8 channel_positive;
> +       u8 channel_negative;
> +       bool sel_bl;
> +       bool sel_raw;
> +       bool sel_diff;
> +       bool sel_lp;
> +       bool enable;
> +};
> +
> +struct hx9023s_data {
> +       struct iio_trigger *trig;
> +       struct regmap *regmap;
> +       unsigned long chan_prox_stat;
> +       unsigned long chan_read;
> +       unsigned long chan_event;
> +       unsigned long ch_en_stat;
> +       unsigned long chan_in_use;
> +       unsigned int prox_state_reg;
> +       bool trigger_enabled;
> +
> +       struct {
> +               __be16 channels[HX9023S_CH_NUM];
> +               s64 ts __aligned(8);
> +       } buffer;

> +       struct hx9023s_ch_data ch_data[HX9023S_CH_NUM];

I would suggest moving this to be the last field. It might help in the
future if we ever need to convert this to VLA.

> +       struct mutex mutex;
> +};
> +
> +static struct hx9023s_addr_val_pair hx9023s_reg_init_list[] = {
> +       /* scan period */
> +       { HX9023S_PRF_CFG,                    0x17 },
> +
> +       /* full scale of conversion phase of each channel */
> +       { HX9023S_RANGE_7_0,                  0x11 },
> +       { HX9023S_RANGE_9_8,                  0x02 },
> +       { HX9023S_RANGE_18_16,                0x00 },
> +
> +       /* adc avg number and osr number of each channel */

ADC
average
OSR

> +       { HX9023S_AVG0_NOSR0_CFG,             0x71 },
> +       { HX9023S_NOSR12_CFG,                 0x44 },
> +       { HX9023S_NOSR34_CFG,                 0x00 },
> +       { HX9023S_AVG12_CFG,                  0x33 },
> +       { HX9023S_AVG34_CFG,                  0x00 },
> +
> +       /* sample & integration frequency of the adc */

ADC

> +       { HX9023S_SAMPLE_NUM_7_0,             0x65 },
> +       { HX9023S_INTEGRATION_NUM_7_0,        0x65 },
> +
> +       /* coefficient of the first order low pass filter during each channel */
> +       { HX9023S_LP_ALP_1_0_CFG,             0x22 },
> +       { HX9023S_LP_ALP_3_2_CFG,             0x22 },
> +       { HX9023S_LP_ALP_4_CFG,               0x02 },
> +
> +       /* up coefficient of the first order low pass filter during each channel */
> +       { HX9023S_UP_ALP_1_0_CFG,             0x88 },
> +       { HX9023S_UP_ALP_3_2_CFG,             0x88 },
> +       { HX9023S_DN_UP_ALP_0_4_CFG,          0x18 },
> +
> +       /* down coefficient of the first order low pass filter during each channel */
> +       { HX9023S_DN_ALP_2_1_CFG,             0x11 },
> +       { HX9023S_DN_ALP_4_3_CFG,             0x11 },
> +
> +       /* output data */

What is this supposed to mean?

> +       { HX9023S_RAW_BL_RD_CFG,              0xF0 },
> +
> +       /* enable the interrupt function */
> +       { HX9023S_INTERRUPT_CFG,              0xFF },
> +       { HX9023S_INTERRUPT_CFG1,             0x3B },
> +       { HX9023S_DITHER_CFG,                 0x21 },
> +
> +       /* threshold of the offset compensation */
> +       { HX9023S_CALI_DIFF_CFG,              0x07 },
> +
> +       /* proximity persistency number(near & far) */
> +       { HX9023S_PROX_INT_HIGH_CFG,          0x01 },
> +       { HX9023S_PROX_INT_LOW_CFG,           0x01 },
> +
> +       /* disable the data lock */
> +       { HX9023S_DSP_CONFIG_CTRL1,           0x00 },
> +};

...

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

Why not MAPLE?

> +       .wr_table = &hx9023s_wr_regs,
> +       .volatile_table = &hx9023s_volatile_regs,
> +};

...

> +static int hx9023s_data_lock(struct hx9023s_data *data, bool locked)
> +{
> +       int ret;

Unneeded, you may return directly.

> +       if (locked)
> +               ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1,
> +                                       HX9023S_DATA_LOCK_MASK, HX9023S_DATA_LOCK_MASK);

  return regmap_update_bits(...);

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

...

> +static int hx9023s_ch_cfg(struct hx9023s_data *data)
> +{
> +       int ret;

> +       int i;

Why signed?

> +       u16 reg;
> +       u8 reg_list[HX9023S_CH_NUM * 2];
> +       u8 ch_pos[HX9023S_CH_NUM];
> +       u8 ch_neg[HX9023S_CH_NUM];
> +
> +       data->ch_data[0].cs_position = 0;
> +       data->ch_data[1].cs_position = 2;
> +       data->ch_data[2].cs_position = 4;
> +       data->ch_data[3].cs_position = 6;
> +       data->ch_data[4].cs_position = 8;
> +
> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
> +               if (data->ch_data[i].channel_positive == 255)

Magic number! Should it be GENMASK()? U8_MAX? Something else semantically?

> +                       ch_pos[i] = 16;
> +               else
> +                       ch_pos[i] = data->ch_data[data->ch_data[i].channel_positive].cs_position;
> +               if (data->ch_data[i].channel_negative == 255)

Ditto!

> +                       ch_neg[i] = 16;
> +               else
> +                       ch_neg[i] = data->ch_data[data->ch_data[i].channel_negative].cs_position;
> +       }

> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
> +               reg = (u16)((0x03 << ch_pos[i]) | (0x02 << ch_neg[i]));

Why casting? What are those magic numbers?

> +               reg_list[i * 2] = (u8)(reg);
> +               reg_list[i * 2 + 1] = (u8)(reg >> 8);

put_unaligned_le16()

> +       }

> +       ret = regmap_bulk_write(data->regmap, HX9023S_CH0_CFG_7_0, reg_list, HX9023S_CH_NUM * 2);
> +
> +       return ret;

Return directly.

> +}
> +
> +static int hx9023s_reg_init(struct hx9023s_data *data)
> +{
> +       int i = 0;

Why signed? What is that assignment for?

> +       int ret;
> +
> +       for (i = 0; i < (int)ARRAY_SIZE(hx9023s_reg_init_list); i++) {
> +               ret = regmap_bulk_write(data->regmap, hx9023s_reg_init_list[i].addr,
> +                                       &hx9023s_reg_init_list[i].val, 1);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}

...

> +static int hx9023s_write_far_debounce(struct hx9023s_data *data, int val)
> +{
> +       int ret;
> +
> +       if (val > 0xF)
> +               val = 0xF;

What is this magic?
Shouldn't you use clamp()?

> +       guard(mutex)(&data->mutex);
> +       ret = regmap_update_bits(data->regmap, HX9023S_PROX_INT_LOW_CFG,
> +                               HX9023S_PROX_DEBOUNCE_MASK, val);
> +
> +       return ret;

Return directly. Really you may reduce your code base by ~50 LoCs just
 by dropping unneeded ret and this kind of code.

> +}

...

> +static int hx9023s_write_near_debounce(struct hx9023s_data *data, int val)
> +{

As per above function.

> +}

...

> +static int hx9023s_get_thres_near(struct hx9023s_data *data, u8 ch, int *val)
> +{
> +       int ret;
> +       u8 buf[2];
> +
> +       if (ch == 4)

Instead, make a temporary variable for the reg and use only a single
call to regmap_bulk_read().

> +               ret = regmap_bulk_read(data->regmap, HX9023S_PROX_HIGH_DIFF_CFG_CH4_0, buf, 2);

sizeof(buf)

> +       else
> +               ret = regmap_bulk_read(data->regmap,
> +                               HX9023S_PROX_HIGH_DIFF_CFG_CH0_0 + (ch * HX9023S_2BYTES), buf, 2);

Ditto.

> +

Redundant blank line.

> +       if (ret)
> +               return ret;

> +       *val = get_unaligned_le16(buf);
> +       *val = (*val & GENMASK(9, 0)) * 32;
> +       data->ch_data[ch].thres.near = *val;

Better to have a temporary variable and do something like

  unsigned int tmp;

  tmp = (get_unaligned_le16(buf) & GENMASK(9, 0)) * 32;
  near = tmp;
  *val = tmp;

See the difference?

> +       return IIO_VAL_INT;
> +}

...

> +static int hx9023s_get_thres_far(struct hx9023s_data *data, u8 ch, int *val)
> +{

As per above.

> +}

...

> +static int hx9023s_set_thres_near(struct hx9023s_data *data, u8 ch, int val)
> +{

As per above.

> +}
> +
> +static int hx9023s_set_thres_far(struct hx9023s_data *data, u8 ch, int val)
> +{

As per above.

> +}

...

> +static void hx9023s_get_prox_state(struct hx9023s_data *data)
> +{
> +       int ret;

Are the 4 LoCs just for fun? Or why does the function return void?

> +       ret = regmap_read(data->regmap, HX9023S_PROX_STATUS, &data->prox_state_reg);
> +       if (ret)
> +               return;
> +}

...

> +static void hx9023s_data_select(struct hx9023s_data *data)

Why void?

> +{
> +       int ret;

> +       int i;

Why signed?

> +       unsigned long buf[1];

Why an array?

> +       ret = regmap_read(data->regmap, HX9023S_RAW_BL_RD_CFG, (unsigned int *)buf);

No. Why do you need this ugly casting?

> +       if (ret)
> +               return;
> +
> +       for (i = 0; i < 4; i++) {
> +               data->ch_data[i].sel_diff = test_bit(i, buf);
> +               data->ch_data[i].sel_lp = !data->ch_data[i].sel_diff;
> +               data->ch_data[i].sel_bl = test_bit(i + 4, buf);
> +               data->ch_data[i].sel_raw = !data->ch_data[i].sel_bl;
> +       }
> +
> +       ret = regmap_read(data->regmap, HX9023S_INTERRUPT_CFG1, (unsigned int *)buf);
> +       if (ret)
> +               return;
> +
> +       data->ch_data[4].sel_diff = test_bit(2, buf);
> +       data->ch_data[4].sel_lp = !data->ch_data[4].sel_diff;
> +       data->ch_data[4].sel_bl = test_bit(3, buf);
> +       data->ch_data[4].sel_raw = !data->ch_data[4].sel_bl;
> +}

...

> +static int hx9023s_sample(struct hx9023s_data *data)
> +{
> +       int ret;
> +       int i;

Why signed?

> +       u8 data_size;
> +       u8 offset_data_size;
> +       int value;
> +       u8 rx_buf[HX9023S_CH_NUM * HX9023S_BYTES_MAX];
> +
> +       hx9023s_data_lock(data, true);
> +       hx9023s_data_select(data);
> +
> +       data_size = HX9023S_3BYTES;
> +
> +       /* ch0~ch3 */
> +       ret = regmap_bulk_read(data->regmap, HX9023S_RAW_BL_CH0_0, rx_buf,
> +                               (HX9023S_CH_NUM * data_size) - data_size);

Make a temporary variable and use  (CH_NUM - 1) * data_size which is
shorter and clearer.

> +       if (ret)
> +               return ret;
> +
> +       /* ch4 */
> +       ret = regmap_bulk_read(data->regmap, HX9023S_RAW_BL_CH4_0,
> +                               rx_buf + ((HX9023S_CH_NUM * data_size) - data_size), data_size);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
> +               value = get_unaligned_le16(&rx_buf[i * data_size + 1]);
> +               value = sign_extend32(value, 15);
> +               data->ch_data[i].raw = 0;
> +               data->ch_data[i].bl = 0;
> +               if (data->ch_data[i].sel_raw == true)
> +                       data->ch_data[i].raw = value;
> +               if (data->ch_data[i].sel_bl == true)
> +                       data->ch_data[i].bl = value;
> +       }
> +
> +       /* ch0~ch3 */
> +       ret = regmap_bulk_read(data->regmap, HX9023S_LP_DIFF_CH0_0, rx_buf,
> +                               (HX9023S_CH_NUM * data_size) - data_size);

Use a temporary constant.

> +       if (ret)
> +               return ret;
> +
> +       /* ch4 */
> +       ret = regmap_bulk_read(data->regmap, HX9023S_LP_DIFF_CH4_0,
> +                               rx_buf + ((HX9023S_CH_NUM * data_size) - data_size), data_size);

Ditto.

> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
> +               value = get_unaligned_le16(&rx_buf[i * data_size + 1]);
> +               value = sign_extend32(value, 15);
> +               data->ch_data[i].lp = 0;
> +               data->ch_data[i].diff = 0;
> +               if (data->ch_data[i].sel_lp == true)
> +                       data->ch_data[i].lp = value;
> +               if (data->ch_data[i].sel_diff == true)
> +                       data->ch_data[i].diff = value;
> +       }
> +
> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
> +               if (data->ch_data[i].sel_lp == true && data->ch_data[i].sel_bl == true)
> +                       data->ch_data[i].diff = data->ch_data[i].lp - data->ch_data[i].bl;
> +       }
> +
> +       /* offset dac */
> +       offset_data_size = HX9023S_2BYTES;
> +       ret = regmap_bulk_read(data->regmap, HX9023S_OFFSET_DAC0_7_0, rx_buf,
> +                               (HX9023S_CH_NUM * offset_data_size));
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
> +               value = get_unaligned_le16(&rx_buf[i * offset_data_size]);
> +               value = FIELD_GET(GENMASK(11, 0), value);
> +               data->ch_data[i].dac = value;
> +       }
> +
> +       hx9023s_data_lock(data, false);
> +
> +       return 0;
> +}

> +static int hx9023s_ch_en(struct hx9023s_data *data, u8 ch_id, bool en)
> +{
> +       int ret;
> +       unsigned int buf;
> +
> +       ret = regmap_read(data->regmap, HX9023S_CH_NUM_CFG, &buf);
> +       if (ret)
> +               return ret;
> +
> +       data->ch_en_stat = buf;
> +
> +       if (en) {
> +               if (data->ch_en_stat == 0)
> +                       data->prox_state_reg = 0;
> +               set_bit(ch_id, &data->ch_en_stat);

Why atomit?

> +               ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
> +               if (ret)
> +                       return ret;
> +               data->ch_data[ch_id].enable = true;
> +       } else {
> +               clear_bit(ch_id, &data->ch_en_stat);
> +               ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
> +               if (ret)
> +                       return ret;
> +               data->ch_data[ch_id].enable = false;
> +       }

This can be compressed to

  if (en && ch_en_stat == 0)
    prox_state_reg = 0;
  __assign_bit(en); // or atomic, but the latter has to be justified
  enable = !!en;
  ret = regmap_bulk_write();
  if (ret)
    return ret;

> +       return 0;
> +}
> +
> +static int hx9023s_property_get(struct hx9023s_data *data)
> +{
> +       int ret, i;

Why is the 'i' signed?

> +       u32 temp;
> +       struct fwnode_handle *child;
> +       struct device *dev = regmap_get_device(data->regmap);
> +
> +       ret = device_property_read_u32(dev, "channel-in-use", &temp);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to read channel-in-use property\n");
> +       data->chan_in_use = temp;

Did you even compile your code? The below uses uninitialised value.

> +       device_for_each_child_node(dev, child) {

You have massive leaks in this loop, you need to use _scoped() variant.

> +               ret = fwnode_property_read_u32(child, "channel-positive", &temp);
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                       "Failed to read channel-positive for channel %d\n", i);
> +               data->ch_data[i].channel_positive = temp;
> +
> +               ret = fwnode_property_read_u32(child, "channel-negative", &temp);
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                       "Failed to read channel-negative for channel %d\n", i);
> +               data->ch_data[i].channel_negative = temp;
> +
> +               i++;
> +       }
> +
> +       return 0;
> +}
> +
> +static int hx9023s_update_chan_en(struct hx9023s_data *data,
> +                               unsigned long chan_read,
> +                               unsigned long chan_event)
> +{
> +       int i;

Why signed?

> +       unsigned long channels = chan_read | chan_event;
> +
> +       if ((data->chan_read | data->chan_event) != channels) {
> +               for_each_set_bit(i, &channels, HX9023S_CH_NUM)
> +                       hx9023s_ch_en(data, i, test_bit(i, &data->chan_in_use));
> +               for_each_clear_bit(i, &channels, HX9023S_CH_NUM)
> +                       hx9023s_ch_en(data, i, false);
> +       }
> +
> +       data->chan_read = chan_read;
> +       data->chan_event = chan_event;
> +
> +       return 0;
> +}

...

> +static int hx9023s_get_samp_freq(struct hx9023s_data *data, int *val, int *val2)
> +{
> +       int ret;
> +       unsigned int odr;
> +       unsigned int buf;
> +
> +       ret = regmap_read(data->regmap, HX9023S_PRF_CFG, &buf);
> +       if (ret)
> +               return ret;
> +
> +       odr = hx9023s_samp_freq_table[buf];

> +       *val = 1000 / odr;

KILO? MILLI?

> +       *val2 = div_u64((1000 % odr) * 1000000ULL, odr);

MILLI / MICRO ?

> +       return IIO_VAL_INT_PLUS_MICRO;
> +}

...

> +static int hx9023s_set_samp_freq(struct hx9023s_data *data, int val, int val2)
> +{
> +       int i;

Why signed?

> +       int ret;

> +       int period_ms;

Why signed?

> +       struct device *dev = regmap_get_device(data->regmap);
> +
> +       period_ms = div_u64(1000000000ULL, (val * 1000000ULL + val2));

Use units.h

> +       for (i = 0; i < ARRAY_SIZE(hx9023s_samp_freq_table); i++) {
> +               if (period_ms == hx9023s_samp_freq_table[i])
> +                       break;
> +       }
> +       if (i == ARRAY_SIZE(hx9023s_samp_freq_table)) {
> +               dev_err(dev, "Period:%dms NOT found!\n", period_ms);
> +               return -EINVAL;
> +       }

> +       ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &i, 1);
> +
> +       return ret;

Argh...

> +}

...

> +static void hx9023s_push_events(struct iio_dev *indio_dev)
> +{
> +       struct hx9023s_data *data = iio_priv(indio_dev);
> +       s64 timestamp = iio_get_time_ns(indio_dev);
> +       unsigned long prox_changed;
> +       unsigned int chan;
> +
> +       hx9023s_sample(data);
> +       hx9023s_get_prox_state(data);
> +
> +       prox_changed = (data->chan_prox_stat ^ data->prox_state_reg) & data->chan_event;
> +

Unneeded blank line.

> +       for_each_set_bit(chan, &prox_changed, HX9023S_CH_NUM) {
> +               int dir;

Why signed?

> +               dir = (data->prox_state_reg & BIT(chan)) ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> +
> +               iio_push_event(indio_dev,
> +                       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan, IIO_EV_TYPE_THRESH, dir),
> +                       timestamp);
> +       }
> +       data->chan_prox_stat = data->prox_state_reg;
> +}

...

> +static int hx9023s_write_event_config(struct iio_dev *indio_dev,
> +                               const struct iio_chan_spec *chan,
> +                               enum iio_event_type type,
> +                               enum iio_event_direction dir,
> +                               int state)
> +{
> +       struct hx9023s_data *data = iio_priv(indio_dev);
> +
> +       if (test_bit(chan->channel, &data->chan_in_use)) {
> +               hx9023s_ch_en(data, chan->channel, !!state);

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

Why atomic?

__assign_bit()

> +       }
> +
> +       return 0;
> +}

...

> +static irqreturn_t hx9023s_trigger_handler(int irq, void *private)
> +{
> +       struct iio_poll_func *pf = private;
> +       struct iio_dev *indio_dev = pf->indio_dev;
> +       struct hx9023s_data *data = iio_priv(indio_dev);

> +       int bit;
> +       int i;

Why are both 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++] =
> +                       cpu_to_be16(data->ch_data[indio_dev->channels[bit].channel].diff);
> +
> +       iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, pf->timestamp);
> +
> +       iio_trigger_notify_done(indio_dev->trig);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int hx9023s_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +       struct hx9023s_data *data = iio_priv(indio_dev);
> +       unsigned long channels;
> +       int bit;

Why signed?

> +       guard(mutex)(&data->mutex);
> +       for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength)
> +               __set_bit(indio_dev->channels[bit].channel, &channels);
> +
> +       hx9023s_update_chan_en(data, channels, data->chan_event);
> +
> +       return 0;
> +}

...

> +static int hx9023s_probe(struct i2c_client *client)
> +{
> +       int ret;
> +       unsigned int id;
> +       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(dev, -ENOMEM, "device alloc failed\n");

We don't issue a message for -ENOMEM.

> +       data = iio_priv(indio_dev);
> +       mutex_init(&data->mutex);
> +
> +       data->regmap = devm_regmap_init_i2c(client, &hx9023s_regmap_config);
> +       if (IS_ERR(data->regmap))
> +               return dev_err_probe(dev, PTR_ERR(data->regmap), "regmap init failed\n");
> +
> +       ret = hx9023s_property_get(data);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "dts phase failed\n");
> +
> +       ret = devm_regulator_get_enable(dev, "vdd");
> +       if (ret)
> +               return dev_err_probe(dev, ret, "regulator get failed\n");

> +       fsleep(1000);

> +       ret = regmap_read(data->regmap, HX9023S_DEVICE_ID, &id);
> +       if (ret)
> +               return dev_err_probe(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(dev, ret, "device init failed\n");
> +
> +       ret = hx9023s_ch_cfg(data);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "channel config failed\n");
> +
> +       ret = regcache_sync(data->regmap);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "regcache sync 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(dev, ret, "irq request failed\n");
> +
> +               data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> +                                               iio_device_id(indio_dev));
> +               if (!data->trig)
> +                       return dev_err_probe(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(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(dev, ret,
> +                               "iio triggered buffer setup failed\n");
> +
> +       ret = devm_iio_device_register(dev, indio_dev);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "iio device register failed\n");
> +
> +       return 0;
> +}

...

> +static const struct acpi_device_id hx9023s_acpi_match[] = {
> +       { "TYHX9023" },
> +       {}
> +};

Btw, do you have a reference to any device on the market that has this ID?

Andy Shevchenko June 7, 2024, 7:45 p.m. UTC | #2

On Fri, Jun 7, 2024 at 10:40 PM Andy Shevchenko
<[e-mail]> wrote:
> On Fri, Jun 7, 2024 at 2:42 PM Yasin Lee <[e-mail]> wrote:

...
.
> > +static const struct acpi_device_id hx9023s_acpi_match[] = {
> > +       { "TYHX9023" },
> > +       {}
> > +};
>
> Btw, do you have a reference to any device on the market that has this ID?

Aaaargh!
Jonathan, we have to have a big rule from now on on ACPI IDs, if
anybody introduces an ID in the driver, they must provide the device
model that is (are going to) use it and excerpt from the ACPI ID
registry to prove the vendor ID is real.

This is the heck fake ID!
NAK.

kernel test robot June 8, 2024, 3:19 a.m. UTC | #3

Hi Yasin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.10-rc2 next-20240607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yasin-Lee/iio-proximity-hx9023s-Add-TYHX-HX9023S-sensor-driver/20240607-194446
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/SN7PR12MB81017291E79E6B61A8DEC9A5A4FB2%40SN7PR12MB8101.namprd12.prod.outlook.com
patch subject: [PATCH v4 2/2] iio:proximity:hx9023s: Add TYHX HX9023S sensor driver
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240608/[e-mail]/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240608/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All warnings (new ones prefixed by >>):

>> drivers/iio/proximity/hx9023s.c:666:58: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
     666 |                                         "Failed to read channel-positive for channel %d\n", i);
         |                                                                                             ^
   drivers/iio/proximity/hx9023s.c:652:12: note: initialize the variable 'i' to silence this warning
     652 |         int ret, i;
         |                   ^
         |                    = 0
   drivers/iio/proximity/hx9023s.c:976:25: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
     976 |                 data->buffer.channels[i++] =
         |                                       ^
   drivers/iio/proximity/hx9023s.c:969:7: note: initialize the variable 'i' to silence this warning
     969 |         int i;
         |              ^
         |               = 0
   2 warnings generated.


vim +/i +666 drivers/iio/proximity/hx9023s.c

   649	
   650	static int hx9023s_property_get(struct hx9023s_data *data)
   651	{
   652		int ret, i;
   653		u32 temp;
   654		struct fwnode_handle *child;
   655		struct device *dev = regmap_get_device(data->regmap);
   656	
   657		ret = device_property_read_u32(dev, "channel-in-use", &temp);
   658		if (ret)
   659			return dev_err_probe(dev, ret, "Failed to read channel-in-use property\n");
   660		data->chan_in_use = temp;
   661	
   662		device_for_each_child_node(dev, child) {
   663			ret = fwnode_property_read_u32(child, "channel-positive", &temp);
   664			if (ret)
   665				return dev_err_probe(dev, ret,
 > 666						"Failed to read channel-positive for channel %d\n", i);
   667			data->ch_data[i].channel_positive = temp;
   668	
   669			ret = fwnode_property_read_u32(child, "channel-negative", &temp);
   670			if (ret)
   671				return dev_err_probe(dev, ret,
   672						"Failed to read channel-negative for channel %d\n", i);
   673			data->ch_data[i].channel_negative = temp;
   674	
   675			i++;
   676		}
   677	
   678		return 0;
   679	}
   680

Jonathan Cameron June 8, 2024, 4:49 p.m. UTC | #4

On Fri, 7 Jun 2024 22:45:49 +0300
Andy Shevchenko <[e-mail]> wrote:

> On Fri, Jun 7, 2024 at 10:40 PM Andy Shevchenko
> <[e-mail]> wrote:
> > On Fri, Jun 7, 2024 at 2:42 PM Yasin Lee <[e-mail]> wrote:  
> 
> ...
> .
> > > +static const struct acpi_device_id hx9023s_acpi_match[] = {
> > > +       { "TYHX9023" },
> > > +       {}
> > > +};  
> >
> > Btw, do you have a reference to any device on the market that has this ID?  
> 
> Aaaargh!
> Jonathan, we have to have a big rule from now on on ACPI IDs, if
> anybody introduces an ID in the driver, they must provide the device
> model that is (are going to) use it and excerpt from the ACPI ID
> registry to prove the vendor ID is real.
> 
> This is the heck fake ID!
> NAK.

Agreed.  Though we should also put together some boilerplate text /
Documentation on how to get a real ID if it makes sense and what
information we need to justify carrying a bad one (which usually
has to include that you've made the supplier aware that the Linux
maintainers are going to be grumpy and our ire wasn't enough to persuade
them to promise to mend their ways - note it has worked a few times!)

For this case, key is:  There are two types of valid ID the one here
is of the ACPI ID form. For that...

ACPI IDs have to be granted by a manufacturer who has registered
with UEFI forum and been granted the use of the four letter sequence
for their products.  For example HiSilicon (my employer) has HISI.
Note that the list on the website is sometimes a bit out of date, so
if you know it has been granted recently just say that in your
patch header.  Note, I can check an would guess Andy can as well :)

That company is then responsible for handling their ID space. In my
case I know who has control of the big spread sheet, so when I want
a valid ID I go ask him and make a case for why.  Those ID spreadsheets
aren't public though in most cases, so we only know it's gone wrong
when we get a clash or a suspicious value (DEAD or BEEF usually ;)

If this process has not been gone through but some device manufacturer
has shipped a firmware with a made up ID, then we are effectively
carrying a workaround for their errata.  We will do that, but we need
much more information and a comment next to the id table entry to provide
at least one example of the shipping product suffering from this bug.

Jonathan

p.s Occasionally these sneak past me (less so with Andy's eagle eyes
on the job) and in the past I was young and didn't know better.
We will rip new ones out if we detect them reasonably quickly and
we reserve the right to rip out old ones to see who screams...
>

Jonathan Cameron June 8, 2024, 5:13 p.m. UTC | #5

On Fri,  7 Jun 2024 19:41:38 +0800
Yasin Lee <[e-mail]> wrote:

> From: Yasin Lee <[e-mail]>
> 
> 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/
> 
> Signed-off-by: Yasin Lee <[e-mail]>
As Andy already did a detailed review, I only took a fairly quick look.
A few comments inline

Jonathan

> diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
> new file mode 100644
> index 000000000000..b4a583105e03
> --- /dev/null
> +++ b/drivers/iio/proximity/hx9023s.c
> @@ -0,0 +1,1162 @@

> +
> +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);
> +	else
> +		ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1, GENMASK(4, 3), 0);
Odd to write one bit and clear 2.  If really the case, add some documentation
comments.

> +
> +	return ret;
> +}
> +
> +static int hx9023s_ch_cfg(struct hx9023s_data *data)
> +{
> +	int ret;
> +	int i;
> +	u16 reg;
> +	u8 reg_list[HX9023S_CH_NUM * 2];
> +	u8 ch_pos[HX9023S_CH_NUM];
> +	u8 ch_neg[HX9023S_CH_NUM];
> +
> +	data->ch_data[0].cs_position = 0;
> +	data->ch_data[1].cs_position = 2;
> +	data->ch_data[2].cs_position = 4;
> +	data->ch_data[3].cs_position = 6;
> +	data->ch_data[4].cs_position = 8;
> +
> +	for (i = 0; i < HX9023S_CH_NUM; i++) {
> +		if (data->ch_data[i].channel_positive == 255)
> +			ch_pos[i] = 16;
> +		else
> +			ch_pos[i] = data->ch_data[data->ch_data[i].channel_positive].cs_position;
> +		if (data->ch_data[i].channel_negative == 255)
> +			ch_neg[i] = 16;
> +		else
> +			ch_neg[i] = data->ch_data[data->ch_data[i].channel_negative].cs_position;
> +	}
> +
> +	for (i = 0; i < HX9023S_CH_NUM; i++) {
> +		reg = (u16)((0x03 << ch_pos[i]) | (0x02 << ch_neg[i]));
> +		reg_list[i * 2] = (u8)(reg);
> +		reg_list[i * 2 + 1] = (u8)(reg >> 8);

Looks like an odd form of endian manipulation. Try to set reg_list directly or use
an appropriate put_unaligned_*


> +	}
> +
> +	ret = regmap_bulk_write(data->regmap, HX9023S_CH0_CFG_7_0, reg_list, HX9023S_CH_NUM * 2);
> +
> +	return ret;
> +}
>
> +
> +static int hx9023s_ch_en(struct hx9023s_data *data, u8 ch_id, bool en)
> +{
> +	int ret;
> +	unsigned int buf;
> +
> +	ret = regmap_read(data->regmap, HX9023S_CH_NUM_CFG, &buf);
> +	if (ret)
> +		return ret;
> +
> +	data->ch_en_stat = buf;
> +
> +	if (en) {
> +		if (data->ch_en_stat == 0)
> +			data->prox_state_reg = 0;
> +		set_bit(ch_id, &data->ch_en_stat);
> +		ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
> +		if (ret)
> +			return ret;
> +		data->ch_data[ch_id].enable = true;
> +	} else {
> +		clear_bit(ch_id, &data->ch_en_stat);
> +		ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);

regmap_write() it's length 1 so not bulk!
Make sure to fix all other cases of this.

>
...


> +
> +static int hx9023s_get_proximity(struct hx9023s_data *data,
> +				const struct iio_chan_spec *chan,
> +				int *val)
> +{
> +	hx9023s_sample(data);
handle errors and return them to userspace.

> +	hx9023s_get_prox_state(data);
handle errors etc.

> +	*val = data->ch_data[chan->channel].diff;
> +	return IIO_VAL_INT;
> +}
> +
>
> +
> +static int hx9023s_set_samp_freq(struct hx9023s_data *data, int val, int val2)
> +{
> +	int i;
> +	int ret;
> +	int period_ms;
> +	struct device *dev = regmap_get_device(data->regmap);
> +
> +	period_ms = div_u64(1000000000ULL, (val * 1000000ULL + val2));
> +
> +	for (i = 0; i < ARRAY_SIZE(hx9023s_samp_freq_table); i++) {
> +		if (period_ms == hx9023s_samp_freq_table[i])
> +			break;
> +	}
> +	if (i == ARRAY_SIZE(hx9023s_samp_freq_table)) {
> +		dev_err(dev, "Period:%dms NOT found!\n", period_ms);
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &i, 1);
> +
> +	return ret;
return regmap_bulk_write()

> +}


> +static int hx9023s_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct hx9023s_data *data = iio_priv(indio_dev);
> +
> +	guard(mutex)(&data->mutex);
> +	if (state)
> +		hx9023s_interrupt_enable(data);
> +	else if (!data->chan_read)
> +		hx9023s_interrupt_disable(data);
> +	data->trigger_enabled = state;

Ah. So you store this but you also need to use it in resume
along with checking if events are enabled or not.

> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops hx9023s_trigger_ops = {
> +	.set_trigger_state = hx9023s_set_trigger_state,
> +};
> +
> +static irqreturn_t hx9023s_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct hx9023s_data *data = iio_priv(indio_dev);
> +	int bit;
> +	int i;
> +
> +	guard(mutex)(&data->mutex);
> +	hx9023s_sample(data);
> +	hx9023s_get_prox_state(data);
No error handling?  If these failed we don't want to provide bad data to
userspace.  Normally we just skip on to iio_trigger_notify_done()
with a warning print to indicate something went wrong.

> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength)
> +		data->buffer.channels[i++] =
> +			cpu_to_be16(data->ch_data[indio_dev->channels[bit].channel].diff);

In IIO, for the buffered interface, we general prefer to leave data in the endian
ordering we get from the bus and describe that to userspace.  The basic
philosophy is that userspace has better knowledge on what it is doing with the data
so we provide it the information to process it rather than doing the work in kernel.

> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, pf->timestamp);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> 
> +
> +static const struct iio_buffer_setup_ops hx9023s_buffer_setup_ops = {
> +	.preenable = hx9023s_buffer_preenable,
> +	.postdisable = hx9023s_buffer_postdisable,
> +};
> +
> +static int hx9023s_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	unsigned int id;
> +	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(dev, -ENOMEM, "device alloc failed\n");
> +
> +	data = iio_priv(indio_dev);
> +	mutex_init(&data->mutex);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &hx9023s_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap), "regmap init failed\n");
> +
> +	ret = hx9023s_property_get(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "dts phase failed\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "regulator get failed\n");
> +
> +	fsleep(1000);

If possible, add a specification reference for why that time.
If not add a comment saying that it has been found to work by experimentation.
That can be useful information if it turns out to be a bit short for someone else.

> +
> +	ret = regmap_read(data->regmap, HX9023S_DEVICE_ID, &id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "id check failed\n");

Having read the ID, normally we'd compare it with expected and print a
warning if it doesn't match, then carry on anyway (to allow for fallback compatibles
being used for future devices that are backwards compatible for this one but
have a different ID).

> +
> +	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(dev, ret, "device init failed\n");
> +
> +	ret = hx9023s_ch_cfg(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "channel config failed\n");
> +
> +	ret = regcache_sync(data->regmap);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "regcache sync 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(dev, ret, "irq request failed\n");
> +
> +		data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> +						iio_device_id(indio_dev));
> +		if (!data->trig)
> +			return dev_err_probe(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(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(dev, ret,
> +				"iio triggered buffer setup failed\n");
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "iio device register failed\n");
> +
> +	return 0;
> +}
> +
> +static int hx9023s_suspend(struct device *dev)
> +{
> +	struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));
> +
> +	hx9023s_interrupt_disable(data);

You call these even if the trigger isn't enabled.  The disable is fine, but
you then enable the interrupt on resume even if it wasn't previously enabled.
This needs some state tracking so you restore to previous state.

> +
> +	return 0;
> +}
> +
> +static int hx9023s_resume(struct device *dev)
> +{
> +	struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));
> +
> +	hx9023s_interrupt_enable(data);
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(hx9023s_pm_ops, hx9023s_suspend, hx9023s_resume);
> +
> +static const struct acpi_device_id hx9023s_acpi_match[] = {
> +	{ "TYHX9023" },
> +	{}

As Andy mentioned, drop this or add a comment on what device uses it.

> +};
> +MODULE_DEVICE_TABLE(acpi, hx9023s_acpi_match);
> +
> +static const struct of_device_id hx9023s_of_match[] = {
> +	{ .compatible = "tyhx,hx9023s" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, hx9023s_of_match);
> +
> +static const struct i2c_device_id hx9023s_id[] = {
> +	{ "hx9023s" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, hx9023s_id);
> +
> +static struct i2c_driver hx9023s_driver = {
> +	.driver = {
> +		.name = "hx9023s",
> +		.acpi_match_table = hx9023s_acpi_match,
> +		.of_match_table = hx9023s_of_match,
> +		.pm = &hx9023s_pm_ops,
> +
> +		/*
> +		 * The I2C operations in hx9023s_reg_init() and hx9023s_ch_cfg()
> +		 * are time-consuming. prefer async so we don't delay boot
Prefer (capital P as new sentence)

> +		 * if we're builtin to the kernel.
> +		 */
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +	},
> +	.probe = hx9023s_probe,
> +	.id_table = hx9023s_id,
> +};
> +module_i2c_driver(hx9023s_driver);
> +
> +MODULE_AUTHOR("Yasin Lee <[e-mail]>");
> +MODULE_DESCRIPTION("Driver for TYHX HX9023S SAR sensor");
> +MODULE_LICENSE("GPL");

Dan Carpenter June 14, 2024, 1:19 p.m. UTC | #6

Hi Yasin,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yasin-Lee/iio-proximity-hx9023s-Add-TYHX-HX9023S-sensor-driver/20240607-194446
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/SN7PR12MB81017291E79E6B61A8DEC9A5A4FB2%40SN7PR12MB8101.namprd12.prod.outlook.com
patch subject: [PATCH v4 2/2] iio:proximity:hx9023s: Add TYHX HX9023S sensor driver
config: um-randconfig-r071-20240614 (https://download.01.org/0day-ci/archive/20240614/[e-mail]/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Reported-by: Dan Carpenter <[e-mail]>
| Closes: https://lore.kernel.org/r/[e-mail]/

New smatch warnings:
drivers/iio/proximity/hx9023s.c:666 hx9023s_property_get() error: uninitialized symbol 'i'.
drivers/iio/proximity/hx9023s.c:976 hx9023s_trigger_handler() error: uninitialized symbol 'i'.
drivers/iio/proximity/hx9023s.c:996 hx9023s_buffer_preenable() error: uninitialized symbol 'channels'.

Old smatch warnings:
drivers/iio/proximity/hx9023s.c:667 hx9023s_property_get() error: uninitialized symbol 'i'.

vim +/i +666 drivers/iio/proximity/hx9023s.c

6133aa711518da Yasin Lee 2024-06-07  650  static int hx9023s_property_get(struct hx9023s_data *data)
6133aa711518da Yasin Lee 2024-06-07  651  {
6133aa711518da Yasin Lee 2024-06-07  652  	int ret, i;

Needs to be initialized.  int i = 0;

6133aa711518da Yasin Lee 2024-06-07  653  	u32 temp;
6133aa711518da Yasin Lee 2024-06-07  654  	struct fwnode_handle *child;
6133aa711518da Yasin Lee 2024-06-07  655  	struct device *dev = regmap_get_device(data->regmap);
6133aa711518da Yasin Lee 2024-06-07  656  
6133aa711518da Yasin Lee 2024-06-07  657  	ret = device_property_read_u32(dev, "channel-in-use", &temp);
6133aa711518da Yasin Lee 2024-06-07  658  	if (ret)
6133aa711518da Yasin Lee 2024-06-07  659  		return dev_err_probe(dev, ret, "Failed to read channel-in-use property\n");
6133aa711518da Yasin Lee 2024-06-07  660  	data->chan_in_use = temp;
6133aa711518da Yasin Lee 2024-06-07  661  
6133aa711518da Yasin Lee 2024-06-07  662  	device_for_each_child_node(dev, child) {
6133aa711518da Yasin Lee 2024-06-07  663  		ret = fwnode_property_read_u32(child, "channel-positive", &temp);
6133aa711518da Yasin Lee 2024-06-07  664  		if (ret)
6133aa711518da Yasin Lee 2024-06-07  665  			return dev_err_probe(dev, ret,
6133aa711518da Yasin Lee 2024-06-07 @666  					"Failed to read channel-positive for channel %d\n", i);
6133aa711518da Yasin Lee 2024-06-07  667  		data->ch_data[i].channel_positive = temp;
6133aa711518da Yasin Lee 2024-06-07  668  
6133aa711518da Yasin Lee 2024-06-07  669  		ret = fwnode_property_read_u32(child, "channel-negative", &temp);
6133aa711518da Yasin Lee 2024-06-07  670  		if (ret)
6133aa711518da Yasin Lee 2024-06-07  671  			return dev_err_probe(dev, ret,
6133aa711518da Yasin Lee 2024-06-07  672  					"Failed to read channel-negative for channel %d\n", i);
6133aa711518da Yasin Lee 2024-06-07  673  		data->ch_data[i].channel_negative = temp;
6133aa711518da Yasin Lee 2024-06-07  674  
6133aa711518da Yasin Lee 2024-06-07  675  		i++;
6133aa711518da Yasin Lee 2024-06-07  676  	}
6133aa711518da Yasin Lee 2024-06-07  677  
6133aa711518da Yasin Lee 2024-06-07  678  	return 0;
6133aa711518da Yasin Lee 2024-06-07  679  }

Yasin Lee June 18, 2024, 5:12 p.m. UTC | #7

Hi Andy,

Thanks for your guidance. I have already incorporated the modifications 
you mentioned and have replied with the details inline below.

Best regards,

Yasin Lee

在 2024/6/8 03:40, Andy Shevchenko 写道:
> On Fri, Jun 7, 2024 at 2:42 PM Yasin Lee <[e-mail]> wrote:
>> From: Yasin Lee <[e-mail]>
>>
>> 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/
> ...
>
>> +#include <linux/acpi.h>
> Unused.
>
>> +#include <linux/array_size.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/byteorder/generic.h>
> It should be asm/byteorder.h after linux/* (you have already
> unaligned.h there, move this one there)
>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/errno.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irqreturn.h>
>> +#include <linux/kernel.h>
> Why do you need this?
>
>> +#include <linux/math.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pm.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/types.h>
>> +#include <linux/units.h>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/events.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/types.h>
> Do you really use all of these iio/*?
>
> ...

The modified header file is as follows, and I have checked line by line 
to confirm that all the listed IIO-related header files are necessary:

#include <linux/array_size.h>
#include <linux/bitfield.h>
#include <linux/bitops.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/errno.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/irqreturn.h>
#include <linux/math.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/pm.h>
#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/types.h>
#include <linux/units.h>

#include <asm/byteorder.h>
#include <asm/unaligned.h>

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

>> +struct hx9023s_ch_data {
>> +       int raw;
>> +       int lp; /* low pass */
>> +       int bl; /* base line */
>> +       int diff; /* lp-bl */
> Decrypt the comment.
     int raw; /* Raw Data*/
     int lp; /* Low Pass Filter Data*/
     int bl; /* Base Line Data */
     int diff; /* difference of Low Pass Data and Base Line Data */
>> +       struct {
>> +               int near;
>> +               int far;
>> +       } thres;
> Do all of the above have to be signed? Why?
I'll get rid of signed integers and use unsigned integers instead.
>> +       u16 dac;
>> +       u8 cs_position;
>> +       u8 channel_positive;
>> +       u8 channel_negative;
>> +       bool sel_bl;
>> +       bool sel_raw;
>> +       bool sel_diff;
>> +       bool sel_lp;
>> +       bool enable;
>> +};
>> +
>> +struct hx9023s_data {
>> +       struct iio_trigger *trig;
>> +       struct regmap *regmap;
>> +       unsigned long chan_prox_stat;
>> +       unsigned long chan_read;
>> +       unsigned long chan_event;
>> +       unsigned long ch_en_stat;
>> +       unsigned long chan_in_use;
>> +       unsigned int prox_state_reg;
>> +       bool trigger_enabled;
>> +
>> +       struct {
>> +               __be16 channels[HX9023S_CH_NUM];
>> +               s64 ts __aligned(8);
>> +       } buffer;
>> +       struct hx9023s_ch_data ch_data[HX9023S_CH_NUM];
> I would suggest moving this to be the last field. It might help in the
> future if we ever need to convert this to VLA.
Okay, I'll revise it based on your suggestions.
>> ...
>> +
>> +       /* adc avg number and osr number of each channel */
> ADC
> average
> OSR
Fixed
>> ...
>> +
>> +       /* sample & integration frequency of the adc */
> ADC
>
Fixed
>> +       { HX9023S_SAMPLE_NUM_7_0,             0x65 },
>> +       { HX9023S_INTEGRATION_NUM_7_0,        0x65 },
>> +
>> +       /* coefficient of the first order low pass filter during each channel */
>> +       { HX9023S_LP_ALP_1_0_CFG,             0x22 },
>> +       { HX9023S_LP_ALP_3_2_CFG,             0x22 },
>> +       { HX9023S_LP_ALP_4_CFG,               0x02 },
>> +
>> +       /* up coefficient of the first order low pass filter during each channel */
>> +       { HX9023S_UP_ALP_1_0_CFG,             0x88 },
>> +       { HX9023S_UP_ALP_3_2_CFG,             0x88 },
>> +       { HX9023S_DN_UP_ALP_0_4_CFG,          0x18 },
>> +
>> +       /* down coefficient of the first order low pass filter during each channel */
>> +       { HX9023S_DN_ALP_2_1_CFG,             0x11 },
>> +       { HX9023S_DN_ALP_4_3_CFG,             0x11 },
>> +
>> +       /* output data */
> What is this supposed to mean?
Modify to: /* selection of data for the Data Mux Register to output data */
>> +       { HX9023S_RAW_BL_RD_CFG,              0xF0 },
>> +
>> +       /* enable the interrupt function */
>> +       { HX9023S_INTERRUPT_CFG,              0xFF },
>> +       { HX9023S_INTERRUPT_CFG1,             0x3B },
>> +       { HX9023S_DITHER_CFG,                 0x21 },
>> +
>> +       /* threshold of the offset compensation */
>> +       { HX9023S_CALI_DIFF_CFG,              0x07 },
>> +
>> +       /* proximity persistency number(near & far) */
>> +       { HX9023S_PROX_INT_HIGH_CFG,          0x01 },
>> +       { HX9023S_PROX_INT_LOW_CFG,           0x01 },
>> +
>> +       /* disable the data lock */
>> +       { HX9023S_DSP_CONFIG_CTRL1,           0x00 },
>> +};
> ...
>
>> +static const struct regmap_config hx9023s_regmap_config = {
>> +       .reg_bits = 8,
>> +       .val_bits = 8,
>> +       .cache_type = REGCACHE_RBTREE,
> Why not MAPLE?
In my test version of the kernel, MAPLE is not supported. Now, I am 
modifying and submitting it to support MAPLE.
>> +       .wr_table = &hx9023s_wr_regs,
>> +       .volatile_table = &hx9023s_volatile_regs,
>> +};
> ...
>
>> +static int hx9023s_data_lock(struct hx9023s_data *data, bool locked)
>> +{
>> +       int ret;
> Unneeded, you may return directly.
Fixed
>
>> +       if (locked)
>> +               ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1,
>> +                                       HX9023S_DATA_LOCK_MASK, HX9023S_DATA_LOCK_MASK);
>    return regmap_update_bits(...);
>
Fixed
>> +       else
>> +               ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1, GENMASK(4, 3), 0);
>> +
>> +       return ret;
>> +}
> ...
>
>> +static int hx9023s_ch_cfg(struct hx9023s_data *data)
>> +{
>> +       int ret;
>> +       int i;
> Why signed?
>
Modify to: unsigned int , same below.
>> +       u16 reg;
>> +       u8 reg_list[HX9023S_CH_NUM * 2];
>> +       u8 ch_pos[HX9023S_CH_NUM];
>> +       u8 ch_neg[HX9023S_CH_NUM];
>> +
>> +       data->ch_data[0].cs_position = 0;
>> +       data->ch_data[1].cs_position = 2;
>> +       data->ch_data[2].cs_position = 4;
>> +       data->ch_data[3].cs_position = 6;
>> +       data->ch_data[4].cs_position = 8;
>> +
>> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
>> +               if (data->ch_data[i].channel_positive == 255)
> Magic number! Should it be GENMASK()? U8_MAX? Something else semantically?
Replace with  #define HX9023S_NOT_CONNECTED  to indicate an unconnected 
channel.
>> +                       ch_pos[i] = 16;
>> +               else
>> +                       ch_pos[i] = data->ch_data[data->ch_data[i].channel_positive].cs_position;
>> +               if (data->ch_data[i].channel_negative == 255)
> Ditto!
>
>> +                       ch_neg[i] = 16;
>> +               else
>> +                       ch_neg[i] = data->ch_data[data->ch_data[i].channel_negative].cs_position;
>> +       }
>> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
>> +               reg = (u16)((0x03 << ch_pos[i]) | (0x02 << ch_neg[i]));
> Why casting? What are those magic numbers?
>
Delete the casting & Replace the magic number with:

#define HX9023S_POS 0x03
#define HX9023S_NEG 0x02

>> +               reg_list[i * 2] = (u8)(reg);
>> +               reg_list[i * 2 + 1] = (u8)(reg >> 8);
> put_unaligned_le16()
>
Fixed.
>> +       }
>> +       ret = regmap_bulk_write(data->regmap, HX9023S_CH0_CFG_7_0, reg_list, HX9023S_CH_NUM * 2);
>> +
>> +       return ret;
> Return directly.
Fixed.
>> +}
>> +
>> +static int hx9023s_reg_init(struct hx9023s_data *data)
>> +{
>> +       int i = 0;
> Why signed? What is that assignment for?
>
>> +       int ret;
>> +
>> +       for (i = 0; i < (int)ARRAY_SIZE(hx9023s_reg_init_list); i++) {
>> +               ret = regmap_bulk_write(data->regmap, hx9023s_reg_init_list[i].addr,
>> +                                       &hx9023s_reg_init_list[i].val, 1);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       return 0;
>> +}
> ...
>
>> +static int hx9023s_write_far_debounce(struct hx9023s_data *data, int val)
>> +{
>> +       int ret;
>> +
>> +       if (val > 0xF)
>> +               val = 0xF;
> What is this magic?
> Shouldn't you use clamp()?
>
...
>> +       guard(mutex)(&data->mutex);
>> +       ret = regmap_update_bits(data->regmap, HX9023S_PROX_INT_LOW_CFG,
>> +                               HX9023S_PROX_DEBOUNCE_MASK, val);
>> +
>> +       return ret;
> Return directly. Really you may reduce your code base by ~50 LoCs just
>   by dropping unneeded ret and this kind of code.
>
>> +}
Fixed:

static int hx9023s_write_far_debounce(struct hx9023s_data *data, int val)

{
     guard(mutex)(&data->mutex);
     return regmap_update_bits(data->regmap, HX9023S_PROX_INT_LOW_CFG,
                 HX9023S_PROX_DEBOUNCE_MASK,
                 FIELD_GET(HX9023S_PROX_DEBOUNCE_MASK, val));
}
> ...
>
>> +static int hx9023s_write_near_debounce(struct hx9023s_data *data, int val)
>> +{
> As per above function.
>
>> +}
> ...
>
>> +static int hx9023s_get_thres_near(struct hx9023s_data *data, u8 ch, int *val)
>> +{
>> +       int ret;
>> +       u8 buf[2];
>> +
>> +       if (ch == 4)
> Instead, make a temporary variable for the reg and use only a single
> call to regmap_bulk_read().
>
Fixed
>> +               ret = regmap_bulk_read(data->regmap, HX9023S_PROX_HIGH_DIFF_CFG_CH4_0, buf, 2);
> sizeof(buf)
>
>> +       else
>> +               ret = regmap_bulk_read(data->regmap,
>> +                               HX9023S_PROX_HIGH_DIFF_CFG_CH0_0 + (ch * HX9023S_2BYTES), buf, 2);
> Ditto.
>
>> +
> Redundant blank line.
>
>> +       if (ret)
>> +               return ret;
>> +       *val = get_unaligned_le16(buf);
>> +       *val = (*val & GENMASK(9, 0)) * 32;
>> +       data->ch_data[ch].thres.near = *val;
> Better to have a temporary variable and do something like
>
>    unsigned int tmp;
>
>    tmp = (get_unaligned_le16(buf) & GENMASK(9, 0)) * 32;
>    near = tmp;
>    *val = tmp;
>
> See the difference?
>
>> +       return IIO_VAL_INT;
>> +}
> ...
>
>> +static int hx9023s_get_thres_far(struct hx9023s_data *data, u8 ch, int *val)
>> +{
> As per above.
>
>> +}
> ...
>
>> +static int hx9023s_set_thres_near(struct hx9023s_data *data, u8 ch, int val)
>> +{
> As per above.
>
>> +}
>> +
>> +static int hx9023s_set_thres_far(struct hx9023s_data *data, u8 ch, int val)
>> +{
> As per above.
>
Fixed all of them.
>> +}
> ...
>
>> +static void hx9023s_get_prox_state(struct hx9023s_data *data)
>> +{
>> +       int ret;
> Are the 4 LoCs just for fun? Or why does the function return void?
>
>> +       ret = regmap_read(data->regmap, HX9023S_PROX_STATUS, &data->prox_state_reg);
>> +       if (ret)
>> +               return;
>> +}
static int hx9023s_get_prox_state(struct hx9023s_data *data)
{
     return regmap_read(data->regmap, HX9023S_PROX_STATUS, 
&data->prox_state_reg);
}

> ...
>
>> +static void hx9023s_data_select(struct hx9023s_data *data)
> Why void?
>
>> +{
>> +       int ret;
>> +       int i;
> Why signed?
>
>> +       unsigned long buf[1];
> Why an array?
>
>> +       ret = regmap_read(data->regmap, HX9023S_RAW_BL_RD_CFG, (unsigned int *)buf);
> No. Why do you need this ugly casting?
>
>> +       if (ret)
>> +               return;
>> +
>> +       for (i = 0; i < 4; i++) {
>> +               data->ch_data[i].sel_diff = test_bit(i, buf);
>> +               data->ch_data[i].sel_lp = !data->ch_data[i].sel_diff;
>> +               data->ch_data[i].sel_bl = test_bit(i + 4, buf);
>> +               data->ch_data[i].sel_raw = !data->ch_data[i].sel_bl;
>> +       }
>> +
>> +       ret = regmap_read(data->regmap, HX9023S_INTERRUPT_CFG1, (unsigned int *)buf);
>> +       if (ret)
>> +               return;
>> +
>> +       data->ch_data[4].sel_diff = test_bit(2, buf);
>> +       data->ch_data[4].sel_lp = !data->ch_data[4].sel_diff;
>> +       data->ch_data[4].sel_bl = test_bit(3, buf);
>> +       data->ch_data[4].sel_raw = !data->ch_data[4].sel_bl;
>> +}

Modified as below:

static int hx9023s_data_select(struct hx9023s_data *data)

{
     int ret;
     unsigned int i, buf;
     unsigned long tmp;

     ret = regmap_read(data->regmap, HX9023S_RAW_BL_RD_CFG, &buf);
     if (ret)
         return ret;

     tmp = buf;
     for (i = 0; i < 4; i++) {
         data->ch_data[i].sel_diff = test_bit(i, &tmp);
         data->ch_data[i].sel_lp = !data->ch_data[i].sel_diff;
         data->ch_data[i].sel_bl = test_bit(i + 4, &tmp);
         data->ch_data[i].sel_raw = !data->ch_data[i].sel_bl;
     }

     ret = regmap_read(data->regmap, HX9023S_INTERRUPT_CFG1, &buf);
     if (ret)
         return ret;

     tmp = buf;
     data->ch_data[4].sel_diff = test_bit(2, &tmp);
     data->ch_data[4].sel_lp = !data->ch_data[4].sel_diff;
     data->ch_data[4].sel_bl = test_bit(3, &tmp);
     data->ch_data[4].sel_raw = !data->ch_data[4].sel_bl;

     return 0;
}
> ...
>
>> +static int hx9023s_sample(struct hx9023s_data *data)
>> +{
>> +       int ret;
>> +       int i;
> Why signed?
>
>> +       u8 data_size;
>> +       u8 offset_data_size;
>> +       int value;
>> +       u8 rx_buf[HX9023S_CH_NUM * HX9023S_BYTES_MAX];
>> +
>> +       hx9023s_data_lock(data, true);
>> +       hx9023s_data_select(data);
>> +
>> +       data_size = HX9023S_3BYTES;
>> +
>> +       /* ch0~ch3 */
>> +       ret = regmap_bulk_read(data->regmap, HX9023S_RAW_BL_CH0_0, rx_buf,
>> +                               (HX9023S_CH_NUM * data_size) - data_size);
> Make a temporary variable and use  (CH_NUM - 1) * data_size which is
> shorter and clearer.
>
Fixed
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* ch4 */
>> +       ret = regmap_bulk_read(data->regmap, HX9023S_RAW_BL_CH4_0,
>> +                               rx_buf + ((HX9023S_CH_NUM * data_size) - data_size), data_size);
>> +       if (ret)
>> +               return ret;
>> +
>> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
>> +               value = get_unaligned_le16(&rx_buf[i * data_size + 1]);
>> +               value = sign_extend32(value, 15);
>> +               data->ch_data[i].raw = 0;
>> +               data->ch_data[i].bl = 0;
>> +               if (data->ch_data[i].sel_raw == true)
>> +                       data->ch_data[i].raw = value;
>> +               if (data->ch_data[i].sel_bl == true)
>> +                       data->ch_data[i].bl = value;
>> +       }
>> +
>> +       /* ch0~ch3 */
>> +       ret = regmap_bulk_read(data->regmap, HX9023S_LP_DIFF_CH0_0, rx_buf,
>> +                               (HX9023S_CH_NUM * data_size) - data_size);
> Use a temporary constant.
Fixed, same below
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* ch4 */
>> +       ret = regmap_bulk_read(data->regmap, HX9023S_LP_DIFF_CH4_0,
>> +                               rx_buf + ((HX9023S_CH_NUM * data_size) - data_size), data_size);
> Ditto.
>
>> +       if (ret)
>> +               return ret;
>> +
>> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
>> +               value = get_unaligned_le16(&rx_buf[i * data_size + 1]);
>> +               value = sign_extend32(value, 15);
>> +               data->ch_data[i].lp = 0;
>> +               data->ch_data[i].diff = 0;
>> +               if (data->ch_data[i].sel_lp == true)
>> +                       data->ch_data[i].lp = value;
>> +               if (data->ch_data[i].sel_diff == true)
>> +                       data->ch_data[i].diff = value;
>> +       }
>> +
>> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
>> +               if (data->ch_data[i].sel_lp == true && data->ch_data[i].sel_bl == true)
>> +                       data->ch_data[i].diff = data->ch_data[i].lp - data->ch_data[i].bl;
>> +       }
>> +
>> +       /* offset dac */
>> +       offset_data_size = HX9023S_2BYTES;
>> +       ret = regmap_bulk_read(data->regmap, HX9023S_OFFSET_DAC0_7_0, rx_buf,
>> +                               (HX9023S_CH_NUM * offset_data_size));
>> +       if (ret)
>> +               return ret;
>> +
>> +       for (i = 0; i < HX9023S_CH_NUM; i++) {
>> +               value = get_unaligned_le16(&rx_buf[i * offset_data_size]);
>> +               value = FIELD_GET(GENMASK(11, 0), value);
>> +               data->ch_data[i].dac = value;
>> +       }
>> +
>> +       hx9023s_data_lock(data, false);
>> +
>> +       return 0;
>> +}
>> +static int hx9023s_ch_en(struct hx9023s_data *data, u8 ch_id, bool en)
>> +{
>> +       int ret;
>> +       unsigned int buf;
>> +
>> +       ret = regmap_read(data->regmap, HX9023S_CH_NUM_CFG, &buf);
>> +       if (ret)
>> +               return ret;
>> +
>> +       data->ch_en_stat = buf;
>> +
>> +       if (en) {
>> +               if (data->ch_en_stat == 0)
>> +                       data->prox_state_reg = 0;
>> +               set_bit(ch_id, &data->ch_en_stat);
> Why atomit?
>
>> +               ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
>> +               if (ret)
>> +                       return ret;
>> +               data->ch_data[ch_id].enable = true;
>> +       } else {
>> +               clear_bit(ch_id, &data->ch_en_stat);
>> +               ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
>> +               if (ret)
>> +                       return ret;
>> +               data->ch_data[ch_id].enable = false;
>> +       }
> This can be compressed to
>
>    if (en && ch_en_stat == 0)
>      prox_state_reg = 0;
>    __assign_bit(en); // or atomic, but the latter has to be justified
>    enable = !!en;
>    ret = regmap_bulk_write();
>    if (ret)
>      return ret;
>
>> +       return 0;
>> +}

Modified as below:

static int hx9023s_ch_en(struct hx9023s_data *data, u8 ch_id, bool en)
{
     int ret;
     unsigned int buf;

     ret = regmap_read(data->regmap, HX9023S_CH_NUM_CFG, &buf);
     if (ret)
         return ret;

     data->ch_en_stat = buf;
     if (en && data->ch_en_stat == 0)
         data->prox_state_reg = 0;

     data->ch_data[ch_id].enable = en;
     __assign_bit(ch_id, &data->ch_en_stat, en);

     return regmap_write(data->regmap, HX9023S_CH_NUM_CFG, 
data->ch_en_stat);
}


>> +
>> +static int hx9023s_property_get(struct hx9023s_data *data)
>> +{
>> +       int ret, i;
> Why is the 'i' signed?
>
>> +       u32 temp;
>> +       struct fwnode_handle *child;
>> +       struct device *dev = regmap_get_device(data->regmap);
>> +
>> +       ret = device_property_read_u32(dev, "channel-in-use", &temp);
>> +       if (ret)
>> +               return dev_err_probe(dev, ret, "Failed to read channel-in-use property\n");
>> +       data->chan_in_use = temp;
> Did you even compile your code? The below uses uninitialised value.

Fixed.

It might be an issue with my local environment; the uninitialized 
problem was not detected during compilation.

>> +       device_for_each_child_node(dev, child) {
> You have massive leaks in this loop, you need to use _scoped() variant.
Add fwnode_handle_put(child); before return err;
>> +               ret = fwnode_property_read_u32(child, "channel-positive", &temp);
>> +               if (ret)
>> +                       return dev_err_probe(dev, ret,
>> +                                       "Failed to read channel-positive for channel %d\n", i);
>> +               data->ch_data[i].channel_positive = temp;
>> +
>> +               ret = fwnode_property_read_u32(child, "channel-negative", &temp);
>> +               if (ret)
>> +                       return dev_err_probe(dev, ret,
>> +                                       "Failed to read channel-negative for channel %d\n", i);
>> +               data->ch_data[i].channel_negative = temp;
>> +
>> +               i++;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int hx9023s_update_chan_en(struct hx9023s_data *data,
>> +                               unsigned long chan_read,
>> +                               unsigned long chan_event)
>> +{
>> +       int i;
> Why signed?
>
>> +       unsigned long channels = chan_read | chan_event;
>> +
>> +       if ((data->chan_read | data->chan_event) != channels) {
>> +               for_each_set_bit(i, &channels, HX9023S_CH_NUM)
>> +                       hx9023s_ch_en(data, i, test_bit(i, &data->chan_in_use));
>> +               for_each_clear_bit(i, &channels, HX9023S_CH_NUM)
>> +                       hx9023s_ch_en(data, i, false);
>> +       }
>> +
>> +       data->chan_read = chan_read;
>> +       data->chan_event = chan_event;
>> +
>> +       return 0;
>> +}
> ...
>
>> +static int hx9023s_get_samp_freq(struct hx9023s_data *data, int *val, int *val2)
>> +{
>> +       int ret;
>> +       unsigned int odr;
>> +       unsigned int buf;
>> +
>> +       ret = regmap_read(data->regmap, HX9023S_PRF_CFG, &buf);
>> +       if (ret)
>> +               return ret;
>> +
>> +       odr = hx9023s_samp_freq_table[buf];
>> +       *val = 1000 / odr;
> KILO? MILLI?
>
>> +       *val2 = div_u64((1000 % odr) * 1000000ULL, odr);
> MILLI / MICRO ?

add comment.

     *val = 1000 / odr; /* odr in Hz */
     *val2 = div_u64((1000 % odr) * 1000000ULL, odr); /* odr in micro Hz */

>> +       return IIO_VAL_INT_PLUS_MICRO;
>> +}
> ...
>
>> +static int hx9023s_set_samp_freq(struct hx9023s_data *data, int val, int val2)
>> +{
>> +       int i;
> Why signed?
>
>> +       int ret;
>> +       int period_ms;
> Why signed?
>
>> +       struct device *dev = regmap_get_device(data->regmap);
>> +
>> +       period_ms = div_u64(1000000000ULL, (val * 1000000ULL + val2));
> Use units.h
OK.
>> +       for (i = 0; i < ARRAY_SIZE(hx9023s_samp_freq_table); i++) {
>> +               if (period_ms == hx9023s_samp_freq_table[i])
>> +                       break;
>> +       }
>> +       if (i == ARRAY_SIZE(hx9023s_samp_freq_table)) {
>> +               dev_err(dev, "Period:%dms NOT found!\n", period_ms);
>> +               return -EINVAL;
>> +       }
>> +       ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &i, 1);
>> +
>> +       return ret;
> Argh...
>
Fixed all of these.
>> +}
> ...
>
>> +static void hx9023s_push_events(struct iio_dev *indio_dev)
>> +{
>> +       struct hx9023s_data *data = iio_priv(indio_dev);
>> +       s64 timestamp = iio_get_time_ns(indio_dev);
>> +       unsigned long prox_changed;
>> +       unsigned int chan;
>> +
>> +       hx9023s_sample(data);
>> +       hx9023s_get_prox_state(data);
>> +
>> +       prox_changed = (data->chan_prox_stat ^ data->prox_state_reg) & data->chan_event;
>> +
> Unneeded blank line.
>
>> +       for_each_set_bit(chan, &prox_changed, HX9023S_CH_NUM) {
>> +               int dir;
> Why signed?
>
>> +               dir = (data->prox_state_reg & BIT(chan)) ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
>> +
>> +               iio_push_event(indio_dev,
>> +                       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan, IIO_EV_TYPE_THRESH, dir),
>> +                       timestamp);
>> +       }
>> +       data->chan_prox_stat = data->prox_state_reg;
>> +}
> ...
>
>> +static int hx9023s_write_event_config(struct iio_dev *indio_dev,
>> +                               const struct iio_chan_spec *chan,
>> +                               enum iio_event_type type,
>> +                               enum iio_event_direction dir,
>> +                               int state)
>> +{
>> +       struct hx9023s_data *data = iio_priv(indio_dev);
>> +
>> +       if (test_bit(chan->channel, &data->chan_in_use)) {
>> +               hx9023s_ch_en(data, chan->channel, !!state);
>> +               if (data->ch_data[chan->channel].enable)
>> +                       set_bit(chan->channel, &data->chan_event);
>> +               else
>> +                       clear_bit(chan->channel, &data->chan_event);
> Why atomic?
>
> __assign_bit()
Fixed
>> +       }
>> +
>> +       return 0;
>> +}
> ...
>
>> +static irqreturn_t hx9023s_trigger_handler(int irq, void *private)
>> +{
>> +       struct iio_poll_func *pf = private;
>> +       struct iio_dev *indio_dev = pf->indio_dev;
>> +       struct hx9023s_data *data = iio_priv(indio_dev);
>> +       int bit;
>> +       int i;
> Why are both 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++] =
>> +                       cpu_to_be16(data->ch_data[indio_dev->channels[bit].channel].diff);
>> +
>> +       iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, pf->timestamp);
>> +
>> +       iio_trigger_notify_done(indio_dev->trig);
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static int hx9023s_buffer_preenable(struct iio_dev *indio_dev)
>> +{
>> +       struct hx9023s_data *data = iio_priv(indio_dev);
>> +       unsigned long channels;
>> +       int bit;
> Why signed?
>
>> +       guard(mutex)(&data->mutex);
>> +       for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength)
>> +               __set_bit(indio_dev->channels[bit].channel, &channels);
>> +
>> +       hx9023s_update_chan_en(data, channels, data->chan_event);
>> +
>> +       return 0;
>> +}
> ...
>
>> +static int hx9023s_probe(struct i2c_client *client)
>> +{
>> +       int ret;
>> +       unsigned int id;
>> +       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(dev, -ENOMEM, "device alloc failed\n");
> We don't issue a message for -ENOMEM.
Fixed
>> +       data = iio_priv(indio_dev);
>> +       mutex_init(&data->mutex);
>> +
>> +       data->regmap = devm_regmap_init_i2c(client, &hx9023s_regmap_config);
>> +       if (IS_ERR(data->regmap))
>> +               return dev_err_probe(dev, PTR_ERR(data->regmap), "regmap init failed\n");
>> +
>> +       ret = hx9023s_property_get(data);
>> +       if (ret)
>> +               return dev_err_probe(dev, ret, "dts phase failed\n");
>> +
>> +       ret = devm_regulator_get_enable(dev, "vdd");
>> +       if (ret)
>> +               return dev_err_probe(dev, ret, "regulator get failed\n");
>> +       fsleep(1000);
>> +       ret = regmap_read(data->regmap, HX9023S_DEVICE_ID, &id);
>> +       if (ret)
>> +               return dev_err_probe(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(dev, ret, "device init failed\n");
>> +
>> +       ret = hx9023s_ch_cfg(data);
>> +       if (ret)
>> +               return dev_err_probe(dev, ret, "channel config failed\n");
>> +
>> +       ret = regcache_sync(data->regmap);
>> +       if (ret)
>> +               return dev_err_probe(dev, ret, "regcache sync 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(dev, ret, "irq request failed\n");
>> +
>> +               data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
>> +                                               iio_device_id(indio_dev));
>> +               if (!data->trig)
>> +                       return dev_err_probe(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(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(dev, ret,
>> +                               "iio triggered buffer setup failed\n");
>> +
>> +       ret = devm_iio_device_register(dev, indio_dev);
>> +       if (ret)
>> +               return dev_err_probe(dev, ret, "iio device register failed\n");
>> +
>> +       return 0;
>> +}
> ...
>
>> +static const struct acpi_device_id hx9023s_acpi_match[] = {
>> +       { "TYHX9023" },
>> +       {}
>> +};
> Btw, do you have a reference to any device on the market that has this ID?
Drop the acpi match

Yasin Lee June 18, 2024, 6:02 p.m. UTC | #8

on 2024/6/9 01:13, Jonathan Cameron wrote:
> On Fri,  7 Jun 2024 19:41:38 +0800
> Yasin Lee <[e-mail]> wrote:
>
>> From: Yasin Lee <[e-mail]>
>>
>> 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/
>>
>> Signed-off-by: Yasin Lee <[e-mail]>
> As Andy already did a detailed review, I only took a fairly quick look.
> A few comments inline
>
> Jonathan
>
Hi Jonathan,

Thanks for your reply, the  details inline below.

Yasin Lee


>> diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
>> new file mode 100644
>> index 000000000000..b4a583105e03
>> --- /dev/null
>> +++ b/drivers/iio/proximity/hx9023s.c
>> @@ -0,0 +1,1162 @@
>> +
>> +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);
>> +	else
>> +		ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1, GENMASK(4, 3), 0);
> Odd to write one bit and clear 2.  If really the case, add some documentation
> comments.
>

This is indeed an error, modified to only set HX9023S_DATA_LOCK_MASK.

>> +
>> +	return ret;
>> +}
>> +
>> +static int hx9023s_ch_cfg(struct hx9023s_data *data)
>> +{
>> +	int ret;
>> +	int i;
>> +	u16 reg;
>> +	u8 reg_list[HX9023S_CH_NUM * 2];
>> +	u8 ch_pos[HX9023S_CH_NUM];
>> +	u8 ch_neg[HX9023S_CH_NUM];
>> +
>> +	data->ch_data[0].cs_position = 0;
>> +	data->ch_data[1].cs_position = 2;
>> +	data->ch_data[2].cs_position = 4;
>> +	data->ch_data[3].cs_position = 6;
>> +	data->ch_data[4].cs_position = 8;
>> +
>> +	for (i = 0; i < HX9023S_CH_NUM; i++) {
>> +		if (data->ch_data[i].channel_positive == 255)
>> +			ch_pos[i] = 16;
>> +		else
>> +			ch_pos[i] = data->ch_data[data->ch_data[i].channel_positive].cs_position;
>> +		if (data->ch_data[i].channel_negative == 255)
>> +			ch_neg[i] = 16;
>> +		else
>> +			ch_neg[i] = data->ch_data[data->ch_data[i].channel_negative].cs_position;
>> +	}
>> +
>> +	for (i = 0; i < HX9023S_CH_NUM; i++) {
>> +		reg = (u16)((0x03 << ch_pos[i]) | (0x02 << ch_neg[i]));
>> +		reg_list[i * 2] = (u8)(reg);
>> +		reg_list[i * 2 + 1] = (u8)(reg >> 8);
> Looks like an odd form of endian manipulation. Try to set reg_list directly or use
> an appropriate put_unaligned_*
>

Modified to put_unaligned_le16.


>> +	}
>> +
>> +	ret = regmap_bulk_write(data->regmap, HX9023S_CH0_CFG_7_0, reg_list, HX9023S_CH_NUM * 2);
>> +
>> +	return ret;
>> +}
>>
>> +
>> +static int hx9023s_ch_en(struct hx9023s_data *data, u8 ch_id, bool en)
>> +{
>> +	int ret;
>> +	unsigned int buf;
>> +
>> +	ret = regmap_read(data->regmap, HX9023S_CH_NUM_CFG, &buf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	data->ch_en_stat = buf;
>> +
>> +	if (en) {
>> +		if (data->ch_en_stat == 0)
>> +			data->prox_state_reg = 0;
>> +		set_bit(ch_id, &data->ch_en_stat);
>> +		ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
>> +		if (ret)
>> +			return ret;
>> +		data->ch_data[ch_id].enable = true;
>> +	} else {
>> +		clear_bit(ch_id, &data->ch_en_stat);
>> +		ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
> regmap_write() it's length 1 so not bulk!
> Make sure to fix all other cases of this.
>

OK, I will fix all such issues.


> ...
>
>
>> +
>> +static int hx9023s_get_proximity(struct hx9023s_data *data,
>> +				const struct iio_chan_spec *chan,
>> +				int *val)
>> +{
>> +	hx9023s_sample(data);
> handle errors and return them to userspace.
>

Fixed


>> +	hx9023s_get_prox_state(data);
> handle errors etc.
>

Fixed


>> +	*val = data->ch_data[chan->channel].diff;
>> +	return IIO_VAL_INT;
>> +}
>> +
>>
>> +
>> +static int hx9023s_set_samp_freq(struct hx9023s_data *data, int val, int val2)
>> +{
>> +	int i;
>> +	int ret;
>> +	int period_ms;
>> +	struct device *dev = regmap_get_device(data->regmap);
>> +
>> +	period_ms = div_u64(1000000000ULL, (val * 1000000ULL + val2));
>> +
>> +	for (i = 0; i < ARRAY_SIZE(hx9023s_samp_freq_table); i++) {
>> +		if (period_ms == hx9023s_samp_freq_table[i])
>> +			break;
>> +	}
>> +	if (i == ARRAY_SIZE(hx9023s_samp_freq_table)) {
>> +		dev_err(dev, "Period:%dms NOT found!\n", period_ms);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &i, 1);
>> +
>> +	return ret;
> return regmap_bulk_write()


Fixed


>> +}
>
>> +static int hx9023s_set_trigger_state(struct iio_trigger *trig, bool state)
>> +{
>> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
>> +	struct hx9023s_data *data = iio_priv(indio_dev);
>> +
>> +	guard(mutex)(&data->mutex);
>> +	if (state)
>> +		hx9023s_interrupt_enable(data);
>> +	else if (!data->chan_read)
>> +		hx9023s_interrupt_disable(data);
>> +	data->trigger_enabled = state;
> Ah. So you store this but you also need to use it in resume
> along with checking if events are enabled or not.
>

Add     if (data->trigger_enabled) in resume Function

>> +
>> +	return 0;
>> +}
>> +
>> +static const struct iio_trigger_ops hx9023s_trigger_ops = {
>> +	.set_trigger_state = hx9023s_set_trigger_state,
>> +};
>> +
>> +static irqreturn_t hx9023s_trigger_handler(int irq, void *private)
>> +{
>> +	struct iio_poll_func *pf = private;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct hx9023s_data *data = iio_priv(indio_dev);
>> +	int bit;
>> +	int i;
>> +
>> +	guard(mutex)(&data->mutex);
>> +	hx9023s_sample(data);
>> +	hx9023s_get_prox_state(data);
> No error handling?  If these failed we don't want to provide bad data to
> userspace.  Normally we just skip on to iio_trigger_notify_done()
> with a warning print to indicate something went wrong.


OK, I added the following error handling logic here:

     ret = hx9023s_sample(data);
     if (ret) {
         dev_warn(dev, "sampling failed\n");
         goto out;
     }

     ret = hx9023s_get_prox_state(data);
     if (ret) {
         dev_warn(dev, "get prox failed\n");
         goto out;
     }

>> +
>> +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength)
>> +		data->buffer.channels[i++] =
>> +			cpu_to_be16(data->ch_data[indio_dev->channels[bit].channel].diff);
> In IIO, for the buffered interface, we general prefer to leave data in the endian
> ordering we get from the bus and describe that to userspace.  The basic
> philosophy is that userspace has better knowledge on what it is doing with the data
> so we provide it the information to process it rather than doing the work in kernel.


Yes, I described the reason for using |cpu_to_*| here in another response.


>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, pf->timestamp);
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>>
>> +
>> +static const struct iio_buffer_setup_ops hx9023s_buffer_setup_ops = {
>> +	.preenable = hx9023s_buffer_preenable,
>> +	.postdisable = hx9023s_buffer_postdisable,
>> +};
>> +
>> +static int hx9023s_probe(struct i2c_client *client)
>> +{
>> +	int ret;
>> +	unsigned int id;
>> +	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(dev, -ENOMEM, "device alloc failed\n");
>> +
>> +	data = iio_priv(indio_dev);
>> +	mutex_init(&data->mutex);
>> +
>> +	data->regmap = devm_regmap_init_i2c(client, &hx9023s_regmap_config);
>> +	if (IS_ERR(data->regmap))
>> +		return dev_err_probe(dev, PTR_ERR(data->regmap), "regmap init failed\n");
>> +
>> +	ret = hx9023s_property_get(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "dts phase failed\n");
>> +
>> +	ret = devm_regulator_get_enable(dev, "vdd");
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "regulator get failed\n");
>> +
>> +	fsleep(1000);
> If possible, add a specification reference for why that time.
> If not add a comment saying that it has been found to work by experimentation.
> That can be useful information if it turns out to be a bit short for someone else.


I confirmed with the IC design team to drop the delay.


>> +
>> +	ret = regmap_read(data->regmap, HX9023S_DEVICE_ID, &id);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "id check failed\n");
> Having read the ID, normally we'd compare it with expected and print a
> warning if it doesn't match, then carry on anyway (to allow for fallback compatibles
> being used for future devices that are backwards compatible for this one but
> have a different ID).
>

Added the id_check function to implement the above functionality.


>> +
>> +	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(dev, ret, "device init failed\n");
>> +
>> +	ret = hx9023s_ch_cfg(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "channel config failed\n");
>> +
>> +	ret = regcache_sync(data->regmap);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "regcache sync 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(dev, ret, "irq request failed\n");
>> +
>> +		data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
>> +						iio_device_id(indio_dev));
>> +		if (!data->trig)
>> +			return dev_err_probe(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(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(dev, ret,
>> +				"iio triggered buffer setup failed\n");
>> +
>> +	ret = devm_iio_device_register(dev, indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "iio device register failed\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int hx9023s_suspend(struct device *dev)
>> +{
>> +	struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));
>> +
>> +	hx9023s_interrupt_disable(data);
> You call these even if the trigger isn't enabled.  The disable is fine, but
> you then enable the interrupt on resume even if it wasn't previously enabled.
> This needs some state tracking so you restore to previous state.
>

>> +
>> +	return 0;
>> +}
>> +


Modified in resume

static int hx9023s_resume(struct device *dev)
{
     struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));

     if (data->trigger_enabled)
         hx9023s_interrupt_enable(data);

     return 0;
}



>> +static int hx9023s_resume(struct device *dev)
>> +{
>> +	struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));
>> +
>> +	hx9023s_interrupt_enable(data);
>> +
>> +	return 0;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(hx9023s_pm_ops, hx9023s_suspend, hx9023s_resume);
>> +
>> +static const struct acpi_device_id hx9023s_acpi_match[] = {
>> +	{ "TYHX9023" },
>> +	{}
> As Andy mentioned, drop this or add a comment on what device uses it.


Drop this


>> +};
>> +MODULE_DEVICE_TABLE(acpi, hx9023s_acpi_match);
>> +
>> +static const struct of_device_id hx9023s_of_match[] = {
>> +	{ .compatible = "tyhx,hx9023s" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, hx9023s_of_match);
>> +
>> +static const struct i2c_device_id hx9023s_id[] = {
>> +	{ "hx9023s" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, hx9023s_id);
>> +
>> +static struct i2c_driver hx9023s_driver = {
>> +	.driver = {
>> +		.name = "hx9023s",
>> +		.acpi_match_table = hx9023s_acpi_match,
>> +		.of_match_table = hx9023s_of_match,
>> +		.pm = &hx9023s_pm_ops,
>> +
>> +		/*
>> +		 * The I2C operations in hx9023s_reg_init() and hx9023s_ch_cfg()
>> +		 * are time-consuming. prefer async so we don't delay boot
> Prefer (capital P as new sentence)


Fixed


>
>> +		 * if we're builtin to the kernel.
>> +		 */
>> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> +	},
>> +	.probe = hx9023s_probe,
>> +	.id_table = hx9023s_id,
>> +};
>> +module_i2c_driver(hx9023s_driver);
>> +
>> +MODULE_AUTHOR("Yasin Lee <[e-mail]>");
>> +MODULE_DESCRIPTION("Driver for TYHX HX9023S SAR sensor");
>> +MODULE_LICENSE("GPL");

Patch

diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 2ca3b0bc5eba..0694f625b432 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -32,6 +32,20 @@  config CROS_EC_MKBP_PROXIMITY
 	  To compile this driver as a module, choose M here: the
 	  module will be called cros_ec_mkbp_proximity.
 
+config HX9023S
+	tristate "TYHX HX9023S SAR sensor"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	select REGMAP_I2C
+	depends on I2C
+	help
+	  Say Y here to build a driver for TYHX HX9023S capacitive SAR sensor.
+	  This driver supports the TYHX HX9023S capacitive
+	  SAR sensors. This sensors is used for proximity detection applications.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called hx9023s.
+
 config IRSD200
 	tristate "Murata IRS-D200 PIR sensor"
 	select IIO_BUFFER
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index f36598380446..ab381cd27dbb 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -6,6 +6,7 @@ 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AS3935)		+= as3935.o
 obj-$(CONFIG_CROS_EC_MKBP_PROXIMITY) += cros_ec_mkbp_proximity.o
+obj-$(CONFIG_HX9023S)		+= hx9023s.o
 obj-$(CONFIG_IRSD200)		+= irsd200.o
 obj-$(CONFIG_ISL29501)		+= isl29501.o
 obj-$(CONFIG_LIDAR_LITE_V2)	+= pulsedlight-lidar-lite-v2.o
diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.c
new file mode 100644
index 000000000000..b4a583105e03
--- /dev/null
+++ b/drivers/iio/proximity/hx9023s.c
@@ -0,0 +1,1162 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 NanjingTianyihexin Electronics Ltd.
+ * http://www.tianyihexin.com
+ *
+ * Driver for NanjingTianyihexin HX9023S Cap Sensor
+ * Author: Yasin Lee <[e-mail]>
+ */
+
+#include <linux/acpi.h>
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/byteorder/generic.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/kernel.h>
+#include <linux/math.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <asm/unaligned.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/types.h>
+
+#define HX9023S_CHIP_ID 0x1D
+#define HX9023S_CH_NUM 5
+#define HX9023S_2BYTES 2
+#define HX9023S_3BYTES 3
+#define HX9023S_BYTES_MAX HX9023S_3BYTES
+
+#define HX9023S_PRF_CFG                        0x02
+#define HX9023S_CH0_CFG_7_0                    0x03
+#define HX9023S_CH4_CFG_9_8                    0x0C
+#define HX9023S_RANGE_7_0                      0x0D
+#define HX9023S_RANGE_9_8                      0x0E
+#define HX9023S_RANGE_18_16                    0x0F
+#define HX9023S_AVG0_NOSR0_CFG                 0x10
+#define HX9023S_NOSR12_CFG                     0x11
+#define HX9023S_NOSR34_CFG                     0x12
+#define HX9023S_AVG12_CFG                      0x13
+#define HX9023S_AVG34_CFG                      0x14
+#define HX9023S_OFFSET_DAC0_7_0                0x15
+#define HX9023S_OFFSET_DAC4_9_8                0x1E
+#define HX9023S_SAMPLE_NUM_7_0                 0x1F
+#define HX9023S_INTEGRATION_NUM_7_0            0x21
+#define HX9023S_CH_NUM_CFG                     0x24
+#define HX9023S_LP_ALP_4_CFG                   0x29
+#define HX9023S_LP_ALP_1_0_CFG                 0x2A
+#define HX9023S_LP_ALP_3_2_CFG                 0x2B
+#define HX9023S_UP_ALP_1_0_CFG                 0x2C
+#define HX9023S_UP_ALP_3_2_CFG                 0x2D
+#define HX9023S_DN_UP_ALP_0_4_CFG              0x2E
+#define HX9023S_DN_ALP_2_1_CFG                 0x2F
+#define HX9023S_DN_ALP_4_3_CFG                 0x30
+#define HX9023S_RAW_BL_RD_CFG                  0x38
+#define HX9023S_INTERRUPT_CFG                  0x39
+#define HX9023S_INTERRUPT_CFG1                 0x3A
+#define HX9023S_CALI_DIFF_CFG                  0x3B
+#define HX9023S_DITHER_CFG                     0x3C
+#define HX9023S_DEVICE_ID                      0x60
+#define HX9023S_PROX_STATUS                    0x6B
+#define HX9023S_PROX_INT_HIGH_CFG              0x6C
+#define HX9023S_PROX_INT_LOW_CFG               0x6D
+#define HX9023S_PROX_HIGH_DIFF_CFG_CH0_0       0x80
+#define HX9023S_PROX_LOW_DIFF_CFG_CH0_0        0x88
+#define HX9023S_PROX_LOW_DIFF_CFG_CH3_1        0x8F
+#define HX9023S_PROX_HIGH_DIFF_CFG_CH4_0       0x9E
+#define HX9023S_PROX_HIGH_DIFF_CFG_CH4_1       0x9F
+#define HX9023S_PROX_LOW_DIFF_CFG_CH4_0        0xA2
+#define HX9023S_PROX_LOW_DIFF_CFG_CH4_1        0xA3
+#define HX9023S_CAP_INI_CH4_0                  0xB3
+#define HX9023S_LP_DIFF_CH4_2                  0xBA
+#define HX9023S_RAW_BL_CH4_0                   0xB5
+#define HX9023S_LP_DIFF_CH4_0                  0xB8
+#define HX9023S_DSP_CONFIG_CTRL1               0xC8
+#define HX9023S_CAP_INI_CH0_0                  0xE0
+#define HX9023S_RAW_BL_CH0_0                   0xE8
+#define HX9023S_LP_DIFF_CH0_0                  0xF4
+#define HX9023S_LP_DIFF_CH3_2                  0xFF
+
+#define HX9023S_DATA_LOCK_MASK BIT(4)
+#define HX9023S_INTERRUPT_MASK GENMASK(9, 0)
+#define HX9023S_PROX_DEBOUNCE_MASK GENMASK(3, 0)
+
+struct hx9023s_addr_val_pair {
+	u8 addr;
+	u8 val;
+};
+
+struct hx9023s_ch_data {
+	int raw;
+	int lp; /* low pass */
+	int bl; /* base line */
+	int diff; /* lp-bl */
+
+	struct {
+		int near;
+		int far;
+	} thres;
+
+	u16 dac;
+	u8 cs_position;
+	u8 channel_positive;
+	u8 channel_negative;
+	bool sel_bl;
+	bool sel_raw;
+	bool sel_diff;
+	bool sel_lp;
+	bool enable;
+};
+
+struct hx9023s_data {
+	struct iio_trigger *trig;
+	struct regmap *regmap;
+	unsigned long chan_prox_stat;
+	unsigned long chan_read;
+	unsigned long chan_event;
+	unsigned long ch_en_stat;
+	unsigned long chan_in_use;
+	unsigned int prox_state_reg;
+	bool trigger_enabled;
+
+	struct {
+		__be16 channels[HX9023S_CH_NUM];
+		s64 ts __aligned(8);
+	} buffer;
+
+	struct hx9023s_ch_data ch_data[HX9023S_CH_NUM];
+	struct mutex mutex;
+};
+
+static struct hx9023s_addr_val_pair hx9023s_reg_init_list[] = {
+	/* scan period */
+	{ HX9023S_PRF_CFG,                    0x17 },
+
+	/* full scale of conversion phase of each channel */
+	{ HX9023S_RANGE_7_0,                  0x11 },
+	{ HX9023S_RANGE_9_8,                  0x02 },
+	{ HX9023S_RANGE_18_16,                0x00 },
+
+	/* adc avg number and osr number of each channel */
+	{ HX9023S_AVG0_NOSR0_CFG,             0x71 },
+	{ HX9023S_NOSR12_CFG,                 0x44 },
+	{ HX9023S_NOSR34_CFG,                 0x00 },
+	{ HX9023S_AVG12_CFG,                  0x33 },
+	{ HX9023S_AVG34_CFG,                  0x00 },
+
+	/* sample & integration frequency of the adc */
+	{ HX9023S_SAMPLE_NUM_7_0,             0x65 },
+	{ HX9023S_INTEGRATION_NUM_7_0,        0x65 },
+
+	/* coefficient of the first order low pass filter during each channel */
+	{ HX9023S_LP_ALP_1_0_CFG,             0x22 },
+	{ HX9023S_LP_ALP_3_2_CFG,             0x22 },
+	{ HX9023S_LP_ALP_4_CFG,               0x02 },
+
+	/* up coefficient of the first order low pass filter during each channel */
+	{ HX9023S_UP_ALP_1_0_CFG,             0x88 },
+	{ HX9023S_UP_ALP_3_2_CFG,             0x88 },
+	{ HX9023S_DN_UP_ALP_0_4_CFG,          0x18 },
+
+	/* down coefficient of the first order low pass filter during each channel */
+	{ HX9023S_DN_ALP_2_1_CFG,             0x11 },
+	{ HX9023S_DN_ALP_4_3_CFG,             0x11 },
+
+	/* output data */
+	{ HX9023S_RAW_BL_RD_CFG,              0xF0 },
+
+	/* enable the interrupt function */
+	{ HX9023S_INTERRUPT_CFG,              0xFF },
+	{ HX9023S_INTERRUPT_CFG1,             0x3B },
+	{ HX9023S_DITHER_CFG,                 0x21 },
+
+	/* threshold of the offset compensation */
+	{ HX9023S_CALI_DIFF_CFG,              0x07 },
+
+	/* proximity persistency number(near & far) */
+	{ HX9023S_PROX_INT_HIGH_CFG,          0x01 },
+	{ HX9023S_PROX_INT_LOW_CFG,           0x01 },
+
+	/* disable the data lock */
+	{ HX9023S_DSP_CONFIG_CTRL1,           0x00 },
+};
+
+static const struct iio_event_spec hx9023s_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+#define HX9023S_CHANNEL(idx)					\
+{								\
+	.type = IIO_PROXIMITY,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
+	.indexed = 1,						\
+	.channel = idx,						\
+	.address = 0,						\
+	.event_spec = hx9023s_events,				\
+	.num_event_specs = ARRAY_SIZE(hx9023s_events),		\
+	.scan_index = idx,					\
+	.scan_type = {						\
+		.sign = 's',					\
+		.realbits = 16,					\
+		.storagebits = 16,				\
+		.endianness = IIO_BE,				\
+	},							\
+}
+
+static const struct iio_chan_spec hx9023s_channels[] = {
+	HX9023S_CHANNEL(0),
+	HX9023S_CHANNEL(1),
+	HX9023S_CHANNEL(2),
+	HX9023S_CHANNEL(3),
+	HX9023S_CHANNEL(4),
+	IIO_CHAN_SOFT_TIMESTAMP(5),
+};
+
+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,
+};
+
+static const struct regmap_range hx9023s_wr_reg_ranges[] = {
+	regmap_reg_range(HX9023S_DSP_CONFIG_CTRL1, HX9023S_DSP_CONFIG_CTRL1),
+	regmap_reg_range(HX9023S_CH0_CFG_7_0, HX9023S_CH4_CFG_9_8),
+	regmap_reg_range(HX9023S_PROX_HIGH_DIFF_CFG_CH0_0, HX9023S_PROX_LOW_DIFF_CFG_CH3_1),
+	regmap_reg_range(HX9023S_PROX_HIGH_DIFF_CFG_CH4_0, HX9023S_PROX_HIGH_DIFF_CFG_CH4_1),
+	regmap_reg_range(HX9023S_PROX_LOW_DIFF_CFG_CH4_0, HX9023S_PROX_LOW_DIFF_CFG_CH4_1),
+	regmap_reg_range(HX9023S_OFFSET_DAC0_7_0, HX9023S_OFFSET_DAC4_9_8),
+};
+
+static const struct regmap_range hx9023s_volatile_reg_ranges[] = {
+	regmap_reg_range(HX9023S_CAP_INI_CH4_0, HX9023S_LP_DIFF_CH4_2),
+	regmap_reg_range(HX9023S_CAP_INI_CH0_0, HX9023S_LP_DIFF_CH3_2),
+	regmap_reg_range(HX9023S_PROX_STATUS, HX9023S_PROX_STATUS),
+};
+
+static const struct regmap_access_table hx9023s_wr_regs = {
+	.yes_ranges = hx9023s_wr_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(hx9023s_wr_reg_ranges),
+};
+
+static const struct regmap_access_table hx9023s_volatile_regs = {
+	.yes_ranges = hx9023s_volatile_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(hx9023s_volatile_reg_ranges),
+};
+
+static const struct regmap_config hx9023s_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_RBTREE,
+	.wr_table = &hx9023s_wr_regs,
+	.volatile_table = &hx9023s_volatile_regs,
+};
+
+static int hx9023s_interrupt_enable(struct hx9023s_data *data)
+{
+	return regmap_update_bits(data->regmap, HX9023S_INTERRUPT_CFG,
+				HX9023S_INTERRUPT_MASK, HX9023S_INTERRUPT_MASK);
+}
+
+static int hx9023s_interrupt_disable(struct hx9023s_data *data)
+{
+	return regmap_update_bits(data->regmap, HX9023S_INTERRUPT_CFG,
+				HX9023S_INTERRUPT_MASK, 0x00);
+}
+
+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);
+	else
+		ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1, GENMASK(4, 3), 0);
+
+	return ret;
+}
+
+static int hx9023s_ch_cfg(struct hx9023s_data *data)
+{
+	int ret;
+	int i;
+	u16 reg;
+	u8 reg_list[HX9023S_CH_NUM * 2];
+	u8 ch_pos[HX9023S_CH_NUM];
+	u8 ch_neg[HX9023S_CH_NUM];
+
+	data->ch_data[0].cs_position = 0;
+	data->ch_data[1].cs_position = 2;
+	data->ch_data[2].cs_position = 4;
+	data->ch_data[3].cs_position = 6;
+	data->ch_data[4].cs_position = 8;
+
+	for (i = 0; i < HX9023S_CH_NUM; i++) {
+		if (data->ch_data[i].channel_positive == 255)
+			ch_pos[i] = 16;
+		else
+			ch_pos[i] = data->ch_data[data->ch_data[i].channel_positive].cs_position;
+		if (data->ch_data[i].channel_negative == 255)
+			ch_neg[i] = 16;
+		else
+			ch_neg[i] = data->ch_data[data->ch_data[i].channel_negative].cs_position;
+	}
+
+	for (i = 0; i < HX9023S_CH_NUM; i++) {
+		reg = (u16)((0x03 << ch_pos[i]) | (0x02 << ch_neg[i]));
+		reg_list[i * 2] = (u8)(reg);
+		reg_list[i * 2 + 1] = (u8)(reg >> 8);
+	}
+
+	ret = regmap_bulk_write(data->regmap, HX9023S_CH0_CFG_7_0, reg_list, HX9023S_CH_NUM * 2);
+
+	return ret;
+}
+
+static int hx9023s_reg_init(struct hx9023s_data *data)
+{
+	int i = 0;
+	int ret;
+
+	for (i = 0; i < (int)ARRAY_SIZE(hx9023s_reg_init_list); i++) {
+		ret = regmap_bulk_write(data->regmap, hx9023s_reg_init_list[i].addr,
+					&hx9023s_reg_init_list[i].val, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int hx9023s_write_far_debounce(struct hx9023s_data *data, int val)
+{
+	int ret;
+
+	if (val > 0xF)
+		val = 0xF;
+
+	guard(mutex)(&data->mutex);
+	ret = regmap_update_bits(data->regmap, HX9023S_PROX_INT_LOW_CFG,
+				HX9023S_PROX_DEBOUNCE_MASK, val);
+
+	return ret;
+}
+
+static int hx9023s_write_near_debounce(struct hx9023s_data *data, int val)
+{
+	int ret;
+
+	if (val > 0xF)
+		val = 0xF;
+
+	guard(mutex)(&data->mutex);
+	ret = regmap_update_bits(data->regmap, HX9023S_PROX_INT_HIGH_CFG,
+				HX9023S_PROX_DEBOUNCE_MASK, val);
+
+	return ret;
+}
+
+static int hx9023s_read_far_debounce(struct hx9023s_data *data, int *val)
+{
+	int ret;
+
+	ret = regmap_read(data->regmap, HX9023S_PROX_INT_LOW_CFG, val);
+	if (ret)
+		return ret;
+
+	*val = FIELD_GET(HX9023S_PROX_DEBOUNCE_MASK, *val);
+
+	return IIO_VAL_INT;
+}
+
+static int hx9023s_read_near_debounce(struct hx9023s_data *data, int *val)
+{
+	int ret;
+
+	ret = regmap_read(data->regmap, HX9023S_PROX_INT_HIGH_CFG, val);
+	if (ret)
+		return ret;
+
+	*val = FIELD_GET(HX9023S_PROX_DEBOUNCE_MASK, *val);
+
+	return IIO_VAL_INT;
+}
+
+static int hx9023s_get_thres_near(struct hx9023s_data *data, u8 ch, int *val)
+{
+	int ret;
+	u8 buf[2];
+
+	if (ch == 4)
+		ret = regmap_bulk_read(data->regmap, HX9023S_PROX_HIGH_DIFF_CFG_CH4_0, buf, 2);
+	else
+		ret = regmap_bulk_read(data->regmap,
+				HX9023S_PROX_HIGH_DIFF_CFG_CH0_0 + (ch * HX9023S_2BYTES), buf, 2);
+
+	if (ret)
+		return ret;
+
+	*val = get_unaligned_le16(buf);
+	*val = (*val & GENMASK(9, 0)) * 32;
+	data->ch_data[ch].thres.near = *val;
+
+	return IIO_VAL_INT;
+}
+
+static int hx9023s_get_thres_far(struct hx9023s_data *data, u8 ch, int *val)
+{
+	int ret;
+	u8 buf[2];
+
+	if (ch == 4)
+		ret = regmap_bulk_read(data->regmap, HX9023S_PROX_LOW_DIFF_CFG_CH4_0, buf, 2);
+	else
+		ret = regmap_bulk_read(data->regmap,
+				HX9023S_PROX_LOW_DIFF_CFG_CH0_0 + (ch * HX9023S_2BYTES), buf, 2);
+
+	if (ret)
+		return ret;
+
+	*val = get_unaligned_le16(buf);
+	*val = (*val & GENMASK(9, 0)) * 32;
+	data->ch_data[ch].thres.far = *val;
+
+	return IIO_VAL_INT;
+}
+
+static int hx9023s_set_thres_near(struct hx9023s_data *data, u8 ch, int val)
+{
+	int ret;
+	__le16 val_le16 = cpu_to_le16((val / 32) & GENMASK(9, 0));
+
+	data->ch_data[ch].thres.near = ((val / 32) & GENMASK(9, 0)) * 32;
+
+	if (ch == 4)
+		ret = regmap_bulk_write(data->regmap, HX9023S_PROX_HIGH_DIFF_CFG_CH4_0,
+					&val_le16, sizeof(val_le16));
+	else
+		ret = regmap_bulk_write(data->regmap,
+					HX9023S_PROX_HIGH_DIFF_CFG_CH0_0 + (ch * HX9023S_2BYTES),
+					&val_le16, sizeof(val_le16));
+
+	return ret;
+}
+
+static int hx9023s_set_thres_far(struct hx9023s_data *data, u8 ch, int val)
+{
+	int ret;
+	__le16 val_le16 = cpu_to_le16((val / 32) & GENMASK(9, 0));
+
+	data->ch_data[ch].thres.far = ((val / 32) & GENMASK(9, 0)) * 32;
+
+	if (ch == 4)
+		ret = regmap_bulk_write(data->regmap, HX9023S_PROX_LOW_DIFF_CFG_CH4_0,
+					&val_le16, sizeof(val_le16));
+	else
+		ret = regmap_bulk_write(data->regmap,
+					HX9023S_PROX_LOW_DIFF_CFG_CH0_0 + (ch * HX9023S_2BYTES),
+					&val_le16, sizeof(val_le16));
+
+	return ret;
+}
+
+static void hx9023s_get_prox_state(struct hx9023s_data *data)
+{
+	int ret;
+
+	ret = regmap_read(data->regmap, HX9023S_PROX_STATUS, &data->prox_state_reg);
+	if (ret)
+		return;
+}
+
+static void hx9023s_data_select(struct hx9023s_data *data)
+{
+	int ret;
+	int i;
+	unsigned long buf[1];
+
+	ret = regmap_read(data->regmap, HX9023S_RAW_BL_RD_CFG, (unsigned int *)buf);
+	if (ret)
+		return;
+
+	for (i = 0; i < 4; i++) {
+		data->ch_data[i].sel_diff = test_bit(i, buf);
+		data->ch_data[i].sel_lp = !data->ch_data[i].sel_diff;
+		data->ch_data[i].sel_bl = test_bit(i + 4, buf);
+		data->ch_data[i].sel_raw = !data->ch_data[i].sel_bl;
+	}
+
+	ret = regmap_read(data->regmap, HX9023S_INTERRUPT_CFG1, (unsigned int *)buf);
+	if (ret)
+		return;
+
+	data->ch_data[4].sel_diff = test_bit(2, buf);
+	data->ch_data[4].sel_lp = !data->ch_data[4].sel_diff;
+	data->ch_data[4].sel_bl = test_bit(3, buf);
+	data->ch_data[4].sel_raw = !data->ch_data[4].sel_bl;
+}
+
+static int hx9023s_sample(struct hx9023s_data *data)
+{
+	int ret;
+	int i;
+	u8 data_size;
+	u8 offset_data_size;
+	int value;
+	u8 rx_buf[HX9023S_CH_NUM * HX9023S_BYTES_MAX];
+
+	hx9023s_data_lock(data, true);
+	hx9023s_data_select(data);
+
+	data_size = HX9023S_3BYTES;
+
+	/* ch0~ch3 */
+	ret = regmap_bulk_read(data->regmap, HX9023S_RAW_BL_CH0_0, rx_buf,
+				(HX9023S_CH_NUM * data_size) - data_size);
+	if (ret)
+		return ret;
+
+	/* ch4 */
+	ret = regmap_bulk_read(data->regmap, HX9023S_RAW_BL_CH4_0,
+				rx_buf + ((HX9023S_CH_NUM * data_size) - data_size), data_size);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < HX9023S_CH_NUM; i++) {
+		value = get_unaligned_le16(&rx_buf[i * data_size + 1]);
+		value = sign_extend32(value, 15);
+		data->ch_data[i].raw = 0;
+		data->ch_data[i].bl = 0;
+		if (data->ch_data[i].sel_raw == true)
+			data->ch_data[i].raw = value;
+		if (data->ch_data[i].sel_bl == true)
+			data->ch_data[i].bl = value;
+	}
+
+	/* ch0~ch3 */
+	ret = regmap_bulk_read(data->regmap, HX9023S_LP_DIFF_CH0_0, rx_buf,
+				(HX9023S_CH_NUM * data_size) - data_size);
+	if (ret)
+		return ret;
+
+	/* ch4 */
+	ret = regmap_bulk_read(data->regmap, HX9023S_LP_DIFF_CH4_0,
+				rx_buf + ((HX9023S_CH_NUM * data_size) - data_size), data_size);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < HX9023S_CH_NUM; i++) {
+		value = get_unaligned_le16(&rx_buf[i * data_size + 1]);
+		value = sign_extend32(value, 15);
+		data->ch_data[i].lp = 0;
+		data->ch_data[i].diff = 0;
+		if (data->ch_data[i].sel_lp == true)
+			data->ch_data[i].lp = value;
+		if (data->ch_data[i].sel_diff == true)
+			data->ch_data[i].diff = value;
+	}
+
+	for (i = 0; i < HX9023S_CH_NUM; i++) {
+		if (data->ch_data[i].sel_lp == true && data->ch_data[i].sel_bl == true)
+			data->ch_data[i].diff = data->ch_data[i].lp - data->ch_data[i].bl;
+	}
+
+	/* offset dac */
+	offset_data_size = HX9023S_2BYTES;
+	ret = regmap_bulk_read(data->regmap, HX9023S_OFFSET_DAC0_7_0, rx_buf,
+				(HX9023S_CH_NUM * offset_data_size));
+	if (ret)
+		return ret;
+
+	for (i = 0; i < HX9023S_CH_NUM; i++) {
+		value = get_unaligned_le16(&rx_buf[i * offset_data_size]);
+		value = FIELD_GET(GENMASK(11, 0), value);
+		data->ch_data[i].dac = value;
+	}
+
+	hx9023s_data_lock(data, false);
+
+	return 0;
+}
+
+static int hx9023s_ch_en(struct hx9023s_data *data, u8 ch_id, bool en)
+{
+	int ret;
+	unsigned int buf;
+
+	ret = regmap_read(data->regmap, HX9023S_CH_NUM_CFG, &buf);
+	if (ret)
+		return ret;
+
+	data->ch_en_stat = buf;
+
+	if (en) {
+		if (data->ch_en_stat == 0)
+			data->prox_state_reg = 0;
+		set_bit(ch_id, &data->ch_en_stat);
+		ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
+		if (ret)
+			return ret;
+		data->ch_data[ch_id].enable = true;
+	} else {
+		clear_bit(ch_id, &data->ch_en_stat);
+		ret = regmap_bulk_write(data->regmap, HX9023S_CH_NUM_CFG, &data->ch_en_stat, 1);
+		if (ret)
+			return ret;
+		data->ch_data[ch_id].enable = false;
+	}
+
+	return 0;
+}
+
+static int hx9023s_property_get(struct hx9023s_data *data)
+{
+	int ret, i;
+	u32 temp;
+	struct fwnode_handle *child;
+	struct device *dev = regmap_get_device(data->regmap);
+
+	ret = device_property_read_u32(dev, "channel-in-use", &temp);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read channel-in-use property\n");
+	data->chan_in_use = temp;
+
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "channel-positive", &temp);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					"Failed to read channel-positive for channel %d\n", i);
+		data->ch_data[i].channel_positive = temp;
+
+		ret = fwnode_property_read_u32(child, "channel-negative", &temp);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					"Failed to read channel-negative for channel %d\n", i);
+		data->ch_data[i].channel_negative = temp;
+
+		i++;
+	}
+
+	return 0;
+}
+
+static int hx9023s_update_chan_en(struct hx9023s_data *data,
+				unsigned long chan_read,
+				unsigned long chan_event)
+{
+	int i;
+	unsigned long channels = chan_read | chan_event;
+
+	if ((data->chan_read | data->chan_event) != channels) {
+		for_each_set_bit(i, &channels, HX9023S_CH_NUM)
+			hx9023s_ch_en(data, i, test_bit(i, &data->chan_in_use));
+		for_each_clear_bit(i, &channels, HX9023S_CH_NUM)
+			hx9023s_ch_en(data, i, false);
+	}
+
+	data->chan_read = chan_read;
+	data->chan_event = chan_event;
+
+	return 0;
+}
+
+static int hx9023s_get_proximity(struct hx9023s_data *data,
+				const struct iio_chan_spec *chan,
+				int *val)
+{
+	hx9023s_sample(data);
+	hx9023s_get_prox_state(data);
+	*val = data->ch_data[chan->channel].diff;
+	return IIO_VAL_INT;
+}
+
+static int hx9023s_get_samp_freq(struct hx9023s_data *data, int *val, int *val2)
+{
+	int ret;
+	unsigned int odr;
+	unsigned int buf;
+
+	ret = regmap_read(data->regmap, HX9023S_PRF_CFG, &buf);
+	if (ret)
+		return ret;
+
+	odr = hx9023s_samp_freq_table[buf];
+	*val = 1000 / odr;
+	*val2 = div_u64((1000 % odr) * 1000000ULL, odr);
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int hx9023s_read_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
+				int *val, int *val2, long mask)
+{
+	struct hx9023s_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = hx9023s_get_proximity(data, chan, val);
+		iio_device_release_direct_mode(indio_dev);
+		return ret;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return hx9023s_get_samp_freq(data, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int hx9023s_set_samp_freq(struct hx9023s_data *data, int val, int val2)
+{
+	int i;
+	int ret;
+	int period_ms;
+	struct device *dev = regmap_get_device(data->regmap);
+
+	period_ms = div_u64(1000000000ULL, (val * 1000000ULL + val2));
+
+	for (i = 0; i < ARRAY_SIZE(hx9023s_samp_freq_table); i++) {
+		if (period_ms == hx9023s_samp_freq_table[i])
+			break;
+	}
+	if (i == ARRAY_SIZE(hx9023s_samp_freq_table)) {
+		dev_err(dev, "Period:%dms NOT found!\n", period_ms);
+		return -EINVAL;
+	}
+
+	ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &i, 1);
+
+	return ret;
+}
+
+static int hx9023s_write_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
+				int val, int val2, long mask)
+{
+	struct hx9023s_data *data = iio_priv(indio_dev);
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
+		return -EINVAL;
+
+	return hx9023s_set_samp_freq(data, val, val2);
+}
+
+static irqreturn_t hx9023s_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct hx9023s_data *data = iio_priv(indio_dev);
+
+	if (data->trigger_enabled)
+		iio_trigger_poll(data->trig);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static void hx9023s_push_events(struct iio_dev *indio_dev)
+{
+	struct hx9023s_data *data = iio_priv(indio_dev);
+	s64 timestamp = iio_get_time_ns(indio_dev);
+	unsigned long prox_changed;
+	unsigned int chan;
+
+	hx9023s_sample(data);
+	hx9023s_get_prox_state(data);
+
+	prox_changed = (data->chan_prox_stat ^ data->prox_state_reg) & data->chan_event;
+
+	for_each_set_bit(chan, &prox_changed, HX9023S_CH_NUM) {
+		int dir;
+
+		dir = (data->prox_state_reg & BIT(chan)) ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+
+		iio_push_event(indio_dev,
+			IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan, IIO_EV_TYPE_THRESH, dir),
+			timestamp);
+	}
+	data->chan_prox_stat = data->prox_state_reg;
+}
+
+static irqreturn_t hx9023s_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct hx9023s_data *data = iio_priv(indio_dev);
+
+	guard(mutex)(&data->mutex);
+	hx9023s_push_events(indio_dev);
+
+	return IRQ_HANDLED;
+}
+
+static int hx9023s_read_event_val(struct iio_dev *indio_dev,
+				 const struct iio_chan_spec *chan,
+				 enum iio_event_type type,
+				 enum iio_event_direction dir,
+				 enum iio_event_info info, int *val, int *val2)
+{
+	struct hx9023s_data *data = iio_priv(indio_dev);
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return hx9023s_get_thres_far(data, chan->channel, val);
+		case IIO_EV_DIR_FALLING:
+			return hx9023s_get_thres_near(data, chan->channel, val);
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_PERIOD:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return hx9023s_read_far_debounce(data, val);
+		case IIO_EV_DIR_FALLING:
+			return hx9023s_read_near_debounce(data, val);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int hx9023s_write_event_val(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan,
+				  enum iio_event_type type,
+				  enum iio_event_direction dir,
+				  enum iio_event_info info, int val, int val2)
+{
+	struct hx9023s_data *data = iio_priv(indio_dev);
+
+	if (chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return hx9023s_set_thres_far(data, chan->channel, val);
+		case IIO_EV_DIR_FALLING:
+			return hx9023s_set_thres_near(data, chan->channel, val);
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_INFO_PERIOD:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			return hx9023s_write_far_debounce(data, val);
+		case IIO_EV_DIR_FALLING:
+			return hx9023s_write_near_debounce(data, val);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int hx9023s_read_event_config(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir)
+{
+	struct hx9023s_data *data = iio_priv(indio_dev);
+
+	return test_bit(chan->channel, &data->chan_event);
+}
+
+static int hx9023s_write_event_config(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				int state)
+{
+	struct hx9023s_data *data = iio_priv(indio_dev);
+
+	if (test_bit(chan->channel, &data->chan_in_use)) {
+		hx9023s_ch_en(data, chan->channel, !!state);
+		if (data->ch_data[chan->channel].enable)
+			set_bit(chan->channel, &data->chan_event);
+		else
+			clear_bit(chan->channel, &data->chan_event);
+	}
+
+	return 0;
+}
+
+static const struct iio_info hx9023s_info = {
+	.read_raw = hx9023s_read_raw,
+	.write_raw = hx9023s_write_raw,
+	.read_event_value = hx9023s_read_event_val,
+	.write_event_value = hx9023s_write_event_val,
+	.read_event_config = hx9023s_read_event_config,
+	.write_event_config = hx9023s_write_event_config,
+};
+
+static int hx9023s_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct hx9023s_data *data = iio_priv(indio_dev);
+
+	guard(mutex)(&data->mutex);
+	if (state)
+		hx9023s_interrupt_enable(data);
+	else if (!data->chan_read)
+		hx9023s_interrupt_disable(data);
+	data->trigger_enabled = state;
+
+	return 0;
+}
+
+static const struct iio_trigger_ops hx9023s_trigger_ops = {
+	.set_trigger_state = hx9023s_set_trigger_state,
+};
+
+static irqreturn_t hx9023s_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct hx9023s_data *data = iio_priv(indio_dev);
+	int bit;
+	int i;
+
+	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++] =
+			cpu_to_be16(data->ch_data[indio_dev->channels[bit].channel].diff);
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, pf->timestamp);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int hx9023s_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct hx9023s_data *data = iio_priv(indio_dev);
+	unsigned long channels;
+	int bit;
+
+	guard(mutex)(&data->mutex);
+	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength)
+		__set_bit(indio_dev->channels[bit].channel, &channels);
+
+	hx9023s_update_chan_en(data, channels, data->chan_event);
+
+	return 0;
+}
+
+static int hx9023s_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct hx9023s_data *data = iio_priv(indio_dev);
+
+	guard(mutex)(&data->mutex);
+	hx9023s_update_chan_en(data, 0, data->chan_event);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops hx9023s_buffer_setup_ops = {
+	.preenable = hx9023s_buffer_preenable,
+	.postdisable = hx9023s_buffer_postdisable,
+};
+
+static int hx9023s_probe(struct i2c_client *client)
+{
+	int ret;
+	unsigned int id;
+	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(dev, -ENOMEM, "device alloc failed\n");
+
+	data = iio_priv(indio_dev);
+	mutex_init(&data->mutex);
+
+	data->regmap = devm_regmap_init_i2c(client, &hx9023s_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap), "regmap init failed\n");
+
+	ret = hx9023s_property_get(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "dts phase failed\n");
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "regulator get failed\n");
+
+	fsleep(1000);
+
+	ret = regmap_read(data->regmap, HX9023S_DEVICE_ID, &id);
+	if (ret)
+		return dev_err_probe(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(dev, ret, "device init failed\n");
+
+	ret = hx9023s_ch_cfg(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "channel config failed\n");
+
+	ret = regcache_sync(data->regmap);
+	if (ret)
+		return dev_err_probe(dev, ret, "regcache sync 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(dev, ret, "irq request failed\n");
+
+		data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
+						iio_device_id(indio_dev));
+		if (!data->trig)
+			return dev_err_probe(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(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(dev, ret,
+				"iio triggered buffer setup failed\n");
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "iio device register failed\n");
+
+	return 0;
+}
+
+static int hx9023s_suspend(struct device *dev)
+{
+	struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));
+
+	hx9023s_interrupt_disable(data);
+
+	return 0;
+}
+
+static int hx9023s_resume(struct device *dev)
+{
+	struct hx9023s_data *data = iio_priv(dev_get_drvdata(dev));
+
+	hx9023s_interrupt_enable(data);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(hx9023s_pm_ops, hx9023s_suspend, hx9023s_resume);
+
+static const struct acpi_device_id hx9023s_acpi_match[] = {
+	{ "TYHX9023" },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, hx9023s_acpi_match);
+
+static const struct of_device_id hx9023s_of_match[] = {
+	{ .compatible = "tyhx,hx9023s" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, hx9023s_of_match);
+
+static const struct i2c_device_id hx9023s_id[] = {
+	{ "hx9023s" },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, hx9023s_id);
+
+static struct i2c_driver hx9023s_driver = {
+	.driver = {
+		.name = "hx9023s",
+		.acpi_match_table = hx9023s_acpi_match,
+		.of_match_table = hx9023s_of_match,
+		.pm = &hx9023s_pm_ops,
+
+		/*
+		 * The I2C operations in hx9023s_reg_init() and hx9023s_ch_cfg()
+		 * are time-consuming. prefer async so we don't delay boot
+		 * if we're builtin to the kernel.
+		 */
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+	.probe = hx9023s_probe,
+	.id_table = hx9023s_id,
+};
+module_i2c_driver(hx9023s_driver);
+
+MODULE_AUTHOR("Yasin Lee <[e-mail]>");
+MODULE_DESCRIPTION("Driver for TYHX HX9023S SAR sensor");
+MODULE_LICENSE("GPL");