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@xxxxxxxxx> > 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��.n��������+%������w��{.n�����{��(��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
