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

[Jakob Hauser <jahau@rocketmail.com>]

Hi Andy,

On 30.07.22 13:32, Andy Shevchenko wrote:
> On Sat, Jul 30, 2022 at 1:10 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> On 29.07.22 19:24, Andy Shevchenko wrote:
>>> On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> 
> ..
> 
>>>> 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));
> 
> Looks like that, yes.
> 
>>         ret = regmap_bulk_write(yas5xx->map, YAS537_ADCCAL, &buf, 2);
> 
> sizeof(buf)

OK

>>         if (ret)
>>                 return ret;
>>
>> The whole block would then look like:
>>
>>         /* Writing ADCCAL and TRM registers */
>>         __be16 buf = cpu_to_be16(GENMASK(9, 3));
> 
> (Taking into account that definitions are at the top of the function it would be
> 
>   __be16 buf;
>   ...
>   buf = cpu_to_be16(...);

Thanks for the details, I'll implement it like this.

>>         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.
> 
> It points out that there is a SMPLTIM, which I decode as Sample Time,
> which is in 4.1 msec steps (up to 255 steps).

Also thanks for this interpretation, that makes sense. Then the
denominator consists of factor 1000 to convert microseconds back to
milliseconds and a factor of 4.1 milliseconds per step. The value
"intrvl", which is written into the YAS537_MEASURE_INTERVAL register,
would then be the number of steps of the sample time.

However, I wouldn't add anything of this into the driver as a comment or
as a name, because we're just guessing.

> 
> ..
> 
>>>> 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?
> 
> You use formula x * 1000 to convert milliseconds to microseconds. My
> suggestion is to replace 1000 with MILLI which adds information about
> exponent sign, i.e. 10^-3 (which may be important to the reader).

Hm, ok, but the MILLI has to be defined. Or is it predefined somewhere?
I couldn't find any examples.

To my interpretation, it would look like this (upper part showing the
location of the define, lower part the formula):

    ...
    #define YAS537_LCK_MASK_GET             GENMASK(3, 0)
    #define YAS537_OC_MASK_GET              GENMASK(5, 0)

    #define MILLI                           1000

    /* Turn off device regulators etc after 5 seconds of inactivity */
    #define YAS5XX_AUTOSUSPEND_DELAY_MS     5000

    enum chip_ids {
            ...
    };

    ...

    intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * MILLI
             - YAS537_MEASURE_TIME_WORST_US) / 4100;

I think the define and the formula both look strange.

Kind regards,
Jakob