Hi Andy,
On 29.07.22 19:24, Andy Shevchenko wrote:
> On Fri, Jul 29, 2022 at 1:13 AM Jakob Hauser <jahau@xxxxxxxxxxxxxx> 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
