Re: [PATCH] iio: hid: Add temperature sensor support

["Pandruvada, Srinivas" <srinivas.pandruvada@intel.com>]

On Sat, 2017-02-04 at 12:02 +0000, Jonathan Cameron wrote:
> On 04/02/17 13:47, Song Hongyan wrote:
> > 
> > Environmental temperature sensor is a hid defined sensor,
> > it measures temperature.
> > 
> > More information can be found in:
> > http://www.usb.org/developers/hidpage/HUTRR39b.pdf
> > 
> > According to IIO ABI definition, IIO_TEMP data output unit is
> > milli degrees Celsius. Add the unit convert from degree to milli
> > degree.
> > 
> > Signed-off-by: Song Hongyan <hongyan.song@intel.com>
> A few bits and bobs inline.  Clearly I'll want Srinivas to comment on
> this
> one as well!


[...]

> 
+/* Capture samples in local storage */
> > +static int temperature_capture_sample(struct hid_sensor_hub_device
> > *hsdev,
> > +				unsigned int usage_id,
> > +				size_t raw_len, char *raw_data,
> > +				void *priv)
> > +{
> > +	struct iio_dev *indio_dev = platform_get_drvdata(priv);
> Why not make priv the indio_dev directly? I would imagine it's the
> pdev
> passed in when registering the callbacks below.

Actually this priv can be renamed to pdev. The callback will be called
with platform_device * of the mfd device.

> > 
> > +	struct temperature_state *temperature_state =
> > iio_priv(indio_dev);
> > +
> > +	switch (usage_id) {
> > +	case HID_USAGE_SENSOR_DATA_ENVIRONMENTAL_TEMPERATURE:
> > +		temperature_state->temperature_data = *(s32
> > *)raw_data;
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > 

[...]

> > +/* Function to initialize the processing for usage id */
> > +static int hid_temperature_probe(struct platform_device *pdev)
> > +{
> > +	static const char *name = "temperature";
> > +	struct iio_dev *indio_dev;
> > +	struct temperature_state *temperature_state;
> > +	struct hid_sensor_hub_device *hsdev = pdev-
> > >dev.platform_data;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&pdev->dev,
> > +				sizeof(struct temperature_state));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	temperature_state = iio_priv(indio_dev);
> > +	temperature_state->common_attributes.hsdev = hsdev;
> > +	temperature_state->common_attributes.pdev = pdev;
> > +
> > +	ret = hid_sensor_parse_common_attributes(hsdev,
> > +					HID_USAGE_SENSOR_TEMPERATU
> > RE,
> > +					&temperature_state-
> > >common_attributes);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->channels = devm_kmemdup(&indio_dev->dev,
> > +					temperature_channels,
> > +					sizeof(temperature_channel
> > s),
> > +					GFP_KERNEL);
> > +	if (!indio_dev->channels)
> > +		return -ENOMEM;
> > +
> > +	ret = temperature_parse_report(pdev, hsdev,
> > +				(struct iio_chan_spec *)indio_dev-
> > >channels,
> > +				HID_USAGE_SENSOR_TEMPERATURE,
> > +				temperature_state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->num_channels =
> > ARRAY_SIZE(temperature_channels);
> > +	indio_dev->dev.parent = &pdev->dev;
> > +	indio_dev->info = &temperature_info;
> > +	indio_dev->name = name;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	ret = iio_triggered_buffer_setup(indio_dev,
> > &iio_pollfunc_store_time,
> > +					NULL, NULL);
> > +	if (ret)
> > +		return ret;
> If you are going all devm (which is fine) then use
> devm_iio_triggered_buffer_setup.
> 
> > 
> > +
> > +	atomic_set(&temperature_state-
> > >common_attributes.data_ready, 0);
> > +	ret = hid_sensor_setup_trigger(indio_dev, name,
> > +				&temperature_state-
> > >common_attributes);
> I'd expect to see this unwound somewhere as well..

I missed this during my review. There has to be call
to  hid_sensor_remove_trigger() to do opposite.

> > 
> > +	if (ret)
> > +		goto error_unreg_buffer_funcs;
> > +
> > +	ret = devm_iio_device_register(indio_dev->dev.parent,
> > indio_dev);
> > +	if (ret)
> > +		goto error_unreg_buffer_funcs;
> Is there a race here in that userspace interfaces are exposed, but
> the
> hid system has no way to send answers to any queries?
Two scenarios possible:
- If somehow user space calls the raw read before the 
sensor_hub_register_callback() is called, then there should not be any
issue as the sensor_hub_input_attr_get_raw_value is called with
"SENSOR_HUB_SYNC" so wait till response arrives or timeout. So doesn't
depend on the callback.

- If user is activating the buffer mode before the callback us
registered, worst case it will miss event till the callback is
registered.

But we can call sensor_hub_register_callback()
before devm_iio_device_register(), but the we need to make sure that
the sensor_hub_remove_callback() is called
if devm_iio_device_register() fails.

> > 
> > +
> > +	platform_set_drvdata(pdev, indio_dev);
> > +
> > +	temperature_state->callbacks.send_event =
> > temperature_proc_event;
> > +	temperature_state->callbacks.capture_sample =
> > +						temperature_captur
> > e_sample;
> > +	temperature_state->callbacks.pdev = pdev;
> > +	ret = sensor_hub_register_callback(hsdev,
> > HID_USAGE_SENSOR_TEMPERATURE,
> > +					&temperature_state-
> > >callbacks); 
> > +	return ret;
> > +
> > +error_unreg_buffer_funcs:
> > +	iio_triggered_buffer_cleanup(indio_dev);
> > +	return ret;
> > +}
> > +
> > +/* Function to deinitialize the processing for usage id */
> > +static int hid_temperature_remove(struct platform_device *pdev)
> > +{
> > +	struct hid_sensor_hub_device *hsdev = pdev-
> > >dev.platform_data;
> > +
> > +	sensor_hub_remove_callback(hsdev,
> > HID_USAGE_SENSOR_TEMPERATURE);

hid_sensor_remove_trigger()

> > +
> > +	return 0;
> > +}
> > +
> > 
Thanks,
Srinivas