Re: [PATCH v3 1/4] iio: hid-sensor-als: Assign channels dynamically

[Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>]

Hi Jonathan,

On 1/13/24 08:03, Jonathan Cameron wrote:
> On Tue,  9 Jan 2024 10:00:04 -0800
> Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
>
> > Instead of assuming that every channel defined statically by
> > als_channels[] is present, assign dynamically based on presence of the
> > respective usage id in the descriptor. This will allow to register ALS
> > with limited channel support. Append the timestamp as the last channel.
> >
> > When not all usage ids are present, the scan index is adjusted to
> > exclude unsupported channels.
> >
> > There is no intentional function changes done.
> >
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > ---
> > v3:
> > Addressed comments from Jonthan:
> > 	- Remove channel allocation and move to iio_priv()
> > 	- Parse all usage IDs in a single loop and continue
> > 	for failure. This way the temperature and chromaticity
> > 	will not need any special processing to parse usage ids.
> > 	- Don't leave empty channel indexes
> There isn't really a problem if you did want to leave them.
> There are a number of other devices that do leave gaps and userspace
> code shouldn't mind that.  I don't mind them being more tightly packed
> though so this is also fine.
Submitted v4 leaving gaps.
> I was just looking at the driver and noticed one other oddity.
> In als_capture_sample, it always returns -EINVAL if the timestamp channel
> is being queried.

Correct. But hid-sensor-hub core ignores return value, so it doesn't 
cause any issue.

But I sent a separate patch to fix this.


> > v2:
> > New change
> >
> >   drivers/iio/light/hid-sensor-als.c | 56 +++++++++++++++++++++---------
> >   1 file changed, 39 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> > index 5cd27f04b45e..72a7c01c97f8 100644
> > --- a/drivers/iio/light/hid-sensor-als.c
> > +++ b/drivers/iio/light/hid-sensor-als.c
> > @@ -25,17 +25,26 @@ struct als_state {
> >   	struct hid_sensor_hub_callbacks callbacks;
> >   	struct hid_sensor_common common_attributes;
> >   	struct hid_sensor_hub_attribute_info als[CHANNEL_SCAN_INDEX_MAX];
> > +	struct iio_chan_spec channels[CHANNEL_SCAN_INDEX_MAX + 1];
> >   	struct {
> >   		u32 illum[CHANNEL_SCAN_INDEX_MAX];
> > +		u32 scan_index[CHANNEL_SCAN_INDEX_MAX];
> This looks unlikely to end up right with a varying number of channels.
> I'm assuming the device always captures all available channels.
> As such, if there are 2 channels the timestamp will end up at byte 8 onwards.
> If there are 3 channels, byte 16 onwards etc.
>
> Usually when we have a floating timestamp like this we just rely on expanding
> the channel array to leave space and force the alignment of that to be suitable
> for taking a timestamp.
>
> I'm a bit confused not to see available_scan_mask being set though.
> If userspace previously requested illumination only would the data
> have ended up in the right location (offset 0?)
Add this.
> If you set available_scan_masks to specify all channels then the
> IIO core will move things around for you. If it's not provided then
> it is up to the driver to figure out where to put the data.
>
> Otherwise the patch looks fine to me.

Please check v4, if it solve the above issues.

Thanks,

Srinivas



> Jonathan
>
>
>
> >   		u64 timestamp __aligned(8);
> >   	} scan;
> >   	int scale_pre_decml;
> >   	int scale_post_decml;
> >   	int scale_precision;
> >   	int value_offset;
> > +	int num_channels;
> >   	s64 timestamp;
> >   };
> >   
> > +/* The order of usage ids must match scan index starting from CHANNEL_SCAN_INDEX_INTENSITY */
> > +static const u32 als_usage_ids[] = {
> > +	HID_USAGE_SENSOR_LIGHT_ILLUM,
> > +	HID_USAGE_SENSOR_LIGHT_ILLUM,
> > +};
> > +
> >   static const u32 als_sensitivity_addresses[] = {
> >   	HID_USAGE_SENSOR_DATA_LIGHT,
> >   	HID_USAGE_SENSOR_LIGHT_ILLUM,
> > @@ -216,11 +225,14 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
> >   	struct als_state *als_state = iio_priv(indio_dev);
> >   	int ret = -EINVAL;
> >   	u32 sample_data = *(u32 *)raw_data;
> > +	int scan_index;
> >   
> >   	switch (usage_id) {
> >   	case HID_USAGE_SENSOR_LIGHT_ILLUM:
> > -		als_state->scan.illum[CHANNEL_SCAN_INDEX_INTENSITY] = sample_data;
> > -		als_state->scan.illum[CHANNEL_SCAN_INDEX_ILLUM] = sample_data;
> > +		scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_INTENSITY];
> > +		als_state->scan.illum[scan_index] = sample_data;
> > +		scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_ILLUM];
> > +		als_state->scan.illum[scan_index] = sample_data;
> >   		ret = 0;
> >   		break;
> >   	case HID_USAGE_SENSOR_TIME_TIMESTAMP:
> > @@ -237,27 +249,39 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
> >   /* Parse report which is specific to an usage id*/
> >   static int als_parse_report(struct platform_device *pdev,
> >   				struct hid_sensor_hub_device *hsdev,
> > -				struct iio_chan_spec *channels,
> >   				unsigned usage_id,
> >   				struct als_state *st)
> >   {
> > -	int ret;
> > +	struct iio_chan_spec *channels;
> > +	int ret, index = 0;
> >   	int i;
> >   
> > +	channels = st->channels;
> > +
> >   	for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
> >   		ret = sensor_hub_input_get_attribute_info(hsdev,
> >   						HID_INPUT_REPORT,
> >   						usage_id,
> > -						HID_USAGE_SENSOR_LIGHT_ILLUM,
> > +						als_usage_ids[i],
> >   						&st->als[i]);
> >   		if (ret < 0)
> > -			return ret;
> > -		als_adjust_channel_bit_mask(channels, i, st->als[i].size);
> > +			continue;
> > +
> > +		channels[index] = als_channels[i];
> > +		st->scan.scan_index[i] = index;
> > +
> > +		als_adjust_channel_bit_mask(channels, index, st->als[i].size);
> > +		++index;
> >   
> >   		dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
> >   			st->als[i].report_id);
> >   	}
> >   
> > +	st->num_channels = index;
> > +	/* Return success even if one usage id is present */
> > +	if (index)
> > +		ret = 0;
> > +
> >   	st->scale_precision = hid_sensor_format_scale(usage_id,
> >   				&st->als[CHANNEL_SCAN_INDEX_INTENSITY],
> >   				&st->scale_pre_decml, &st->scale_post_decml);
> > @@ -293,15 +317,7 @@ static int hid_als_probe(struct platform_device *pdev)
> >   		return ret;
> >   	}
> >   
> > -	indio_dev->channels = devm_kmemdup(&pdev->dev, als_channels,
> > -					   sizeof(als_channels), GFP_KERNEL);
> > -	if (!indio_dev->channels) {
> > -		dev_err(&pdev->dev, "failed to duplicate channels\n");
> > -		return -ENOMEM;
> > -	}
> > -
> >   	ret = als_parse_report(pdev, hsdev,
> > -			       (struct iio_chan_spec *)indio_dev->channels,
> >   			       hsdev->usage,
> >   			       als_state);
> >   	if (ret) {
> > @@ -309,8 +325,14 @@ static int hid_als_probe(struct platform_device *pdev)
> >   		return ret;
> >   	}
> >   
> > -	indio_dev->num_channels =
> > -				ARRAY_SIZE(als_channels);
> > +	/* Add timestamp channel */
> > +	als_state->channels[als_state->num_channels] = als_channels[CHANNEL_SCAN_INDEX_TIMESTAMP];
> > +	als_state->channels[als_state->num_channels].scan_index = als_state->num_channels;
> > +
> > +	/* +1 for adding timestamp channel */
> > +	indio_dev->num_channels = als_state->num_channels + 1;
> > +
> > +	indio_dev->channels = als_state->channels;
> >   	indio_dev->info = &als_info;
> >   	indio_dev->name = name;
> >   	indio_dev->modes = INDIO_DIRECT_MODE;