| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 19 Jun 2024 01:12:05 +0800
From: Yasin Lee <yasin.lee.x@...look.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: jic23@...nel.org, lars@...afoo.de, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, nuno.a@...log.com, swboyd@...omium.org,
u.kleine-koenig@...gutronix.de, yasin.lee.x@...il.com
Subject: Re: [PATCH v4 2/2] iio:proximity:hx9023s: Add TYHX HX9023S sensor
driver
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 <yasin.lee.x@...look.com> wrote:
>> From: Yasin Lee <yasin.lee.x@...il.com>
>>
>> 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
Powered by blists - more mailing lists