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

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

[Jakob Hauser]

Hi Andy,

On 10.06.22 16:31, Andy Shevchenko wrote:
> I understand that Linus knows well this code and may review this, but
> can you please split register renaming (at least, maybe something else
> can be split as well as preparatory change) to the separate patch?

That makes sense, I'll move the renaming into a separate patch.

>> +                               regmap_read(yas5xx->map, i, &val);
>> +                               dev_dbg(yas5xx->dev, "register 0x%02x: %u\n",
>> +                                       i, val);
> 
> Please, drop all these value reads/writes debug messages, they are
> quite expensive (by resource consuming), noisy (may spam logs), and
> most important duplicative. regmap API has tracepoints, use them!
> 
> Perhaps it would require an additional patch to clean this up, if
> anything like this is present in the current code base..

Ok, I'll remove those direct regmap reads in yas537_dump_calibration().

However, I'd like to keep the others. The calibration data is dumped
before [1] and after [2] being processed by the driver. This is helpful
to check if it was processed correctly. Dumping the data is done only
once at probing.

In yas537_dump_calibration(), I'd also like to keep dumping the
"hard_offsets". Currently there is no linearization formula known for
YAS537. Providing the "hard_offsets" may help to find a way.

[1]
https://github.com/torvalds/linux/blob/v5.18/drivers/iio/magnetometer/yamaha-yas530.c#L592
[2]
https://github.com/torvalds/linux/blob/v5.18/drivers/iio/magnetometer/yamaha-yas530.c#L678-L699

Kind regards,
Jakob

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

[Andy Shevchenko]

On Sat, Jun 11, 2022 at 1:57 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> On 10.06.22 16:31, Andy Shevchenko wrote:

...

> >> +                               dev_dbg(yas5xx->dev, "register 0x%02x: %u\n",
> >> +                                       i, val);
> >
> > Please, drop all these value reads/writes debug messages, they are
> > quite expensive (by resource consuming), noisy (may spam logs), and
> > most important duplicative. regmap API has tracepoints, use them!
> >
> > Perhaps it would require an additional patch to clean this up, if
> > anything like this is present in the current code base..
>
> Ok, I'll remove those direct regmap reads in yas537_dump_calibration().

I'm not sure I understand what you are going to drop. I was talking
about debug messages, the regmap reads are fine. Or you are talking
about them as they are tightened together and one makes no sense
without the other?

> However, I'd like to keep the others. The calibration data is dumped
> before [1] and after [2] being processed by the driver. This is helpful
> to check if it was processed correctly. Dumping the data is done only
> once at probing.

Then it should be probably dev_info() in such cases.

> In yas537_dump_calibration(), I'd also like to keep dumping the
> "hard_offsets". Currently there is no linearization formula known for
> YAS537. Providing the "hard_offsets" may help to find a way.

I understand that, but per se this is not for production esp. taking
into account that regmap has a tracepoint mechanism.

-- 
With Best Regards,
Andy Shevchenko

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

[Jakob Hauser]

Hi Andy,

