Hi Jonathan,
On Sat, 27 Jul 2024 16:02:16 +0100, jic23@xxxxxxxxxx wrote:
>On Fri, 26 Jul 2024 06:13:12 +0000
>wangshuaijie@xxxxxxxxxx wrote:
>
>> From: shuaijie wang <wangshuaijie@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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
