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

From: Jakob Hauser <jahau@rocketmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Linus Walleij <linus.walleij@linaro.org>,
	Hans de Goede <hdegoede@redhat.com>,
	linux-iio <linux-iio@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	phone-devel@vger.kernel.org,
	~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH v4 10/10] iio: magnetometer: yas530: Add YAS537 variant
Date: Wed, 27 Jul 2022 00:13:45 +0200	[thread overview]
Message-ID: <c5643cee-8491-3fae-b9d4-ac03d86c1e8a@rocketmail.com> (raw)
In-Reply-To: <CAHp75Ve5Z9OOx94FtXY77_Fwkp27D32Hn7bAG+Da++HXmupytA@mail.gmail.com>

Hi Andy,

On 04.07.22 21:47, Andy Shevchenko wrote:
> On Mon, Jul 4, 2022 at 12:05 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>

...

>>         yas530,
>>         yas532,
>>         yas533,
>> +       yas537,
>>  };
>>
>>  static const char yas5xx_product_name[][13] = {
>>         "YAS530 MS-3E",
>>         "YAS532 MS-3R",
>> -       "YAS533 MS-3F"
>> +       "YAS533 MS-3F",
> 
> This is exactly the point why it's good practice to add comma for
> non-terminator entries.
> 
>> +       "YAS537 MS-3T"
> 
> ..here.
> 
>>  };

Yes, makes sense.

...

>> +       /* 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.

...

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

...

>> +       /* 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.

...

>> -       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?

>> +               ret = yas5xx->chip_info->measure_offsets(yas5xx);
>> +               if (ret)
>> +                       goto assert_reset;
>> +       }
> 

Kind regards,
Jakob