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

[Jakob Hauser <jahau@rocketmail.com>]

Hi Andy,

On 27.07.22 19:46, Andy Shevchenko wrote:
> On Wed, Jul 27, 2022 at 12:13 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> On 04.07.22 21:47, Andy Shevchenko wrote:
>>> On Mon, Jul 4, 2022 at 12:05 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> 
> ..
> 
>>>> +       /* Write registers according to Android driver */
>>>
>>> Would be nice to elaborate in the comment what exactly the flow is
>>> here, like "a) setting value of X;  b) ...".
>>
>> Unfortunately, I don't know more than what the code already says. In the
>> Yamaha Android driver, this part looks like this:
>> https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L345-L350
>>
>> The comment "Write registers according to Android driver" actually says
>> "I don't know what I'm doing here, this is copy-paste from Android".
>>
>> I can remove the comment if you find it inappropriate. Though from my
>> point of view the comment is ok.
> 
> The comment is okay for you, but for me it needs elaboration. So,
> something like above in compressed format (couple of short sentences
> to explain that nobody knows what the heck is that) will be
> appreciated.

I was thinking about:

/*
 * Write registers according to Android driver, the exact meaning
 * is unknown
 */

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".

Accordingly, I would choose the following comment here:

/* Writing ADCCAL and TRM registers */

>>>> +       ret = regmap_write(yas5xx->map, YAS537_ADCCAL, GENMASK(1, 0));
>>>> +       if (ret)
>>>> +               return ret;
>>>> +       ret = regmap_write(yas5xx->map, YAS537_ADCCAL + 1, GENMASK(7, 3));
>>>> +       if (ret)
>>>> +               return ret;
>>>
>>> Can bulk write be used here?
>>
>> Technically yes. But in this case I strongly would like to keep the
>> single regmap_write as we go through different registers step by step
>> and write them. That way it's much better readable. And it's just these
>> two that are neighboring each other. As this happens in
>> yas537_power_on(), it isn't done very often, thus doesn't cost much
>> resources.
> 
> You seems 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.

Not all the registers are properly named in the original driver. E.g.
there is a register called "YAS537_REG_MTCR" [1] with no names for the
following registers. Further down, this and the following 11 registers
are written by just counting up the register number [2].

It looks similar to the situation at register "YAS537_REG_ADCCALR",
where the numerically following register (0x92) doesn't have a name [3].
It could be because of a 16-bit register, as you say, but it could also
be a naming thing.

At the location in discussion, the original driver uses two single
writes [4]. I'd stick to that.

[1]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L38
[2]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L277-L279
[3]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L37
[4]
https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L345-L348

>>>> +       /* The interval value is static in regular operation */
>>>> +       intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000
>>>
>>> MILLI ?
>>
>> What do you mean by this comment?
>>
>> The suffixes _MS and _US were proposed by you in v2. I think they are fine.
> 
> 1000 --> MILLI ?
> 
> 10^-3 sec * 10^-3 = 10^-6 sec.

Ah, well, the full formula is ...

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

... with the fixed defined values:

        #define YAS537_DEFAULT_SENSOR_DELAY_MS	50
        #define YAS537_MEASURE_TIME_WORST_US	1500

So it means ...

        intrvl = (50 milliseconds * 1000 - 1500 microseconds) / 4100;

... which is:

        intrvl = (50000 microseconds - 1500 microseconds) / 4100;

The units of "4100" and "intrvl" are unclear. I don't know if "intrvl"
is a time or some abstract value.

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

>>>> -       ret = yas5xx->chip_info->measure_offsets(yas5xx);
>>>> -       if (ret)
>>>> -               goto assert_reset;
>>>
>>>> +       if (yas5xx->chip_info->measure_offsets) {
>>>
>>> This can be done when you introduce this callback in the chip_info
>>> structure, so this patch won't need to shuffle code again. I.o.w. we
>>> can reduce ping-pong development in this series.
>>
>> I did this change in this patch on purpose because it's the introduction
>> of YAS537 variant that's causing this change. YAS537 is the first
>> variant that doesn't use yas5xx->chip_info->measure_offsets.
>>
>> Shall I move it to patch 9 nonetheless?
> 
> It's a bit hard to answer yes or no, I think after you try to resplit,
> we will see what is the best for this part.

Hm... to avoid discussions and shorten iterations, I'll move it to the
newly "add function pointers" patch in v5. I'll add a comment on this in
the commit message of that patch.

...

Kind regards,
Jakob