| Message ID | 18f223776f6942d52af2e41dd10160e220a23311.1655509425.git.jahau@rocketmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for magnetometer Yamaha YAS537 | expand |
On Sat, 18 Jun 2022 02:13:13 +0200 Jakob Hauser <jahau@rocketmail.com> wrote: > This is a preparation for adding YAS537 variant. > > YAS537 uses other data types on the calibration coefficients [1] than YAS530 [2] > and YAS532 [3]. > > On YAS537, at least for a4 and a7 this could matter because 8-bit unsigned data > from the register gets stored into a signed data type, therefore this should be > 8-bit as well. > > For YAS530/532, on the other hand, it doesn't seem to matter. The size of a2-a9 > and k is smaller than 8-bit at extraction, also the applied math is low. And > Cx/Cy1/Cy2, now being defined as signed 16-bit, are extracted as unsigned 8-bit > and undergo only minor math. Ok. If this is harmless to existing drivers fair enough, though my personal inclination would have been to take the easier approach of making the new variant sign extend on variable load (sign_extend_32() and similar) just so we didn't need to check the older parts weren't affected. Thanks, Jonathan > > [1] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas537.c#L76-L78 > [2] https://github.com/NovaFusion/android_kernel_samsung_golden/blob/cm-12.1/drivers/sensor/compass/yas_mag_driver-yas530.c#L526-L527 > [3] https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/GT-I9195I/drivers/iio/magnetometer/yas_mag_drv-yas532.c#L76-L77 > > Cc: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Jakob Hauser <jahau@rocketmail.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > drivers/iio/magnetometer/yamaha-yas530.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c > index 9b45a550f31e..c6f5f25793c4 100644 > --- a/drivers/iio/magnetometer/yamaha-yas530.c > +++ b/drivers/iio/magnetometer/yamaha-yas530.c > @@ -103,9 +103,11 @@ struct yas5xx_calibration { > s32 r[3]; > u32 f[3]; > /* Temperature compensation calibration */ > - s32 Cx, Cy1, Cy2; > + s16 Cx, Cy1, Cy2; > /* Misc calibration coefficients */ > - s32 a2, a3, a4, a5, a6, a7, a8, a9, k; > + s8 a2, a3, a4, a6, a7, a8; > + s16 a5, a9; > + u8 k; > /* clock divider */ > u8 dck; > };
Hi Jonathan, On 18.06.22 16:56, Jonathan Cameron wrote: > On Sat, 18 Jun 2022 02:13:13 +0200 > Jakob Hauser <jahau@rocketmail.com> wrote: > >> This is a preparation for adding YAS537 variant. >> >> YAS537 uses other data types on the calibration coefficients [1] than YAS530 [2] >> and YAS532 [3]. >> >> On YAS537, at least for a4 and a7 this could matter because 8-bit unsigned data >> from the register gets stored into a signed data type, therefore this should be >> 8-bit as well. >> >> For YAS530/532, on the other hand, it doesn't seem to matter. The size of a2-a9 >> and k is smaller than 8-bit at extraction, also the applied math is low. And >> Cx/Cy1/Cy2, now being defined as signed 16-bit, are extracted as unsigned 8-bit >> and undergo only minor math. > Ok. If this is harmless to existing drivers fair enough, though my personal > inclination would have been to take the easier approach of making the > new variant sign extend on variable load (sign_extend_32() and similar) > just so we didn't need to check the older parts weren't affected. I didn't know that operation :) Let's take this. Not sure how to handle the "Reviewed-by:" tags. Even though it's a small patch, it gets modified a lot. Therefore I'd remove the tags of Linus and Andy. Kind regards, Jakob
On Tue, Jun 21, 2022 at 2:51 AM Jakob Hauser <jahau@rocketmail.com> wrote: > On 18.06.22 16:56, Jonathan Cameron wrote: > > On Sat, 18 Jun 2022 02:13:13 +0200 > > Jakob Hauser <jahau@rocketmail.com> wrote: > > Ok. If this is harmless to existing drivers fair enough, though my personal > > inclination would have been to take the easier approach of making the > > new variant sign extend on variable load (sign_extend_32() and similar) > > just so we didn't need to check the older parts weren't affected. > > I didn't know that operation :) Let's take this. > > Not sure how to handle the "Reviewed-by:" tags. Even though it's a small > patch, it gets modified a lot. Therefore I'd remove the tags of Linus > and Andy. Keep mine. I very seldom disagree with Jonathan (or Andy) so the default to any comment from them is to keep my ACKs/Reviews. Yours, Linus Walleij
Hi Jonathan, On 21.06.22 02:51, Jakob Hauser wrote: > > > On 18.06.22 16:56, Jonathan Cameron wrote: > >> On Sat, 18 Jun 2022 02:13:13 +0200 >> Jakob Hauser <jahau@rocketmail.com> wrote: >> >>> This is a preparation for adding YAS537 variant. >>> >>> YAS537 uses other data types on the calibration coefficients [1] than YAS530 [2] >>> and YAS532 [3]. >>> >>> On YAS537, at least for a4 and a7 this could matter because 8-bit unsigned data >>> from the register gets stored into a signed data type, therefore this should be >>> 8-bit as well. >>> >>> For YAS530/532, on the other hand, it doesn't seem to matter. The size of a2-a9 >>> and k is smaller than 8-bit at extraction, also the applied math is low. And >>> Cx/Cy1/Cy2, now being defined as signed 16-bit, are extracted as unsigned 8-bit >>> and undergo only minor math. >> >> Ok. If this is harmless to existing drivers fair enough, though my personal >> inclination would have been to take the easier approach of making the >> new variant sign extend on variable load (sign_extend_32() and similar) >> just so we didn't need to check the older parts weren't affected. > > I didn't know that operation :) Let's take this. While working on patchset v4, I just realized that sign_extend32() can't be used at the variable declaration but instead needs to be applied at "variable load", as you wrote. I wasn't aware of this until now. In that case, I'd prefer to leave the patch unchanged. Overall the resulting code looks simpler that way. Applying sign_extend32() at all locations where we extract calibration coefficients makes it more dizzy. Kind regards, Jakob
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c index 9b45a550f31e..c6f5f25793c4 100644 --- a/drivers/iio/magnetometer/yamaha-yas530.c +++ b/drivers/iio/magnetometer/yamaha-yas530.c @@ -103,9 +103,11 @@ struct yas5xx_calibration { s32 r[3]; u32 f[3]; /* Temperature compensation calibration */ - s32 Cx, Cy1, Cy2; + s16 Cx, Cy1, Cy2; /* Misc calibration coefficients */ - s32 a2, a3, a4, a5, a6, a7, a8, a9, k; + s8 a2, a3, a4, a6, a7, a8; + s16 a5, a9; + u8 k; /* clock divider */ u8 dck; };