(Side note: There is something wrong with the Cc list in your e-mail,
the list of addresses isn't handled correctly.)

On 11.06.22 12:56, Andy Shevchenko wrote:
> On Sat, Jun 11, 2022 at 1:57 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> Ok, I'll remove those direct regmap reads in yas537_dump_calibration().
> 
> I'm not sure I understand what you are going to drop. I was talking
> about debug messages, the regmap reads are fine. Or you are talking
> about them as they are tightened together and one makes no sense
> without the other?

Yes on your question. In the new function yas537_dump_calibration() I
added some regmap reads that are performed only because of debug
messages. I'll remove those regmap reads incl. the corresponding debug
messages. However, I'd like to keep the "other" debug messages.

I'll explain more detailed.

Generally speaking on YAS530/532:
At driver probe, calibration "raw" data is read from the calibration
register (currently YAS5XX_CAL). Within the driver, this data gets
processed into several calibration coefficients like e.g. c->a2. These
are later on applied on the measure data.

Calibration debug on YAS530/532:
After reading the calibration "raw" data from the register (for further
processing), we dump that "raw" data to debug. After having "calculated"
the calibration coefficients like c->a2 within the driver, we dump them
to debug too.

Generally speaking on YAS537 version 0:
There are two versions of YAS537. Version 0 reads calibration data from
the register (YAS537_CAL) and writes it directly back into some other
registers. The driver doesn't touch anything. This is done in new
function yas537_get_calibration_data() after "case YAS537_VERSION_0:".

Generally speaking on YAS537 version 1:
Version 1 of YAS537 is a mixture. Some of the data being read from the
calibration register (YAS537_CAL) is directly written back to some other
registers. But additionally, some calibration coefficients like c->a2
need to be "calculated". This happens in new function
yas537_get_calibration_data() after "case YAS537_VERSION_1:". The
calibration coefficients will be applied on the measure data later on.

Calibration debug on YAS537:
In the new function yas537_dump_calibration(), I implemented that
mixture also. Firstly, I added some regmap reads and debug dumps of the
registers where some of the calibration data was written into. Secondly,
for YAS537 version 1, I dumped the "calculated" calibration coefficients
like c->a2.

What I intend to change for patchset v2:
In the new function yas537_dump_calibration(), I'll remove the two "for
loops" (they contain the "unnecessary" regmap reads and debug dumps).
But I'd like to keep the debug dump of the "calculated" calibration
coefficients after the "for loops".

>> However, I'd like to keep the others. The calibration data is dumped
>> before [1] and after [2] being processed by the driver. This is helpful
>> to check if it was processed correctly. Dumping the data is done only
>> once at probing.
> 
> Then it should be probably dev_info() in such cases.

To my understanding, in this case it's rather debug. If at some point it
turns out that the driver doesn't work correctly, it allows a deeper
insight on the intermediate steps the driver is doing internally.

>> In yas537_dump_calibration(), I'd also like to keep dumping the
>> "hard_offsets". Currently there is no linearization formula known for
>> YAS537. Providing the "hard_offsets" may help to find a way.
> 
> I understand that, but per se this is not for production esp. taking
> into account that regmap has a tracepoint mechanism.

Well, ok, I'll drop the "hard_offsets" debug dump.

Kind regards,
Jakob

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

[Andy Shevchenko]

On Sat, Jun 11, 2022 at 4:00 PM Jakob Hauser <jahau@rocketmail.com> wrote:
>
> Hi Andy,
>
> (Side note: There is something wrong with the Cc list in your e-mail,
> the list of addresses isn't handled correctly.)

The list is the same as in your mail. Dunno what exactly is the
problem you are referring to?

> On 11.06.22 12:56, Andy Shevchenko wrote:
> > On Sat, Jun 11, 2022 at 1:57 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> >> Ok, I'll remove those direct regmap reads in yas537_dump_calibration().
> >
> > I'm not sure I understand what you are going to drop. I was talking
> > about debug messages, the regmap reads are fine. Or you are talking
> > about them as they are tightened together and one makes no sense
> > without the other?
>
> Yes on your question. In the new function yas537_dump_calibration() I
> added some regmap reads that are performed only because of debug
> messages. I'll remove those regmap reads incl. the corresponding debug
> messages. However, I'd like to keep the "other" debug messages.
>
> I'll explain more detailed.
>
> Generally speaking on YAS530/532:
> At driver probe, calibration "raw" data is read from the calibration
> register (currently YAS5XX_CAL). Within the driver, this data gets
> processed into several calibration coefficients like e.g. c->a2. These
> are later on applied on the measure data.
>
> Calibration debug on YAS530/532:
> After reading the calibration "raw" data from the register (for further
> processing), we dump that "raw" data to debug. After having "calculated"
> the calibration coefficients like c->a2 within the driver, we dump them
> to debug too.
>
> Generally speaking on YAS537 version 0:
> There are two versions of YAS537. Version 0 reads calibration data from
> the register (YAS537_CAL) and writes it directly back into some other
> registers. The driver doesn't touch anything. This is done in new
> function yas537_get_calibration_data() after "case YAS537_VERSION_0:".
>
> Generally speaking on YAS537 version 1:
> Version 1 of YAS537 is a mixture. Some of the data being read from the
> calibration register (YAS537_CAL) is directly written back to some other
> registers. But additionally, some calibration coefficients like c->a2
> need to be "calculated". This happens in new function
> yas537_get_calibration_data() after "case YAS537_VERSION_1:". The
> calibration coefficients will be applied on the measure data later on.
>
> Calibration debug on YAS537:
> In the new function yas537_dump_calibration(), I implemented that
> mixture also. Firstly, I added some regmap reads and debug dumps of the
> registers where some of the calibration data was written into. Secondly,
> for YAS537 version 1, I dumped the "calculated" calibration coefficients
> like c->a2.
>
> What I intend to change for patchset v2:
> In the new function yas537_dump_calibration(), I'll remove the two "for
> loops" (they contain the "unnecessary" regmap reads and debug dumps).
> But I'd like to keep the debug dump of the "calculated" calibration
> coefficients after the "for loops".

Sounds reasonable to me.

> >> However, I'd like to keep the others. The calibration data is dumped
> >> before [1] and after [2] being processed by the driver. This is helpful
> >> to check if it was processed correctly. Dumping the data is done only
> >> once at probing.
> >
> > Then it should be probably dev_info() in such cases.
>
> To my understanding, in this case it's rather debug. If at some point it
> turns out that the driver doesn't work correctly, it allows a deeper
> insight on the intermediate steps the driver is doing internally.

OK!

> >> In yas537_dump_calibration(), I'd also like to keep dumping the
> >> "hard_offsets". Currently there is no linearization formula known for
> >> YAS537. Providing the "hard_offsets" may help to find a way.
> >
> > I understand that, but per se this is not for production esp. taking
> > into account that regmap has a tracepoint mechanism.
>
> Well, ok, I'll drop the "hard_offsets" debug dump.

-- 
With Best Regards,
Andy Shevchenko

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

[Jakob Hauser]

Hi Andy,

On 11.06.22 22:22, Andy Shevchenko wrote:
>
> On Sat, Jun 11, 2022 at 4:00 PM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> (Side note: There is something wrong with the Cc list in your e-mail,
>> the list of addresses isn't handled correctly.)
> 
> The list is the same as in your mail. Dunno what exactly is the
> problem you are referring to?

It somehow got confused by the colon after "open list" and the tilde
before "postmarketos". Everything in between got interpreted as the
name/description of the postmarketos mailing list. Your first reply at
2022-06-10 14:31 didn't make it to the devicetree list and likely not to
Hans. Since then, these addresses show up twice in the thread, once for
real and once still interpreted as name/description of the postmarketos
mailing list.

The thread in the devicetree list:
https://lore.kernel.org/linux-devicetree/CAHp75VfV1E1WaLVDG3V9460AsVxahb9Epod-63T35eDV+h8Ubg@mail.gmail.com/T/#t

On the patchset, I'll send v2 probably at Sunday or Monday.

Kind regards,
Jakob