Re: [PATCH v2 7/7] iio: magnetometer: yas530: Add YAS537 variant

[Jakob Hauser <jahau@rocketmail.com>]

Hi Andy,

On 13.06.22 17:22, Andy Shevchenko wrote:
>
> Forgot to add that please try to split even more preparatory patches.
> For example, you may convert existing code to stubs / switch-cases /
> etc and in the last patch just add the new case or new function /
> branch.

That's a good idea. I had a closer look. But I can't spot something that
could be beneficial.

The new functions for YAS537 are similar in structure like their
counterparts of YAS530/YAS532. However, in detail they differ too much,
which hinders merging them into each other.

yas537_measure() being different to yas530_532_measure():
 - regmap_write() has an additional bit YAS5XX_MEASURE_CONT
 - regmap_read_poll_timeout() differs in the read location
 - calculating the values t, x, y1 and y2 are different

yas537_get_measure() being different to yas530_532_get_measure():
 - no linearization
 - no temperature compensation
 - different way of calculating x, y, z from x, y1, y2

yas537_get_calibration_data() being different to
yas530.../yas532_get_calibration_data():
 - one regmap_bulk_read() only
 - different way of getting or extracting the calibration data

yas537_power_on()
 - different procedure and registers

The function yas537_dump_calibration() shares a notable part with
yas530_532_dump_calibration() after the changes applied in patchset v2.
Although merging them together would need some "if" or "switch"
statements because YAS537 version 0 needs to be excluded from that
function and YAS537 version 1 would need to be excluded from the first
and last part of that function. I would leave it like it is, it's easier
readable.

Another approach could be to outsource some parts, which are used by all
variants, into separate functions. But again I don't see much beneficial
pars here.

In yas537_measure() and yas530_532_measure() it could be:
 - regmap_bulk_read()

In yas537_get_calibration_data() and
yas530.../yas532_get_calibration_data() it could be:
 - add_device_randomness()

I think that's it. Both are too small for being worth outsourcing into a
separate function.

Kind regards,
Jakob