Hi Jonathan,
On 18.06.22 17:28, Jonathan Cameron wrote:
>
> On Sat, 18 Jun 2022 02:13:16 +0200
> Jakob Hauser <jahau@xxxxxxxxxxxxxx> wrote:
...
>> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
>> index 07eb619bcfe8..868128cee835 100644
>> --- a/drivers/iio/magnetometer/Kconfig
>> +++ b/drivers/iio/magnetometer/Kconfig
>> @@ -216,8 +216,8 @@ config YAMAHA_YAS530
>> select IIO_TRIGGERED_BUFFER
>> help
>> Say Y here to add support for the Yamaha YAS530 series of
>> - 3-Axis Magnetometers. Right now YAS530, YAS532 and YAS533 are
>> - fully supported.
>> + 3-Axis Magnetometers. Right now YAS530, YAS532, YAS533 and
>> + YAS537 are fully supported.
> Whilst here I'd reduce this to
>
> 3-Axis Magnetometers. YAS530, YAS532, YAS533 and YAS537
> are supported.
>
> "Right now" provides no information - we are hardly likely to be talking
> about code at some stage in the past or future.
> "fully" isn't all that well defined and doesn't add anything over supported.
OK, will change that.
...
>> static int yas5xx_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan,
>> int *val, int *val2,
>> @@ -450,7 +602,18 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev,
>> case IIO_CHAN_INFO_PROCESSED:
>> case IIO_CHAN_INFO_RAW:
>> pm_runtime_get_sync(yas5xx->dev);
>> - ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z);
>> + switch (yas5xx->devid) {
>> + case YAS530_DEVICE_ID:
>> + case YAS532_DEVICE_ID:
>> + ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z);
>
> As below, use function pointers in a chip_info struct to avoid switch
> statements like this and give shorter + more readable code.
I'll try to implement this.
...
>> +static int yas537_get_calibration_data(struct yas5xx *yas5xx)
>> +{
>> + struct yas5xx_calibration *c = &yas5xx->calibration;
>> + u8 data[17];
>> + u32 val1, val2, val3, val4;
>> + int i, ret;
>> +
>> + /* Writing SRST register, the exact meaning is unknown */
>> + ret = regmap_write(yas5xx->map, YAS537_SRST, BIT(1));
>> + if (ret)
>> + return ret;
>> +
>> + /* Calibration readout, YAS537 needs one readout only */
>> + ret = regmap_bulk_read(yas5xx->map, YAS537_CAL, data, sizeof(data));
>> + if (ret)
>> + return ret;
>> + dev_dbg(yas5xx->dev, "calibration data: %17ph\n", data);
>> +
>> + /* Sanity check, is this all zeroes? */
>> + if (!memchr_inv(data, 0x00, 16)) {
>> + if (FIELD_GET(GENMASK(5, 0), data[16]) == 0)
>> + dev_warn(yas5xx->dev, "calibration is blank!\n");
>> + }
>> +
>> + /* Contribute calibration data to the input pool for kernel entropy */
>> + add_device_randomness(data, sizeof(data));
>> +
>> + /* Extract version information */
>> + yas5xx->version = FIELD_GET(GENMASK(7, 6), data[16]);
>> +
>> + /* There are two versions of YAS537 behaving differently */
>> + switch (yas5xx->version) {
>> +
>> + case YAS537_VERSION_0:
> Seems like we might need more chip_info variants, though perhaps in this case
> it is worth handling it as a switch statement as hopefully the number of
> way s this is done for a given part number won't grow any further.
Yes, I think I'll introduce chip_info for the variants like YAS530,
YAS532, etc. but keep handling the versions (like 0 and 1 in this case)
within the functions. I'll have a look again when preparing patchset v4.
>> +
>> + /*
>> + * The first version simply writes data back into registers:
>> + *
>> + * data[0] YAS537_MTC 0x93
>> + * data[1] 0x94
>> + * data[2] 0x95
>> + * data[3] 0x96
>> + * data[4] 0x97
>> + * data[5] 0x98
>> + * data[6] 0x99
>> + * data[7] 0x9a
>> + * data[8] 0x9b
>> + * data[9] 0x9c
>> + * data[10] 0x9d
>> + * data[11] YAS537_OC 0x9e
>> + *
>> + * data[12] YAS537_OFFSET_X 0x84
>> + * data[13] YAS537_OFFSET_Y1 0x85
>> + * data[14] YAS537_OFFSET_Y2 0x86
>> + *
>> + * data[15] YAS537_HCK 0x88
>> + * data[16] YAS537_LCK 0x89
>> + */
>> +
>> + for (i = 0; i < 17; i++) {
>
> Reduce indent by doing this as multiple loops.
> Though even better if you can use bulk writes.
>
> int j = 0;
> for (i = 0; i < 12; i++) {
> ret = regmap_write(yas5xx->map, YAS537_MTC + i,
> data[j++])
> if (ret)
> return ret;
> }
>
> for (i = 0; i < 4; i++) {
> ret = regmap_write(yas5xx->map, YAS573_OFFSET_X + i,
> data[j++]);
> if (ret)
> return ret;
> }
I'll change that.
It would also work without adding "int j = 0;" when using data[i + 12]
in the second loop. But I guess you added integer j on purpose, I'll
apply it.
>> + if (i < 12) {
>> + ret = regmap_write(yas5xx->map,
>> + YAS537_MTC + i,
>> + data[i]);
>> + if (ret)
>> + return ret;
>> + } else if (i < 15) {
>> + ret = regmap_write(yas5xx->map,
>> + YAS537_OFFSET_X + i - 12,
>> + data[i]);
>> + if (ret)
>> + return ret;
>> + yas5xx->hard_offsets[i - 12] = data[i];
>> + } else {
>> + ret = regmap_write(yas5xx->map,
>> + YAS537_HCK + i - 15,
>> + data[i]);
>> + if (ret)
>> + return ret;
>> + }
>> + }
>> +
>> + break;
>> +
>> + case YAS537_VERSION_1:
>> +
>> + /*
>> + * The second version writes some data into registers but also
>> + * extracts calibration coefficients.
>> + *
>> + * Registers being written:
>> + *
>> + * data[0] YAS537_MTC 0x93
>> + * data[1] YAS537_MTC+1 0x94
>> + * data[2] YAS537_MTC+2 0x95
>> + * data[3] YAS537_MTC+3 (partially) 0x96
>> + *
>> + * data[12] YAS537_OFFSET_X 0x84
>> + * data[13] YAS537_OFFSET_Y1 0x85
>> + * data[14] YAS537_OFFSET_Y2 0x86
>> + *
>> + * data[15] YAS537_HCK (partially) 0x88
>> + * YAS537_LCK (partially) 0x89
>> + * data[16] YAS537_OC (partially) 0x9e
>> + */
>> +
>> + for (i = 0; i < 3; i++) {
>> + ret = regmap_write(yas5xx->map, YAS537_MTC + i,
>> + data[i]);
>> + if (ret)
>> + return ret;
>> + ret = regmap_write(yas5xx->map, YAS537_OFFSET_X + i,
>> + data[i + 12]);
>> + if (ret)
>> + return ret;
>> + yas5xx->hard_offsets[i] = data[i + 12];
>> + }
>> +
>> + /*
>> + * Visualization of partially taken data:
>> + *
>> + * data[3] n 7 6 5 4 3 2 1 0
>> + * YAS537_MTC+3 x x x 1 0 0 0 0
>> + *
>> + * data[15] n 7 6 5 4 3 2 1 0
>> + * YAS537_HCK x x x x 0
>> + *
>> + * data[15] n 7 6 5 4 3 2 1 0
>> + * YAS537_LCK x x x x 0
>> + *
>> + * data[16] n 7 6 5 4 3 2 1 0
>> + * YAS537_OC x x x x x x
>> + */
>> +
>> + ret = regmap_write(yas5xx->map, YAS537_MTC + 3,
>> + (data[3] & GENMASK(7, 5)) | BIT(4));
>
> If we can give the masks meaningful names that would be great then
> use FIELD_GET and FIELD_PREP with those defines (even if it puts it back
> in the same place like here). Let the compiler optimise out anything
> unnecessary in the way of masks and shifts.
I don't know what these abbreviations stand for. We could do:
#define YAS537_MTC3_MASK_PREP ...
#define YAS537_MTC3_MASK_GET ...
#define YAS537_HCK_MASK_PREP ...
#define YAS537_HCK_MASK_GET ...
etc.
We need both FIELD_GET and FIELD_PREP, right? The first to retrieve the
data and the second to place it at the right position.
Doesn't that get strange-looking like:
ret = regmap_write(yas5xx->map, YAS537_HCK,
FIELD_PREP(YAS537_HCK_MASK_PREP,
FIELD_GET(YAS537_HCK_MASK_GET,
data[15])));
Or maybe different indentation, would that be acceptable?
ret = regmap_write(yas5xx->map, YAS537_HCK,
FIELD_PREP(YAS537_HCK_MASK_PREP,
FIELD_GET(YAS537_HCK_MASK_GET, data[15])));
On the one above your comment, the "YAS537_MTC + 3", we would still need
the "| BIT(4)" to place that "1" there. Or is there some other trick to
do this?
>> + if (ret)
>> + return ret;
>> + ret = regmap_write(yas5xx->map, YAS537_HCK,
>> + FIELD_GET(GENMASK(7, 4), data[15]) << 1);
>
> FIELD_PREP() with suitable mask defined to make it clear what field is being written.
>
>> + if (ret)
>> + return ret;
>> + ret = regmap_write(yas5xx->map, YAS537_LCK,
>> + FIELD_GET(GENMASK(3, 0), data[15]) << 1);
>> + if (ret)
>> + return ret;
>> + ret = regmap_write(yas5xx->map, YAS537_OC,
>> + FIELD_GET(GENMASK(5, 0), data[16]));
>> + if (ret)
>> + return ret;
>> +
...
>> @@ -946,35 +1415,53 @@ static int yas5xx_probe(struct i2c_client *i2c,
>>
>
> Below becomes something like
> ret = yas5xx->chip_info.get_calib_data(yas5xx);
> if (ret)
> goto assert_reset;
>
> yas5xx->chip_info.dump_calibration(yas5xx);
> yas5xx->chip_info.power_on(yas5xx)
> if (yas5xx->chip_info.measure_offsets) {
> ret = yas5xx->chip_info.measure_offsets(yas5xx);
> if (ret)
> got asert_reset;
> }
> Which is a lot more readable and less code at the expense
> of static const data (a good trade off in most cases).
Thanks for the example, that's helpful.
If YAS537 doesn't have a .measure_offsets function pointer in chip_info,
it skips that if statement cleanly?
...
Kind regards,
Jakob
