On Thu, Dec 10, 2020 at 10:40:36PM +0800, Shawn Guo wrote:
Hi Uwe,
On Thu, Dec 10, 2020 at 9:05 PM Uwe Kleine-König u.kleine-koenig@pengutronix.de wrote:
@@ -111,6 +118,8 @@
#define SN_LINK_TRAINING_TRIES 10
+#define SN_PWM_GPIO 3
So this maps to the GPIO4 described in sn65dsi86 datasheet. I'm wondering if it's more readable to define the following SHIFT constants (your code), and use GPIO_MUX_GPIO4_SHIFT >> 2 where you need GPIO offset?
#define GPIO_MUX_GPIO1_SHIFT 0 #define GPIO_MUX_GPIO2_SHIFT 2 #define GPIO_MUX_GPIO3_SHIFT 4 #define GPIO_MUX_GPIO4_SHIFT 6
If you agree, you may consider to integrate this patch beforehand:
https://github.com/shawnguo2/linux/commit/7cde887ffb3b27a36e77a08bee3666d149...
My preferred way here would be to add a prefix for the other constants. It (IMHO) looks nicer and
GPIO_INPUT_SHIFTlooks like a quite generic name for a hardware specific definition.
While this looks like a reasonable argument, I also like the naming choice for these constants in the beginning for that distinction between registers and bits. And changing the names the other way around means there will be a much bigger diffstat, which I would like to avoid. I suggest let's just focus on what really matters here - keep the naming consistent, so that people do not get confused when they want to add more constants in there.
In my eyes the bigger diffstat is justified. As I wrote, GPIO_INPUT_SHIFT isn't used in other files, but please look how many definitions there are for RESET. The usefulness of ctags/cscope is quite reduced if generic terms are used this way.
Best regards Uwe