On Sat, 18 Jun 2022 02:13:16 +0200 Jakob Hauser <jahau@xxxxxxxxxxxxxx> wrote: > This adds support for the magnetometer Yamaha YAS537. The additions are based > on comparison of Yamaha Android kernel drivers for YAS532 [1] and YAS537 [2]. > > In the Yamaha YAS537 Android driver, there is an overflow/underflow control > implemented. For regular usage, this seems not necessary. A similar overflow/ > underflow control of Yamaha YAS530/532 Android driver isn't integrated in the > mainline driver. It is therefore skipped for YAS537 in mainline too. > > Also in the Yamaha YAS537 Android driver, at the end of the reset_yas537() > function, a measurement is saved in "last_after_rcoil". Later on, this is > compared to current measurements. If the difference gets too big, a new > reset is initialized. The difference in measurements needs to be quite big, > it's hard to say if this is necessary for regular operation. Therefore this > isn't integrated in the mainline driver either. > > [1] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c > [2] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c > > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx> > Signed-off-by: Jakob Hauser <jahau@xxxxxxxxxxxxxx> > Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx> Various comments inline, but mostly following through from earlier feedback that you would end up with much simpler and more readable code by using a nice const chip_info structure the address of which you assign in a single place in _probe() It's a common pattern and it almost always ends up simpler for drivers that end up supporting more than one or two different device variants. Thanks, Jonathan > --- > The diff on function yas5xx_probe() is a bit confusing. Maybe better to > understand when comparing before and after code. > before: https://github.com/torvalds/linux/blob/v5.19-rc2/drivers/iio/magnetometer/yamaha-yas530.c#L873-L902 > after: https://github.com/Jakko3/linux/blob/yas537_v3/drivers/iio/magnetometer/yamaha-yas530.c#L1416-L1463 > > drivers/iio/magnetometer/Kconfig | 4 +- > drivers/iio/magnetometer/yamaha-yas530.c | 553 +++++++++++++++++++++-- > 2 files changed, 529 insertions(+), 28 deletions(-) > > 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. > > This driver can also be compiled as a module. > To compile this driver as a module, choose M here: the module > diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c > index 72a75dc57e11..e6123d1d9383 100644 > --- a/drivers/iio/magnetometer/yamaha-yas530.c > +++ b/drivers/iio/magnetometer/yamaha-yas530.c > @@ -17,6 +17,9 @@ > * named "inv_compass" in the Tegra Android kernel tree. > * Copyright (C) 2012 InvenSense Corporation > * > + * Code functions for YAS537 based on Yamaha Android kernel driver. > + * Copyright (c) 2014 Yamaha Corporation > + * > * Author: Linus Walleij <linus.walleij@xxxxxxxxxx> > */ > #include <linux/bitfield.h> > @@ -56,6 +59,23 @@ > #define YAS530_532_TEST2 0x89 > #define YAS530_532_CAL 0x90 > > +/* Registers used by YAS537 */ > +#define YAS537_MEASURE 0x81 /* Originally YAS537_REG_CMDR */ > +#define YAS537_CONFIG 0x82 /* Originally YAS537_REG_CONFR */ > +#define YAS537_MEASURE_INTERVAL 0x83 /* Originally YAS537_REG_INTRVLR */ > +#define YAS537_OFFSET_X 0x84 /* Originally YAS537_REG_OXR */ > +#define YAS537_OFFSET_Y1 0x85 /* Originally YAS537_REG_OY1R */ > +#define YAS537_OFFSET_Y2 0x86 /* Originally YAS537_REG_OY2R */ > +#define YAS537_AVR 0x87 > +#define YAS537_HCK 0x88 > +#define YAS537_LCK 0x89 > +#define YAS537_SRST 0x90 > +#define YAS537_ADCCAL 0x91 > +#define YAS537_MTC 0x93 > +#define YAS537_OC 0x9E > +#define YAS537_TRM 0x9F > +#define YAS537_CAL 0xC0 > + > /* Bits in the YAS5xx config register */ > #define YAS5XX_CONFIG_INTON BIT(0) /* Interrupt on? */ > #define YAS5XX_CONFIG_INTHACT BIT(1) /* Interrupt active high? */ > @@ -67,6 +87,7 @@ > #define YAS5XX_MEASURE_LDTC BIT(1) > #define YAS5XX_MEASURE_FORS BIT(2) > #define YAS5XX_MEASURE_DLYMES BIT(4) > +#define YAS5XX_MEASURE_CONT BIT(5) > > /* Bits in the measure data register */ > #define YAS5XX_MEASURE_DATA_BUSY BIT(7) > @@ -93,8 +114,15 @@ > #define YAS532_DATA_OVERFLOW (BIT(YAS532_DATA_BITS) - 1) > #define YAS532_20DEGREES 390 /* Counts starting at -50 °C */ > > -/* These variant IDs are known from code dumps */ > #define YAS537_DEVICE_ID 0x07 /* YAS537 (MS-3T) */ > +#define YAS537_VERSION_0 0 /* Version naming unknown */ > +#define YAS537_VERSION_1 1 /* Version naming unknown */ > +#define YAS537_MAG_AVERAGE_32_MASK GENMASK(6, 4) > +#define YAS537_MEASURE_TIME_WORST_US 1500 > +#define YAS537_DEFAULT_SENSOR_DELAY_MS 50 > +#define YAS537_MAG_RCOIL_TIME_US 65 > +#define YAS537_20DEGREES 8120 /* Counts starting at -386 °C */ > + > #define YAS539_DEVICE_ID 0x08 /* YAS539 (MS-3S) */ > > /* Turn off device regulators etc after 5 seconds of inactivity */ > @@ -267,6 +295,78 @@ static int yas530_532_measure(struct yas5xx *yas5xx, u16 *t, > return ret; > } > > +/** > + * yas537_measure() - Make a measure from the hardware > + * @yas5xx: The device state > + * @t: the raw temperature measurement > + * @x: the raw x axis measurement > + * @y1: the y1 axis measurement > + * @y2: the y2 axis measurement > + * @return: 0 on success or error code > + */ > +static int yas537_measure(struct yas5xx *yas5xx, u16 *t, > + u16 *x, u16 *y1, u16 *y2) > +{ > + struct yas5xx_calibration *c = &yas5xx->calibration; > + unsigned int busy; > + u8 data[8]; > + u16 xy1y2[3]; > + s32 h[3], s[3]; > + int i, ret; > + > + mutex_lock(&yas5xx->lock); > + > + /* Contrary to YAS530/532, also a "cont" bit is set, meaning unknown */ > + ret = regmap_write(yas5xx->map, YAS537_MEASURE, YAS5XX_MEASURE_START | > + YAS5XX_MEASURE_CONT); > + if (ret < 0) > + goto out_unlock; > + > + /* Use same timeout like YAS530/532 but the bit is in data row 2 */ > + ret = regmap_read_poll_timeout(yas5xx->map, YAS5XX_MEASURE_DATA + 2, busy, > + !(busy & YAS5XX_MEASURE_DATA_BUSY), > + 500, 20000); > + if (ret) { > + dev_err(yas5xx->dev, "timeout waiting for measurement\n"); > + goto out_unlock; > + } > + > + ret = regmap_bulk_read(yas5xx->map, YAS5XX_MEASURE_DATA, > + data, sizeof(data)); > + if (ret) > + goto out_unlock; > + > + mutex_unlock(&yas5xx->lock); > + > + *t = get_unaligned_be16(&data[0]); > + xy1y2[0] = FIELD_GET(GENMASK(13, 0), get_unaligned_be16(&data[2])); > + xy1y2[1] = get_unaligned_be16(&data[4]); > + xy1y2[2] = get_unaligned_be16(&data[6]); > + > + /* The second version of YAS537 needs to include calibration coefficients */ > + if (yas5xx->version == YAS537_VERSION_1) { > + for (i = 0; i < 3; i++) > + s[i] = xy1y2[i] - BIT(13); > + h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / BIT(13); > + h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / BIT(13); > + h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / BIT(13); > + for (i = 0; i < 3; i++) { > + clamp_val(h[i], -BIT(13), BIT(13) - 1); > + xy1y2[i] = h[i] + BIT(13); > + } > + } > + > + *x = xy1y2[0]; > + *y1 = xy1y2[1]; > + *y2 = xy1y2[2]; > + > + return 0; > + > +out_unlock: > + mutex_unlock(&yas5xx->lock); > + return ret; > +} > + > static s32 yas530_532_linearize(struct yas5xx *yas5xx, u16 val, int axis) > { > struct yas5xx_calibration *c = &yas5xx->calibration; > @@ -437,6 +537,58 @@ static int yas530_532_get_measure(struct yas5xx *yas5xx, s32 *to, > return 0; > } > > +/** > + * yas537_get_measure() - Measure a sample of all axis and process > + * @yas5xx: The device state > + * @to: Temperature out > + * @xo: X axis out > + * @yo: Y axis out > + * @zo: Z axis out > + * @return: 0 on success or error code > + */ > +static int yas537_get_measure(struct yas5xx *yas5xx, s32 *to, > + s32 *xo, s32 *yo, s32 *zo) > +{ > + u16 t_ref, t, x, y1, y2; > + int ret; > + > + /* We first get raw data that needs to be translated to [x,y,z] */ > + ret = yas537_measure(yas5xx, &t, &x, &y1, &y2); > + if (ret) > + return ret; > + > + /* Set the temperature reference value (unit: counts) */ > + t_ref = YAS537_20DEGREES; > + > + /* > + * Raw temperature value t is the number of counts. YAS537 product > + * description No. PBAS537A-000-01-e mentions a temperature resolution > + * of 0.05 °C/count. A readout of the t value at ca. 20 °C returns > + * approx. 8120 counts. > + * > + * 8120 counts x 0.05 °C/count corresponds to a range of 406 °C. > + * 0 counts would be at theoretical -386 °C. > + * > + * The formula used for YAS530/532 needs to be adapted to this > + * theoretical starting temperature, again calculating with 1/10:s > + * of degrees Celsius and finally multiplying by 100 to get milli > + * degrees Celsius. > + */ > + *to = ((4060 * t / t_ref) - 3860) * 100; > + > + /* > + * Unfortunately, no linearization or temperature compensation formulas > + * are known for YAS537. > + */ > + > + /* Calculate x, y, z from x, y1, y2 */ > + *xo = (x - BIT(13)) * 300; > + *yo = (y1 - y2) * 1732 / 10; > + *zo = (-y1 - y2 + BIT(14)) * 300; > + > + return 0; > +} > + > 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. > + break; > + case YAS537_DEVICE_ID: > + ret = yas537_get_measure(yas5xx, &t, &x, &y, &z); > + break; > + default: > + dev_err(yas5xx->dev, "unknown device type\n"); > + return -EINVAL; > + } > pm_runtime_mark_last_busy(yas5xx->dev); > pm_runtime_put_autosuspend(yas5xx->dev); > if (ret) > @@ -484,9 +647,10 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev, > *val2 = 100000000; > break; > case YAS532_DEVICE_ID: > + case YAS537_DEVICE_ID: > /* > - * Raw values of YAS532 are in nanotesla. Divide by > - * 100000 (10^5) to get Gauss. > + * Raw values of YAS532 and YAS537 are in nanotesla. > + * Divide by 100000 (10^5) to get Gauss. > */ > *val = 1; > *val2 = 100000; > @@ -506,7 +670,18 @@ static void yas5xx_fill_buffer(struct iio_dev *indio_dev) > int ret; > > pm_runtime_get_sync(yas5xx->dev); > - ret = yas5xx_get_measure(yas5xx, &t, &x, &y, &z); yas5xx->chip_info.get_measure() > + switch (yas5xx->devid) { > + case YAS530_DEVICE_ID: > + case YAS532_DEVICE_ID: > + ret = yas530_532_get_measure(yas5xx, &t, &x, &y, &z); > + break; > + case YAS537_DEVICE_ID: > + ret = yas537_get_measure(yas5xx, &t, &x, &y, &z); > + break; > + default: > + dev_err(yas5xx->dev, "unknown device type\n"); > + return; > + } > pm_runtime_mark_last_busy(yas5xx->dev); > pm_runtime_put_autosuspend(yas5xx->dev); > if (ret) { > @@ -592,9 +767,34 @@ static const struct iio_info yas5xx_info = { > > static bool yas5xx_volatile_reg(struct device *dev, unsigned int reg) > { > - return reg == YAS5XX_ACTUATE_INIT_COIL || > - reg == YAS5XX_MEASURE || > - (reg >= YAS5XX_MEASURE_DATA && reg <= YAS5XX_MEASURE_DATA + 7); > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct yas5xx *yas5xx = iio_priv(indio_dev); > + > + if (reg >= YAS5XX_MEASURE_DATA && reg <= YAS5XX_MEASURE_DATA + 7) > + return true; > + > + /* > + * YAS versions share different registers on the same address, > + * need to differentiate. Better as separate functions via the chip_info structure I'm suggesting you add. That will end up more readable as additional devices are added at the cost of a slightly more code in this case. > + */ > + 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 */ > + } > + > + return false; > } > > /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */ > @@ -749,6 +949,216 @@ static int yas532_get_calibration_data(struct yas5xx *yas5xx) > return 0; > } > > +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. > + > + /* > + * 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; } > + 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. > + 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; > + > + /* > + * For data extraction, build some blocks. Four 32-bit blocks > + * look appropriate. > + * > + * n 7 6 5 4 3 2 1 0 > + * data[0] 0 [ Cx Cx Cx Cx Cx Cx Cx Cx ] bits 31 .. 24 > + * data[1] 1 [ Cx C1 C1 C1 C1 C1 C1 C1 ] bits 23 .. 16 > + * data[2] 2 [ C1 C1 C2 C2 C2 C2 C2 C2 ] bits 15 .. 8 > + * data[3] 3 [ C2 C2 C2 ] bits 7 .. 0 > + * > + * n 7 6 5 4 3 2 1 0 > + * data[3] 0 [ a2 a2 a2 a2 a2 ] bits 31 .. 24 > + * data[4] 1 [ a2 a2 a3 a3 a3 a3 a3 a3 ] bits 23 .. 16 > + * data[5] 2 [ a3 a4 a4 a4 a4 a4 a4 a4 ] bits 15 .. 8 > + * data[6] 3 [ a4 ] bits 7 .. 0 > + * > + * n 7 6 5 4 3 2 1 0 > + * data[6] 0 [ a5 a5 a5 a5 a5 a5 a5 ] bits 31 .. 24 > + * data[7] 1 [ a5 a5 a6 a6 a6 a6 a6 a6 ] bits 23 .. 16 > + * data[8] 2 [ a6 a7 a7 a7 a7 a7 a7 a7 ] bits 15 .. 8 > + * data[9] 3 [ a7 ] bits 7 .. 0 > + * > + * n 7 6 5 4 3 2 1 0 > + * data[9] 0 [ a8 a8 a8 a8 a8 a8 a8 ] bits 31 .. 24 > + * data[10] 1 [ a9 a9 a9 a9 a9 a9 a9 a9 ] bits 23 .. 16 > + * data[11] 2 [ a9 k k k k k k k ] bits 15 .. 8 > + * data[12] 3 [ ] bits 7 .. 0 > + */ > + > + /* Get data into these four blocks val1 to val4 */ > + val1 = get_unaligned_be32(&data[0]); > + val2 = get_unaligned_be32(&data[3]); > + val3 = get_unaligned_be32(&data[6]); > + val4 = get_unaligned_be32(&data[9]); > + > + /* Extract calibration coefficients and modify */ > + c->Cx = FIELD_GET(GENMASK(31, 23), val1) - 256; > + c->Cy1 = FIELD_GET(GENMASK(22, 14), val1) - 256; > + c->Cy2 = FIELD_GET(GENMASK(13, 5), val1) - 256; > + c->a2 = FIELD_GET(GENMASK(28, 22), val2) - 64; > + c->a3 = FIELD_GET(GENMASK(21, 15), val2) - 64; > + c->a4 = FIELD_GET(GENMASK(14, 7), val2) - 128; > + c->a5 = FIELD_GET(GENMASK(30, 22), val3) - 112; > + c->a6 = FIELD_GET(GENMASK(21, 15), val3) - 64; > + c->a7 = FIELD_GET(GENMASK(14, 7), val3) - 128; > + c->a8 = FIELD_GET(GENMASK(30, 24), val4) - 64; > + c->a9 = FIELD_GET(GENMASK(23, 15), val4) - 112; > + c->k = FIELD_GET(GENMASK(14, 8), val4); > + > + break; > + > + default: > + dev_err(yas5xx->dev, "unknown version of YAS537\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > static void yas530_532_dump_calibration(struct yas5xx *yas5xx) > { > struct yas5xx_calibration *c = &yas5xx->calibration; > @@ -772,6 +1182,26 @@ static void yas530_532_dump_calibration(struct yas5xx *yas5xx) > dev_dbg(yas5xx->dev, "dck = %d\n", c->dck); > } > > +static void yas537_dump_calibration(struct yas5xx *yas5xx) > +{ > + struct yas5xx_calibration *c = &yas5xx->calibration; > + > + if (yas5xx->version == YAS537_VERSION_1) { > + dev_dbg(yas5xx->dev, "Cx = %d\n", c->Cx); > + dev_dbg(yas5xx->dev, "Cy1 = %d\n", c->Cy1); > + dev_dbg(yas5xx->dev, "Cy2 = %d\n", c->Cy2); > + dev_dbg(yas5xx->dev, "a2 = %d\n", c->a2); > + dev_dbg(yas5xx->dev, "a3 = %d\n", c->a3); > + dev_dbg(yas5xx->dev, "a4 = %d\n", c->a4); > + dev_dbg(yas5xx->dev, "a5 = %d\n", c->a5); > + dev_dbg(yas5xx->dev, "a6 = %d\n", c->a6); > + dev_dbg(yas5xx->dev, "a7 = %d\n", c->a7); > + dev_dbg(yas5xx->dev, "a8 = %d\n", c->a8); > + dev_dbg(yas5xx->dev, "a9 = %d\n", c->a9); > + dev_dbg(yas5xx->dev, "k = %d\n", c->k); > + } > +} > + > static int yas530_532_set_offsets(struct yas5xx *yas5xx, s8 ox, s8 oy1, s8 oy2) > { > int ret; > @@ -888,6 +1318,45 @@ static int yas530_532_power_on(struct yas5xx *yas5xx) > return regmap_write(yas5xx->map, YAS530_532_MEASURE_INTERVAL, 0); > } > > +static int yas537_power_on(struct yas5xx *yas5xx) > +{ > + int ret; > + u8 intrvl; > + > + /* Write registers according to Android driver */ > + ret = regmap_write(yas5xx->map, YAS537_ADCCAL, GENMASK(1, 0)); > + if (ret) > + return ret; > + ret = regmap_write(yas5xx->map, YAS537_ADCCAL+1, GENMASK(7, 3)); Spaces around the + > + if (ret) > + return ret; > + ret = regmap_write(yas5xx->map, YAS537_TRM, GENMASK(7, 0)); > + if (ret) > + return ret; > + > + /* The interval value is static in regular operation */ > + intrvl = (YAS537_DEFAULT_SENSOR_DELAY_MS * 1000 > + - YAS537_MEASURE_TIME_WORST_US) / 4100; > + ret = regmap_write(yas5xx->map, YAS537_MEASURE_INTERVAL, intrvl); > + if (ret) > + return ret; > + > + /* The average value is also static in regular operation */ > + ret = regmap_write(yas5xx->map, YAS537_AVR, YAS537_MAG_AVERAGE_32_MASK); > + if (ret) > + return ret; > + > + /* Perform the "rcoil" part but skip the "last_after_rcoil" read */ > + ret = regmap_write(yas5xx->map, YAS537_CONFIG, BIT(3)); > + if (ret) > + return ret; > + > + /* Wait until the coil has ramped up */ > + usleep_range(YAS537_MAG_RCOIL_TIME_US, YAS537_MAG_RCOIL_TIME_US + 100); > + > + return 0; > +} > + > static int yas5xx_probe(struct i2c_client *i2c, > const struct i2c_device_id *id) > { Hopefully I conveyed the point in earlier review, but I'd expcte a simple switch statement in here based on id and version to set yas5xx->chip_info to appropriate static const structure which includes both data and function pointers to flatten all the device specific switches into simple assignments or calls of function pointers. > @@ -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). > switch (yas5xx->devid) { > case YAS530_DEVICE_ID: > - ret = yas530_get_calibration_data(yas5xx); > + case YAS532_DEVICE_ID: > + > + if (yas5xx->devid == YAS530_DEVICE_ID) { > + ret = yas530_get_calibration_data(yas5xx); > + if (ret) > + goto assert_reset; > + dev_info(dev, "detected YAS530 MS-3E %s", > + yas5xx->version ? "B" : "A"); > + strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name)); Indirection as suggested below and this is already part of yas5xx->chip_info > + } else { > + ret = yas532_get_calibration_data(yas5xx); > + if (ret) > + goto assert_reset; > + dev_info(dev, "detected YAS532/YAS533 MS-3R/F %s", > + yas5xx->version ? "AC" : "AB"); > + strncpy(yas5xx->name, "yas532", sizeof(yas5xx->name)); > + } > + > + yas530_532_dump_calibration(yas5xx); > + ret = yas530_532_power_on(yas5xx); > + if (ret) > + goto assert_reset; > + ret = yas530_532_measure_offsets(yas5xx); > if (ret) > goto assert_reset; > - dev_info(dev, "detected YAS530 MS-3E %s", > - yas5xx->version ? "B" : "A"); > - strncpy(yas5xx->name, "yas530", sizeof(yas5xx->name)); > break; > - case YAS532_DEVICE_ID: > - ret = yas532_get_calibration_data(yas5xx); > + > + case YAS537_DEVICE_ID: > + ret = yas537_get_calibration_data(yas5xx); > + if (ret) > + goto assert_reset; > + dev_info(dev, "detected YAS537 MS-3T version %s", > + yas5xx->version ? "1" : "0"); > + strncpy(yas5xx->name, "yas537", sizeof(yas5xx->name)); > + > + yas537_dump_calibration(yas5xx); > + ret = yas537_power_on(yas5xx); > if (ret) > goto assert_reset; > - dev_info(dev, "detected YAS532/YAS533 MS-3R/F %s", > - yas5xx->version ? "AC" : "AB"); > - strncpy(yas5xx->name, "yas532", sizeof(yas5xx->name)); > break; > + > default: > ret = -ENODEV; > dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid); > goto assert_reset; > } > > - yas530_532_dump_calibration(yas5xx); > - ret = yas530_532_power_on(yas5xx); > - if (ret) > - goto assert_reset; > - ret = yas530_532_measure_offsets(yas5xx); > - if (ret) > - goto assert_reset; > - > indio_dev->info = &yas5xx_info; > indio_dev->available_scan_masks = yas5xx_scan_masks; > indio_dev->modes = INDIO_DIRECT_MODE; > @@ -1070,7 +1557,19 @@ static int __maybe_unused yas5xx_runtime_resume(struct device *dev) > usleep_range(31000, 40000); > gpiod_set_value_cansleep(yas5xx->reset, 0); > > - ret = yas5xx_power_on(yas5xx); > + switch (yas5xx->devid) { > + case YAS530_DEVICE_ID: > + case YAS532_DEVICE_ID: > + ret = yas530_532_power_on(yas5xx); After you've added that device type specific structure I keep talking about it can have a function pointer and this switch statement becomes as simple bit of indirection to a constant data. yas5xx->chip_info.power_on(yas5xx); > + break; > + case YAS537_DEVICE_ID: > + ret = yas537_power_on(yas5xx); > + break; > + default: > + dev_err(yas5xx->dev, "unknown device type\n"); > + ret = -EINVAL; > + break; > + } > if (ret) { > dev_err(dev, "cannot power on\n"); > goto out_reset;
