From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rayagonda Kokatanur Date: Sun, 24 May 2020 16:16:10 +0530 Subject: [PATCH v1 1/3] drivers: pinctrl-single: handle different register width In-Reply-To: References: <20200429163446.15390-1-rayagonda.kokatanur@broadcom.com> <20200429163446.15390-2-rayagonda.kokatanur@broadcom.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On Fri, May 8, 2020 at 7:07 AM Simon Glass wrote: > > Hi Rayagonda, > > On Thu, 30 Apr 2020 at 04:06, Rayagonda Kokatanur > wrote: > > > > On Wed, Apr 29, 2020 at 11:34 PM Simon Glass wrote: > > > > > > Hi Rayagonda, > > > > > > +Stephen Warren > > > > > > On Wed, 29 Apr 2020 at 10:35, Rayagonda Kokatanur > > > wrote: > > > > > > > > Add support to use different register read/write api's > > > > based on register width. > > > > > > > > Signed-off-by: Rayagonda Kokatanur > > > > --- > > > > drivers/pinctrl/pinctrl-single.c | 98 ++++++++++++++++++++++++-------- > > > > 1 file changed, 74 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > > > > index 738f5bd636..aed113b083 100644 > > > > --- a/drivers/pinctrl/pinctrl-single.c > > > > +++ b/drivers/pinctrl/pinctrl-single.c > > > > @@ -10,12 +10,24 @@ > > > > #include > > > > #include > > > > > > > > +/** > > > > + * struct single_pdata - pinctrl device instance > > > > + * @base first configuration register > > > > + * @offset index of last configuration register > > > > + * @mask configuration-value mask bits > > > > + * @width configuration register bit width > > > > + * @bits_per_mux > > > > + * @read register read function to use > > > > + * @write register write function to use > > > > + */ > > > > struct single_pdata { > > > > fdt_addr_t base; /* first configuration register */ > > > > int offset; /* index of last configuration register */ > > > > u32 mask; /* configuration-value mask bits */ > > > > int width; /* configuration register bit width */ > > > > bool bits_per_mux; > > > > + u32 (*read)(phys_addr_t reg); > > > > + void (*write)(u32 val, phys_addr_t reg); > > > > > > Can't we just have a read & write function with a switch statement? > > > Why do we need function pointers? > > > > I referred to the linux pinctrl-single.c and kept code similar to linux. > > Please let me know. > > See the regmap discussion here which is related: > > http://patchwork.ozlabs.org/project/uboot/patch/20191105114700.24989-3-jjhiblot at ti.com/ > > Should this driver use regmap, then? I think using a function pointer is a better approach, we can check for errors in one place ie invalid register width. Right now it's been done in single_probe() function. Please let me know. Best regards, Rayagonda > > Regards, > Simon