Hi Benjamin, 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 ? -- Regards, Laurent Pinchart
