Message-ID: <20240810111637.3280eff6@jic23-huawei>
Date: Sat, 10 Aug 2024 11:16:37 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: wangshuaijie@...nic.com
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org, kangjiajun@...nic.com,
 krzk+dt@...nel.org, lars@...afoo.de, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, liweilei@...nic.com, robh@...nel.org,
 waqar.hameed@...s.com
Subject: Re: [PATCH V5 2/2] iio: proximity: aw9610x: Add support for aw9610x
 proximity sensor


> 
> >> +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

Powered by blists - more mailing lists