On Mon, Jul 4, 2022 at 12:04 AM Jakob Hauser <jahau@xxxxxxxxxxxxxx> 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?
--
With Best Regards,
Andy Shevchenko
