Message-ID: <20250418170107.2fa397e7@jic23-huawei>
Date: Fri, 18 Apr 2025 17:01:07 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: <Victor.Duicu@...rochip.com>
Cc: <Marius.Cristea@...rochip.com>, <andy@...nel.org>,
 <dlechner@...libre.com>, <linux-iio@...r.kernel.org>, <nuno.sa@...log.com>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/2] dt-bindings: iio: temperature: add support for
 MCP998X

On Thu, 17 Apr 2025 11:40:53 +0000
<Victor.Duicu@...rochip.com> wrote:

> On Tue, 2025-04-15 at 18:52 +0100, Jonathan Cameron wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Tue, 15 Apr 2025 16:26:22 +0300
> > <victor.duicu@...rochip.com> wrote:
> >   
> > > From: Victor Duicu <victor.duicu@...rochip.com>
> > > 
> > > This is the devicetree schema for Microchip MCP998X/33 and
> > > MCP998XD/33D Multichannel Automotive Temperature Monitor Family.  
> > Hi Victor,
> >   
> Hi Jonathan,
> 
> > Please state briefly here in what way the parts are incompatible
> > as a justification for no fallback compatibles.  Quite a bit
> > of that will become apparent when you enforce validity of parameters
> > as suggested below.
> >   
> I am a bit confused, could you elaborate a bit on this point? Are you
> asking if the chips mcp9982, 83, 84 etc. are compatible among each
> other?

yes. It makes it easier for binding review to just have statement in the
patch description of how they are different in ways the driver needs
to know about.  Can be simple things like 'only some devices have X'
or they have differing numbers of channels.


> 
> 
> > Various comments inline.  
> > > 
> > > Signed-off-by: Victor Duicu <victor.duicu@...rochip.com>
> > > ---  
> >   
> ...
> >   
> > > +
> > > +  microchip,extended-temp-range:
> > > +    description: |
> > > +      Set the chip to work in the extended temperature range -64
> > > degrees C to 191.875 degrees C.
> > > +      Omit this tag to set the default range 0 degrees C to
> > > 127.875 degrees C
> > > +    type: boolean  
> > 
> > I'm curious.  Why does this belong in the DT binding?
> >   
> 
> Regarding microchip,extended-temp-range, my perspective is that the
> user knows beforehand which specific range of temperatures he needs.
> For example, if the device to be measured is a freezer, the user would
> be interested in temperatures below 0 degrees C. If we monitor a CPU,
> the user would be interested in temperatures above 0 degrees C.

Maybe - though also easy to control from userspace by making the
offset writeable.
> 
> > > +
> > > +  microchip,beta-channel1:
> > > +    description: |
> > > +      The beta compensation factor for external channel 1 can be
> > > set
> > > +      by the user, or can be set automatically by the chip.
> > > +      If one wants to enable beta autodetection, omit this tag.
> > > +      Please consult the documentation if one wants to set a
> > > specific beta.
> > > +      If anti-parallel diode operation is enabled, the default
> > > value is set
> > > +      and can't be changed.
> > > +    type: boolean  
> > 
> > Why is this a hardware thing that belongs in dt?  Enforce the
> > constraint
> > in the schema rather than text.
> >   
> 
> With respect to the beta parameter, it is directly affected by the
> hardware part used. For example a CPU diode would require a different
> beta (that could be known by the manufacturer of the device and not
> know by the final user) as opposed to a diode connected transistor
> (that could be easily measured by the end user).
> 
> However, I remain open to the idea of moving temperature range and
> channel betas to user space if you think it is better that way.

For the betas I was more curious about why the dt needs to distinguish
between a manual setting and autodetection.  When is autodetection
a bad idea for example?

Jonathan

> 
> Kind regards,
> Victor Duicu
> ...
> > >   
> >   
> 

Powered by blists - more mailing lists