Re: [PATCH v4 09/10] iio: magnetometer: yas530: Introduce "chip_info" structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux