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

[Jakob Hauser <jahau@rocketmail.com>]

Hi Andy,

On 18.06.22 12:57, Andy Shevchenko wrote:
>
> On Sat, Jun 18, 2022 at 2:15 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>>
>> -/* These variant IDs are known from code dumps */
> 
> Not sure why this change
> a) is here (means maybe you need to move the comment somewhere else?);
> b) done at all (means maybe this comment is still actual?).

The device IDs of YAS537 and YAS539 were placed there as placeholders
for future additions. The comment kind of explained why they are there
without being used within the driver. Now, with YAS537 being added, it
becomes clear that there is only YAS539 left to complete the family, the
comment is not necessary.

...

>> +       switch (yas5xx->devid) {
>> +       case YAS530_DEVICE_ID:
>> +       case YAS532_DEVICE_ID:
>> +               if (reg == YAS530_532_ACTUATE_INIT_COIL ||
>> +                   reg == YAS530_532_MEASURE)
>> +                       return true;
>> +               break;
>> +       case YAS537_DEVICE_ID:
>> +               if (reg == YAS537_MEASURE)
>> +                       return true;
>> +               break;
>> +       default:
>> +               dev_err(yas5xx->dev, "unknown device type\n");
>> +               break;
> 
>> +               /* fall through */
> 
> What does this mean?
> 
> It seems here you may actually to return directly.

I wanted to avoid:

        default:
                dev_err(yas5xx->dev, "unknown device type\n");
                return false;
        }

        return false;

I'll just remove the "fall through" comment and keep it like this:

        default:
                dev_err(yas5xx->dev, "unknown device type\n");
                break;
        }

        return false;

...

>> +       /* Writing SRST register, the exact meaning is unknown */
> 
> Wild guess: Software ReSeT (SRST). Would explain why it should be done
> before calibration.

I'll remove the "the exact meaning is unknown" part.

...

>> +       case YAS537_VERSION_0:
> 
>> +
> 
> Redundant blank line.

Within switch statements no empty lines for visual structuring allowed
at all?

...

>> -               strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name));
> 
> Okay, strncpy() -> strscpy() perhaps in the separate patch.

Yes, this needs to go into a separate patch, it's implemented like this
in the current driver.

...

Kind regards,
Jakob