Re: [PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Gerald Loacker <gerald.loacker@wolfvision.net>]

Am 15.11.2022 um 18:43 schrieb Jonathan Cameron:
> On Tue, 15 Nov 2022 08:37:17 +0100
> Gerald Loacker <gerald.loacker@wolfvision.net> wrote:
> 
>> Add bindings documentation file for TI TMAG5273.
>>
>> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
>> ---
>>  .../iio/magnetometer/ti,tmag5273.yaml         | 72 +++++++++++++++++++
>>  MAINTAINERS                                   |  6 ++
>>  2 files changed, 78 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
>> new file mode 100644
>> index 000000000000..2f5b0a4d2f40
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
>> @@ -0,0 +1,72 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: https://eur04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fiio%2Fmagnetometer%2Fti%2Ctmag5273.yaml%23&amp;data=05%7C01%7Cgerald.loacker%40wolfvision.net%7C9788e9788f344fcff9b808dac730f926%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638041310400330990%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=nczO1QC74gD6eGXAkm%2B6LRrc7fyEsr62r%2B3aoW%2Bcfu4%3D&amp;reserved=0
>> +$schema: https://eur04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=05%7C01%7Cgerald.loacker%40wolfvision.net%7C9788e9788f344fcff9b808dac730f926%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638041310400330990%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=raKUiSntfhvdSSnqiR1Wm%2Fqr9cI3XEu5HCprqvISlLE%3D&amp;reserved=0
>> +
>> +title: TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor
>> +
>> +maintainers:
>> +  - Gerald Loacker <gerald.loacker@wolfvision.net>
>> +
>> +description:
>> +  The TI TMAG5273 is a low-power linear 3D Hall-effect sensor. This device
>> +  integrates three independent Hall-effect sensors in the X, Y, and Z axes.
>> +  The device has an integrated temperature sensor available. The TMAG5273
>> +  can be configured through the I2C interface to enable any combination of
>> +  magnetic axes and temperature measurements. An integrated angle calculation
>> +  engine (CORDIC) provides full 360° angular position information for both
>> +  on-axis and off-axis angle measurement topologies. The angle calculation is
>> +  performed using two user-selected magnetic axes.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: '^magnetometer@[0-9a-f]+$'
> 
> What Krzysztof said on this ;)
> 
>> +
>> +  compatible:
>> +    const: ti,tmag5273
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#io-channel-cells":
>> +    const: 1
>> +
>> +  ti,angle-enable:
>> +    description:
>> +      Enables angle measurement in the selected plane.
>> +      0 = OFF
>> +      1 = X-Y (default)
>> +      2 = Y-Z
>> +      3 = X-Z
> 
> This feels like something we should be configuring at runtime rather that
> DT, or is it driven by board design or similar?
> 

We use this sensor for a zoom wheel application, there is an EVM from TI
for this as well. So this is for setting the mounting position of the wheel.

>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
>> +    maximum: 3
>> +
>> +  vcc-supply:
>> +    description:
>> +      A regulator providing 1.7 V to 3.6 V supply voltage on the VCC pin,
>> +      typically 3.3 V.
>> +
> 
> The dt binding should attempt to describe the hardware, not what we happen
> to support in the driver so far. So I'd expect to also see an interrupt.
> That way if someone ships a dts file today, and we enable it sometime in the
> future they will be ready for it.
>

Is it fine to add just the description without example then? The
interrupt has many options such as low active or low pulse.

>> +required:
>> +  - compatible
>> +  - reg
>> +  - vcc-supply
> 
> Ah. This is presumably as side effect of having the driver set the voltage.
> Normally we only need to 'require' a supply if we need to read it's voltage
> (for scaling on ADCs and similar). That's not the case here so I wouldn't
> expect to see it.
> 
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c-0 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        magnetometer@35 {
>> +            compatible = "ti,tmag5273";
>> +            reg = <0x35>;
>> +            #io-channel-cells = <1>;
>> +            ti,angle-enable = <3>;
>> +            vcc-supply = <&vcc3v3>;
>> +        };
>> +    };
>> +...
> 
> 

Thanks for the review,
Gerald