On Tue, Nov 2, 2021 at 1:55 PM Yury Norov [off-list ref] wrote:

On Tue, Nov 2, 2021 at 12:59 PM Emil Renner Berthing [off-list ref] wrote:

quoted

On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko [off-list ref] wrote:

quoted

+Cc: Yury (bitmap expert)

On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing [off-list ref] wrote:

quoted

Add a driver for the StarFive JH7100 reset controller.

...

quoted

+#define BIT_MASK32(x) BIT((x) % 32)

Possible namespace collision.

...

quoted

+/*
+ * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
+ * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
+ * same line.

We don't have 32-bit bitmaps. Bitmaps are always arrays of unsigned longs. On a
64-bit system this '32-bit bitmap' may be broken due to endianness issues.

quoted

quoted

quoted

+ * most reset lines have their status inverted so a 0 in the STATUS register
+ * means the line is asserted and a 1 means it's deasserted. a few lines don't
+ * though, so store the expected value of the status registers when all lines
+ * are asserted.
+ */

Besides missing capitalization,

I'm confused. it was you who wanted all comments to capitalized the same..
64bi
if it sounds like bitmap, use bitmap.

quoted

I have checked DT definitions and it seems you don't even need the
BIT_MASK() macro,

quoted

+static const u32 jh7100_reset_asserted[4] = {
+       /* STATUS0 register */
+       BIT_MASK32(JH7100_RST_U74) |

I think we have no BIT_MASK32() for a good reason. Natural alignment is
always preferable.

quoted

quoted

quoted

+       BIT_MASK32(JH7100_RST_VP6_DRESET) |
+       BIT_MASK32(JH7100_RST_VP6_BRESET),
+       /* STATUS1 register */
+       BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
+       BIT_MASK32(JH7100_RST_HIFI4_BRESET),
+       /* STATUS2 register */
+       BIT_MASK32(JH7100_RST_E24),
+       /* STATUS3 register */
+       0,
+};

Yury, do we have any clever (clean) way to initialize a bitmap with
particular bits so that it will be a constant from the beginning? If
no, any suggestion what we can provide to such users?

If you want your array to be a true bitmap, ie, all bitmap functions should
work with it correctly, you'd initialize it like this:

static const unsigned long jh7100_reset_asserted[] = {
        BITMAP_FROM_U64(BIT_MASK(JH7100_RST_VP6_DRESET) |
                          BIT_MASK(JH7100_RST_VP6_BRESET) |
                          BIT_MASK(JH7100_RST_HIFI4_DRESET) |
                          BIT_MASK(JH7100_RST_HIFI4_BRESET)),
        BITMAP_FROM_U64(BIT_MASK(JH7100_RST_E24)),
}

My bad, it should be BIT_ULL_MASK.

Thanks,
Yury