Re: [PATCH v3 8/8] iio: magnetometer: yas530: Add YAS537 variant

[Jonathan Cameron <jic23@kernel.org>]

> 
> >> +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.  
> 
> Yes, I think I'll introduce chip_info for the variants like YAS530,
> YAS532, etc. but keep handling the versions (like 0 and 1 in this case)
> within the functions. I'll have a look again when preparing patchset v4.
> 

Up to you, though I'm guessing it'll end up better with just having a bit
more duplicate data to handle the versions as separate device types.


> >> +
> >> +		/*
> >> +		 * 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.  
> 
> I don't know what these abbreviations stand for. We could do:
> 
> #define YAS537_MTC3_MASK_PREP ...
> #define YAS537_MTC3_MASK_GET ...
> #define YAS537_HCK_MASK_PREP ...
> #define YAS537_HCK_MASK_GET ...
> etc.
> 
> We need both FIELD_GET and FIELD_PREP, right? The first to retrieve the
> data and the second to place it at the right position.
> 
> Doesn't that get strange-looking like:
> 

yup. you get this sort of oddity sometimes, but it still keeps
it clear which field you are working with.

>         ret = regmap_write(yas5xx->map, YAS537_HCK,
>                            FIELD_PREP(YAS537_HCK_MASK_PREP,
>                                       FIELD_GET(YAS537_HCK_MASK_GET,
>                                                 data[15])));
> 
> Or maybe different indentation, would that be acceptable?
> 
>         ret = regmap_write(yas5xx->map, YAS537_HCK,
>                            FIELD_PREP(YAS537_HCK_MASK_PREP,
>                            FIELD_GET(YAS537_HCK_MASK_GET, data[15])));
Definitely nicer this way.

> 
> On the one above your comment, the "YAS537_MTC + 3", we would still need
> the "| BIT(4)" to place that "1" there. Or is there some other trick to
> do this?

Give it a define and FIELD_PREP() that one as well so we have some
info on what it is via the code.  Obviously it's longer code, but
generally easier to maintain as puts the register definitions in one place
to check against any docs.


> 
> >> +		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;
> >> +  
> 
>
...

> 
> If YAS537 doesn't have a .measure_offsets function pointer in chip_info,
> it skips that if statement cleanly?

Two options - either add a stub that doesn't do anything or
(I prefer this one as makes it obvious the function might not do anything) check 
if (chip_info->measure_offsets)
	chip_info->measure_offsets()...

> 
> ...
> 
> Kind regards,
> Jakob