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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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.

...

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

...

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

...

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

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

-- 
With Best Regards,
Andy Shevchenko