linux-kernel - Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8aa11758-5eee-427b-adcb-c3578a521daf@linaro.org>
Date:   Mon, 20 Nov 2023 15:04:07 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Petre Rodan <petre.rodan@...dimension.ro>
Cc:     linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, Conor Dooley <conor+dt@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        Jonathan Cameron <jic23@...nel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: iio: pressure: add honeywell,hsc030

On 20/11/2023 14:42, Petre Rodan wrote:

>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - honeywell,hsc
>>
>> Way too generic
> 
> I'm new to this, please excuse my ignorance.
> my driver covers all Honeywell pressure sensors under the "TruStability board mount HSC/SSC" moniker.

We talk here about bindings, not driver. For the driver you can use
whatever name is approved by reviewers of your driver.

> that is why my intention was to provide a rather generic name for the driver itself.
> are you afraid that they will come up with a different device that they will call "hsc" in the future?
> in this case honeywell,trustability-hsc would be fine?
> 
> as I see you prefer to target a particular chip, but I am a bit afraid that the end-user will be confused by needing to set up something like
> 
> pressure@28 {
> 	compatible = "honeywell,hsc030pa";

The compatible should be specific, thus for example match exact model
number.

If you can guarantee that all devices from given family are the same in
respect of programming model and hardware requirements (e.g. supplies),
then you could go with family name. However such guarantees are rarely
given. Therefore for mprls0025pa I agreed for using one specific model
for entire family.

https://lore.kernel.org/all/d577bc44-780f-f25d-29c6-ed1d353b540c@linaro.org/


> 	reg = <0x28>;
> 	honeywell,transfer-function = <0>;
> 	honeywell,pressure-range = "250MD";
> };
> 
> ie. specifying "hsc030pa" as driver while his chip is not in the 030PA range, but 250MD.
> 
> so do you prefer
>  honeywell,trustability-hsc  OR
>  honeywell,hsc030pa

I think the latter, just like we did for mprls0025pa. How many devices
do you have there?


> 
>>> +  honeywell,range_str:
>>
>> No underscores in property names.
>>
>> "str" is redundant. Instead say what is it, because "range" is way too
>> vague.
> 
> will rename to honeywell,pressure-range if that is ok with you.

Yes

> 
>>> +    description: |
>>> +      Five character string that defines "pressure range, unit and type"
>>> +      as part of the device nomenclature. In the unlikely case of a custom
>>> +      chip, set to "NA" and provide honeywell,pmin-pascal honeywell,pmax-pascal
>>> +    enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 010BA, 1.6MD, 2.5MD, 004MD,
>>> +           006MD, 010MD, 016MD, 025MD, 040MD, 060MD, 100MD, 160MD, 250MD,
>>> +           400MD, 600MD, 001BD, 1.6BD, 2.5BD, 004BD, 2.5MG, 004MG, 006MG,
>>> +           010MG, 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG,
>>> +           600MG, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 010BG, 100KA, 160KA,
>>> +           250KA, 400KA, 600KA, 001GA, 160LD, 250LD, 400LD, 600LD, 001KD,
>>> +           1.6KD, 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD,
>>> +           100KD, 160KD, 250KD, 400KD, 250LG, 400LG, 600LG, 001KG, 1.6KG,
>>> +           2.5KG, 004KG, 006KG, 010KG, 016KG, 025KG, 040KG, 060KG, 100KG,
>>> +           160KG, 250KG, 400KG, 600KG, 001GG, 015PA, 030PA, 060PA, 100PA,
>>> +           150PA, 0.5ND, 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND,
>>> +           001PD, 005PD, 015PD, 030PD, 060PD, 001NG, 002NG, 004NG, 005NG,
>>> +           010NG, 020NG, 030NG, 001PG, 005PG, 015PG, 030PG, 060PG, 100PG,
>>> +           150PG, NA]
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +  honeywell,pmin-pascal:
>>> +    description: |
>>> +      Minimum pressure value the sensor can measure in pascal.
>>> +      To be specified only if honeywell,range_str is set to "NA".
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>
>> That's uint32. Why do you need negative values?
> 
> signed int32 is intentional. some chips have two physical input ports and measure a pressure differential in which case pmin is negative.
> see either of the pdfs at page 14, table 8, column 2, row 7+

Then the best would be to change the type in other bindings to have
int32 everywhere... but dtschema also requires unt32:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

I think pressure can be negative (e.g. when device measures pressure in
relation to atmospheric pressure), thus maybe we need to fix dtschema first.

> 
>>> +  honeywell,pmax-pascal:
>>> +    description: |
>>> +      Maximum pressure value the sensor can measure in pascal.
>>> +      To be specified only if honeywell,range_str is set to "NA".
>>> +    $ref: /schemas/types.yaml#/definitions/int32
>>
>> Ditto
> 
> well, since we saw pmin needs to be signed should we have pmax unsigned?

I guess this could stay uint32, although it depends on final units for
pascal.

Best regards,
Krzysztof