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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart