Re: [PATCH v3 05/11] thermal: armada: Add support for Armada AP806

From: Miquel RAYNAL <miquel.raynal@free-electrons.com>
To: Baruch Siach <baruch@tkos.co.il>
Cc: Mark Rutland <mark.rutland@arm.com>, Andrew Lunn <andrew@lunn.ch>,
	Jason Cooper <jason@lakedaemon.net>,
	Nadav Haklai <nadavh@marvell.com>,
	devicetree@vger.kernel.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Antoine Tenart <antoine.tenart@free-electrons.com>,
	linux-pm@vger.kernel.org, Will Deacon <will.deacon@arm.com>,
	Eduardo Valentin <edubezval@gmail.com>,
	David Sniatkiwicz <davidsn@marvell.com>,
	Rob Herring <robh+dt@kernel.org>,
	Gregory CLEMENT <gregory.clement@free-electrons.com>,
	Zhang Rui <rui.zhang@intel.com>,
	Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
	linux-arm-kernel@lists.infradead.org,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Subject: Re: [PATCH v3 05/11] thermal: armada: Add support for Armada AP806
Date: Mon, 18 Dec 2017 13:25:00 +0100	[thread overview]
Message-ID: <20171218132500.7f7225e9@xps13> (raw)
In-Reply-To: <20171218111156.76a66q5kwh3advs7@sapphire.tkos.co.il>

Hi Baruch,

On Mon, 18 Dec 2017 13:11:56 +0200
Baruch Siach <baruch@tkos.co.il> wrote:

> Hi Miquèl,
> 
> On Mon, Dec 18, 2017 at 10:41:27AM +0100, Miquel RAYNAL wrote:
> > Hello Gregory & Baruch,
> > 
> > On Thu, 14 Dec 2017 12:05:43 +0100
> > Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
> >   
> > > > @@ -184,9 +214,9 @@ static int armada_get_temp(struct
> > > > thermal_zone_device *thermal, div = priv->data->coef_div;
> > > >  
> > > >  	if (priv->data->inverted)
> > > > -		*temp = ((m * reg) - b) / div;
> > > > +		*temp = ((m * sample) - b) / div;
> > > >  	else
> > > > -		*temp = (b - (m * reg)) / div;
> > > > +		*temp = (b - (m * sample)) / div;
> > > >  	return 0;
> > > >  }
> > > >  
> > > > @@ -237,6 +267,19 @@ static const struct armada_thermal_data
> > > > armada380_data = { .inverted = true,
> > > >  };
> > > >  
> > > > +static const struct armada_thermal_data armada_ap806_data = {
> > > > +	.is_valid = armada_is_valid,
> > > > +	.init_sensor = armada_ap806_init_sensor,
> > > > +	.is_valid_bit = BIT(16),
> > > > +	.temp_shift = 0,
> > > > +	.temp_mask = 0x3ff,
> > > > +	.coef_b = -150000,    
> > > 
> > > Don't you expect any side effect by storing a negative value in a
> > > unsigned variable?  
> > 
> > That is a fair question, I did not spot that.
> > 
> > As other values are really close to 2^32 I don't know what is the
> > best option for us in this case. Should I:
> > - don't care?
> > - use signed values? (dangerous IMHO)
> > - use a union with a signed and an unsigned value? (problem moved
> > to ->get_temp())  
> 
> Another option is to use s64 type.

I prefer this one!

Thank you,
Miquèl

> 
> baruch
> 

-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel