Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant

[Jakob Hauser <jahau@rocketmail.com>]

Hi Andy,

On 29.07.22 19:24, Andy Shevchenko wrote:
> On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> On 27.07.22 19:46, Andy Shevchenko wrote:
> 
> ..
> 
>> /*
>>  * Write registers according to Android driver, the exact meaning
>>  * is unknown
> 
> With a period at the end :-)
> 
>>  */
> 
>> This reminded me of another location where I first had a comment
>> "Writing SRST register, the exact meaning is unknown". There you
>> criticized the part "the exact meaning is unknown", so I changed it to
>> simply "Writing SRST register".
> 
> Yeah, but that is different, SRST seems like easy to deduce to "soft
> reset" (taking into account where it's programmed in the run flow).
> 
>> Accordingly, I would choose the following comment here:
>>
>> /* Writing ADCCAL and TRM registers */
> 
> Fine with me!

OK, I'll apply the comment "Writing ADCCAL and TRM registers".

> 
> ..
> 
>>> You seem to program the 16-bit register with a single value, I don't
>>> think it's a good idea to split a such. When it's a bulk write and
>>> value defined with __be16 / __le16 it makes much more clear what
>>> hardware is and what it expects.
>>
>> We don't know for sure whether it is a 16-bit register or an incomplete
>> register naming.
> 
> By the values you write into it seems to be a __be16 calibration
> register. The value to write is 0x3f8 which might ring a bell to you
> if you know what other values related to ADC.

Sigh, ok, I'll apply bulk write.

How to do it correctly? I guess:

        __be16 buf = cpu_to_be16(GENMASK(9, 3));
        ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);
        if (ret)
                return ret;

The whole block would then look like:

        /* Writing ADCCAL and TRM registers */
        __be16 buf = cpu_to_be16(GENMASK(9, 3));
        ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);
        if (ret)
                return ret;
        ret = regmap_write(yas5xx->map, YAS537_TRM, GENMASK(7, 0));
        if (ret)
                return ret;

...

> To the 4100 denominator:
> https://github.com/XPerience-AOSP-Lollipop/android_kernel_wingtech_msm8916/blob/xpe-11.1/drivers/input/misc/yas_mag_drv-yas537.c#L235,
> seems you can find a lot by browsing someone's code and perhaps a Git
> history.

I've seen that comment before but I don't understand its meaning.

>> Still I didn't get your comment. Is your intention to change the "50
>> milliseconds * 1000" to "50000 microseconds" in the define?
>>
>> It would look like ...
>>
>>         #define YAS537_DEFAULT_SENSOR_DELAY_US  50000
>>
>> ... though I would prefer to keep current define, as it is implemented
>> now and stated above:
>>
>>         #define YAS537_DEFAULT_SENSOR_DELAY_MS  50
> 
> No, just to show in the actual calculation that you convert MS to US
> using MILLI.

Sorry, I still don't get what you want me to do. What do you mean by
"using MILLI", can you elaborate?

Kind regards,
Jakob