Re: [PATCH] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

From: Uwe Kleine-König
Date: Thu Dec 10 2020 - 12:48:53 EST


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@xxxxxxxxxxxxxx> 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/7cde887ffb3b27a36e77a08bee3666d14968b586
> >
> > My preferred way here would be to add a prefix for the other constants.
> > It (IMHO) looks nicer and
> >
> > GPIO_INPUT_SHIFT
> >
> > looks 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

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature