On Fri, 7 Jun 2024 19:41:38 +0800Hi Jonathan,
Yasin Lee <yasin.lee.x@xxxxxxxxxxx> wrote:
From: Yasin Lee <yasin.lee.x@xxxxxxxxx>As Andy already did a detailed review, I only took a fairly quick look.
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 <yasin.lee.x@xxxxxxxxx>
A few comments inline
Jonathan
diff --git a/drivers/iio/proximity/hx9023s.c b/drivers/iio/proximity/hx9023s.cOdd to write one bit and clear 2. If really the case, add some documentation
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);
comments.
+Looks like an odd form of endian manipulation. Try to set reg_list directly or use
+ 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);
an appropriate put_unaligned_*
+ }regmap_write() it's length 1 so not bulk!
+
+ 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);
Make sure to fix all other cases of this.
...
+handle errors and return them to userspace.
+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);handle errors etc.
+ *val = data->ch_data[chan->channel].diff;return regmap_bulk_write()
+ 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;
+}
+static int hx9023s_set_trigger_state(struct iio_trigger *trig, bool state)Ah. So you store this but you also need to use it in resume
+{
+ 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;
along with checking if events are enabled or not.
+No error handling? If these failed we don't want to provide bad data to
+ 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);
userspace. Normally we just skip on to iio_trigger_notify_done()
with a warning print to indicate something went wrong.
+In IIO, for the buffered interface, we general prefer to leave data in the endian
+ 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);
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.
+If possible, add a specification reference for why that time.
+ 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 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.
+Having read the ID, normally we'd compare it with expected and print a
+ ret = regmap_read(data->regmap, HX9023S_DEVICE_ID, &id);
+ if (ret)
+ return dev_err_probe(dev, ret, "id check failed\n");
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).
+You call these even if the trigger isn't enabled. The disable is fine, but
+ 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 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)As Andy mentioned, drop this or add a comment on what device uses it.
+{
+ 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" },
+ {}
+};Prefer (capital P as new sentence)
+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 <yasin.lee.x@xxxxxxxxx>");
+MODULE_DESCRIPTION("Driver for TYHX HX9023S SAR sensor");
+MODULE_LICENSE("GPL");