Am 21.11.2022 um 15:04 schrieb Andy Shevchenko:
> On Mon, Nov 21, 2022 at 01:35:42PM +0100, Gerald Loacker wrote:
>> Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
>> Additionally to temperature and magnetic X, Y and Z-axes the angle and
>> magnitude are reported.
>> The sensor is operating in continuous measurement mode and changes to sleep
>> mode if not used for 5 seconds.
>
> Thank you for an update! My comments below.
> (I believe the next version will be ready for inclusion)
>
> ...
>
>> +/*
>> + * bits in the TMAG5273_MANUFACTURER_ID_LSB / MSB register
>> + * 16-bit read-only unique manufacturer ID
>
> Is it ASCII or not? ('TI' suggests something...)
> If it is, please put it in the comments explicitly in ASCII.
>
Ok, got it.
>> + */
>> +#define TMAG5273_MANUFACTURER_ID 0x5449
>
> ...
>
>> +#define TMAG5273_MAG_CH_EN_X_Y_Z 0x07
>
> This is hexadecimal, while below you are using decimal values.
> Also if this is like above, isn't it a bit mask? (Otherwise
> using decimal value hints that it's probably not)
>
Ok, I will use decimal value. It's not a bit mask, as there are special
modes (XYX, ...) defined as well. I didn't want to list all 12 configs,
but this could clarify this.
>
> ...
>
>> +static const struct {
>> + unsigned int scale_int;
>> + unsigned int scale_micro;
>
> Can we have a separate patch to define this one eventually in the (one of) IIO
> generic headers? It's a bit pity that every new driver seems to reinvent the
> wheel.
>
I'll try to find a solution.
>> +} tmag5273_scale_table[4][2] = {
>> + { { 0, 0 }, { 0, 0 } },
>> + { { 0, 12200 }, { 0, 24400 } },
>> + { { 0, 40600 }, { 0, 81200 } },
>> + { { 0, 0 }, { 0, 0 } },
>> +};
>
> You probably can reformat there to have fixed-width columns to see better the
> pairs of the values. And as I told you before, 4 is not needed (AFAIR, or 2 in
> the square brackets).
>
Ok, got it.
>> + for (i = 0; i < ARRAY_SIZE(tmag5273_scale_table[0]);
>> + i++) {
>
> I would definitely go with one line here.
>
>> + if (tmag5273_scale_table[data->version][i]
>> + .scale_micro == val2)
>
> Ugly indentation.
>
You're right, best to rename tmag5273_scale_table to tmag5273_scale.
> ...
>
>> + if (!device_property_read_string(data->dev, "ti,angle-measurement",
>> + &angle_measurement)) {
>> + if (!strcmp(angle_measurement, "off"))
>> + data->angle_measurement = TMAG5273_ANGLE_EN_OFF;
>> + else if (!strcmp(angle_measurement, "x-y"))
>> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
>> + else if (!strcmp(angle_measurement, "y-z"))
>> + data->angle_measurement = TMAG5273_ANGLE_EN_Y_Z;
>> + else if (!strcmp(angle_measurement, "x-z"))
>> + data->angle_measurement = TMAG5273_ANGLE_EN_X_Z;
>> + else
>> + dev_warn(data->dev,
>> + "failed to read angle-measurement\n");
>
> Can't you use match_string()?
>
Yes, I will use match_string().
> And you actually can do a bit differently, can you?
>
> angle_measurement = "...default...";
> if (device_property_...)
> dev_warn(data->dev, "failed to read angle-measurement\n");
I think we shouldn't warn here, as angle_measurement isn't a required
property. Besides that I will do it this way.
> ret = match_string();
> if (ret < 0)
> dev_warn(data->dev, "invalid angle-measurement value\n");
> else
> data->angle_measurement = ret;
>
>> + }
>
> ...
>
>> + switch (data->devid) {
>> + case TMAG5273_MANUFACTURER_ID:
>> + snprintf(data->name, sizeof(data->name), "tmag5273x%1u",
>
> There is a difference between %1u and %.1u. And I believe you wanted the
> latter, but...
>
%1u works fine for me. Can you point me to the documentation for %.1u?
>> + data->version);
>> + if (data->version < 1 || data->version > 2)
>> + dev_warn(data->dev, "Unsupported device version 0x%x\n",
>> + data->version);
>
> ...with the current approach you may replace above with
>
> dev_warn(data->dev, "Unsupported device %s\n", data->name);
>
Ok, fine.
>> + break;
>> + default:
>> + dev_warn(data->dev, "Unknown device ID 0x%x\n", data->devid);
>> + break;
>> + }
>
> ...
>
Others are clear.
Thanks,
Gerald
