Re: [PATCH v2 2/5] media: v4l: ctrls: Add a control for temperature

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi everyone,

Just some little comments for my use case. Hope that helps.

On 20/04/2022 00:04, Guenter Roeck wrote:
> On 4/19/22 14:01, Laurent Pinchart wrote:
>> On Tue, Apr 19, 2022 at 12:28:06PM -0700, Guenter Roeck wrote:
>>> On 4/19/22 11:24, Nicolas Dufresne wrote:
>>>> Hi,
>>>>
>>>> adding linux-hwmon in CC for a wider feedback.
>>>>
>>>> Le vendredi 15 avril 2022 à 13:18 +0200, Benjamin Mugnier a écrit :
>>>>> Add V4L2_CID_TEMPERATURE control to get temperature from sensor in
>>>>> celsius as a volatile and read-only control, and its documentation.
>>>>> Useful to monitor thermals from v4l controls for sensors that support
>>>>> this.
>>>>
>>>> Any justification to expose a temperature sensor outside of the dedicated kernel
>>>> API hwmon ? I know if may makes it harder to use, as the sensor isn't bound to
>>>> the camera driver, and also the sensor may not work if the camera is not
>>>> streaming. But in the end, the API in hwmon does not look that complex, and is
>>>> perhaps more precise ?

This sensor is able to read the temperature even if not streaming.

>>>>
>>>> All in all, I think we need a strong justification to implement a custom
>>>> thermometer interface, something described here and documented with care to
>>>> prevent abuse. I would also see opinion from folks outside of the linux-media,
>>>> hence why I have CCed hwmon mailing list.
>>>
>>> All I can say is that this seems to be odd and a bit outside the scope of
>>> v4l2. I would have expected the vgxy61 driver to register a hwmon device
>>> instead.
>>
>> I don't have a definitive opinion yet, but as Nicolas raised the issue
>> by pushing towards hwmon, I'll offer counter-arguments for the sake of
>> it :-)
>>
>> The temperature sensor in the imaging sensor is used to measure the die
>> temperature, in order to tune the noise and spectral response model of
>> the imaging sensor. It's thus not a generic-purpose temperature sensor.
>> If the feature were to be exposed through hwmon, userspace would need to
>> associate an hwmon device to the imaging sensor V4L2 subdev (we have a
>> way to do so through the MC API, it doesn't support hwmon at this point,
>> but I suppose it could be added). There are also various constraints
>> that tie the temperature reading to the imaging side, it could be that
>> the temperature would only be readable while capturing frames. That's
>> probably possible to handle too but returning an error from the
>> temperature read.
>>
>> Code-wise, both the driver and userspace would be more complex, for very
>> little practical gain (I don't dispute a theorical gain).
>>
> 
> All I can say is - not my subsystem, not my call to make. If you say this
> is special and is better handled in V4L2, I'll take you by your word.
> 
> Guenter
> 

I'll happily implement whatever conclusion we make here.

I could also drop this control for the first iteration of the driver, and come back later once a consensus is reached.

Benjamin



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux