| Message ID | 20240726061312.1371450-1-wangshuaijie@awinic.com |
|---|---|
| Headers | show |
| Series | Add support for aw9610x proximity sensor | expand |
On 26/07/2024 08:13, wangshuaijie@awinic.com wrote: > From: shuaijie wang <wangshuaijie@awinic.com> > > AW9610X is a low power consumption capacitive touch and proximity controller. > Each channel can be independently config as sensor input, shield output. > > Channel Information: > aw96103: 3-channel > aw96105: 5-channel > > Signed-off-by: shuaijie wang <wangshuaijie@awinic.com> > --- > drivers/iio/proximity/Kconfig | 11 + > drivers/iio/proximity/Makefile | 1 + > drivers/iio/proximity/aw9610x.c | 791 ++++++++++++++++++++++++++++++++ > drivers/iio/proximity/aw9610x.h | 140 ++++++ > 4 files changed, 943 insertions(+) > create mode 100644 drivers/iio/proximity/aw9610x.c > create mode 100644 drivers/iio/proximity/aw9610x.h > > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig > index 2ca3b0bc5eba..ca1b8bde2def 100644 > --- a/drivers/iio/proximity/Kconfig > +++ b/drivers/iio/proximity/Kconfig > @@ -219,4 +219,15 @@ config VL53L0X_I2C > To compile this driver as a module, choose M here: the > module will be called vl53l0x-i2c. > > +config AW9610X > + tristate "Awinic AW9610X proximity sensor" > + select REGMAP_I2C > + depends on I2C > + help > + Say Y here to build a driver for Awinic's AW9610X capacitive > + proximity sensor. > + > + To compile this driver as a module, choose M here: the > + module will be called aw9610x. > + > endmenu > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile > index f36598380446..483f5bf1ac8b 100644 > --- a/drivers/iio/proximity/Makefile > +++ b/drivers/iio/proximity/Makefile > @@ -21,4 +21,5 @@ obj-$(CONFIG_SX_COMMON) += sx_common.o > obj-$(CONFIG_SX9500) += sx9500.o > obj-$(CONFIG_VCNL3020) += vcnl3020.o > obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o > +obj-$(CONFIG_AW9610X) += aw9610x.o > > diff --git a/drivers/iio/proximity/aw9610x.c b/drivers/iio/proximity/aw9610x.c > new file mode 100644 > index 000000000000..de5be3918e4f > --- /dev/null > +++ b/drivers/iio/proximity/aw9610x.c > @@ -0,0 +1,791 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AWINIC aw9610x proximity sensor driver > + * > + * Author: Wang Shuaijie <wangshuaijie@awinic.com> > + * > + * Copyright (c) 2024 awinic Technology CO., LTD > + */ > + > +#include <linux/bits.h> > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/firmware.h> > +#include <linux/i2c.h> > +#include <linux/input.h> Where do you use it? > +#include <linux/interrupt.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/of_gpio.h> Where do you use it? > +#include <linux/power_supply.h> > +#include <linux/pinctrl/consumer.h> Where do you use it? > +#include <linux/regulator/consumer.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> Maybe several others are not used... > +#include "aw9610x.h" > + > +static unsigned int aw9610x_reg_default[] = { Why this cannot be const? > + 0x0000, 0x00003f3f, 0x0004, 0x00000064, 0x0008, 0x0017c11e, > + 0x000c, 0x05000000, 0x0010, 0x00093ffd, 0x0014, 0x19240009, > + 0x0018, 0xd81c0207, 0x001c, 0xff000000, 0x0020, 0x00241900, > + 0x0024, 0x00093ff7, 0x0028, 0x58020009, 0x002c, 0xd81c0207, > + 0x0030, 0xff000000, 0x0034, 0x00025800, 0x0038, 0x00093fdf, > + 0x003c, 0x7d3b0009, 0x0040, 0xd81c0207, 0x0044, 0xff000000, > + 0x0048, 0x003b7d00, 0x004c, 0x00093f7f, 0x0050, 0xe9310009, > + 0x0054, 0xd81c0207, 0x0058, 0xff000000, 0x005c, 0x0031e900, > + 0x0060, 0x00093dff, 0x0064, 0x1a0c0009, 0x0068, 0xd81c0207, > + 0x006c, 0xff000000, 0x0070, 0x000c1a00, 0x0074, 0x80093fff, > + 0x0078, 0x043d0009, 0x007c, 0xd81c0207, 0x0080, 0xff000000, > + 0x0084, 0x003d0400, 0x00a0, 0xe6400000, 0x00a4, 0x00000000, > + 0x00a8, 0x010408d2, 0x00ac, 0x00000000, 0x00b0, 0x00000000, > + 0x00b8, 0x00005fff, 0x00bc, 0x00000000, 0x00c0, 0x00000000, > + 0x00c4, 0x00000000, 0x00c8, 0x00000000, 0x00cc, 0x00000000, > + 0x00d0, 0x00000000, 0x00d4, 0x00000000, 0x00d8, 0x00000000, > + 0x00dc, 0xe6447800, 0x00e0, 0x78000000, 0x00e4, 0x010408d2, > + 0x00e8, 0x00000000, 0x00ec, 0x00000000, 0x00f4, 0x00005fff, > + 0x00f8, 0x00000000, 0x00fc, 0x00000000, 0x0100, 0x00000000, > + 0x0104, 0x00000000, 0x0108, 0x00000000, 0x010c, 0x02000000, > + 0x0110, 0x00000000, 0x0114, 0x00000000, 0x0118, 0xe6447800, > + 0x011c, 0x78000000, 0x0120, 0x010408d2, 0x0124, 0x00000000, > + 0x0128, 0x00000000, 0x0130, 0x00005fff, 0x0134, 0x00000000, > + 0x0138, 0x00000000, 0x013c, 0x00000000, 0x0140, 0x00000000, > + 0x0144, 0x00000000, 0x0148, 0x02000000, 0x014c, 0x00000000, > + 0x0150, 0x00000000, 0x0154, 0xe6447800, 0x0158, 0x78000000, > + 0x015c, 0x010408d2, 0x0160, 0x00000000, 0x0164, 0x00000000, > + 0x016c, 0x00005fff, 0x0170, 0x00000000, 0x0174, 0x00000000, > + 0x0178, 0x00000000, 0x017c, 0x00000000, 0x0180, 0x00000000, > + 0x0184, 0x02000000, 0x0188, 0x00000000, 0x018c, 0x00000000, > + 0x0190, 0xe6447800, 0x0194, 0x78000000, 0x0198, 0x010408d2, > + 0x019c, 0x00000000, 0x01a0, 0x00000000, 0x01a8, 0x00005fff, > + 0x01ac, 0x00000000, 0x01b0, 0x00000000, 0x01b4, 0x00000000, > + 0x01b8, 0x00000000, 0x01bc, 0x00000000, 0x01c0, 0x02000000, > + 0x01c4, 0x00000000, 0x01c8, 0x00000000, 0x01cc, 0xe6407800, > + 0x01d0, 0x78000000, 0x01d4, 0x010408d2, 0x01d8, 0x00000000, > + 0x01dc, 0x00000000, 0x01e4, 0x00005fff, 0x01e8, 0x00000000, > + 0x01ec, 0x00000000, 0x01f0, 0x00000000, 0x01f4, 0x00000000, > + 0x01f8, 0x00000000, 0x01fc, 0x02000000, 0x0200, 0x00000000, > + 0x0204, 0x00000000, 0x0208, 0x00000008, 0x020c, 0x0000000d, > + 0x41fc, 0x00000000, 0x4400, 0x00000000, 0x4410, 0x00000000, > + 0x4420, 0x00000000, 0x4430, 0x00000000, 0x4440, 0x00000000, > + 0x4450, 0x00000000, 0x4460, 0x00000000, 0x4470, 0x00000000, > + 0xf080, 0x00003018, 0xf084, 0x00000fff, 0xf800, 0x00000000, > + 0xf804, 0x00002e00, 0xf8d0, 0x00000001, 0xf8d4, 0x00000000, > + 0xff00, 0x00000301, 0xff0c, 0x01000000, 0xffe0, 0x00000000, > + 0xfff4, 0x00004011, 0x0090, 0x00000000, 0x0094, 0x00000000, > + 0x0098, 0x00000000, 0x009c, 0x3f3f3f3f, > +}; > + ... > +static int aw9610x_read_chipid(struct aw9610x *aw9610x) > +{ > + unsigned char cnt = 0; > + u32 reg_val; > + int ret; > + > + while (cnt < AW_READ_CHIPID_RETRIES) { > + ret = aw9610x_i2c_read(aw9610x, REG_CHIPID, ®_val); > + if (ret < 0) { > + cnt++; > + usleep_range(2000, 3000); > + } else { > + reg_val = FIELD_GET(AW9610X_CHIPID_MASK, reg_val); > + break; > + } > + } > + > + if (reg_val == AW9610X_CHIP_ID) > + return 0; So devices are detectable? Encode this in the bindings (oneOf and a fallback compatible) and drop unneeded entry from ID tables. Best regards, Krzysztof
On Fri, 26 Jul 2024 06:13:12 +0000 wangshuaijie@awinic.com wrote: > From: shuaijie wang <wangshuaijie@awinic.com> > > AW9610X is a low power consumption capacitive touch and proximity controller. > Each channel can be independently config as sensor input, shield output. Needs more information, particularly what the USB powersupply notification is about. It's unlikely that belongs directly in an IIO driver, unless that supply is part of this same chip. > > Channel Information: > aw96103: 3-channel > aw96105: 5-channel I don't see where this is implemented. Seems to assume 5 channels for both. > > Signed-off-by: shuaijie wang <wangshuaijie@awinic.com> > --- > drivers/iio/proximity/Kconfig | 11 + > drivers/iio/proximity/Makefile | 1 + > drivers/iio/proximity/aw9610x.c | 791 ++++++++++++++++++++++++++++++++ > drivers/iio/proximity/aw9610x.h | 140 ++++++ > 4 files changed, 943 insertions(+) > create mode 100644 drivers/iio/proximity/aw9610x.c > create mode 100644 drivers/iio/proximity/aw9610x.h > > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig > index 2ca3b0bc5eba..ca1b8bde2def 100644 > --- a/drivers/iio/proximity/Kconfig > +++ b/drivers/iio/proximity/Kconfig > @@ -219,4 +219,15 @@ config VL53L0X_I2C > To compile this driver as a module, choose M here: the > module will be called vl53l0x-i2c. > > +config AW9610X As with dt-binding no wild cards in driver naming for the same reasons. > + tristate "Awinic AW9610X proximity sensor" Also not in the title here though the same 'and similar' postfix can work to indicate it's a set of devices. > + select REGMAP_I2C > + depends on I2C > + help > + Say Y here to build a driver for Awinic's AW9610X capacitive > + proximity sensor. List the full set of parts supported explicitly here because people like tog rep for them. > + > + To compile this driver as a module, choose M here: the > + module will be called aw9610x. No wild card here either. > + > endmenu > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile > index f36598380446..483f5bf1ac8b 100644 > --- a/drivers/iio/proximity/Makefile > +++ b/drivers/iio/proximity/Makefile > @@ -21,4 +21,5 @@ obj-$(CONFIG_SX_COMMON) += sx_common.o > obj-$(CONFIG_SX9500) += sx9500.o > obj-$(CONFIG_VCNL3020) += vcnl3020.o > obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o > +obj-$(CONFIG_AW9610X) += aw9610x.o > > diff --git a/drivers/iio/proximity/aw9610x.c b/drivers/iio/proximity/aw9610x.c > new file mode 100644 > index 000000000000..de5be3918e4f > --- /dev/null > +++ b/drivers/iio/proximity/aw9610x.c > @@ -0,0 +1,791 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AWINIC aw9610x proximity sensor driver > + * > + * Author: Wang Shuaijie <wangshuaijie@awinic.com> > + * > + * Copyright (c) 2024 awinic Technology CO., LTD > + */ > + > +#include <linux/bits.h> > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/firmware.h> > +#include <linux/i2c.h> > +#include <linux/input.h> As already pointed out, some of these at least don't make sense. Check them all. > +#include <linux/interrupt.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/of_gpio.h> not used. > +#include <linux/power_supply.h> > +#include <linux/pinctrl/consumer.h> > +#include <linux/regulator/consumer.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > +#include "aw9610x.h" No wild cards in header either. > + > +static const struct regmap_config aw9610x_regmap_confg = { > + .reg_bits = 16, > + .val_bits = 32, > +}; > + > +static int aw9610x_i2c_write(struct aw9610x *aw9610x, > + u16 reg_addr16, u32 reg_data32) > +{ > + return regmap_write(aw9610x->regmap, (unsigned int)reg_addr16, > + (unsigned int)reg_data32); AS below. No to this one as well. Just use regmap_write() inline. > +} > + > +static int aw9610x_i2c_read(struct aw9610x *aw9610x, > + u16 reg_addr16, u32 *reg_data32) > +{ > + unsigned int data; > + int ret; > + > + ret = regmap_read(aw9610x->regmap, (unsigned int)reg_addr16, &data); > + if (ret == 0) > + *reg_data32 = data; > + As below, just do this inline not via a trivial wrapper function. > + return ret; > +} > + > +static int aw9610x_i2c_write_bits(struct aw9610x *aw9610x, > + u16 reg_addr16, u32 mask, u32 reg_data32) > +{ > + return regmap_update_bits(aw9610x->regmap, (unsigned int)reg_addr16, > + (unsigned int)(~mask), (unsigned int)reg_data32); Get rid of these wrappers and use regmap directly inline. The wrappers are obscuring what is happening not making it easier to follow > +} > + > +static const struct iio_event_spec aw_common_events[2] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_RISING, > + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD), Unusual to have only a period control. Device provides no means to turn them off, or adjust the thresholds? > + }, > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_FALLING, > + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD), > + }, > +}; > + > +static int aw9610x_get_diff_raw(struct aw9610x *aw9610x, > + unsigned int chan, int *buf) > +{ > + u32 data; > + int ret; > + > + ret = aw9610x_i2c_read(aw9610x, REG_DIFF_CH0 + chan * 4, &data); > + if (ret) > + return ret; > + *buf = (int)data / AW_DATA_PROCESS_FACTOR; cast after paths as otherwise might not fit. > + > + return ret; return 0; as can't be anything else and nice to make that obvious. > +} > + > +static int aw9610x_read_raw(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + int *val, int *val2, long mask) > +{ > + struct aw9610x *aw9610x; > + > + aw9610x = iio_priv(indio_dev); > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + aw9610x_get_diff_raw(aw9610x, chan->channel, val); > + break; return IIO_VAL_INT; here > + default: > + return -EINVAL; > + } > + > + return IIO_VAL_INT; and drop this. > +} > + > +struct iio_info iio_info = { > + .read_raw = aw9610x_read_raw, > +}; > + > +static int aw9610x_iio_init(struct aw9610x *aw9610x) Pass in the iio_dev and then use iio_priv() to get the struct aw96101 > +{ > + struct iio_chan_spec *aw9610x_channels; > + unsigned int chan_num = 0, j = 0; > + unsigned int i; > + > + aw9610x->channels_arr = devm_kcalloc(aw9610x->dev, AW_CHANNEL_MAX, > + sizeof(struct aw_channels_info), GFP_KERNEL); > + if (!aw9610x->channels_arr) > + return -ENOMEM; > + > + for (i = 0; i < AW_CHANNEL_MAX; i++) { > + aw9610x->channels_arr[i].last_channel_info = 0; you calloc it just above. It should be already 0. > + if ((AW9610X_CHANNEL_USED_MASK >> i) & 0x01) { This is hard coded in the driver which makes no sense. If you need this, expand the dt bindings to include channel sub nodes so firmware describes what is enabled. If all enabled is fine, then drop this code as they are all always used. > + aw9610x->channels_arr[i].used = true; > + chan_num++; > + } else { > + aw9610x->channels_arr[i].used = false; > + } > + } > + > + aw9610x_channels = devm_kcalloc(aw9610x->dev, chan_num, > + sizeof(*aw9610x_channels), GFP_KERNEL); > + if (!aw9610x_channels) > + return -ENOMEM; > + > + for (i = 0; i < AW_CHANNEL_MAX; i++) { > + if (aw9610x->channels_arr[i].used) { > + aw9610x_channels[j].type = IIO_PROXIMITY; > + aw9610x_channels[j].info_mask_separate = > + BIT(IIO_CHAN_INFO_RAW); > + aw9610x_channels[j].indexed = 1; > + aw9610x_channels[j].channel = i; > + aw9610x_channels[j].event_spec = aw_common_events; > + aw9610x_channels[j].num_event_specs = > + ARRAY_SIZE(aw_common_events); > + j++; > + } > + } > + > + aw9610x->aw_iio_dev->modes = INDIO_DIRECT_MODE; > + aw9610x->aw_iio_dev->num_channels = chan_num; > + aw9610x->aw_iio_dev->channels = aw9610x_channels; > + aw9610x->aw_iio_dev->info = &iio_info; > + aw9610x->aw_iio_dev->name = "aw9610x_sensor"; > + aw9610x->aw_iio_dev->dev.parent = aw9610x->dev; > + > + return devm_iio_device_register(aw9610x->dev, aw9610x->aw_iio_dev); > +} > + > +static int aw9610x_reg_version_comp(struct aw9610x *aw9610x, > + struct aw_bin *aw_bin) > +{ > + u32 blfilt1_data, blfilt1_tmp; > + unsigned char i; > + int ret = 0; > + > + if ((aw9610x->vers == AW9610XA) && (aw_bin->chip_type[7] == '\0')) { Needs documentation / comments. > + for (i = 0; i < 6; i++) { > + ret = aw9610x_i2c_read(aw9610x, > + REG_BLFILT_CH0 + > + (AW9610X_BLFILT_CH_STEP * i), > + &blfilt1_data); > + if (ret) > + return ret; > + blfilt1_tmp = FIELD_GET(AW9610X_BLERRTRIG_MASK, > + blfilt1_data); > + if (blfilt1_tmp == 1) { > + ret = aw9610x_i2c_write_bits(aw9610x, > + REG_BLRSTRNG_CH0 + > + (AW9610X_BLFILT_CH_STEP * i), > + ~(AW9610X_BLRSTRNG_MASK), 1 << i); > + if (ret) > + return ret; > + } > + } > + } > + > + return ret; > +} > + > +static void aw9610x_aot_set(struct aw9610x *aw9610x) > +{ > + aw9610x_i2c_write_bits(aw9610x, REG_SCANCTRL0, > + ~(AW9610X_AOT_MASK << AW9610X_AOT_BIT), > + (AW9610X_AOT_MASK) << AW9610X_AOT_BIT); FIELD_PREP() > +} > + > +static int aw9610x_bin_valid_loaded(struct aw9610x *aw9610x, > + struct aw_bin *aw_bin_data_s) > +{ > + unsigned int start_addr = aw_bin_data_s->valid_data_addr; > + u32 i, reg_data; > + u16 reg_addr; > + int ret; > + > + for (i = 0; i < aw_bin_data_s->valid_data_len; > + i += 6, start_addr += 6) { Fix alignment (see comments later) > + reg_addr = *(u16 *)(aw_bin_data_s->data + start_addr); > + reg_data = *(u32 *)(aw_bin_data_s->data + start_addr + 2); > + if ((reg_addr == REG_EEDA0) || (reg_addr == REG_EEDA1)) > + continue; > + if (reg_addr == REG_IRQEN) { > + aw9610x->hostirqen = reg_data; > + continue; > + } > + ret = aw9610x_i2c_write(aw9610x, reg_addr, reg_data); > + if (ret < 0) > + return ret; > + > + } > + ret = aw9610x_reg_version_comp(aw9610x, aw_bin_data_s); > + if (ret) > + return ret; > + ret = aw9610x_channel_scan_start(aw9610x); > + > + return ret; return aw961.. Fix all other cases of this. > +} > + > +static int aw9610x_para_loaded(struct aw9610x *aw9610x) > +{ > + int i, ret; > + > + for (i = 0; i < ARRAY_SIZE(aw9610x_reg_default); i += 2) { > + ret = aw9610x_i2c_write(aw9610x, > + (u16)aw9610x_reg_default[i], > + (u32)aw9610x_reg_default[i+1]); spacing around + in all similar places. > + if (ret) > + return ret; > + if (aw9610x_reg_default[i] == REG_IRQEN) > + aw9610x->hostirqen = aw9610x_reg_default[i+1]; > + } > + ret = aw9610x_channel_scan_start(aw9610x); > + > + return ret; > +} > + > +static int aw9610x_cfg_all_loaded(const struct firmware *cont, > + struct aw9610x *aw9610x) > +{ > + struct aw_bin *aw_bin; > + int ret; > + > + if (!cont) > + return -EINVAL; > + > + aw_bin = kzalloc(cont->size + sizeof(*aw_bin), GFP_KERNEL); Use __free(kfree) lots of examples in tree, but will avoid need to manually free and simplify this code a little. > + if (!aw_bin) > + return -ENOMEM; > + > + aw_bin->len = cont->size; > + memcpy(aw_bin->data, cont->data, cont->size); > + aw9610x_parsing_bin_file(aw_bin); > + > + snprintf(aw9610x->chip_type, sizeof(aw9610x->chip_type), "%s", > + aw_bin->chip_type); > + ret = aw9610x_bin_valid_loaded(aw9610x, aw_bin); > + kfree(aw_bin); > + > + return ret; > +} > + > +static int aw9610x_cfg_update(struct aw9610x *aw9610x) > +{ > + const struct firmware *fw; > + int ret; > + > + ret = request_firmware(&fw, "aw9610x_0.bin", aw9610x->dev); No wild cards in this either. > + if (ret) > + return ret; > + ret = aw9610x_cfg_all_loaded(fw, aw9610x); > + if (ret) > + ret = aw9610x_para_loaded(aw9610x); > + release_firmware(fw); > + > + return ret; > +} > + > +static void aw9610x_cfg_work_routine(struct work_struct *work) > +{ > + struct aw9610x *aw9610x = container_of(work, struct aw9610x, > + cfg_work.work); > + > + aw9610x_cfg_update(aw9610x); So this is polling in driver. We'd normally hook up to a hrtimer trigger for that. Perhaps you need this for your events sampling though? If so that may be fine to do somewhat like this. I'm just not sure of the usecase currently. > +} > + > +static int aw9610x_sar_cfg_init(struct aw9610x *aw9610x) > +{ > + INIT_DELAYED_WORK(&aw9610x->cfg_work, aw9610x_cfg_work_routine); > + schedule_delayed_work(&aw9610x->cfg_work, msecs_to_jiffies(5000)); > + > + return 0; > +} > + > +static int aw9610x_sw_reset(struct aw9610x *aw9610x) > +{ > + int ret; > + > + ret = aw9610x_i2c_write(aw9610x, REG_RESET, 0); > + msleep(20); > + > + return ret; > +} > + > +static ssize_t update_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct aw9610x *aw9610x = dev_get_drvdata(dev); > + unsigned int state; > + ssize_t ret; > + > + ret = kstrtouint(buf, 10, &state); > + if (ret) > + return ret; > + if (state) { > + aw9610x_i2c_write(aw9610x, REG_IRQEN, 0); > + aw9610x_sw_reset(aw9610x); > + schedule_delayed_work(&aw9610x->cfg_work, > + msecs_to_jiffies(10)); > + } > + > + return count; > +} > + > +static DEVICE_ATTR_WO(update); > + > +static struct attribute *aw9610x_sar_attributes[] = { > + &dev_attr_update.attr, This needs documenting as it's custom ABI. Note that we don't often accept custom ABI. Particularly not a hook that seems to reset the device. If you want to do that, unbind and rebind the whole drive so we are in a known state etc. > + NULL > +}; > + > +static struct attribute_group aw9610x_sar_attribute_group = { > + .attrs = aw9610x_sar_attributes > +}; > + > +static void aw9610x_irq_handle(struct aw9610x *aw9610x) > +{ > + u32 curr_status_val; > + u32 curr_status; > + unsigned char i; > + int ret; > + > + ret = aw9610x_i2c_read(aw9610x, REG_STAT0, &curr_status_val); > + if (ret) > + return; > + > + for (i = 0; i < AW_CHANNEL_MAX; i++) { > + curr_status = (((curr_status_val >> (24 + i)) & 0x1)) | > + (((curr_status_val >> (16 + i)) & 0x1) << 1) | > + (((curr_status_val >> (8 + i)) & 0x1) << 2) | > + (((curr_status_val >> (i)) & 0x1) << 3); Add a comment on what is going on here as it's tricky to read. Also, no brackets around the i in last line. Probably better expressed as a series of FIELD_GET() calls with appropriat masks of the 32 bit value. > + > + if (!aw9610x->channels_arr[i].used || > + (aw9610x->channels_arr[i].last_channel_info == > + curr_status)) Align as if (!aw (aw9610... > + continue; > + > + switch (curr_status) { > + case FAR: > + iio_push_event(aw9610x->aw_iio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, i, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_RISING), > + iio_get_time_ns(aw9610x->aw_iio_dev)); > + break; > + case TRIGGER_TH0: > + case TRIGGER_TH1: > + case TRIGGER_TH2: > + case TRIGGER_TH3: 4 thresholds on the same channel? This is confusing given we are reporting them as events on different channels. but this loop is over the channels. > + iio_push_event(aw9610x->aw_iio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, i, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_FALLING), > + iio_get_time_ns(aw9610x->aw_iio_dev)); > + break; > + default: > + return; > + } > + aw9610x->channels_arr[i].last_channel_info = curr_status; > + } > +} > + > +static void aw9610x_interrupt_clear(struct aw9610x *aw9610x) > +{ > + int ret; > + > + ret = aw9610x_i2c_read(aw9610x, REG_IRQSRC, &aw9610x->irq_status); > + if (ret) > + return; > + > + aw9610x_irq_handle(aw9610x); > +} > + > +static irqreturn_t aw9610x_irq(int irq, void *data) > +{ > + struct aw9610x *aw9610x = data; Pass in the iio_dev not the priv() data as you need some access to things in that structure and you can go the other way with iio_priv() > + > + aw9610x_interrupt_clear(aw9610x); > + > + return IRQ_HANDLED; > +} > + > +static int aw9610x_interrupt_init(struct aw9610x *aw9610x) > +{ > + unsigned int irq_status; > + int ret; > + > + ret = aw9610x_i2c_write(aw9610x, REG_IRQEN, 0); Never combine return values. Check each one and return if an error. If these return different values the resulting error code provided to the caller will be garbage. > + ret |= aw9610x_i2c_read(aw9610x, REG_IRQSRC, &irq_status); > + if (ret) > + return ret; > + ret = devm_request_threaded_irq(aw9610x->dev, aw9610x->i2c->irq, > + NULL, aw9610x_irq, IRQF_TRIGGER_FALLING | IRQF_ONESHOT, IRQF_TRIGGER_FALLING should be provided by FW not set by the driver. Doing it here removes the possibility of a firmware coping with inverters in the signal path (often done as a cheap form of level conversion). So just specify IRQF_ONESHOT > + "aw9610x_irq", aw9610x); no wild cards here either. > + if (ret) > + return ret; Blank line > + ret = aw9610x_i2c_write(aw9610x, REG_IRQEN, aw9610x->hostirqen); return aw9601x... > + > + return ret; > +} > +static int aw9610x_power_init(struct aw9610x *aw9610x) > +{ > + int rc; > + > + aw9610x->vcc = devm_regulator_get(aw9610x->dev, "vcc"); devm_regulator_get_enabled() You only turn it off in error paths or on remove. Both are handled fine via devm / device managed cleanup. So no need to do it manually or keep a copy of the vcc pointer around. This function then becomes a single call, so don't have a separate function. Also for future reference look at what devm_regulator_get() does if a regulator is not supplied by fw. It doesn't return NULL. > + if (!aw9610x->vcc) > + return 0; > + > + rc = aw9610x_power_enable(aw9610x, true); > + > + return rc; > +} > + > +static int aw9610x_wait_chip_init(struct aw9610x *aw9610x) > +{ > + unsigned int cnt = 20; > + u32 reg_data; > + int ret; > + > + while (cnt--) { > + ret = aw9610x_i2c_read(aw9610x, REG_IRQSRC, ®_data); > + if (ret) > + return ret; > + > + if ((reg_data & 0x01) == 0x01) That bit should have a meaningful named define. if ((FIELD_GET(xxxx)) return 0; > + return 0; > + mdelay(1); > + } > + > + return -EINVAL; > +} ... > +static int aw9610x_ps_notify_init(struct aw9610x *aw9610x) > +{ > + struct power_supply *psy; > + int ret; > + > + INIT_WORK(&aw9610x->ps_notify_work, aw9610x_ps_notify_callback_work); > + aw9610x->ps_notif.notifier_call = aw9610x_ps_notify_callback; > + ret = power_supply_reg_notifier(&aw9610x->ps_notif); > + if (ret) > + return ret; Add devm_add_action_or_reset() to handle unregistering the notifier. That will remove the need to manually clean it up in remove or in error paths here. > + > + psy = power_supply_get_by_name(USB_POWER_SUPPLY_NAME); This is unusual to see in an IIO driver. So please provide more information on this. Is this supply coupled to this sensor? If not you may need a consumer driver for the IIO device driver that handles this notification part and allows the description of what power supply the notification is relevant to. This stuff all belongs in DT, not via a magic name based looking up deep in a driver. > + if (!psy) > + goto free_ps_notifier; > + > + ret = aw9610x_ps_get_state(aw9610x, psy, &aw9610x->ps_is_present); > + if (ret) > + goto free_ps_notifier; > + > + return ret; > + > +free_ps_notifier: > + power_supply_unreg_notifier(&aw9610x->ps_notif); > + > + return -EINVAL; > +} > + > +static int aw9610x_i2c_probe(struct i2c_client *i2c) > +{ > + struct iio_dev *aw_iio_dev; > + struct aw9610x *aw9610x; > + int ret; > + > + aw_iio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*aw9610x)); > + if (!aw_iio_dev) > + return -ENOMEM; > + > + aw9610x = iio_priv(aw_iio_dev); > + aw9610x->aw_iio_dev = aw_iio_dev; As elsewhere. Get rid of this reverse pointer. It is easy to avoid needing it and that leads to much more predictable code for reviewers. > + aw9610x->dev = &i2c->dev; > + aw9610x->i2c = i2c; > + i2c_set_clientdata(i2c, aw9610x); > + > + aw9610x->regmap = devm_regmap_init_i2c(i2c, &aw9610x_regmap_confg); > + if (IS_ERR(aw9610x->regmap)) > + return PTR_ERR(aw9610x->regmap); Blank line here > + ret = aw9610x_power_init(aw9610x); As above, just pull the regulator get and enable here and don't bother with the wrapper for what will be one line of code. > + if (ret) > + return ret; > + > + ret = aw9610x_read_chipid(aw9610x); > + if (ret) > + return ret; > + > + ret = aw9610x_sw_reset(aw9610x); > + if (ret) > + return ret; > + > + ret = aw9610x_wait_chip_init(aw9610x); > + if (ret) > + return ret; > + > + ret = aw9610x_version_init(aw9610x); > + if (ret) > + return ret; > + > + ret = aw9610x_ps_notify_init(aw9610x); > + if (ret < 0) > + goto err_ps_notify; register a cleanup for this here with devm_add_action_or_reset() so that you can avoid manual cleanup. > + > + ret = aw9610x_iio_init(aw9610x); Once this call is done, the userspace interface is available to callers. Normally that's done last, because I assume the code below is doing something useful and as such is necessary before we starting using the device? > + if (ret) > + goto err_iio_init; > + > + ret = aw9610x_sar_cfg_init(aw9610x); > + if (ret) > + goto err_iio_init; > + > + ret = sysfs_create_group(&i2c->dev.kobj, &aw9610x_sar_attribute_group); Why are you attaching attributes to the parent device? > + if (ret) > + goto err_iio_init; > + > + ret = aw9610x_interrupt_init(aw9610x); > + if (ret) > + goto err_interrupt_init; > + > + return ret; > + > +err_interrupt_init: > + sysfs_remove_group(&i2c->dev.kobj, &aw9610x_sar_attribute_group); > +err_iio_init: > + power_supply_unreg_notifier(&aw9610x->ps_notif); > +err_ps_notify: > + if (aw9610x->vcc) > + aw9610x_power_enable(aw9610x, false); > + > + return ret; > +} > + > +static void aw9610x_i2c_remove(struct i2c_client *i2c) > +{ > + struct aw9610x *aw9610x = i2c_get_clientdata(i2c); > + > + if (aw9610x->vcc) > + aw9610x_power_enable(aw9610x, false); No need for this dance. The regulator framework will provide a stub regulator not return NULL. So you will always have one to pass to these calls. However, as elsewhere - you don't really need power supply control anyway. > + power_supply_unreg_notifier(&aw9610x->ps_notif); Use a devm_add_action_or_reset() to call this automatically. Lots of examples in tree for how to use those. > + sysfs_remove_group(&i2c->dev.kobj, &aw9610x_sar_attribute_group); This is a sign of a design problem. There shouldn't be any dynamic attributes in a driver like this one. Anyhow, I'll address that above (I'm reviewing backwards as easier to understand a driver from coarse to fine). > +} > + > +static void aw9610x_i2c_shutdown(struct i2c_client *i2c) > +{ > + struct aw9610x *aw9610x = i2c_get_clientdata(i2c); > + u32 irq_status_temp; > + > + aw9610x_i2c_write(aw9610x, REG_IRQEN, 0); > + aw9610x_i2c_read(aw9610x, REG_IRQSRC, &irq_status_temp); > + aw9610x_i2c_write(aw9610x, REG_CMD, AW9610X_SLEEP_MODE); Worth doing given the power is going to be turned off anyway? Pretty rare to provide a shutdown callback form an i2c device. > +} > + > +static const struct of_device_id aw9610x_dt_match[] = { > + { .compatible = "awinic,aw96103" }, > + { .compatible = "awinic,aw96105" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, aw9610x_dt_match); > + > +static const struct i2c_device_id aw9610x_i2c_id[] = { > + { "aw96103", 0 }, > + { "aw96105", 0 }, > + { }, No comma on null terminator like entries as we don't want to make it easy to add things after them. > +}; > +MODULE_DEVICE_TABLE(i2c, aw9610x_i2c_id); > + > +static struct i2c_driver aw9610x_i2c_driver = { > + .driver = { > + .name = "aw9610x_sensor", > + .of_match_table = aw9610x_dt_match, > + }, > + .probe = aw9610x_i2c_probe, > + .remove = aw9610x_i2c_remove, > + .shutdown = aw9610x_i2c_shutdown, > + .id_table = aw9610x_i2c_id, > +}; > +module_i2c_driver(aw9610x_i2c_driver); > + > +MODULE_AUTHOR("Wang Shuaijie <wangshuaijie@awinic.com>"); > +MODULE_DESCRIPTION("Driver for Awinic AW9610X proximity sensor"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/iio/proximity/aw9610x.h b/drivers/iio/proximity/aw9610x.h > new file mode 100644 > index 000000000000..38b3f36ebb7a > --- /dev/null > +++ b/drivers/iio/proximity/aw9610x.h > @@ -0,0 +1,140 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#ifndef _AW9610X_H_ > +#define _AW9610X_H_ > + > +#define AW9610X_CHANNEL_USED_MASK 0x1F > +#define AW_VCC_MIN_UV 1700000 > +#define AW_VCC_MAX_UV 3600000 > +#define AW_DATA_PROCESS_FACTOR 1024 > +#define AW9610X_CPU_WORK_MASK 1 > +#define AW9610X_CHIP_MIN_VOLTAGE 1600000 > +#define AW_CHIP_AW9610XA 0x03000b00 > +#define AW_READ_CHIPID_RETRIES 3 > +#define AW_I2C_RETRIES 5 > +#define USB_POWER_SUPPLY_NAME "charger" > +#define AW9610X_AOT_MASK 0x3f > +#define AW9610X_AOT_BIT 8 > +#define AW9610X_CHIP_ID 0xa961 > +#define AW_CHIP_AW9610XA 0x03000b00 > +#define AW9610X_CPU_WORK_MASK 1 > +#define AW9610X_AOT_MASK 0x3f > +#define AW9610X_AOT_BIT 8 > +#define REG_EEDA0 0x0408 > +#define REG_EEDA1 0x040C > +#define AW9610X_BIN_VALID_DATA_OFFSET 64 > +#define AW9610X_BIN_DATA_LEN_OFFSET 16 > +#define AW9610X_BIN_DATA_REG_NUM_SIZE 4 > +#define AW9610X_BIN_CHIP_TYPE_SIZE 8 > +#define AW9610X_BIN_CHIP_TYPE_OFFSET 24 > +#define AW9610X_BLFILT_CH_STEP 0x3C > +#define AW9610X_BLRSTRNG_MASK 0x3F > +#define AW9610X_CHIPID_MASK GENMASK(31, 16) > +#define AW9610X_BLERRTRIG_MASK BIT(25) > + > +#define AFE_BASE_ADDR 0x0000 If the bases are all zero, get rid of them. > +#define DSP_BASE_ADDR 0x0000 > +#define STAT_BASE_ADDR 0x0000 > +#define SFR_BASE_ADDR 0x0000 > +#define DATA_BASE_ADDR 0x0000 > +#define REG_SCANCTRL0 (0x0000 + AFE_BASE_ADDR) > +#define REG_AFECFG1_CH0 (0x0014 + AFE_BASE_ADDR) > +#define REG_FWVER (0x0088 + STAT_BASE_ADDR) > +#define REG_WST (0x008C + STAT_BASE_ADDR) > +#define REG_STAT0 (0x0090 + STAT_BASE_ADDR) > +#define REG_STAT1 (0x0094 + STAT_BASE_ADDR) > +#define REG_CHINTEN (0x009C + STAT_BASE_ADDR) > +#define REG_BLFILT_CH0 (0x00A8 + DSP_BASE_ADDR) > +#define REG_BLRSTRNG_CH0 (0x00B4 + DSP_BASE_ADDR) > +#define REG_BLFILT_CH1 (0x00E4 + DSP_BASE_ADDR) > +#define REG_COMP_CH0 (0x0210 + DATA_BASE_ADDR) > +#define REG_BASELINE_CH0 (0x0228 + DATA_BASE_ADDR) > +#define REG_DIFF_CH0 (0x0240 + DATA_BASE_ADDR) > +#define REG_FWVER2 (0x0410 + DATA_BASE_ADDR) > +#define REG_CMD (0xF008 + SFR_BASE_ADDR) > +#define REG_IRQSRC (0xF080 + SFR_BASE_ADDR) > +#define REG_IRQEN (0xF084 + SFR_BASE_ADDR) > +#define REG_OSCEN (0xFF00 + SFR_BASE_ADDR) > +#define REG_RESET (0xFF0C + SFR_BASE_ADDR) > +#define REG_CHIPID (0xFF10 + SFR_BASE_ADDR) More common I think to do (SFR_BASE_ADDR + 0xFF10) however, given the base is 0, just drop that part. Prefix these all with AW96103_ Otherwise high chance of a naming clash and it's not obvious these are device specific defines. > + > +#define REG_NONE_ACCESS 0 > +#define REG_RD_ACCESS (1 << 0) > +#define REG_WR_ACCESS (1 << 1) > +struct aw_reg_data { > + unsigned char rw; > + unsigned short reg; > +}; Not used that I can see. > + > +/** > + * struct aw_bin - Needs a short description. Run the kernel-doc script over these files and fix any warnings etc. > + * @chip_type: Frame header information-chip type > + * @valid_data_len: Length of valid data obtained after parsing > + * @valid_data_addr: The offset address of the valid data obtained > + * after parsing relative to info > + * @len: The size of the bin file obtained from the firmware > + * @data: Store the bin file obtained from the firmware > + */ > +struct aw_bin { > + unsigned char chip_type[8]; > + unsigned int valid_data_len; > + unsigned int valid_data_addr; > + unsigned int len; > + unsigned char data[]; use a __counted_by marking on this so the compiler can tell if you write code that will go off the end. > +}; > + > +enum aw9610x_sar_vers { > + AW9610X = 2, > + AW9610XA = 6, > + AW9610XB = 0xa, > +}; > + > +enum aw9610x_operation_mode { > + AW9610X_ACTIVE_MODE = 1, > + AW9610X_SLEEP_MODE, > + AW9610X_DEEPSLEEP_MODE, > + AW9610XB_DEEPSLEEP_MODE, > +}; > + > +enum aw9610x_channel { > + AW_CHANNEL0, > + AW_CHANNEL1, > + AW_CHANNEL2, > + AW_CHANNEL3, > + AW_CHANNEL4, > + AW_CHANNEL5, > + AW_CHANNEL_MAX, Drop comma on _MAX as we don't want anything after this. > +}; > + > +enum aw9610x_irq_trigger_position { > + FAR, > + TRIGGER_TH0, > + TRIGGER_TH1 = 0x03, > + TRIGGER_TH2 = 0x07, > + TRIGGER_TH3 = 0x0f, > +}; > + > +struct aw_channels_info { > + bool used; > + unsigned int last_channel_info; > +}; > + > +struct aw9610x { Even structs need to not use wildcards for naming. aw96101 > + struct iio_dev *aw_iio_dev; That is an alarm bell. If you are in situation where you need to go from the iio_priv() data back to to the iio_dev that contains it you probably have a layering issue and should have passed the iio_dev in to the relevant function in the first place. That usually means using struct iio_dev instance in things like the private data of callbacks. > + unsigned char vers; > + unsigned int irq_status; > + unsigned int hostirqen; > + struct work_struct ps_notify_work; > + struct notifier_block ps_notif; > + bool ps_is_present; > + struct delayed_work cfg_work; > + struct i2c_client *i2c; > + struct regmap *regmap; > + struct device *dev; > + struct aw_bin *aw_bin; > + struct regulator *vcc; > + struct aw_channels_info *channels_arr; > + unsigned char chip_type[9]; > +}; > + > +#endif > +
> ... > > > +static int aw9610x_read_chipid(struct aw9610x *aw9610x) > > +{ > > + unsigned char cnt = 0; > > + u32 reg_val; > > + int ret; > > + > > + while (cnt < AW_READ_CHIPID_RETRIES) { Why retries? > > + ret = aw9610x_i2c_read(aw9610x, REG_CHIPID, ®_val); > > + if (ret < 0) { > > + cnt++; > > + usleep_range(2000, 3000); > > + } else { > > + reg_val = FIELD_GET(AW9610X_CHIPID_MASK, reg_val); > > + break; > > + } > > + } > > + > > + if (reg_val == AW9610X_CHIP_ID) > > + return 0; > > So devices are detectable? Encode this in the bindings (oneOf and a > fallback compatible) and drop unneeded entry from ID tables. Hi Krzysztof, I think this is not a good idea. Even though these two are detectable, this breaks if along comes a 3rd device in the future which is truly compatible with one of these two parts but that we don't yet know about (so can't discover). For that part we will want to provide a meaningful fallback compatible. It needs to fallback to either the 3 channel or the 5 channel chip and handle it as appropriate. (Note that this difference is non obvious as right now the code pretends there are always 5 channels and that needs fixing). If the chips provided a register that told all the chip specific data like how many channels, then sure making one fallback to the other would be fine as future devices could use those standard registers. With just an Id register, we can't discover enough. Hence these two parts should not be listed as compatible with each other. Jonathan > > Best regards, > Krzysztof >
On 27/07/2024 17:06, Jonathan Cameron wrote: > >> ... >> >>> +static int aw9610x_read_chipid(struct aw9610x *aw9610x) >>> +{ >>> + unsigned char cnt = 0; >>> + u32 reg_val; >>> + int ret; >>> + >>> + while (cnt < AW_READ_CHIPID_RETRIES) { > Why retries? >>> + ret = aw9610x_i2c_read(aw9610x, REG_CHIPID, ®_val); >>> + if (ret < 0) { >>> + cnt++; >>> + usleep_range(2000, 3000); >>> + } else { >>> + reg_val = FIELD_GET(AW9610X_CHIPID_MASK, reg_val); >>> + break; >>> + } >>> + } >>> + >>> + if (reg_val == AW9610X_CHIP_ID) >>> + return 0; >> >> So devices are detectable? Encode this in the bindings (oneOf and a >> fallback compatible) and drop unneeded entry from ID tables. > > Hi Krzysztof, > > I think this is not a good idea. > > Even though these two are detectable, this breaks if along comes a 3rd device > in the future which is truly compatible with one of these two parts but that > we don't yet know about (so can't discover). For that part we will want to > provide a meaningful fallback compatible. > > It needs to fallback to either the 3 channel or the 5 channel chip and handle > it as appropriate. (Note that this difference is non obvious as right now the > code pretends there are always 5 channels and that needs fixing). > > If the chips provided a register that told all the chip specific data like > how many channels, then sure making one fallback to the other would be fine > as future devices could use those standard registers. > > With just an Id register, we can't discover enough. Hence these two > parts should not be listed as compatible with each other. Sure Best regards, Krzysztof
Hi Jonathan, On Sat, 27 Jul 2024 16:02:16 +0100, jic23@kernel.org wrote: >On Fri, 26 Jul 2024 06:13:12 +0000 >wangshuaijie@awinic.com wrote: > >> From: shuaijie wang <wangshuaijie@awinic.com> >> >> AW9610X is a low power consumption capacitive touch and proximity controller. >> Each channel can be independently config as sensor input, shield output. > >Needs more information, particularly what the USB powersupply notification is about. >It's unlikely that belongs directly in an IIO driver, unless that supply is >part of this same chip. > >> >> Channel Information: >> aw96103: 3-channel >> aw96105: 5-channel >I don't see where this is implemented. Seems to assume 5 channels for both. > >> >> Signed-off-by: shuaijie wang <wangshuaijie@awinic.com> >> --- >> drivers/iio/proximity/Kconfig | 11 + >> drivers/iio/proximity/Makefile | 1 + >> drivers/iio/proximity/aw9610x.c | 791 ++++++++++++++++++++++++++++++++ >> drivers/iio/proximity/aw9610x.h | 140 ++++++ >> 4 files changed, 943 insertions(+) >> create mode 100644 drivers/iio/proximity/aw9610x.c >> create mode 100644 drivers/iio/proximity/aw9610x.h >> >> ... >> +static int aw9610x_cfg_all_loaded(const struct firmware *cont, >> + struct aw9610x *aw9610x) >> +{ >> + struct aw_bin *aw_bin; >> + int ret; >> + >> + if (!cont) >> + return -EINVAL; >> + >> + aw_bin = kzalloc(cont->size + sizeof(*aw_bin), GFP_KERNEL); >Use __free(kfree) > >lots of examples in tree, but will avoid need to manually free and >simplify this code a little. > I'm sorry, I didn't quite understand what you meant. Are you suggesting the use of devm_? Could you please provide more detailed suggestions? Thank you! >> + if (!aw_bin) >> + return -ENOMEM; >> + >> + aw_bin->len = cont->size; >> + memcpy(aw_bin->data, cont->data, cont->size); >> + aw9610x_parsing_bin_file(aw_bin); >> + >> + snprintf(aw9610x->chip_type, sizeof(aw9610x->chip_type), "%s", >> + aw_bin->chip_type); >> + ret = aw9610x_bin_valid_loaded(aw9610x, aw_bin); >> + kfree(aw_bin); >> + >> + return ret; >> +} >> + >> +static int aw9610x_cfg_update(struct aw9610x *aw9610x) >> +{ >> + const struct firmware *fw; >> + int ret; >> + >> + ret = request_firmware(&fw, "aw9610x_0.bin", aw9610x->dev); > >No wild cards in this either. > >> + if (ret) >> + return ret; >> + ret = aw9610x_cfg_all_loaded(fw, aw9610x); >> + if (ret) >> + ret = aw9610x_para_loaded(aw9610x); >> + release_firmware(fw); >> + >> + return ret; >> +} >> + >> +static void aw9610x_cfg_work_routine(struct work_struct *work) >> +{ >> + struct aw9610x *aw9610x = container_of(work, struct aw9610x, >> + cfg_work.work); >> + >> + aw9610x_cfg_update(aw9610x); > >So this is polling in driver. We'd normally hook up to a hrtimer >trigger for that. Perhaps you need this for your events sampling though? >If so that may be fine to do somewhat like this. I'm just not sure >of the usecase currently. > The primary objective of this delayed task is to load the register configuration file. The chip needs to load the register configuration file during power-on initialization. In cases where the driver is compiled directly into the kernel, rather than existing as a dynamically loaded module, there may be a situation where the driver attempts to load before the file system is fully prepared, resulting in an inability to access the register configuration file. Therefore, a delayed task mechanism is employed to ensure the register configuration file is loaded properly. If there are any concerns about my understanding or approach, please feel free to offer suggestions. Thank you very much! >> +} >> + >> +static int aw9610x_sar_cfg_init(struct aw9610x *aw9610x) >> +{ >> + INIT_DELAYED_WORK(&aw9610x->cfg_work, aw9610x_cfg_work_routine); >> + schedule_delayed_work(&aw9610x->cfg_work, msecs_to_jiffies(5000)); >> + >> + return 0; >> +} >> + >> +static int aw9610x_sw_reset(struct aw9610x *aw9610x) >> +{ >> + int ret; >> + >> + ret = aw9610x_i2c_write(aw9610x, REG_RESET, 0); >> + msleep(20); >> + >> + return ret; >> +} >> + >> +static ssize_t update_store(struct device *dev, struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct aw9610x *aw9610x = dev_get_drvdata(dev); >> + unsigned int state; >> + ssize_t ret; >> + >> + ret = kstrtouint(buf, 10, &state); >> + if (ret) >> + return ret; >> + if (state) { >> + aw9610x_i2c_write(aw9610x, REG_IRQEN, 0); >> + aw9610x_sw_reset(aw9610x); >> + schedule_delayed_work(&aw9610x->cfg_work, >> + msecs_to_jiffies(10)); >> + } >> + >> + return count; >> +} >> + >> +static DEVICE_ATTR_WO(update); >> + >> +static struct attribute *aw9610x_sar_attributes[] = { >> + &dev_attr_update.attr, > >This needs documenting as it's custom ABI. >Note that we don't often accept custom ABI. >Particularly not a hook that seems to reset the device. >If you want to do that, unbind and rebind the whole drive so >we are in a known state etc. > > >> + NULL >> +}; >> + >> +static struct attribute_group aw9610x_sar_attribute_group = { >> + .attrs = aw9610x_sar_attributes >> +}; >> + >> +static void aw9610x_irq_handle(struct aw9610x *aw9610x) >> +{ >> + u32 curr_status_val; >> + u32 curr_status; >> + unsigned char i; >> + int ret; >> + >> + ret = aw9610x_i2c_read(aw9610x, REG_STAT0, &curr_status_val); >> + if (ret) >> + return; >> + >> + for (i = 0; i < AW_CHANNEL_MAX; i++) { >> + curr_status = (((curr_status_val >> (24 + i)) & 0x1)) | >> + (((curr_status_val >> (16 + i)) & 0x1) << 1) | >> + (((curr_status_val >> (8 + i)) & 0x1) << 2) | >> + (((curr_status_val >> (i)) & 0x1) << 3); > >Add a comment on what is going on here as it's tricky to read. >Also, no brackets around the i in last line. >Probably better expressed as a series of FIELD_GET() calls with appropriat >masks of the 32 bit value. > > The work processed here is to parse the interrupt status of different channels. bit0/bit8/bit16/bit24 represent the interrupt status of channel 0, with each of the 4 bits corresponding to an interrupt status for approaching a threshold. Similarly, bit1/bit9/bit17/bit25 represent the interrupt status of channel 1. To facilitate subsequent interrupt status judgments, the 4 interrupt statuses of the same channel are combined into a single data. Sorry, I have not found a suitable way to utilize FIELD_GET for this purpose. >> + >> + if (!aw9610x->channels_arr[i].used || >> + (aw9610x->channels_arr[i].last_channel_info == >> + curr_status)) >Align as > if (!aw > (aw9610... > >> + continue; >> + >> + switch (curr_status) { >> + case FAR: >> + iio_push_event(aw9610x->aw_iio_dev, >> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, i, >> + IIO_EV_TYPE_THRESH, >> + IIO_EV_DIR_RISING), >> + iio_get_time_ns(aw9610x->aw_iio_dev)); >> + break; >> + case TRIGGER_TH0: >> + case TRIGGER_TH1: >> + case TRIGGER_TH2: >> + case TRIGGER_TH3: >4 thresholds on the same channel? This is confusing given we are reporting them >as events on different channels. but this loop is over the channels. > > There are 4 proximity thresholds on the same channel, each representing a different level of proximity. TRIGGER_TH0/TRIGGER_TH1/TRIGGER_TH2/TRIGGER_TH3 all represent proximity states, but with varying degrees of proximity. Here I have a question to ask. I'm not sure how to use iio to report different proximity states. Can you give me some suggestions? Thank you! >> + iio_push_event(aw9610x->aw_iio_dev, >> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, i, >> + IIO_EV_TYPE_THRESH, >> + IIO_EV_DIR_FALLING), >> + iio_get_time_ns(aw9610x->aw_iio_dev)); >> + break; >> + default: >> + return; >> + } >> + aw9610x->channels_arr[i].last_channel_info = curr_status; >> + } >> +} >> + Kind regards, Wang Shuaijie
> > >> +static int aw9610x_cfg_all_loaded(const struct firmware *cont, > >> + struct aw9610x *aw9610x) > >> +{ > >> + struct aw_bin *aw_bin; > >> + int ret; > >> + > >> + if (!cont) > >> + return -EINVAL; > >> + > >> + aw_bin = kzalloc(cont->size + sizeof(*aw_bin), GFP_KERNEL); > >Use __free(kfree) > > > >lots of examples in tree, but will avoid need to manually free and > >simplify this code a little. > > > > I'm sorry, I didn't quite understand what you meant. Are you suggesting > the use of devm_? Could you please provide more detailed suggestions? > Thank you! No. Search for that string __free(kfree) and you will see what I mean. It provides scope based destructors. devm is about tying lifetime to device registration, here we need to tie it to this scope of this function. > > >> + if (!aw_bin) > >> + return -ENOMEM; > >> + > >> + aw_bin->len = cont->size; > >> + memcpy(aw_bin->data, cont->data, cont->size); > >> + aw9610x_parsing_bin_file(aw_bin); > >> + > >> + snprintf(aw9610x->chip_type, sizeof(aw9610x->chip_type), "%s", > >> + aw_bin->chip_type); > >> + ret = aw9610x_bin_valid_loaded(aw9610x, aw_bin); > >> + kfree(aw_bin); > >> + > >> + return ret; > >> +} > >> + > >> +static void aw9610x_cfg_work_routine(struct work_struct *work) > >> +{ > >> + struct aw9610x *aw9610x = container_of(work, struct aw9610x, > >> + cfg_work.work); > >> + > >> + aw9610x_cfg_update(aw9610x); > > > >So this is polling in driver. We'd normally hook up to a hrtimer > >trigger for that. Perhaps you need this for your events sampling though? > >If so that may be fine to do somewhat like this. I'm just not sure > >of the usecase currently. > > > > The primary objective of this delayed task is to load the register > configuration file. The chip needs to load the register configuration > file during power-on initialization. In cases where the driver is compiled > directly into the kernel, rather than existing as a dynamically loaded > module, there may be a situation where the driver attempts to load before > the file system is fully prepared, resulting in an inability to access the > register configuration file. I believe there are standard ways to handle this. the driver shouldn't be relying on local tricks to make this work. https://www.kernel.org/doc/html/v4.16/driver-api/firmware/fallback-mechanisms.html > Therefore, a delayed task mechanism is employed > to ensure the register configuration file is loaded properly. > > If there are any concerns about my understanding or approach, please feel > free to offer suggestions. Thank you very much! > > >> +} > >> +static void aw9610x_irq_handle(struct aw9610x *aw9610x) > >> +{ > >> + u32 curr_status_val; > >> + u32 curr_status; > >> + unsigned char i; > >> + int ret; > >> + > >> + ret = aw9610x_i2c_read(aw9610x, REG_STAT0, &curr_status_val); > >> + if (ret) > >> + return; > >> + > >> + for (i = 0; i < AW_CHANNEL_MAX; i++) { > >> + curr_status = (((curr_status_val >> (24 + i)) & 0x1)) | > >> + (((curr_status_val >> (16 + i)) & 0x1) << 1) | > >> + (((curr_status_val >> (8 + i)) & 0x1) << 2) | > >> + (((curr_status_val >> (i)) & 0x1) << 3); > > > >Add a comment on what is going on here as it's tricky to read. > >Also, no brackets around the i in last line. > >Probably better expressed as a series of FIELD_GET() calls with appropriat > >masks of the 32 bit value. > > > > > > The work processed here is to parse the interrupt status of different channels. > bit0/bit8/bit16/bit24 represent the interrupt status of channel 0, with each of > the 4 bits corresponding to an interrupt status for approaching a threshold. > Similarly, bit1/bit9/bit17/bit25 represent the interrupt status of channel 1. > To facilitate subsequent interrupt status judgments, the 4 interrupt statuses > of the same channel are combined into a single data. > > Sorry, I have not found a suitable way to utilize FIELD_GET for this purpose. That's fine. Just add a comment to say it is gathering up the status bits for a particular channel. > > >> + > >> + if (!aw9610x->channels_arr[i].used || > >> + (aw9610x->channels_arr[i].last_channel_info == > >> + curr_status)) > >Align as > > if (!aw > > (aw9610... > > > >> + continue; > >> + > >> + switch (curr_status) { > >> + case FAR: > >> + iio_push_event(aw9610x->aw_iio_dev, > >> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, i, > >> + IIO_EV_TYPE_THRESH, > >> + IIO_EV_DIR_RISING), > >> + iio_get_time_ns(aw9610x->aw_iio_dev)); > >> + break; > >> + case TRIGGER_TH0: > >> + case TRIGGER_TH1: > >> + case TRIGGER_TH2: > >> + case TRIGGER_TH3: > >4 thresholds on the same channel? This is confusing given we are reporting them > >as events on different channels. but this loop is over the channels. > > > > > > There are 4 proximity thresholds on the same channel, each representing > a different level of proximity. TRIGGER_TH0/TRIGGER_TH1/TRIGGER_TH2/TRIGGER_TH3 > all represent proximity states, but with varying degrees of proximity. > > Here I have a question to ask. I'm not sure how to use iio to report > different proximity states. Can you give me some suggestions? Thank you! This is a limitation of IIO and need to limit the scale of the event descriptor. We don't support more than one threshold of a given type per channel. It's an design decision based on the fact that for sensors, it is rarely useful to actually support multiple thresholds. If software has gotten an interrupt of the first one, it can read the channel value and if it likes modify the threshold. As such, what does having another level actually bring to a usecase? Lots of hardware supports multiple thresholds we just choose not to expose it. I might consider ways around that, but first I need to understand the user space usecase for it. Note there are lots of reasons to do this in baremetal / realtime systems where particular actions are triggered in a timely fashion. By the time you are dealing with a full Linux software stack most of those don't make sense any more because there is a lot more software processing in the loop. Jonathan > > >> + iio_push_event(aw9610x->aw_iio_dev, > >> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, i, > >> + IIO_EV_TYPE_THRESH, > >> + IIO_EV_DIR_FALLING), > >> + iio_get_time_ns(aw9610x->aw_iio_dev)); > >> + break; > >> + default: > >> + return; > >> + } > >> + aw9610x->channels_arr[i].last_channel_info = curr_status; > >> + } > >> +} > >> + > > Kind regards, > Wang Shuaijie
From: shuaijie wang <wangshuaijie@awinic.com> Add drivers that support Awinic aw9610x proximity sensors. The aw9610x series are high-sensitivity capacitive proximity detection sensors. This device detects human proximity and assists electronic devices in reducing specific absorption rate (SAR) to pass SAR related certifications. The device reduces RF power and reduces harm when detecting human proximity. Increase power and improve signal quality when the human body is far away. The specific absorption rate (SAR) is a metric that measures the degree of absorption of electromagnetic radiation emitted by wireless devices, such as mobile phones and tablets, by human tissue. This patch implements device initialization, registration, I/O operation handling and interrupt handling, and passed basic testing. v1->v2: ------- - Remove unnecessary log printing. - Optimize comment style. - Issues with modifying the device tree. - Optimize code style. v2->v3: ------- - Add a description about the hardware device. - Remove inappropriate configuration items. - Modify the formatting issues. - Modify the structure of the driver. - Change the style of the driver's comments. - Remove unnecessary log printing. - Modify the function used for memory allocation. - Modify the driver registration process. - Remove the functionality related to updating firmware. - Change the input subsystem in the driver to the iio subsystem. - Modify the usage of the interrupt pin. v3->v4: ------- The changes in this patch version are quite significant, and I concur with Krzysztof's viewpoint that this driver is indeed overly complex for the proximity sensor. Therefore, I have removed the compatibility for the aw963xx series, and the driver will now exclusively support the aw9610x series. - Modify the software architecture to remove compatibility for the aw963xx series. - Optimize the parsing of register configuration files (.bin). - Remove unnecessary log printing. - Delete redefinition of true and false. - Remove unnecessary interfaces. - Optimize regulator usage. - Convert the I2C communication interface to regmap. v4->v5: ------- - Solve errors that occur when executing the make dt_binding_check DT_SCHEMA_FILES. shuaijie wang (2): dt-bindings: iio: aw9610x: Add bindings for aw9610x sensor iio: proximity: aw9610x: Add support for aw9610x proximity sensor .../iio/proximity/awinic,aw9610x.yaml | 61 ++ drivers/iio/proximity/Kconfig | 11 + drivers/iio/proximity/Makefile | 1 + drivers/iio/proximity/aw9610x.c | 791 ++++++++++++++++++ drivers/iio/proximity/aw9610x.h | 140 ++++ 5 files changed, 1004 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/proximity/awinic,aw9610x.yaml create mode 100644 drivers/iio/proximity/aw9610x.c create mode 100644 drivers/iio/proximity/aw9610x.h base-commit: 1722389b0d863056d78287a120a1d6cadb8d4f7b