Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure

[Jakob Hauser <jahau@rocketmail.com>]

Hi Andy,

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

...

> Thanks for a new version, it's getting better. My comments below.
> 
> But first of all, can you split this to at least two patches, i.e.
> 1) split out functions without introducing chip->info yet;
> 2) adding chip_info.

The only function that's split out newly is yas5xx_calc_temperature().
However, it uses "yas5xx->chip" to look up the right values in the array
for "t_ref" and "min_temp_x10". So it cannot be easily split from the
introduction to the chip_info approach.

> Possible alternative would be more steps in 2), i.e. introducing
> chip_info for the callback only, then add field (or semantically
> unified fields) by field with corresponding changes in the code. In
> this case it would be easier to review.

What do you mean by "introducing chip_info for the callback only"? I
guess you mean the function pointers?

According to this, the patch could be split into:
1) introduce function pointers
2) introduce arrays to look up values

However, what's missing there is the introduction of the chip_info
approach as such. Admittedly difficult to follow are the changes in
yas5xx_probe():
 - yas5xx->devid gets removed, it was the main switch decider before
 - (for usage on other switches it is moved to yas5xx->chip_info->devid)
 - yas5xx->chip as an enumeration is newly introduced
 - yas5xx->chip_info as a structure gets initiated
 - As the chip_info now chooses according to the i2c_device_id (via
   enum chip_ids, thus yas5xx->chip), a new check is introduced to
   make sure it matches the device ID read from the register. This
   is done by "if (id_check != yas5xx->chip_info->devid)".
 - The way of reporting product name and version name was changed
   because introducing chip_info required to do this in one place
   for all versions.
 - As a consequence of this, yas5xx->name became obsolete.

This would have to be done before introducing function pointers and
arrays, like:
1) introduce chip_info structural changes
2) add function pointers
3) add arrays to look up values

But it can't be easily split that way. I can't establish an "empty"
chip_info approach as a fist step without removing or changing many
other things. The structural changes are related to other changes in
that patch.

I'm thinking about first introducing chip_info and thereafter
establishing the function pointers. In between I could try to split out
the new temperature function:
1) introduce chip_info structural changes incl. arrays to look up values
2) split out new yas5xx_calc_temperature() function
3) add function pointers

I'm not yet sure it can be split that way because things are entangled.
But I will try to this in v5.

Btw, looking through this, I realized that in patch 6 "Rename functions
and registers" I unfortunately missed to rename instances of
"yas5xx_get_measure", "yas5xx_power_on", "YAS5XX_ACTUATE_INIT_COIL" and
"YAS5XX_MEASURE". I'll correct this in v5.

...

>> -#define YAS530_20DEGREES               182 /* Counts starting at -62 °C */
> 
>> -#define YAS532_20DEGREES               390 /* Counts starting at -50 °C */
> 
> The comments suddenly disappear from the file. See below.

The comment's didn't actually disappear but rather got restructured by
introducing new chip_info approach. See below.

