On 20/04/2022 16:23, Laurent Pinchart wrote: > On Wed, Apr 20, 2022 at 06:58:32AM -0700, Guenter Roeck wrote: >> On 4/20/22 06:21, Laurent Pinchart wrote: >>> On Wed, Apr 20, 2022 at 03:01:18PM +0200, Benjamin Mugnier wrote: >>>> 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. >>> >>> That would work too. By the way, what are your use cases for the >>> temperature sensor ? Have you added the control for the sake of >>> completeness, or do you have use cases ? >> >> You provided a use case above. Are you saying you made it up ? >> Still fine with me, your call, just wondering. > > It's the two most common use cases for imaging sensor temperature > measurements that I know of. My question to Benjamin is if he has the > same and/or other use cases. > Just like you said in a previous mail. This temperature sensor can be used to implement a retroactive loop from the host according to its value, such as noise correction for instance. We don't have anything in the Linux user space that implements this yet, this was in anticipation. So dropping it is fine, I will come back to it if need be ;)
