Received-SPF: pass (google.com: domain of linux-kernel+bounces-204765-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 6 Jun 2024 18:05:30 +0100
From: Conor Dooley <conor@kernel.org>
To: Haylen Chu <heylenay@outlook.com>
Cc: Inochi Amaoto <inochiama@outlook.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Zhang Rui <rui.zhang@intel.com>, Lukasz Luba <lukasz.luba@arm.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Chen Wang <unicorn_wang@outlook.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Jisheng Zhang <jszhang@kernel.org>, linux-pm@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-riscv@lists.infradead.org
Subject: Re: [PATCH v2 1/3] dt-bindings: thermal: sophgo,cv180x-thermal: Add
 Sophgo CV180x thermal
Message-ID: <20240606-reaction-thirsting-8c22d1b5ab72@spud>
References: <SG2PR01MB4218013241B3EED779D3BAE8D7F82@SG2PR01MB4218.apcprd01.prod.exchangelabs.com>
 <SG2PR01MB42189977B4172405F5704CC4D7F82@SG2PR01MB4218.apcprd01.prod.exchangelabs.com>
 <IA1PR20MB49531F55C8D7DC5D0050CAF9BBF92@IA1PR20MB4953.namprd20.prod.outlook.com>
 <20240605-tightwad-janitor-82cfceb1469d@spud>
 <SEYPR01MB4221F0E46F600E013974F21BD7FA2@SEYPR01MB4221.apcprd01.prod.exchangelabs.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="xNgNBLWP/RMLxW5/"
Content-Disposition: inline
In-Reply-To: <SEYPR01MB4221F0E46F600E013974F21BD7FA2@SEYPR01MB4221.apcprd01.prod.exchangelabs.com>


--xNgNBLWP/RMLxW5/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 06, 2024 at 01:32:46PM +0000, Haylen Chu wrote:
> On Wed, Jun 05, 2024 at 06:54:17PM +0100, Conor Dooley wrote:
> > > > +  accumulation-period:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    description: Accumulation period for a sample
> > > > +    oneOf:
> > > > +      - const: 0
> > > > +        description: 512 ticks
> > > > +      - const: 1
> > > > +        description: 1024 ticks
> > > > +      - const: 2
> > > > +        description: 2048 ticks
> > > > +      - const: 3
> > > > +        description: 4096 ticks
> > > > +    default: 2
> > > > +
> > > > +  chop-period:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    description: ADC chop period
> >=20
> > What's a "chop" and why is either this or the accumulation-period a
> > fixed property of the hardware? Shouldn't this choice really be up to
> > the user?
>=20
> The chop-period is an ADC parameter.
>=20
> Both accumulation-period and chop-period specify how the sensor
> measures temperature. Making these parameters up to end users brings
> extra unnecessary code complexity. Being configurable for each board
> should be enough and other thermal drivers have been doing things in
> this way.

Other systems may well have properties for this, but something being
done in the past doesn't mean it might be the right thing to do now.
I don't really buy that this is something you set to a fixed value per
board, but rather the use case of a particular board would factor into
whether or not you would want to use a shorter or longer accumulation
period.

> > > > +    oneOf:
> > > > +      - const: 0
> > > > +        description: 128 ticks
> > > > +      - const: 1
> > > > +        description: 256 ticks
> > > > +      - const: 2
> > > > +        description: 512 ticks
> > > > +      - const: 3
> > > > +        description: 1024 ticks
> >=20
> > Can we just make the number of ticks the unit here, and above?
> > Also, a "oneOf: - const" structure is just an enum.
>=20
> I do not catch your idea. These values directly map to raw register
> configuration, which simplify the implementation a lot.

It should be trivial to convert them to register values in your driver.

> > > > +    default: 3
> > > > +
> > > > +  sample-cycle-us:
> > > > +    description: Period between samples
> > > > +    default: 1000000
> > No constraints?
>=20
> Sample cycle is more flexible because of hardware designing.

It quite likely has constraints, flexible or not. Is the hardware
capable of both 1 us and uint32_max us?

Thanks,
Conor.

--xNgNBLWP/RMLxW5/
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZmHsWgAKCRB4tDGHoIJi
0vG7AP48IalU1RGZ/KbtaGu/vPghK4eP4Nk9f7t4VJwKdTosBwD9F3Yrkl2G2Ylr
82IeEwZmoQQrxmHthTZEV+dAIf6BJQk=
=4835
-----END PGP SIGNATURE-----

--xNgNBLWP/RMLxW5/--