Re: [PATCH v5 14/14] 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 v5 14/14] iio: magnetometer: yas530: Add YAS537 variant
Date: Wed, 10 Aug 2022 01:41:24 +0200	[thread overview]
Message-ID: <850de937-8fe6-ebce-4e9d-35a617c3ade6@rocketmail.com> (raw)
In-Reply-To: <CAHp75VegA6zzvQXEi_9-K3832o+j48Af3X8LivYY47Xav+w-xQ@mail.gmail.com>

Hi Andy,

On 08.08.22 13:47, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 1:12 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> This adds support for the magnetometer Yamaha YAS537. The additions are based
> 
> Add support

OK

>> on comparison of Yamaha Android kernel drivers for YAS532 [1] and YAS537 [2].
>>
>> In the Yamaha YAS537 Android driver, there is an overflow/underflow control
>> implemented. For regular usage, this seems not necessary. A similar overflow/
>> underflow control of Yamaha YAS530/532 Android driver isn't integrated in the
>> mainline driver. It is therefore skipped for YAS537 in mainline too.
> 
> the mainline

OK

>> Also in the Yamaha YAS537 Android driver, at the end of the reset_yas537()
>> function, a measurement is saved in "last_after_rcoil". Later on, this is
>> compared to current measurements. If the difference gets too big, a new
>> reset is initialized. The difference in measurements needs to be quite big,
>> it's hard to say if this is necessary for regular operation. Therefore this
>> isn't integrated in the mainline driver either.
> 
> ..
> 
>>         help
>>           Say Y here to add support for the Yamaha YAS530 series of
>> -         3-Axis Magnetometers. Right now YAS530, YAS532 and YAS533 are
>> -         fully supported.
>> +         3-Axis Magnetometers. YAS530, YAS532, YAS533 and YAS537 are
>> +         supported.
> 
> So, after this change the rest become partially supported?
> 
> Perhaps you want to leave the original and add a new sentence like:
> 
>   "The YAS537 is partially supported."
> 
> ?

All four variants are fully supported. I changed the sentence according
to Jonathan's suggestion in v3:
https://lore.kernel.org/linux-iio/cover.1655509425.git.jahau@rocketmail.com/T/#m6ba6b576cdf6f86f550f7598fcf7c408ac5f2d46

> 
> ..
> 
>> - * For YAS532/533, this value is known from the Android driver. For YAS530,
> 
> It seems this comma is unneeded in the original comment.

I'll remove the comma here and in patch 12, where it was introduced.

>> - * it was approximately measured.
>> + * For YAS532/533, this value is known from the Android driver. For YAS530
>> + * and YAS537, it was approximately measured.
> 
> P.S. Do you see now how your series and the end result become better?

The driver improves. Though we kind of get lost in details, I have the
impression we could go on like this forever.

Kind regards,
Jakob