Re: [PATCH v2 1/3] dt-bindings: thermal: sophgo,cv180x-thermal: Add Sophgo CV180x thermal

[Conor Dooley]

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
> > 
> > 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?
> 
> The chop-period is an ADC parameter.
> 
> 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
> > 
> > Can we just make the number of ticks the unit here, and above?
> > Also, a "oneOf: - const" structure is just an enum.
> 
> 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?
> 
> 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.

Attachment: signature.asc
Description: PGP signature