| Message ID | 28a2a9ec27c6fb4073149b897415475a8f04e3f7.1656883851.git.jahau@rocketmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for magnetometer Yamaha YAS537 | expand |
On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote: > > This commit introduces the "chip_info" structure approach for better variant > handling. > > The variant to be used is now chosen by the devicetree (enum "chip_ids"), Device Tree > not by the chip ID in the register. However, there is a check to make sure > they match (using integer "id_check"). ... Thanks for a new version, it's getting better. My comments below. But first of all, can you split this to at least two patches, i.e. 1) split out functions without introducing chip->info yet; 2) adding chip_info. Possible alternative would be more steps in 2), i.e. introducing chip_info for the callback only, then add field (or semantically unified fields) by field with corresponding changes in the code. In this case it would be easier to review. I leave this exercise to you if Jonathan thinks it worth it. ... > -#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */ > -#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */ The comments suddenly disappear from the file. See below. ... > +enum chip_ids { > + yas530, > + yas532, > + yas533, > +}; > + > +static const char yas5xx_product_name[][13] = { > + "YAS530 MS-3E", > + "YAS532 MS-3R", > + "YAS533 MS-3F" > +}; > + > +static const char yas5xx_version_name[][2][3] = { > + { "A", "B" }, > + { "AB", "AC" }, > + { "AB", "AC" } Shan't we put indices here? Also, use * instead of one dimension of array. > +}; ... > +static const int yas530_volatile_reg[] = { > + YAS530_ACTUATE_INIT_COIL, > + YAS530_MEASURE + Comma. > +}; ... > +/* Number of counts between minimum and reference temperature */ > +const u16 t_ref_counts[] = { 182, 390, 390 }; > + > +/* Starting point of temperature counting in 1/10:s degrees Celsius */ > +const s16 min_temp_celcius_x10[] = { -620, -500, -500 }; See above. ... > +struct yas5xx_chip_info { > + unsigned int devid; > + const int *volatile_reg; > + const int volatile_reg_qty; > + const u32 scaling_val2; Why const here? I assume entire structure is const, no? > + int (*get_measure)(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo); > + int (*get_calibration_data)(struct yas5xx *yas5xx); > + void (*dump_calibration)(struct yas5xx *yas5xx); > + int (*measure_offsets)(struct yas5xx *yas5xx); > + int (*power_on)(struct yas5xx *yas5xx); > +}; ... > + int i, j; j can have a proper name. > + j = yas5xx->chip_info->volatile_reg_qty; ... > +static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > + [yas530] = { > + .devid = YAS530_DEVICE_ID, > + .volatile_reg = yas530_volatile_reg, > + .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg), > + .scaling_val2 = 100000000, /* picotesla to Gauss */ > + .get_measure = yas530_get_measure, > + .get_calibration_data = yas530_get_calibration_data, > + .dump_calibration = yas530_dump_calibration, > + .measure_offsets = yas530_measure_offsets, > + .power_on = yas530_power_on, > + }, > + [yas532] = { > + .devid = YAS532_DEVICE_ID, > + .volatile_reg = yas530_volatile_reg, > + .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg), > + .scaling_val2 = 100000, /* nanotesla to Gauss */ > + .get_measure = yas530_get_measure, > + .get_calibration_data = yas532_get_calibration_data, > + .dump_calibration = yas530_dump_calibration, > + .measure_offsets = yas530_measure_offsets, > + .power_on = yas530_power_on, > + }, > + [yas533] = { > + .devid = YAS532_DEVICE_ID, > + .volatile_reg = yas530_volatile_reg, > + .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg), > + .scaling_val2 = 100000, /* nanotesla to Gauss */ > + .get_measure = yas530_get_measure, > + .get_calibration_data = yas532_get_calibration_data, > + .dump_calibration = yas530_dump_calibration, > + .measure_offsets = yas530_measure_offsets, > + .power_on = yas530_power_on, > + } Keep comma here. > +}; ... > - int ret; > + int id_check, ret; Don't add variables with different semantics on the same line. ... > + if (id_check != yas5xx->chip_info->devid) { > ret = -ENODEV; > - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid); > + dev_err(dev, "device ID %02x doesn't match %s\n", > + id_check, id->name); ret = dev_err_probe() ? > goto assert_reset; > } ... > + dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip], > + yas5xx_version_name[yas5xx->chip][yas5xx->version]); I'm wondering if these arrays can be actually embedded into chip_info?
On Mon, 4 Jul 2022 21:37:30 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote: > > > > This commit introduces the "chip_info" structure approach for better variant > > handling. > > > > The variant to be used is now chosen by the devicetree (enum "chip_ids"), > > Device Tree > > > not by the chip ID in the register. However, there is a check to make sure > > they match (using integer "id_check"). > > ... > > Thanks for a new version, it's getting better. My comments below. > > But first of all, can you split this to at least two patches, i.e. > 1) split out functions without introducing chip->info yet; > 2) adding chip_info. > > Possible alternative would be more steps in 2), i.e. introducing > chip_info for the callback only, then add field (or semantically > unified fields) by field with corresponding changes in the code. In > this case it would be easier to review. > > I leave this exercise to you if Jonathan thinks it worth it. You are of course correct that it would be nicer to have it split, but I'm not going to be fussy about it this time ;) Other than addressing Andy's eagle eyed review comments, this series looks good to me. Thanks, Jonathan
Hi Andy, On 04.07.22 21:37, Andy Shevchenko wrote: > On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote: >> ... > Thanks for a new version, it's getting better. My comments below. > > But first of all, can you split this to at least two patches, i.e. > 1) split out functions without introducing chip->info yet; > 2) adding chip_info. The only function that's split out newly is yas5xx_calc_temperature(). However, it uses "yas5xx->chip" to look up the right values in the array for "t_ref" and "min_temp_x10". So it cannot be easily split from the introduction to the chip_info approach. > Possible alternative would be more steps in 2), i.e. introducing > chip_info for the callback only, then add field (or semantically > unified fields) by field with corresponding changes in the code. In > this case it would be easier to review. What do you mean by "introducing chip_info for the callback only"? I guess you mean the function pointers? According to this, the patch could be split into: 1) introduce function pointers 2) introduce arrays to look up values However, what's missing there is the introduction of the chip_info approach as such. Admittedly difficult to follow are the changes in yas5xx_probe(): - yas5xx->devid gets removed, it was the main switch decider before - (for usage on other switches it is moved to yas5xx->chip_info->devid) - yas5xx->chip as an enumeration is newly introduced - yas5xx->chip_info as a structure gets initiated - As the chip_info now chooses according to the i2c_device_id (via enum chip_ids, thus yas5xx->chip), a new check is introduced to make sure it matches the device ID read from the register. This is done by "if (id_check != yas5xx->chip_info->devid)". - The way of reporting product name and version name was changed because introducing chip_info required to do this in one place for all versions. - As a consequence of this, yas5xx->name became obsolete. This would have to be done before introducing function pointers and arrays, like: 1) introduce chip_info structural changes 2) add function pointers 3) add arrays to look up values But it can't be easily split that way. I can't establish an "empty" chip_info approach as a fist step without removing or changing many other things. The structural changes are related to other changes in that patch. I'm thinking about first introducing chip_info and thereafter establishing the function pointers. In between I could try to split out the new temperature function: 1) introduce chip_info structural changes incl. arrays to look up values 2) split out new yas5xx_calc_temperature() function 3) add function pointers I'm not yet sure it can be split that way because things are entangled. But I will try to this in v5. Btw, looking through this, I realized that in patch 6 "Rename functions and registers" I unfortunately missed to rename instances of "yas5xx_get_measure", "yas5xx_power_on", "YAS5XX_ACTUATE_INIT_COIL" and "YAS5XX_MEASURE". I'll correct this in v5. ... >> -#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */ > >> -#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */ > > The comments suddenly disappear from the file. See below. The comment's didn't actually disappear but rather got restructured by introducing new chip_info approach. See below. >> +enum chip_ids { >> + yas530, >> + yas532, >> + yas533, >> +}; >> + >> +static const char yas5xx_product_name[][13] = { >> + "YAS530 MS-3E", >> + "YAS532 MS-3R", >> + "YAS533 MS-3F" >> +}; >> + >> +static const char yas5xx_version_name[][2][3] = { >> + { "A", "B" }, >> + { "AB", "AC" }, >> + { "AB", "AC" } > > Shan't we put indices here? Do you mean: static const char yas5xx_version_name[][2][3] = { [yas530] = { "A", "B" }, [yas532] = { "AB", "AC" }, [yas533] = { "AB", "AC" }, }; I can add this. > Also, use * instead of one dimension of array. Sorry, probably I lack the basic knowledge here. Can you explain how to do that? ... >> +static const int yas530_volatile_reg[] = { >> + YAS530_ACTUATE_INIT_COIL, >> + YAS530_MEASURE > > + Comma. OK, I didn't know this is allowed. I'll add it here and at the other locations. >> +/* Number of counts between minimum and reference temperature */ >> +const u16 t_ref_counts[] = { 182, 390, 390 }; >> + >> +/* Starting point of temperature counting in 1/10:s degrees Celsius */ >> +const s16 min_temp_celcius_x10[] = { -620, -500, -500 }; > > See above. The former comments... 182 /* Counts starting at -62 °C */ 390 /* Counts starting at -50 °C */ ... and the two new comments above the arrays actually should say the same thing. At least that was my intention. The first value is a number of counts. And the counting starts at a certain temperature, which is the second value. Both the old and the new comments are intended to describe this. In the introduction of this temperature handling (patch 4), there is a lot more description on these values in function yas5xx_get_measure(). When creating the new "chip_info" patch 9, I was thinking about moving some of that description up here to these arrays. However, instead I tried to following Jonathan's suggestion in v3 to keep the describing text low and rather let the formulas speak for themselves. These values are applied at a formula in function yas5xx_calc_temperature() which is supposed to by kind of self explanatory. What may lead to confusion is the equivalent usage of "starting" and "minimum" here. In the initial patchset I used "starting" related to the counts, Jonathan suggested "minimum" or actually "min_temp" related to the temperature. The comments here above are bit of a mixture of both. I still think it's good enough to understand. The sentence "Starting point of temperature ..." describes the value min_temp_celcius_x10. Using a term like "starting temperature" would probably be more confusing. >> +struct yas5xx_chip_info { >> + unsigned int devid; > >> + const int *volatile_reg; >> + const int volatile_reg_qty; >> + const u32 scaling_val2; > > Why const here? > I assume entire structure is const, no? I'm rather new to C language, not having a good grasp of "const" in structures yet. I would have guessed it doesn't work well with the function pointers. However, having some compile tests, there are no complaints about the function pointers. To change the whole chip_info structure to "const", I would: - within the "struct yas5xx" change to "const struct yas5xx_chip_info *chip_info;" - change following typedef to "static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = ..." Then, within the "struct yas5xx_chip_info", I can remove "const" from: - int volatile_reg_qty; - u32 scaling_val2; However, I have to keep "const" at the following, otherwise the complier complains that "initialization discards 'const' qualifier from pointer target type": - const int *volatile_reg; Summarizing, after the changes it would look like the following (snippets): struct yas5xx_chip_info { unsigned int devid; const int *volatile_reg; int volatile_reg_qty; u32 scaling_val2; int (*get_measure)(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo); int (*get_calibration_data)(struct yas5xx *yas5xx); void (*dump_calibration)(struct yas5xx *yas5xx); int (*measure_offsets)(struct yas5xx *yas5xx); int (*power_on)(struct yas5xx *yas5xx); }; struct yas5xx { struct device *dev; enum chip_ids chip; const struct yas5xx_chip_info *chip_info; ... }; static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { [yas530] = { ... }, }; If that's reasonable, I'll change it that way. ... >> + int i, j; > > j can have a proper name. Ok, makes sense. I'll probably name it "reg_qty". >> + j = yas5xx->chip_info->volatile_reg_qty; > ... >> - int ret; >> + int id_check, ret; > > Don't add variables with different semantics on the same line. Ok. I wasn't aware of putting different semantics on different lines, thanks for the hint, makes sense. >> + if (id_check != yas5xx->chip_info->devid) { >> ret = -ENODEV; >> - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid); >> + dev_err(dev, "device ID %02x doesn't match %s\n", >> + id_check, id->name); > > ret = dev_err_probe() ? Makes sense, will change that. Though I have difficulties to implement this nicely. dev_err_probe() requires an "error value to test". The current "if (id_check != yas5xx->chip_info->devid)" doesn't offer such a value by itself. Do you think the following would be appropriate, nesting the check within the dev_err_probe()? It doesn't look too good to me. ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid, "device ID %02x doesn't match %s\n", id_check, id->name); if (ret) goto assert_reset; If there are no better ideas, I would implement it that way. Additionally adding a comment and putting it into a block with the regmap_read before: /* * Check if the device ID from the register matches the one set * in the Device Tree. */ ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check); if (ret) goto assert_reset; ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid, "device ID %02x doesn't match %s\n", id_check, id->name); if (ret) goto assert_reset; Hm, I think it's a bit hard to read. Suggestions for improvement are welcome. Otherwise I'd add it that way. ... >> + dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip], >> + yas5xx_version_name[yas5xx->chip][yas5xx->version]); > > I'm wondering if these arrays can be actually embedded into chip_info? While the variants (like "YAS530") are listed in the chip_info, the versions (like "A") are not. Yet, including the versions in chip_info would double the amount, making it visually more unclear, with only minor benefit. The first part of this call, the "product name", applies to the variants. Going the detour via chip_info, the array element to call could just be hard-coded in the chip_info table, like: static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { [yas530] = { ... .product_name = yas5xx_product_name[0], ... The second part, the "version name", is currently in the three-dimensional array yas5xx_version_name[]. The first dimension is the variant, this would need to be hard-coded in the chip_info table, similar to the example above. The second dimension is the version, this would need to come from within the yas5xx_probe() the function. I couldn't find a way how to assign one dimension in one place and another dimension in another place. To include the second part in the chip_info, I would split the three-dimensional array yas5xx_version_name[] into three separate two-dimensional arrays, one per variant. It would look like this (snippets): static const char yas530_version_name[][3] = { "A", "B" }; static const char yas532_version_name[][3] = { "AB", "AC" }; static const char yas533_version_name[][3] = { "AB", "AC" }; ... struct yas5xx_chip_info { ... const char *product_name; const char (*version_name)[3]; ... ... static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { [yas530] = { ... .product_name = yas5xx_product_name[0], .version_name = yas530_version_name, ... ... dev_info(dev, "detected %s %s\n", yas5xx->chip_info->product_name, yas5xx->chip_info->version_name[yas5xx->version]); I'm not fully sure this is the best way. Also note that I had to set the size of the second dimension of yas530_version_name to 3. Do you think I should do it this way to include "product_name" and "version_name" in the chip_info? If yes, should I probably do a similar thing for the values "t_ref_counts" and "min_temp_celcius_x10"? Here as well, the array elements are directly called in function yas5xx_calc_temperature() without using the chip_info structure. Also I noticed that I should add "static" to the typedef of arrays "t_ref_counts" and "min_temp_celcius_x10". Darn, and I have to correct the spelling of "celcius" into "celsius" in "min_temp_celcius_x10". Kind regards, Jakob
On Wed, Jul 27, 2022 at 12:01 AM Jakob Hauser <jahau@rocketmail.com> wrote: > On 04.07.22 21:37, Andy Shevchenko wrote: > > On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote: ... > > Possible alternative would be more steps in 2), i.e. introducing > > chip_info for the callback only, then add field (or semantically > > unified fields) by field with corresponding changes in the code. In > > this case it would be easier to review. > > What do you mean by "introducing chip_info for the callback only"? I > guess you mean the function pointers? Yes. > According to this, the patch could be split into: > 1) introduce function pointers > 2) introduce arrays to look up values > > However, what's missing there is the introduction of the chip_info > approach as such. Admittedly difficult to follow are the changes in > yas5xx_probe(): > - yas5xx->devid gets removed, it was the main switch decider before > - (for usage on other switches it is moved to yas5xx->chip_info->devid) > - yas5xx->chip as an enumeration is newly introduced > - yas5xx->chip_info as a structure gets initiated > - As the chip_info now chooses according to the i2c_device_id (via > enum chip_ids, thus yas5xx->chip), a new check is introduced to > make sure it matches the device ID read from the register. This > is done by "if (id_check != yas5xx->chip_info->devid)". > - The way of reporting product name and version name was changed > because introducing chip_info required to do this in one place > for all versions. > - As a consequence of this, yas5xx->name became obsolete. > > This would have to be done before introducing function pointers and > arrays, like: > 1) introduce chip_info structural changes > 2) add function pointers > 3) add arrays to look up values > > But it can't be easily split that way. I can't establish an "empty" > chip_info approach as a fist step without removing or changing many > other things. The structural changes are related to other changes in > that patch. > > I'm thinking about first introducing chip_info and thereafter > establishing the function pointers. In between I could try to split out > the new temperature function: > 1) introduce chip_info structural changes incl. arrays to look up values > 2) split out new yas5xx_calc_temperature() function > 3) add function pointers > > I'm not yet sure it can be split that way because things are entangled. > But I will try to this in v5. Yes, my main point is to try to see under different angles on how you can split the series. We have plenty of time. > Btw, looking through this, I realized that in patch 6 "Rename functions > and registers" I unfortunately missed to rename instances of > "yas5xx_get_measure", "yas5xx_power_on", "YAS5XX_ACTUATE_INIT_COIL" and > "YAS5XX_MEASURE". I'll correct this in v5. Also my point why smaller changes are better. ... > >> -#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */ > > > >> -#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */ > > > > The comments suddenly disappear from the file. See below. > > The comment's didn't actually disappear but rather got restructured by > introducing new chip_info approach. See below. See below. ... > >> +static const char yas5xx_version_name[][2][3] = { > >> + { "A", "B" }, > >> + { "AB", "AC" }, > >> + { "AB", "AC" } > > > > Shan't we put indices here? > > Do you mean: > > static const char yas5xx_version_name[][2][3] = { > [yas530] = { "A", "B" }, > [yas532] = { "AB", "AC" }, > [yas533] = { "AB", "AC" }, > }; > > I can add this. Yes. > > Also, use * instead of one dimension of array. > > Sorry, probably I lack the basic knowledge here. Can you explain how to > do that? static const char *_name[][] = { }; ? ... > >> +/* Number of counts between minimum and reference temperature */ > >> +const u16 t_ref_counts[] = { 182, 390, 390 }; > >> + > >> +/* Starting point of temperature counting in 1/10:s degrees Celsius */ > >> +const s16 min_temp_celcius_x10[] = { -620, -500, -500 }; > > > > See above. > > The former comments... > 182 /* Counts starting at -62 °C */ > 390 /* Counts starting at -50 °C */ > > ... and the two new comments above the arrays actually should say the > same thing. At least that was my intention. > > The first value is a number of counts. And the counting starts at a > certain temperature, which is the second value. Both the old and the new > comments are intended to describe this. > > In the introduction of this temperature handling (patch 4), there is a > lot more description on these values in function yas5xx_get_measure(). > When creating the new "chip_info" patch 9, I was thinking about moving > some of that description up here to these arrays. However, instead I > tried to following Jonathan's suggestion in v3 to keep the describing > text low and rather let the formulas speak for themselves. These values > are applied at a formula in function yas5xx_calc_temperature() which is > supposed to by kind of self explanatory. > > What may lead to confusion is the equivalent usage of "starting" and > "minimum" here. In the initial patchset I used "starting" related to the > counts, Jonathan suggested "minimum" or actually "min_temp" related to > the temperature. The comments here above are bit of a mixture of both. I > still think it's good enough to understand. The sentence "Starting point > of temperature ..." describes the value min_temp_celcius_x10. Using a > term like "starting temperature" would probably be more confusing. Confusing to me, who doesn't know the specifics of the chip, it is easy to read '-62 °C' vs. "read comment, look into an array, calculate in your brain". ... > >> +struct yas5xx_chip_info { > >> + unsigned int devid; > > > >> + const int *volatile_reg; > >> + const int volatile_reg_qty; > >> + const u32 scaling_val2; > > > > Why const here? > > I assume entire structure is const, no? > > I'm rather new to C language, not having a good grasp of "const" in > structures yet. I would have guessed it doesn't work well with the > function pointers. > > However, having some compile tests, there are no complaints about the > function pointers. > > To change the whole chip_info structure to "const", I would: > - within the "struct yas5xx" change to "const struct > yas5xx_chip_info *chip_info;" > - change following typedef to "static const struct > yas5xx_chip_info yas5xx_chip_info_tbl[] = ..." > > Then, within the "struct yas5xx_chip_info", I can remove "const" from: > - int volatile_reg_qty; > - u32 scaling_val2; > > However, I have to keep "const" at the following, otherwise the complier > complains that "initialization discards 'const' qualifier from pointer > target type": > - const int *volatile_reg; > > Summarizing, after the changes it would look like the following (snippets): > > struct yas5xx_chip_info { > unsigned int devid; > const int *volatile_reg; > int volatile_reg_qty; > u32 scaling_val2; > int (*get_measure)(struct yas5xx *yas5xx, s32 *to, > s32 *xo, s32 *yo, s32 *zo); > int (*get_calibration_data)(struct yas5xx *yas5xx); > void (*dump_calibration)(struct yas5xx *yas5xx); > int (*measure_offsets)(struct yas5xx *yas5xx); > int (*power_on)(struct yas5xx *yas5xx); > }; > > struct yas5xx { > struct device *dev; > enum chip_ids chip; > const struct yas5xx_chip_info *chip_info; > > ... > > }; > > static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > [yas530] = { > > ... > > }, > }; > > If that's reasonable, I'll change it that way. Yes, chip_info should be immutable, right? Otherwise it's something we may not rely on. > >> + if (id_check != yas5xx->chip_info->devid) { > >> ret = -ENODEV; > >> - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid); > >> + dev_err(dev, "device ID %02x doesn't match %s\n", > >> + id_check, id->name); > > > > ret = dev_err_probe() ? > > Makes sense, will change that. > > Though I have difficulties to implement this nicely. dev_err_probe() > requires an "error value to test". The current "if (id_check != > yas5xx->chip_info->devid)" doesn't offer such a value by itself. > > Do you think the following would be appropriate, nesting the check > within the dev_err_probe()? It doesn't look too good to me. > > ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid, > "device ID %02x doesn't match %s\n", > id_check, id->name); > if (ret) > goto assert_reset; > > If there are no better ideas, I would implement it that way. > Additionally adding a comment and putting it into a block with the > regmap_read before: > > /* > * Check if the device ID from the register matches the one set > * in the Device Tree. > */ > ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check); > if (ret) > goto assert_reset; > ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid, > "device ID %02x doesn't match %s\n", > id_check, id->name); > if (ret) > goto assert_reset; > > Hm, I think it's a bit hard to read. Suggestions for improvement are > welcome. Otherwise I'd add it that way. It should be like: if (id_check != ...) return dev_err_probe(dev, -ENODEV, ...); ... > >> + dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip], > >> + yas5xx_version_name[yas5xx->chip][yas5xx->version]); > > > > I'm wondering if these arrays can be actually embedded into chip_info? > > While the variants (like "YAS530") are listed in the chip_info, the > versions (like "A") are not. Yet, including the versions in chip_info > would double the amount, making it visually more unclear, with only > minor benefit. > > The first part of this call, the "product name", applies to the > variants. Going the detour via chip_info, the array element to call > could just be hard-coded in the chip_info table, like: > > static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > [yas530] = { > ... > .product_name = yas5xx_product_name[0], > ... > > The second part, the "version name", is currently in the > three-dimensional array yas5xx_version_name[]. The first dimension is > the variant, this would need to be hard-coded in the chip_info table, > similar to the example above. The second dimension is the version, this > would need to come from within the yas5xx_probe() the function. I > couldn't find a way how to assign one dimension in one place and another > dimension in another place. > > To include the second part in the chip_info, I would split the > three-dimensional array yas5xx_version_name[] into three separate > two-dimensional arrays, one per variant. It would look like this (snippets): > > > static const char yas530_version_name[][3] = { "A", "B" }; > > static const char yas532_version_name[][3] = { "AB", "AC" }; > > static const char yas533_version_name[][3] = { "AB", "AC" }; > > ... > > struct yas5xx_chip_info { > ... > const char *product_name; > const char (*version_name)[3]; > ... > > ... > > static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > [yas530] = { > ... > .product_name = yas5xx_product_name[0], > .version_name = yas530_version_name, > ... > > ... > > > dev_info(dev, "detected %s %s\n", > yas5xx->chip_info->product_name, > yas5xx->chip_info->version_name[yas5xx->version]); > > > I'm not fully sure this is the best way. Also note that I had to set the > size of the second dimension of yas530_version_name to 3. > > Do you think I should do it this way to include "product_name" and > "version_name" in the chip_info? Again, you are the author and my point is just to make you look at this from different angles. If you see no better way, go with the current approach, but just spend a half an hour and perhaps we may have a cleaner solution? > If yes, should I probably do a similar thing for the values > "t_ref_counts" and "min_temp_celcius_x10"? Here as well, the array > elements are directly called in function yas5xx_calc_temperature() > without using the chip_info structure. > > Also I noticed that I should add "static" to the typedef of arrays > "t_ref_counts" and "min_temp_celcius_x10". Darn, and I have to correct > the spelling of "celcius" into "celsius" in "min_temp_celcius_x10".
Hi Andy, On 27.07.22 19:39, Andy Shevchenko wrote: > On Wed, Jul 27, 2022 at 12:01 AM Jakob Hauser <jahau@rocketmail.com> wrote: >> On 04.07.22 21:37, Andy Shevchenko wrote: >>> On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@rocketmail.com> wrote: > > .. > >>> Possible alternative would be more steps in 2), i.e. introducing >>> chip_info for the callback only, then add field (or semantically >>> unified fields) by field with corresponding changes in the code. In >>> this case it would be easier to review. >> >> What do you mean by "introducing chip_info for the callback only"? I >> guess you mean the function pointers? > > Yes. > >> According to this, the patch could be split into: >> 1) introduce function pointers >> 2) introduce arrays to look up values >> >> However, what's missing there is the introduction of the chip_info >> approach as such. Admittedly difficult to follow are the changes in >> yas5xx_probe(): >> - yas5xx->devid gets removed, it was the main switch decider before >> - (for usage on other switches it is moved to yas5xx->chip_info->devid) >> - yas5xx->chip as an enumeration is newly introduced >> - yas5xx->chip_info as a structure gets initiated >> - As the chip_info now chooses according to the i2c_device_id (via >> enum chip_ids, thus yas5xx->chip), a new check is introduced to >> make sure it matches the device ID read from the register. This >> is done by "if (id_check != yas5xx->chip_info->devid)". >> - The way of reporting product name and version name was changed >> because introducing chip_info required to do this in one place >> for all versions. >> - As a consequence of this, yas5xx->name became obsolete. >> >> This would have to be done before introducing function pointers and >> arrays, like: >> 1) introduce chip_info structural changes >> 2) add function pointers >> 3) add arrays to look up values >> >> But it can't be easily split that way. I can't establish an "empty" >> chip_info approach as a fist step without removing or changing many >> other things. The structural changes are related to other changes in >> that patch. >> >> I'm thinking about first introducing chip_info and thereafter >> establishing the function pointers. In between I could try to split out >> the new temperature function: >> 1) introduce chip_info structural changes incl. arrays to look up values >> 2) split out new yas5xx_calc_temperature() function >> 3) add function pointers >> >> I'm not yet sure it can be split that way because things are entangled. >> But I will try to this in v5. > > Yes, my main point is to try to see under different angles on how you > can split the series. We have plenty of time. OK, I'll try it like this in v5. ... >>>> -#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */ >>> >>>> -#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */ >>> >>> The comments suddenly disappear from the file. See below. >> >> The comment's didn't actually disappear but rather got restructured by >> introducing new chip_info approach. See below. > > See below. > > .. > ... >>> Also, use * instead of one dimension of array. >> >> Sorry, probably I lack the basic knowledge here. Can you explain how to >> do that? > > static const char *_name[][] = { > }; > > ? Indeed I was missing that basic knowledge. Thanks for showing! After some trying and reading, I think it should be: static const char * const _name[][] = { }; >>>> +/* Number of counts between minimum and reference temperature */ >>>> +const u16 t_ref_counts[] = { 182, 390, 390 }; >>>> + >>>> +/* Starting point of temperature counting in 1/10:s degrees Celsius */ >>>> +const s16 min_temp_celcius_x10[] = { -620, -500, -500 }; >>> >>> See above. >> >> The former comments... >> 182 /* Counts starting at -62 °C */ >> 390 /* Counts starting at -50 °C */ >> >> ... and the two new comments above the arrays actually should say the >> same thing. At least that was my intention. >> >> The first value is a number of counts. And the counting starts at a >> certain temperature, which is the second value. Both the old and the new >> comments are intended to describe this. >> >> In the introduction of this temperature handling (patch 4), there is a >> lot more description on these values in function yas5xx_get_measure(). >> When creating the new "chip_info" patch 9, I was thinking about moving >> some of that description up here to these arrays. However, instead I >> tried to following Jonathan's suggestion in v3 to keep the describing >> text low and rather let the formulas speak for themselves. These values >> are applied at a formula in function yas5xx_calc_temperature() which is >> supposed to by kind of self explanatory. >> >> What may lead to confusion is the equivalent usage of "starting" and >> "minimum" here. In the initial patchset I used "starting" related to the >> counts, Jonathan suggested "minimum" or actually "min_temp" related to >> the temperature. The comments here above are bit of a mixture of both. I >> still think it's good enough to understand. The sentence "Starting point >> of temperature ..." describes the value min_temp_celcius_x10. Using a >> term like "starting temperature" would probably be more confusing. > > Confusing to me, who doesn't know the specifics of the chip, it is > easy to read '-62 °C' vs. "read comment, look into an array, calculate > in your brain". I don't have a good idea how to fulfill your requirement in a brief way when the values are stored in an array altogether. Instead I end up at a longer comment again. Though this also offers the chance to add some additional information where the values were taken from. Is it appropriate to include this to kernel doc? Generally I'm unsure on kernel doc but I guess yes... It would look like: /** * const u16 t_ref_counts[] - Number of counts at reference temperature * * The temperature value at YAS magnetometers is a number of counts. The * values in t_ref_counts[] are the counts at the reference temperature * of 20 °C. * * For YAS532/533, this value is known from the Android driver. For * YAS530, it was approximately measured. * / static const u16 t_ref_counts[] = { 182, 390, 390, }; /** * const s16 min_temp_celsius_x10[] - Starting point of temperature * counting in 1/10:s degrees Celsius * * The array min_temp_celsius_x10[] contains the temperatures where the * temperature value count is 0. The values are in 1/10:s degrees * Celsius to ease the further temperature calculation. * * These temperatures are derived from the temperature resolutions given * in the data sheets. */ static const s16 min_temp_celsius_x10[] = { -620, -500, -500, }; Please note that I have to touch and extend these comments in the last patch "Add YAS537 variant". In the first comment I'll add that the value for YAS537 was approximately measured too. And in the second command I have to add the word "theoretical" temperatures because on YAS537 it is at -386 °C, which is below absolute zero Kelvin, thus a mere theoretical temperature. >>>> +struct yas5xx_chip_info { >>>> + unsigned int devid; >>> >>>> + const int *volatile_reg; >>>> + const int volatile_reg_qty; >>>> + const u32 scaling_val2; >>> >>> Why const here? >>> I assume entire structure is const, no? >> >> I'm rather new to C language, not having a good grasp of "const" in >> structures yet. I would have guessed it doesn't work well with the >> function pointers. >> >> However, having some compile tests, there are no complaints about the >> function pointers. >> >> To change the whole chip_info structure to "const", I would: >> - within the "struct yas5xx" change to "const struct >> yas5xx_chip_info *chip_info;" >> - change following typedef to "static const struct >> yas5xx_chip_info yas5xx_chip_info_tbl[] = ..." >> >> Then, within the "struct yas5xx_chip_info", I can remove "const" from: >> - int volatile_reg_qty; >> - u32 scaling_val2; >> >> However, I have to keep "const" at the following, otherwise the complier >> complains that "initialization discards 'const' qualifier from pointer >> target type": >> - const int *volatile_reg; >> >> Summarizing, after the changes it would look like the following (snippets): >> >> struct yas5xx_chip_info { >> unsigned int devid; >> const int *volatile_reg; >> int volatile_reg_qty; >> u32 scaling_val2; >> int (*get_measure)(struct yas5xx *yas5xx, s32 *to, >> s32 *xo, s32 *yo, s32 *zo); >> int (*get_calibration_data)(struct yas5xx *yas5xx); >> void (*dump_calibration)(struct yas5xx *yas5xx); >> int (*measure_offsets)(struct yas5xx *yas5xx); >> int (*power_on)(struct yas5xx *yas5xx); >> }; >> >> struct yas5xx { >> struct device *dev; >> enum chip_ids chip; >> const struct yas5xx_chip_info *chip_info; >> >> ... >> >> }; >> >> static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { >> [yas530] = { >> >> ... >> >> }, >> }; >> >> If that's reasonable, I'll change it that way. > > Yes, chip_info should be immutable, right? Otherwise it's something we > may not rely on. OK >>>> + if (id_check != yas5xx->chip_info->devid) { >>>> ret = -ENODEV; >>>> - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid); >>>> + dev_err(dev, "device ID %02x doesn't match %s\n", >>>> + id_check, id->name); >>> >>> ret = dev_err_probe() ? >> >> Makes sense, will change that. >> >> Though I have difficulties to implement this nicely. dev_err_probe() >> requires an "error value to test". The current "if (id_check != >> yas5xx->chip_info->devid)" doesn't offer such a value by itself. >> >> Do you think the following would be appropriate, nesting the check >> within the dev_err_probe()? It doesn't look too good to me. >> >> ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid, >> "device ID %02x doesn't match %s\n", >> id_check, id->name); >> if (ret) >> goto assert_reset; >> >> If there are no better ideas, I would implement it that way. >> Additionally adding a comment and putting it into a block with the >> regmap_read before: >> >> /* >> * Check if the device ID from the register matches the one set >> * in the Device Tree. >> */ >> ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check); >> if (ret) >> goto assert_reset; >> ret = dev_err_probe(dev, id_check != yas5xx->chip_info->devid, >> "device ID %02x doesn't match %s\n", >> id_check, id->name); >> if (ret) >> goto assert_reset; >> >> Hm, I think it's a bit hard to read. Suggestions for improvement are >> welcome. Otherwise I'd add it that way. > > It should be like: > > if (id_check != ...) > return dev_err_probe(dev, -ENODEV, ...); Ah! Thanks :) >>>> + dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip], >>>> + yas5xx_version_name[yas5xx->chip][yas5xx->version]); >>> >>> I'm wondering if these arrays can be actually embedded into chip_info? >> >> While the variants (like "YAS530") are listed in the chip_info, the >> versions (like "A") are not. Yet, including the versions in chip_info >> would double the amount, making it visually more unclear, with only >> minor benefit. >> >> The first part of this call, the "product name", applies to the >> variants. Going the detour via chip_info, the array element to call >> could just be hard-coded in the chip_info table, like: >> >> static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { >> [yas530] = { >> ... >> .product_name = yas5xx_product_name[0], >> ... >> >> The second part, the "version name", is currently in the >> three-dimensional array yas5xx_version_name[]. The first dimension is >> the variant, this would need to be hard-coded in the chip_info table, >> similar to the example above. The second dimension is the version, this >> would need to come from within the yas5xx_probe() the function. I >> couldn't find a way how to assign one dimension in one place and another >> dimension in another place. >> >> To include the second part in the chip_info, I would split the >> three-dimensional array yas5xx_version_name[] into three separate >> two-dimensional arrays, one per variant. It would look like this (snippets): >> >> >> static const char yas530_version_name[][3] = { "A", "B" }; >> >> static const char yas532_version_name[][3] = { "AB", "AC" }; >> >> static const char yas533_version_name[][3] = { "AB", "AC" }; >> >> ... >> >> struct yas5xx_chip_info { >> ... >> const char *product_name; >> const char (*version_name)[3]; >> ... >> >> ... >> >> static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { >> [yas530] = { >> ... >> .product_name = yas5xx_product_name[0], >> .version_name = yas530_version_name, >> ... >> >> ... >> >> >> dev_info(dev, "detected %s %s\n", >> yas5xx->chip_info->product_name, >> yas5xx->chip_info->version_name[yas5xx->version]); >> >> >> I'm not fully sure this is the best way. Also note that I had to set the >> size of the second dimension of yas530_version_name to 3. >> >> Do you think I should do it this way to include "product_name" and >> "version_name" in the chip_info? > > Again, you are the author and my point is just to make you look at > this from different angles. If you see no better way, go with the > current approach, but just spend a half an hour and perhaps we may > have a cleaner solution? Indeed, to my own surprise I found a solution with the 2D array: static const char * const yas5xx_product_name[] = { "YAS530 MS-3E", "YAS532 MS-3R", "YAS533 MS-3F", }; static const char * const yas5xx_version_name[][2] = { [yas530] = { "A", "B" }, [yas532] = { "AB", "AC" }, [yas533] = { "AB", "AC" }, }; ... struct yas5xx_chip_info { ... const char *product_name; const char * const *version_name; ... }; static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { [yas530] = { ... .product_name = yas5xx_product_name[0], .version_name = yas5xx_version_name[0], ... }, }; ... dev_info(dev, "detected %s %s\n", yas5xx->chip_info->product_name, yas5xx->chip_info->version_name[yas5xx->version]); >> If yes, should I probably do a similar thing for the values >> "t_ref_counts" and "min_temp_celcius_x10"? Here as well, the array >> elements are directly called in function yas5xx_calc_temperature() >> without using the chip_info structure. I then will include "product_name" and "version_name" as well as "t_ref_counts" and "min_temp_celsius_x10" to the chip_info struct to have everything collected. ... Kind regards, Jakob
On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote: > On 27.07.22 19:39, Andy Shevchenko wrote: > > On Wed, Jul 27, 2022 at 12:01 AM Jakob Hauser <jahau@rocketmail.com> wrote: ... > Instead I end up at a longer comment again. It's fine! > Though this also offers the chance to add some additional information > where the values were taken from. > > Is it appropriate to include this to kernel doc? Generally I'm unsure on > kernel doc but I guess yes... I'm unsure if it's appropriate for static (integer) arrays, you may run kernel doc script yourself and check man, html and pdf formats to see if it's rendered correctly. ... > > Again, you are the author and my point is just to make you look at > > this from different angles. If you see no better way, go with the > > current approach, but just spend a half an hour and perhaps we may > > have a cleaner solution? > > Indeed, to my own surprise I found a solution with the 2D array: It looks better! ... > dev_info(dev, "detected %s %s\n", > yas5xx->chip_info->product_name, > yas5xx->chip_info->version_name[yas5xx->version]); Very good!
On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote: > On 27.07.22 19:39, Andy Shevchenko wrote: ... Just a couple of remarks. > Indeed, to my own surprise I found a solution with the 2D array: > > static const char * const yas5xx_product_name[] = { > "YAS530 MS-3E", > "YAS532 MS-3R", > "YAS533 MS-3F", > }; > > static const char * const yas5xx_version_name[][2] = { yas5xx_version_names (note S at the end) > [yas530] = { "A", "B" }, > [yas532] = { "AB", "AC" }, > [yas533] = { "AB", "AC" }, > }; > > ... > > struct yas5xx_chip_info { > ... > const char *product_name; > const char * const *version_name; > ... > }; > > static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { > [yas530] = { > ... > .product_name = yas5xx_product_name[0], > .version_name = yas5xx_version_name[0], Also 0 --> yas530 (use enum:ed indices) > }, > }; >
Hi Andy, On 29.07.22 18:08, Andy Shevchenko wrote: > On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote: ... >> Is it appropriate to include this to kernel doc? Generally I'm unsure on >> kernel doc but I guess yes... > > I'm unsure if it's appropriate for static (integer) arrays, you may > run kernel doc script yourself and check man, html and pdf formats to > see if it's rendered correctly. Nope, doesn't work... It was a beginner's mistake on my side. I did check Documentation/doc-guide/kernel-doc.rst before and followed the "typedef" part, which is quite short. I thought "typedef" is a more general designation – but after reading more on "typedef" I now know what it is :/ https://github.com/torvalds/linux/blob/v5.18/Documentation/doc-guide/kernel-doc.rst#typedef-documentation So, as far as I can see, kernel doc applies to functions, structures, unions, enumerations and typedefs only. I would then switch the comments to regular comments: /* * t_ref_counts[] is the number of counts at reference temperature. * * The temperature value at YAS magnetometers is a number of counts. The * values in t_ref_counts[] are the counts at the reference temperature * of 20 °C. * * For YAS532/533, this value is known from the Android driver. For * YAS530, it was approximately measured. */ static const u16 t_ref_counts[] = { 182, 390, 390, }; /* * min_temp_celsius_x10[] is the starting point of temperature counting * in 1/10:s degrees Celsius. * * The array min_temp_celsius_x10[] contains the temperatures where the * temperature value count is 0. The values are in 1/10:s degrees * Celsius to ease the further temperature calculation. * * These temperatures are derived from the temperature resolutions given * in the data sheets. */ static const s16 min_temp_celsius_x10[] = { -620, -500, -500, }; ... Kind regards, Jakob
Hi Andy, On 29.07.22 18:13, Andy Shevchenko wrote: > On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote: > > .. > > Just a couple of remarks. > >> Indeed, to my own surprise I found a solution with the 2D array: >> >> static const char * const yas5xx_product_name[] = { >> "YAS530 MS-3E", >> "YAS532 MS-3R", >> "YAS533 MS-3F", >> }; >> >> static const char * const yas5xx_version_name[][2] = { > > yas5xx_version_names (note S at the end) As I understand you, it's to be applied on "yas5xx_version_names" only. In the chip_info table, it would then look like: .product_name = yas5xx_product_name[yas530], .version_name = yas5xx_version_names[yas530], ^ S > >> [yas530] = { "A", "B" }, >> [yas532] = { "AB", "AC" }, >> [yas533] = { "AB", "AC" }, >> }; >> >> ... >> >> struct yas5xx_chip_info { >> ... >> const char *product_name; >> const char * const *version_name; >> ... >> }; >> >> static const struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { >> [yas530] = { >> ... >> .product_name = yas5xx_product_name[0], >> .version_name = yas5xx_version_name[0], > > Also 0 --> yas530 (use enum:ed indices) OK > >> }, >> }; >> > Kind regards, Jakob
On Sat, Jul 30, 2022 at 12:56 AM Jakob Hauser <jahau@rocketmail.com> wrote: > On 29.07.22 18:13, Andy Shevchenko wrote: > > On Fri, Jul 29, 2022 at 1:06 AM Jakob Hauser <jahau@rocketmail.com> wrote: ... > >> static const char * const yas5xx_version_name[][2] = { > > > > yas5xx_version_names (note S at the end) > > As I understand you, it's to be applied on "yas5xx_version_names" only. > In the chip_info table, it would then look like: > > .product_name = yas5xx_product_name[yas530], > .version_name = yas5xx_version_names[yas530], > ^ > S Yes. > >> [yas530] = { "A", "B" }, > >> [yas532] = { "AB", "AC" }, > >> [yas533] = { "AB", "AC" }, > >> };
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c index 4e2f460a4efd..ce9c1077c121 100644 --- a/drivers/iio/magnetometer/yamaha-yas530.c +++ b/drivers/iio/magnetometer/yamaha-yas530.c @@ -79,7 +79,6 @@ #define YAS530_DATA_BITS 12 #define YAS530_DATA_CENTER BIT(YAS530_DATA_BITS - 1) #define YAS530_DATA_OVERFLOW (BIT(YAS530_DATA_BITS) - 1) -#define YAS530_20DEGREES 182 /* Counts starting at -62 °C */ #define YAS532_DEVICE_ID 0x02 /* YAS532/YAS533 (MS-3R/F) */ #define YAS532_VERSION_AB 0 /* YAS532/533 AB (MS-3R/F AB) */ @@ -91,11 +90,39 @@ #define YAS532_DATA_BITS 13 #define YAS532_DATA_CENTER BIT(YAS532_DATA_BITS - 1) #define YAS532_DATA_OVERFLOW (BIT(YAS532_DATA_BITS) - 1) -#define YAS532_20DEGREES 390 /* Counts starting at -50 °C */ /* Turn off device regulators etc after 5 seconds of inactivity */ #define YAS5XX_AUTOSUSPEND_DELAY_MS 5000 +enum chip_ids { + yas530, + yas532, + yas533, +}; + +static const char yas5xx_product_name[][13] = { + "YAS530 MS-3E", + "YAS532 MS-3R", + "YAS533 MS-3F" +}; + +static const char yas5xx_version_name[][2][3] = { + { "A", "B" }, + { "AB", "AC" }, + { "AB", "AC" } +}; + +static const int yas530_volatile_reg[] = { + YAS530_ACTUATE_INIT_COIL, + YAS530_MEASURE +}; + +/* Number of counts between minimum and reference temperature */ +const u16 t_ref_counts[] = { 182, 390, 390 }; + +/* Starting point of temperature counting in 1/10:s degrees Celsius */ +const s16 min_temp_celcius_x10[] = { -620, -500, -500 }; + struct yas5xx_calibration { /* Linearization calibration x, y1, y2 */ s32 r[3]; @@ -110,12 +137,38 @@ struct yas5xx_calibration { u8 dck; }; +struct yas5xx; + +/** + * struct yas5xx_chip_info - device-specific data and function pointers + * @devid: device ID number + * @volatile_reg: device-specific volatile registers + * @volatile_reg_qty: quantity of device-specific volatile registers + * @scaling_val2: scaling value for IIO_CHAN_INFO_SCALE + * @get_measure: function pointer to get a measurement + * @get_calibration_data: function pointer to get calibration data + * @dump_calibration: function pointer to dump calibration for debugging + * @measure_offsets: function pointer to measure the offsets + * @power_on: function pointer to power-on procedure + */ +struct yas5xx_chip_info { + unsigned int devid; + const int *volatile_reg; + const int volatile_reg_qty; + const u32 scaling_val2; + int (*get_measure)(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo); + int (*get_calibration_data)(struct yas5xx *yas5xx); + void (*dump_calibration)(struct yas5xx *yas5xx); + int (*measure_offsets)(struct yas5xx *yas5xx); + int (*power_on)(struct yas5xx *yas5xx); +}; + /** * struct yas5xx - state container for the YAS5xx driver * @dev: parent device pointer - * @devid: device ID number + * @chip: enumeration of the device variant + * @chip_info: device-specific data and function pointers * @version: device version - * @name: device name * @calibration: calibration settings from the OTP storage * @hard_offsets: offsets for each axis measured with initcoil actuated * @orientation: mounting matrix, flipped axis etc @@ -129,9 +182,9 @@ struct yas5xx_calibration { */ struct yas5xx { struct device *dev; - unsigned int devid; + enum chip_ids chip; + struct yas5xx_chip_info *chip_info; unsigned int version; - char name[16]; struct yas5xx_calibration calibration; s8 hard_offsets[3]; struct iio_mount_matrix orientation; @@ -221,7 +274,7 @@ static int yas530_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y mutex_unlock(&yas5xx->lock); - switch (yas5xx->devid) { + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: /* * The t value is 9 bits in big endian format @@ -275,7 +328,7 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis) s32 coef; /* Select coefficients */ - switch (yas5xx->devid) { + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: if (yas5xx->version == YAS530_VERSION_A) coef = YAS530_VERSION_A_COEF; @@ -305,6 +358,20 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis) (yas5xx->hard_offsets[axis] - c->r[axis]) * coef; } +static s32 yas5xx_calc_temperature(struct yas5xx *yas5xx, u16 t) +{ + s32 to; + u16 t_ref; + int min_temp_x10, ref_temp_x10; + + t_ref = t_ref_counts[yas5xx->chip]; + min_temp_x10 = min_temp_celcius_x10[yas5xx->chip]; + ref_temp_x10 = 200; + + to = (min_temp_x10 + ((ref_temp_x10 - min_temp_x10) * t / t_ref)) * 100; + return to; +} + /** * yas530_get_measure() - Measure a sample of all axis and process * @yas5xx: The device state @@ -318,7 +385,7 @@ static s32 yas530_linearize(struct yas5xx *yas5xx, u16 val, int axis) static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, s32 *zo) { struct yas5xx_calibration *c = &yas5xx->calibration; - u16 t_ref, t, x, y1, y2; + u16 t_ref, t_comp, t, x, y1, y2; /* These are signed x, signed y1 etc */ s32 sx, sy1, sy2, sy, sz; int ret; @@ -333,47 +400,30 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, sy1 = yas530_linearize(yas5xx, y1, 1); sy2 = yas530_linearize(yas5xx, y2, 2); - /* Set the temperature reference value (unit: counts) */ - switch (yas5xx->devid) { - case YAS530_DEVICE_ID: - t_ref = YAS530_20DEGREES; - break; - case YAS532_DEVICE_ID: - t_ref = YAS532_20DEGREES; - break; - default: - dev_err(yas5xx->dev, "unknown device type\n"); - return -EINVAL; - } - - /* Temperature compensation for x, y1, y2 respectively */ - if (yas5xx->devid == YAS532_DEVICE_ID && + /* + * Set the temperature for compensation (unit: counts): + * YAS532/YAS533 version AC uses the temperature deviation as a + * multiplier. YAS530 and YAS532 version AB use solely the t value. + */ + t_ref = t_ref_counts[yas5xx->chip]; + if (yas5xx->chip_info->devid == YAS532_DEVICE_ID && yas5xx->version == YAS532_VERSION_AC) { - /* - * YAS532 version AC uses the temperature deviation as a - * multiplier. - * - * Cx * (t - t_ref) - * x' = x - ---------------- - * 100 - */ - sx = sx - (c->Cx * (t - t_ref)) / 100; - sy1 = sy1 - (c->Cy1 * (t - t_ref)) / 100; - sy2 = sy2 - (c->Cy2 * (t - t_ref)) / 100; + t_comp = t - t_ref; } else { - /* - * YAS530 and YAS532 version AB use solely the t value as a - * multiplier. - * - * Cx * t - * x' = x - ------ - * 100 - */ - sx = sx - (c->Cx * t) / 100; - sy1 = sy1 - (c->Cy1 * t) / 100; - sy2 = sy2 - (c->Cy2 * t) / 100; + t_comp = t; } + /* + * Temperature compensation for x, y1, y2 respectively: + * + * Cx * t_comp + * x' = x - ----------- + * 100 + */ + sx = sx - (c->Cx * t_comp) / 100; + sy1 = sy1 - (c->Cy1 * t_comp) / 100; + sy2 = sy2 - (c->Cy2 * t_comp) / 100; + /* * Break y1 and y2 into y and z, y1 and y2 are apparently encoding * y and z. @@ -381,36 +431,8 @@ static int yas530_get_measure(struct yas5xx *yas5xx, s32 *to, s32 *xo, s32 *yo, sy = sy1 - sy2; sz = -sy1 - sy2; - /* Process temperature readout */ - switch (yas5xx->devid) { - case YAS530_DEVICE_ID: - /* - * Raw temperature value t is the number of counts starting - * at -62 °C. Reference value t_ref is the number of counts - * between -62 °C and 20 °C (82 °C range). - * - * Temperature in °C would be (82 / t_ref * t) - 62. - * - * Contrary to this, perform multiplication first and division - * second due to calculating with integers. - * - * To get a nicer result, calculate with 1/10:s degrees Celsius - * and finally multiply by 100 to return millidegrees Celsius. - */ - *to = ((820 * t / t_ref) - 620) * 100; - break; - case YAS532_DEVICE_ID: - /* - * Actually same procedure for YAS532 but the starting point is - * at -50 °C. Reference value t_ref is the number of counts - * between -50 °C and 20 °C (70 °C range). - */ - *to = ((700 * t / t_ref) - 500) * 100; - break; - default: - dev_err(yas5xx->dev, "unknown device type\n"); - return -EINVAL; - } + /* Calculate temperature readout */ + *to = yas5xx_calc_temperature(yas5xx, t); /* * Calibrate [x,y,z] with some formulas like this: @@ -447,7 +469,7 @@ 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); + ret = yas5xx->chip_info->get_measure(yas5xx, &t, &x, &y, &z); pm_runtime_mark_last_busy(yas5xx->dev); pm_runtime_put_autosuspend(yas5xx->dev); if (ret) @@ -471,27 +493,8 @@ static int yas5xx_read_raw(struct iio_dev *indio_dev, } return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: - switch (yas5xx->devid) { - case YAS530_DEVICE_ID: - /* - * Raw values of YAS530 are in picotesla. Divide by - * 100000000 (10^8) to get Gauss. - */ - *val = 1; - *val2 = 100000000; - break; - case YAS532_DEVICE_ID: - /* - * Raw values of YAS532 are in nanotesla. Divide by - * 100000 (10^5) to get Gauss. - */ - *val = 1; - *val2 = 100000; - break; - default: - dev_err(yas5xx->dev, "unknown device type\n"); - return -EINVAL; - } + *val = 1; + *val2 = yas5xx->chip_info->scaling_val2; return IIO_VAL_FRACTIONAL; default: /* Unknown request */ @@ -506,7 +509,7 @@ 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); + ret = yas5xx->chip_info->get_measure(yas5xx, &t, &x, &y, &z); pm_runtime_mark_last_busy(yas5xx->dev); pm_runtime_put_autosuspend(yas5xx->dev); if (ret) { @@ -592,9 +595,24 @@ 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 + 8); + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct yas5xx *yas5xx = iio_priv(indio_dev); + int i, j; + + if (reg >= YAS5XX_MEASURE_DATA && reg < YAS5XX_MEASURE_DATA + 8) + return true; + + /* + * YAS versions share different registers on the same address, + * need to differentiate. + */ + j = yas5xx->chip_info->volatile_reg_qty; + for (i = 0; i < j; i++) { + if (reg == yas5xx->chip_info->volatile_reg[i]) + return true; + } + + return false; } /* TODO: enable regmap cache, using mark dirty and sync at runtime resume */ @@ -811,7 +829,7 @@ static int yas530_measure_offsets(struct yas5xx *yas5xx) return ret; /* When the initcoil is active this should be around the center */ - switch (yas5xx->devid) { + switch (yas5xx->chip_info->devid) { case YAS530_DEVICE_ID: center = YAS530_DATA_CENTER; break; @@ -892,13 +910,49 @@ static int yas530_power_on(struct yas5xx *yas5xx) return regmap_write(yas5xx->map, YAS530_MEASURE_INTERVAL, 0); } +static struct yas5xx_chip_info yas5xx_chip_info_tbl[] = { + [yas530] = { + .devid = YAS530_DEVICE_ID, + .volatile_reg = yas530_volatile_reg, + .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg), + .scaling_val2 = 100000000, /* picotesla to Gauss */ + .get_measure = yas530_get_measure, + .get_calibration_data = yas530_get_calibration_data, + .dump_calibration = yas530_dump_calibration, + .measure_offsets = yas530_measure_offsets, + .power_on = yas530_power_on, + }, + [yas532] = { + .devid = YAS532_DEVICE_ID, + .volatile_reg = yas530_volatile_reg, + .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg), + .scaling_val2 = 100000, /* nanotesla to Gauss */ + .get_measure = yas530_get_measure, + .get_calibration_data = yas532_get_calibration_data, + .dump_calibration = yas530_dump_calibration, + .measure_offsets = yas530_measure_offsets, + .power_on = yas530_power_on, + }, + [yas533] = { + .devid = YAS532_DEVICE_ID, + .volatile_reg = yas530_volatile_reg, + .volatile_reg_qty = ARRAY_SIZE(yas530_volatile_reg), + .scaling_val2 = 100000, /* nanotesla to Gauss */ + .get_measure = yas530_get_measure, + .get_calibration_data = yas532_get_calibration_data, + .dump_calibration = yas530_dump_calibration, + .measure_offsets = yas530_measure_offsets, + .power_on = yas530_power_on, + } +}; + static int yas5xx_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct iio_dev *indio_dev; struct device *dev = &i2c->dev; struct yas5xx *yas5xx; - int ret; + int id_check, ret; indio_dev = devm_iio_device_alloc(dev, sizeof(*yas5xx)); if (!indio_dev) @@ -944,45 +998,41 @@ static int yas5xx_probe(struct i2c_client *i2c, goto assert_reset; } - ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &yas5xx->devid); + yas5xx->chip = id->driver_data; + yas5xx->chip_info = &yas5xx_chip_info_tbl[yas5xx->chip]; + + ret = regmap_read(yas5xx->map, YAS5XX_DEVICE_ID, &id_check); if (ret) goto assert_reset; - switch (yas5xx->devid) { - case 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)); - break; - case YAS532_DEVICE_ID: - 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)); - break; - default: + if (id_check != yas5xx->chip_info->devid) { ret = -ENODEV; - dev_err(dev, "unhandled device ID %02x\n", yas5xx->devid); + dev_err(dev, "device ID %02x doesn't match %s\n", + id_check, id->name); goto assert_reset; } - yas530_dump_calibration(yas5xx); - ret = yas530_power_on(yas5xx); + ret = yas5xx->chip_info->get_calibration_data(yas5xx); if (ret) goto assert_reset; - ret = yas530_measure_offsets(yas5xx); + + dev_info(dev, "detected %s %s\n", yas5xx_product_name[yas5xx->chip], + yas5xx_version_name[yas5xx->chip][yas5xx->version]); + + yas5xx->chip_info->dump_calibration(yas5xx); + + ret = yas5xx->chip_info->power_on(yas5xx); + if (ret) + goto assert_reset; + + ret = yas5xx->chip_info->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; - indio_dev->name = yas5xx->name; + indio_dev->name = id->name; indio_dev->channels = yas5xx_channels; indio_dev->num_channels = ARRAY_SIZE(yas5xx_channels); @@ -1074,7 +1124,7 @@ 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); + ret = yas5xx->chip_info->power_on(yas5xx); if (ret) { dev_err(dev, "cannot power on\n"); goto out_reset; @@ -1097,9 +1147,9 @@ static const struct dev_pm_ops yas5xx_dev_pm_ops = { }; static const struct i2c_device_id yas5xx_id[] = { - {"yas530", }, - {"yas532", }, - {"yas533", }, + {"yas530", yas530 }, + {"yas532", yas532 }, + {"yas533", yas533 }, {} }; MODULE_DEVICE_TABLE(i2c, yas5xx_id);
This commit introduces the "chip_info" structure approach for better variant handling. The variant to be used is now chosen by the devicetree (enum "chip_ids"), not by the chip ID in the register. However, there is a check to make sure they match (using integer "id_check"). Signed-off-by: Jakob Hauser <jahau@rocketmail.com> --- Result of the patch can be seen at: https://github.com/Jakko3/linux/blob/28a2a9ec27c6fb4073149b897415475a8f04e3f7/drivers/iio/magnetometer/yamaha-yas530.c drivers/iio/magnetometer/yamaha-yas530.c | 314 +++++++++++++---------- 1 file changed, 182 insertions(+), 132 deletions(-)