>> +enum chip_ids {
>> +       yas530,
>> +       yas532,
>> +       yas533,
>> +};
>> +
>> +static const char yas5xx_product_name[][13] = {
>> +       "YAS530 MS-3E",
>> +       "YAS532 MS-3R",
>> +       "YAS533 MS-3F"
>> +};
>> +
>> +static const char yas5xx_version_name[][2][3] = {
>> +       { "A", "B" },
>> +       { "AB", "AC" },
>> +       { "AB", "AC" }
> 
> Shan't we put indices here?

Do you mean:

        static const char yas5xx_version_name[][2][3] = {
                [yas530] = { "A", "B" },
                [yas532] = { "AB", "AC" },
                [yas533] = { "AB", "AC" },
        };

I can add this.

> Also, use * instead of one dimension of array.

Sorry, probably I lack the basic knowledge here. Can you explain how to
do that?

...

>> +static const int yas530_volatile_reg[] = {
>> +       YAS530_ACTUATE_INIT_COIL,
>> +       YAS530_MEASURE
> 
> + Comma.

OK, I didn't know this is allowed. I'll add it here and at the other
locations.

>> +/* Number of counts between minimum and reference temperature */
>> +const u16 t_ref_counts[] = { 182, 390, 390 };
>> +
>> +/* Starting point of temperature counting in 1/10:s degrees Celsius */
>> +const s16 min_temp_celcius_x10[] = { -620, -500, -500 };
> 
> See above.

The former comments...
    182 /* Counts starting at -62 °C */
    390 /* Counts starting at -50 °C */

... and the two new comments above the arrays actually should say the
same thing. At least that was my intention.

The first value is a number of counts. And the counting starts at a
certain temperature, which is the second value. Both the old and the new
comments are intended to describe this.

In the introduction of this temperature handling (patch 4), there is a
lot more description on these values in function yas5xx_get_measure().
When creating the new "chip_info" patch 9, I was thinking about moving
some of that description up here to these arrays. However, instead I
tried to following Jonathan's suggestion in v3 to keep the describing
text low and rather let the formulas speak for themselves. These values
are applied at a formula in function yas5xx_calc_temperature() which is
supposed to by kind of self explanatory.

What may lead to confusion is the equivalent usage of "starting" and
"minimum" here. In the initial patchset I used "starting" related to the
counts, Jonathan suggested "minimum" or actually "min_temp" related to
the temperature. The comments here above are bit of a mixture of both. I
still think it's good enough to understand. The sentence "Starting point
of temperature ..." describes the value min_temp_celcius_x10. Using a
term like "starting temperature" would probably be more confusing.

>> +struct yas5xx_chip_info {
>> +       unsigned int devid;
> 
>> +       const int *volatile_reg;
>> +       const int volatile_reg_qty;
>> +       const u32 scaling_val2;
> 
> Why const here?
> I assume entire structure is const, no?

I'm rather new to C language, not having a good grasp of "const" in
structures yet. I would have guessed it doesn't work well with the
function pointers.

However, having some compile tests, there are no complaints about the
function pointers.

To change the whole chip_info structure to "const", I would:
 - within the "struct yas5xx" change to "const struct
   yas5xx_chip_info *chip_info;"
 - change following typedef to "static const struct
   yas5xx_chip_info yas5xx_chip_info_tbl[] = ..."

Then, within the "struct yas5xx_chip_info", I can remove "const" from:
 - int volatile_reg_qty;
 - u32 scaling_val2;

However, I have to keep "const" at the following, otherwise the complier
complains that "initialization discards 'const' qualifier from pointer
target type":
 - const int *volatile_reg;

Summarizing, after the changes it would look like the following (snippets):

        struct yas5xx_chip_info {
                unsigned int devid;
                const int *volatile_reg;
                int volatile_reg_qty;
                u32 scaling_val2;
                int (*get_measure)(struct yas5xx *yas5xx, s32 *to,
                                   s32 *xo, s32 *yo, s32 *zo);
                int (*get_calibration_data)(struct yas5xx *yas5xx);
                void (*dump_calibration)(struct yas5xx *yas5xx);
                int (*measure_offsets)(struct yas5xx *yas5xx);
                int (*power_on)(struct yas5xx *yas5xx);
        };

        struct yas5xx {
                struct device *dev;
                enum chip_ids chip;
                const struct yas5xx_chip_info *chip_info;

        	...

        };

        static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
                [yas530] = {

        		...

                },
        };

If that's reasonable, I'll change it that way.

...

>> +       int i, j;
> 
> j can have a proper name.

Ok, makes sense. I'll probably name it "reg_qty".

>> +       j = yas5xx->chip_info->volatile_reg_qty;
> 

...

>> -       int ret;
>> +       int id_check, ret;
> 
> Don't add variables with different semantics on the same line.

Ok. I wasn't aware of putting different semantics on different lines,
thanks for the hint, makes sense.

>> +       if (id_check != yas5xx->chip_info->devid) {
>>                 ret = -ENODEV;
>> -               dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid);
>> +               dev_err(dev, "device ID %02x doesn't match %s\n",
>> +                       id_check, id->name);
> 
> ret = dev_err_probe() ?

Makes sense, will change that.

Though I have difficulties to implement this nicely. dev_err_probe()
requires an "error value to test". The current "if (id_check !=
yas5xx->chip_info->devid)" doesn't offer such a value by itself.

Do you think the following would be appropriate, nesting the check
within the dev_err_probe()? It doesn't look too good to me.

        ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid,
                            "device ID %02x doesn't match %s\n",
                            id_check, id->name);
        if (ret)
                goto assert_reset;

If there are no better ideas, I would implement it that way.
Additionally adding a comment and putting it into a block with the
regmap_read before:

        /*
         * Check if the device ID from the register matches the one set
         * in the Device Tree.
         */
        ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check);
        if (ret)
                goto assert_reset;
        ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid,
                            "device ID %02x doesn't match %s\n",
                            id_check, id->name);
        if (ret)
                goto assert_reset;

Hm, I think it's a bit hard to read. Suggestions for improvement are
welcome. Otherwise I'd add it that way.

...

>> +       dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip],
>> +                yas5xx_version_name[yas5xx->chip][yas5xx->version]);
> 
> I'm wondering if these arrays can be actually embedded into chip_info?

While the variants (like "YAS530") are listed in the chip_info, the
versions (like "A") are not. Yet, including the versions in chip_info
would double the amount, making it visually more unclear, with only
minor benefit.

The first part of this call, the "product name", applies to the
variants. Going the detour via chip_info, the array element to call
could just be hard-coded in the chip_info table, like:

        static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
                [yas530] = {
                        ...
                        .product_name = yas5xx_product_name[0],
			...

The second part, the "version name", is currently in the
three-dimensional array yas5xx_version_name[]. The first dimension is
the variant, this would need to be hard-coded in the chip_info table,
similar to the example above. The second dimension is the version, this
would need to come from within the yas5xx_probe() the function. I
couldn't find a way how to assign one dimension in one place and another
dimension in another place.

To include the second part in the chip_info, I would split the
three-dimensional array yas5xx_version_name[] into three separate
two-dimensional arrays, one per variant. It would look like this (snippets):


        static const char yas530_version_name[][3] = { "A", "B" };

        static const char yas532_version_name[][3] = { "AB", "AC" };

        static const char yas533_version_name[][3] = { "AB", "AC" };

        ...

        struct yas5xx_chip_info {
                ...
                const char *product_name;
                const char (*version_name)[3];
        	...

        ...

        static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = {
                [yas530] = {
                        ...
                        .product_name = yas5xx_product_name[0],
                        .version_name = yas530_version_name,
                        ...

        ...


        dev_info(dev, "detected %s %s\n",
                 yas5xx->chip_info->product_name,
                 yas5xx->chip_info->version_name[yas5xx->version]);


I'm not fully sure this is the best way. Also note that I had to set the
size of the second dimension of yas530_version_name to 3.

Do you think I should do it this way to include "product_name" and
"version_name" in the chip_info?

If yes, should I probably do a similar thing for the values
"t_ref_counts" and "min_temp_celcius_x10"? Here as well, the array
elements are directly called in function yas5xx_calc_temperature()
without using the chip_info structure.

Also I noticed that I should add "static" to the typedef of arrays
"t_ref_counts" and "min_temp_celcius_x10". Darn, and I have to correct
the spelling of "celcius" into "celsius" in "min_temp_celcius_x10".

Kind regards,
Jakob