Re: [PATCH v2 2/3] soundwire: SDCA: add helper macro to access controls

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

 



Thanks for the review Vinod,

This is good, thanks for adding it in changelog. Can you also add this
description to Documentation (that can come as an individual patch),

ok

+/*
+ * v1.2 device - SDCA address mapping
+ *
+ * Spec definition
+ *	Bits		Contents
+ *	31		0 (required by addressing range)
+ *	30:26		0b10000 (Control Prefix)

So this is for 30:26

I don't get the comment, sorry.


+ *	25		0 (Reserved)
+ *	24:22		Function Number [2:0]
+ *	21		Entity[6]
+ *	20:19		Control Selector[5:4]
+ *	18		0 (Reserved)
+ *	17:15		Control Number[5:3]
+ *	14		Next
+ *	13		MBQ
+ *	12:7		Entity[5:0]
+ *	6:3		Control Selector[3:0]
+ *	2:0		Control Number[2:0]
+ */
+
+#define SDW_SDCA_CTL(fun, ent, ctl, ch)						\
+	(BIT(30)							|	\

Programmatically this is fine, but then since we are defining for the
description above, IMO it would actually make sense for this to be defined
as FIELD_PREP:

         FIELD_PREP(GENMASK(30, 26), 1)

or better

         u32_encode_bits(GENMASK(30, 26), 1)

+	FIELD_PREP(GENMASK(24, 22), FIELD_GET(GENMASK(2, 0), (fun)))	|	\

Why not use u32_encode_bits(GENMASK(24, 22), (fun)) instead for this and
below?

Because your comment for the v1 review was to use FIELD_PREP/FIELD_GET, and your other patches for bitfield access only use FIELD_PREP/FIELD_GET.

I really don't care about which macro is used but it wouldn't hurt to have some level of consistency between different parts of the code? Why not use FIELD_PREP/GET everywhere?

+	FIELD_PREP(BIT(21), FIELD_GET(BIT(6), (ent)))			|	\
+	FIELD_PREP(GENMASK(20, 19), FIELD_GET(GENMASK(5, 4), (ctl)))	|	\
+	FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), (ch)))	|	\
+	FIELD_PREP(GENMASK(12, 7), FIELD_GET(GENMASK(5, 0), (ent)))	|	\
+	FIELD_PREP(GENMASK(6, 3), FIELD_GET(GENMASK(3, 0), (ctl)))	|	\
+	FIELD_PREP(GENMASK(2, 0), FIELD_GET(GENMASK(2, 0), (ch))))

Also, can we rather have a nice function for this, that would look much
cleaner

I am not sure what would be cleaner but fine.

And while at it, consider defining masks for various fields rather than
using numbers in GENMASK() above, that would look better, be more
readable and people can reuse it.

Actually on this one I disagree. These fields are not intended to be used by anyone, the goal is precisely to hide them behind regmap, and the use of raw numbers makes it easier to cross-check the documentation and the code. Adding a separate set of definitions would not increase readability.




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux