From mboxrd@z Thu Jan 1 00:00:00 1970 From: Manivannan Sadhasivam Subject: Re: [PATCH v4 04/10] pinctrl: actions: Add Actions S900 pinctrl driver Date: Fri, 2 Mar 2018 23:26:57 +0530 Message-ID: <20180302175657.cnudp24fp3ejevwc@linaro.org> References: <20180302035045.14660-1-manivannan.sadhasivam@linaro.org> <20180302035045.14660-5-manivannan.sadhasivam@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Linus Walleij Cc: Rob Herring , Andreas =?iso-8859-1?Q?F=E4rber?= , liuwei@actions-semi.com, mp-cs@actions-semi.com, 96boards@ucrobotics.com, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Daniel Thompson , Amit Kucheria , Linux ARM , "open list:GPIO SUBSYSTEM" , "linux-kernel@vger.kernel.org" , hzhang@ucrobotics.com, bdong@ucrobotics.com, Mani Sadhasivam List-Id: linux-gpio@vger.kernel.org Hi Linus, On Fri, Mar 02, 2018 at 02:21:37PM +0100, Linus Walleij wrote: > On Fri, Mar 2, 2018 at 4:50 AM, Manivannan Sadhasivam > wrote: > > > Add pinctrl driver for Actions Semi S900 SoC. The driver supports > > pinctrl, pinmux and pinconf functionalities through a range of registers > > common to both gpio driver and pinctrl driver. > > > > Pinmux functionality is available only for the pin groups while the > > pinconf functionality is available for both pin groups and individual > > pins. > > > > Signed-off-by: Manivannan Sadhasivam > > Seems like this is pretty much finished. > > Let's see if you can collect some ACKs before we apply it. > Andreas is at Embedded World conference. I hope once he is back, he will have a look at this patchset. > Now just minor things remain. > > Random chosen example: > > > +static unsigned int lvds_e_drv_pads[] = { > > + LVDS_EEP, > > + LVDS_EEN, > > + LVDS_EDP, > > + LVDS_EDN, > > + LVDS_ECP, > > + LVDS_ECN, > > + LVDS_EBP, > > + LVDS_EBN, > > +}; > > + > > +static unsigned int sd0_d3_d0_drv_pads[] = { > > + SD0_D3, > > + SD0_D2, > > + SD0_D1, > > + SD0_D0, > > +}; > > People (e.g. Torvalds) sometimes get upset with files with too many lines > in them. This file has a lot of lines. A lot of pin control drivers try to cut > down the lines with macros, and you do it in some places too, > would you consider to see if you can cut down these tables with > macros? > > S900_PADS(LVDS_EEP, LVDS_EEN, LVDS_EDP, LVDS_EDN, > LVDS_ECP, LVDS_ECN, LVDS_EBP, LVDS_EBN); > S900_PADS(SD0_D3, SD0_D2, SD0_D1, SD0_D0); > I don't think it would be efficient to use macros here. However, I can align the pads and func definitions in a single line. This will also save a considerable amount of space. Thanks, Mani > Would be so much more compact. > > It's not the biggest problem though. > > Yours, > Linus Walleij From mboxrd@z Thu Jan 1 00:00:00 1970 From: manivannan.sadhasivam@linaro.org (Manivannan Sadhasivam) Date: Fri, 2 Mar 2018 23:26:57 +0530 Subject: [PATCH v4 04/10] pinctrl: actions: Add Actions S900 pinctrl driver In-Reply-To: References: <20180302035045.14660-1-manivannan.sadhasivam@linaro.org> <20180302035045.14660-5-manivannan.sadhasivam@linaro.org> Message-ID: <20180302175657.cnudp24fp3ejevwc@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Linus, On Fri, Mar 02, 2018 at 02:21:37PM +0100, Linus Walleij wrote: > On Fri, Mar 2, 2018 at 4:50 AM, Manivannan Sadhasivam > wrote: > > > Add pinctrl driver for Actions Semi S900 SoC. The driver supports > > pinctrl, pinmux and pinconf functionalities through a range of registers > > common to both gpio driver and pinctrl driver. > > > > Pinmux functionality is available only for the pin groups while the > > pinconf functionality is available for both pin groups and individual > > pins. > > > > Signed-off-by: Manivannan Sadhasivam > > Seems like this is pretty much finished. > > Let's see if you can collect some ACKs before we apply it. > Andreas is at Embedded World conference. I hope once he is back, he will have a look at this patchset. > Now just minor things remain. > > Random chosen example: > > > +static unsigned int lvds_e_drv_pads[] = { > > + LVDS_EEP, > > + LVDS_EEN, > > + LVDS_EDP, > > + LVDS_EDN, > > + LVDS_ECP, > > + LVDS_ECN, > > + LVDS_EBP, > > + LVDS_EBN, > > +}; > > + > > +static unsigned int sd0_d3_d0_drv_pads[] = { > > + SD0_D3, > > + SD0_D2, > > + SD0_D1, > > + SD0_D0, > > +}; > > People (e.g. Torvalds) sometimes get upset with files with too many lines > in them. This file has a lot of lines. A lot of pin control drivers try to cut > down the lines with macros, and you do it in some places too, > would you consider to see if you can cut down these tables with > macros? > > S900_PADS(LVDS_EEP, LVDS_EEN, LVDS_EDP, LVDS_EDN, > LVDS_ECP, LVDS_ECN, LVDS_EBP, LVDS_EBN); > S900_PADS(SD0_D3, SD0_D2, SD0_D1, SD0_D0); > I don't think it would be efficient to use macros here. However, I can align the pads and func definitions in a single line. This will also save a considerable amount of space. Thanks, Mani > Would be so much more compact. > > It's not the biggest problem though. > > Yours, > Linus Walleij