Re: [PATCH v2 2/2] iio: magnetometer: add ti tmag5273 driver

[Gerald Loacker]

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