Re: [PATCH v3 2/2] iio:proximity:hx9023s: Add TYHX HX9023S sensor driver
From: Andy Shevchenko
Date: Wed May 29 2024 - 05:14:45 EST
On Wed, May 29, 2024 at 7:58 AM Yasin Lee <yasin.lee.x@xxxxxxxxxxx> wrote:
>
> From: Yasin Lee <yasin.lee.x@xxxxxxxxx>
>
> A SAR sensor from NanjingTianyihexin Electronics Ltd.
>
> The device has the following entry points:
>
> Usual frequency:
> - sampling_frequency
>
> Instant reading of current values for different sensors:
> - in_proximity0_raw
> - in_proximity1_raw
> - in_proximity2_raw
> - in_proximity3_raw
> - in_proximity4_raw
> and associated events in events/
..
> @@ -21,4 +22,3 @@ obj-$(CONFIG_SX_COMMON) += sx_common.o
> obj-$(CONFIG_SX9500) += sx9500.o
> obj-$(CONFIG_VCNL3020) += vcnl3020.o
> obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o
> -
Stray change.
..
> +#include <linux/i2c.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
Move this group (linux/iio/*) headers...
> +#include <linux/regmap.h>
>
> +#include <asm-generic/unaligned.h>
..here.
Also the rest (only two inclusions?!) is too little to this big
driver. Follow the IWYU principle ("include what you use").
..
> +#define TYHX_DELAY_MS(x) msleep(x)
This is misleading and actually useless. Use msleep() directly (btw,
you missed delay.h to be included).
..
> +#define HX9023S_RAW_BL_CH1_2 0xED
> +#define HX9023S_RAW_BL_CH2_0 0xEE
> +#define HX9023S_RAW_BL_CH2_1 0xEF
> +#define HX9023S_RAW_BL_CH2_2 0xF0
> +#define HX9023S_RAW_BL_CH3_0 0xF1
> +#define HX9023S_RAW_BL_CH3_1 0xF2
> +#define HX9023S_RAW_BL_CH3_2 0xF3
> +#define HX9023S_RAW_BL_CH4_0 0xB5
> +#define HX9023S_RAW_BL_CH4_1 0xB6
> +#define HX9023S_RAW_BL_CH4_2 0xB7
> +#define HX9023S_LP_DIFF_CH0_0 0xF4
> +#define HX9023S_LP_DIFF_CH0_1 0xF5
> +#define HX9023S_LP_DIFF_CH0_2 0xF6
> +#define HX9023S_LP_DIFF_CH1_0 0xF7
> +#define HX9023S_LP_DIFF_CH1_1 0xF8
> +#define HX9023S_LP_DIFF_CH1_2 0xF9
> +#define HX9023S_LP_DIFF_CH2_0 0xFA
> +#define HX9023S_LP_DIFF_CH2_1 0xFB
> +#define HX9023S_LP_DIFF_CH2_2 0xFC
> +#define HX9023S_LP_DIFF_CH3_0 0xFD
> +#define HX9023S_LP_DIFF_CH3_1 0xFE
> +#define HX9023S_LP_DIFF_CH3_2 0xFF
> +#define HX9023S_LP_DIFF_CH4_0 0xB8
> +#define HX9023S_LP_DIFF_CH4_1 0xB9
> +#define HX9023S_LP_DIFF_CH4_2 0xBA
Please, jeep sorted by the register offset.
..
> +#define HX9023S_DATA_LOCK_MASK BIT(4)
> +#define HX9023S_INTERRUPT_MASK GENMASK(9, 0)
> +#define HX9023S_PROX_DEBOUNCE_MASK GENMASK(3, 0)
bits.h is missing above.
..
> +struct hx9023s_addr_val_pair {
> + uint8_t addr;
> + uint8_t val;
> +};
Can you use regular in-kernel types, i.e. u8?
> +struct hx9023s_channel_info {
> + bool enabled;
> + bool used;
Despite the above, you missed types.h to be included.
> + int state;
Have you run `pahole` to check if it would be better to have this field first?
> +};
..
> +static struct hx9023s_addr_val_pair hx9023s_reg_init_list[] = {
I would like to see a comment along each initialisation value to
explain what it does. Otherwise it looks like a magic blob.
Also make comments, if needed, about the ordering of this list. I.o.w.
does it have dependencies or all registers can be initialised in
arbitrary order?
> +};
..
> +struct hx9023s_data {
> + struct mutex mutex;
> + struct i2c_client *client;
For what purpose?
> + struct iio_trigger *trig;
> + struct regmap *regmap;
> + unsigned long chan_prox_stat;
> + bool trigger_enabled;
> + struct {
> + __be16 channels[HX9023S_CH_NUM];
> +
Redundant blank line.
> + s64 ts __aligned(8);
> +
Ditto.
> + } buffer;
> + unsigned long chan_read;
> + unsigned long chan_event;
> +
> + struct hx9023s_threshold thres[HX9023S_CH_NUM];
> + struct hx9023s_channel_info *chs_info;
> + unsigned long ch_en_stat;
> + unsigned int prox_state_reg;
> + unsigned int accuracy;
> + unsigned long channel_used_flag;
> + unsigned int cs_position[HX9023S_CH_NUM];
> + unsigned int channel_positive[HX9023S_CH_NUM];
> + unsigned int channel_negative[HX9023S_CH_NUM];
> + int raw[HX9023S_CH_NUM];
> + int lp[HX9023S_CH_NUM]; /*low pass*/
> + int bl[HX9023S_CH_NUM]; /*base line*/
> + int diff[HX9023S_CH_NUM]; /*lp - bl*/
Mind spaces in the comments.
> + uint16_t dac[HX9023S_CH_NUM];
u16
> + bool sel_bl[HX9023S_CH_NUM];
> + bool sel_raw[HX9023S_CH_NUM];
> + bool sel_diff[HX9023S_CH_NUM];
> + bool sel_lp[HX9023S_CH_NUM];
> + unsigned int odr;
> + unsigned int integration_sample;
> + unsigned int osr[HX9023S_CH_NUM];
> + unsigned int avg[HX9023S_CH_NUM];
> + unsigned int lp_alpha[HX9023S_CH_NUM];
Can you rather make a per-channel structure and then have only one array here
struct foo_channel chan_context[_CH_NUM];
?
> +};
..
> +static const unsigned int hx9023s_samp_freq_table[] = {
> + 2, 2, 4, 6, 8, 10, 14, 18, 22, 26,
> + 30, 34, 38, 42, 46, 50, 56, 62, 68, 74,
> + 80, 90, 100, 200, 300, 400, 600, 800, 1000, 2000,
> + 3000, 4000
Keep trailing comma.
> +};
..
> +static const struct regmap_config hx9023s_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_NONE,
Why no cache?
Do you need a regmap lock on top of what you have already?
> +};
..
> + dev_err(&data->client->dev, "i2c read failed\n");
> + dev_err(&data->client->dev, "i2c read failed\n");
struct device *dev = regmap_get_dev(...);
dev_err(dev, ...);
here and everywhere else.
..
> +static int hx9023s_data_lock(struct hx9023s_data *data, bool locked)
> +{
> + int ret;
> +
> + if (locked) {
> + ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1,
> + HX9023S_DATA_LOCK_MASK, HX9023S_DATA_LOCK_MASK);
> + if (ret < 0) {
Why ' < 0' ?
> + dev_err(&data->client->dev, "i2c read failed\n");
> + return ret;
> + }
if (ret)
dev_err();
return ret;
> + } else {
Redundant. see above.
> + ret = regmap_update_bits(data->regmap, HX9023S_DSP_CONFIG_CTRL1,
> + GENMASK(4, 3), 0x00);
With 0x00 --> 0 and above this will be one line.
> + if (ret < 0) {
> + dev_err(&data->client->dev, "i2c read failed\n");
> + return ret;
> + }
> + }
> +
> + return ret;
Too many unneeded LoCs, see above how to optimise.
> +}
..
> +static int hx9023s_get_id(struct hx9023s_data *data)
> +{
> + int ret;
> + unsigned int rxbuf[1];
> +
> + ret = regmap_read(data->regmap, HX9023S_DEVICE_ID, rxbuf);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "i2c read failed\n");
> + return ret;
> + }
> +
> + return 0;
Same optimisation as per above function applicable here. Do it
everywhere to remove a few LoCs here and there.
> +}
..
> +static int hx9023s_para_cfg(struct hx9023s_data *data)
> +{
> + int ret;
> + uint8_t buf[3];
> +
> + ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &data->odr, 1);
> + if (ret) {
> + dev_err(&data->client->dev, "i2c write failed\n");
> + return ret;
> + }
> + buf[0] = data->integration_sample & 0xFF;
> + buf[1] = data->integration_sample >> 8;
put_unaligned_le16()
> + ret = regmap_bulk_write(data->regmap, HX9023S_SAMPLE_NUM_7_0, buf, 2);
> + if (ret) {
> + dev_err(&data->client->dev, "i2c write failed\n");
This is a very repetitive and useless message AFAICS.
> + return ret;
> + }
> +
> + ret = regmap_bulk_write(data->regmap, HX9023S_INTEGRATION_NUM_7_0, buf, 2);
> + if (ret) {
> + dev_err(&data->client->dev, "i2c write failed\n");
> + return ret;
> + }
> +
> + buf[0] = (data->avg[2] << 4) | data->avg[1];
> + buf[1] = (data->avg[4] << 4) | data->avg[3];
I believe this also can be optimised, esp. if you reconsider the avg[]
data type.
> + ret = regmap_bulk_write(data->regmap, HX9023S_AVG12_CFG, buf, 2);
> + if (ret) {
> + dev_err(&data->client->dev, "i2c write failed\n");
> + return ret;
> + }
> +
> + buf[0] = (data->osr[2] << 4) | data->osr[1];
> + buf[1] = (data->osr[4] << 4) | data->osr[3];
Ditto.
> + ret = regmap_bulk_write(data->regmap, HX9023S_NOSR12_CFG, buf, 2);
> + if (ret) {
> + dev_err(&data->client->dev, "i2c write failed\n");
> + return ret;
> + }
> +
> + ret = regmap_update_bits(data->regmap, HX9023S_AVG0_NOSR0_CFG, GENMASK(7, 2),
> + ((data->avg[0] << 5) | (data->osr[0] << 2)));
Too many parentheses.
> + if (ret < 0) {
> + dev_err(&data->client->dev, "i2c read failed\n");
> + return ret;
> + }
> + buf[0] = data->lp_alpha[4];
> + buf[1] = (data->lp_alpha[1] << 4) | data->lp_alpha[0];
> + buf[2] = (data->lp_alpha[3] << 4) | data->lp_alpha[2];
Also sounds like put_unaligned_be24() with a properly cooked argument.
> + ret = regmap_bulk_write(data->regmap, HX9023S_LP_ALP_4_CFG, buf, 3);
> + if (ret) {
> + dev_err(&data->client->dev, "i2c write failed\n");
> + return ret;
> + }
> +
> + return ret;
> +}
I stopped here as most of your functions have the same problems and
can be shrinked with a few % gain of the overall number of LoCs.
..
> + for (i = 0; i < HX9023S_CH_NUM; i++) {
> + value = get_unaligned_le16(&rx_buf[i * offset_data_size]);
> + value = value & 0xFFF;
> + data->dac[i] = value;
Just
->dac = value & GENMASK();
> + }
..
> +static int hx9023s_dts_phase(struct hx9023s_data *data)
> +{
> + struct device_node *np = data->client->dev.of_node;
No for at least two reasons:
- for the sensors we do not accept new code that is OF-centric, make
use of the agnostic device property APIs
- it's bad to dereference of_node/fwnode as it adds unneeded churn in the future
You will need property.h to be included.
> + return 0;
> +}
> + if ((data->chan_read | data->chan_event) != channels) {
> + for (i = 0; i < HX9023S_CH_NUM; i++) {
> + if (test_bit(i, &data->channel_used_flag) && test_bit(i, &channels))
Make it
for_each_set_bit(i, &channels, ...) {
if (test_bit(..., _is_used)) // rename _used_flag to _is_used or
even _in_use
}
(Replace bits.h with bitops.h in the inclusion block for these)
> + hx9023s_ch_en_hal(data, i, true);
> + else
> + hx9023s_ch_en_hal(data, i, false);
> + }
> + }
> +
> + data->chan_read = chan_read;
> + data->chan_event = chan_event;
> + return 0;
> +}
..
> + odr = hx9023s_samp_freq_table[buf[0]];
> + *val = 1000 / odr;
> + *val2 = ((1000 % odr) * 1000000ULL) / odr;
Include units.h and use the proper definitions from there.
..
> + period_ms = div_u64(1000000000ULL, (val * 1000000ULL + val2));
math.h is missing.
Also consider using proper time constants live NSEC_PER_SEC or so.
..
> + for (i = 0; i < ARRAY_SIZE(hx9023s_samp_freq_table); i++) {
array_size.h is missing.
> + if (period_ms == hx9023s_samp_freq_table[i])
> + break;
> + }
> + if (i == ARRAY_SIZE(hx9023s_samp_freq_table)) {
> + dev_err(&data->client->dev, "Period:%dms NOT found!\n", period_ms);
dev_printk.h
> + return -EINVAL;
errno.h
> + }
..
> + ret = regmap_bulk_write(data->regmap, HX9023S_PRF_CFG, &buf[0], 1);
, buf, sizeof(buf) ?
> + if (ret)
You are not even consistent with the checks in one file!
> + dev_err(&data->client->dev, "i2c read failed\n");
> + return ret;
Can it not be 0 here?
..
> + if (data->chs_info[chan->channel].enabled)
> + set_bit(chan->channel, &data->chan_event);
> + else
> + clear_bit(chan->channel, &data->chan_event);
Why atomic?
In any case, use assign_bit() / __assign_bit().
..
> + int i = 0;
Why signed?
> + guard(mutex)(&data->mutex);
> + hx9023s_sample(data);
> + hx9023s_get_prox_state(data);
> +
> + for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength)
> + data->buffer.channels[i++] = data->diff[indio_dev->channels[bit].channel];
..
> +static int hx9023s_probe(struct i2c_client *client)
> +{
> + int ret;
> + int i;
> + struct device *dev = &client->dev;
> + struct iio_dev *indio_dev;
> + struct hx9023s_data *data;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(struct hx9023s_data));
> + if (!indio_dev)
> + return dev_err_probe(&client->dev, -ENOMEM, "device alloc failed\n");
You are even inconsistent in the use of dev in a single function! Why
not dev here?
> + data = iio_priv(indio_dev);
> + data->client = client;
> + mutex_init(&data->mutex);
mutex.h
> + ret = hx9023s_dts_phase(data);
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret, "dts phase failed\n");
> +
> + data->chs_info = devm_kzalloc(&data->client->dev,
> + sizeof(struct hx9023s_channel_info) * HX9023S_CH_NUM, GFP_KERNEL);
Okay, you need to replace dev_printk.h I mentioned above by device.h,
but on top of that this should be devm_kcalloc().
> + if (data->chs_info == NULL)
Pattern is if (!...)
> + return dev_err_probe(&data->client->dev, -ENOMEM, "channel info alloc failed\n");
Ouch, as I said, this is the third variant of dev to be used. Use dev
everywhere.
> + for (i = 0; i < HX9023S_CH_NUM; i++)
> + if (test_bit(i, &data->channel_used_flag))
for_each_set_bit()
> + data->chs_info[i].used = true;
Interesting why you need this.
> + data->regmap = devm_regmap_init_i2c(client, &hx9023s_regmap_config);
> + if (IS_ERR(data->regmap)) {
> + ret = PTR_ERR(data->regmap);
> + return dev_err_probe(&data->client->dev, ret, "regmap init failed\n");
Use dev and move PTR_ERR() to be in the parameter for dev_err_probe().
> + }
> +
> + ret = devm_regulator_get_enable(&data->client->dev, "vdd");
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret, "regulator get failed\n");
> + usleep_range(1000, 1100);
fsleep()
> + ret = hx9023s_get_id(data);
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret, "id check failed\n");
> +
> + indio_dev->channels = hx9023s_channels;
> + indio_dev->num_channels = ARRAY_SIZE(hx9023s_channels);
> + indio_dev->info = &hx9023s_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->name = "hx9023s";
> + i2c_set_clientdata(client, indio_dev);
> +
> + ret = hx9023s_reg_init(data);
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret, "device init failed\n");
> +
> + ret = hx9023s_ch_cfg(data);
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret, "channel config failed\n");
> +
> + ret = hx9023s_para_cfg(data);
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret, "parameter config failed\n");
> +
> + if (client->irq) {
> + ret = devm_request_threaded_irq(dev, client->irq, hx9023s_irq_handler,
> + hx9023s_irq_thread_handler, IRQF_ONESHOT,
> + "hx9023s_event", indio_dev);
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret, "irq request failed\n");
> +
> + data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> + iio_device_id(indio_dev));
I'm wondering if there is a default naming in that API... Would be
nice to have it for cases like this.
> + if (!data->trig)
> + return dev_err_probe(&data->client->dev, -ENOMEM,
> + "iio trigger alloc failed\n");
> +
> + data->trig->ops = &hx9023s_trigger_ops;
> + iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> + ret = devm_iio_trigger_register(dev, data->trig);
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret,
> + "iio trigger register failed\n");
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, iio_pollfunc_store_time,
> + hx9023s_trigger_handler, &hx9023s_buffer_setup_ops);
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret,
> + "iio triggered buffer setup failed\n");
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return dev_err_probe(&data->client->dev, ret, "iio device register failed\n");
> +
> + return 0;
> +}
..
> +static const struct acpi_device_id hx9023s_acpi_match[] = {
> + { .id = "TYHX9023", .driver_data = HX9023S_CHIP_ID },
We don't use C99 for ACPI ID tables, moreover you need mod_devicetable.h.
See also below.
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, hx9023s_acpi_match);
> +static const struct of_device_id hx9023s_of_match[] = {
> + { .compatible = "tyhx,hx9023s", (void *)HX9023S_CHIP_ID },
No, use a proper pointer that will give a chip info like structure.
Same for above and below ID tables.
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, hx9023s_of_match);
> +
> +static const struct i2c_device_id hx9023s_id[] = {
> + { .name = "hx9023s", .driver_data = HX9023S_CHIP_ID },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, hx9023s_id);
..
Also, for better review experience, can you split this to a few patches, like
1. main functionality
2. trigger support
3. ACPI support (ID table)
?
Reviewing 1.5 kLoCs at once is kinda big load.
--
With Best Regards,
Andy Shevchenko