From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Andersson Subject: Re: [PATCH] pinctrl: qcom: Add msm8998 pinctrl driver Date: Thu, 12 Jan 2017 08:20:37 -0800 Message-ID: <20170112162037.GA27322@minitux> References: <1483974019-8235-1-git-send-email-kimran@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-pf0-f178.google.com ([209.85.192.178]:35192 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750755AbdALQUn (ORCPT ); Thu, 12 Jan 2017 11:20:43 -0500 Received: by mail-pf0-f178.google.com with SMTP id f144so15425637pfa.2 for ; Thu, 12 Jan 2017 08:20:42 -0800 (PST) Content-Disposition: inline In-Reply-To: <1483974019-8235-1-git-send-email-kimran@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Imran Khan Cc: andy.gross@linaro.org, Linus Walleij , Rob Herring , Mark Rutland , David Brown , "open list:PIN CONTROL SUBSYSTEM" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list , "open list:ARM/QUALCOMM SUPPORT" , "open list:ARM/QUALCOMM SUPPORT" On Mon 09 Jan 07:00 PST 2017, Imran Khan wrote: > diff --git a/drivers/pinctrl/qcom/pinctrl-msm8998.c b/drivers/pinctrl/qcom/pinctrl-msm8998.c [..] > + > +#define NORTH 0x500000 > +#define WEST 0x100000 > +#define EAST 0x900000 So the control registers are now laid out in 3 non-contiguous ranges? Please move them to the top of the file. > +#define REG_SIZE 0x1000 Just inline 0x1000 in the macro below. > +#define PINGROUP(id, base, f1, f2, f3, f4, f5, f6, f7, f8, f9) \ > + { \ > + .name = "gpio" #id, \ > + .pins = gpio##id##_pins, \ > + .npins = (unsigned int)ARRAY_SIZE(gpio##id##_pins), \ > + .funcs = (int[]){ \ > + msm_mux_gpio, /* gpio mode */ \ > + msm_mux_##f1, \ > + msm_mux_##f2, \ > + msm_mux_##f3, \ > + msm_mux_##f4, \ > + msm_mux_##f5, \ > + msm_mux_##f6, \ > + msm_mux_##f7, \ > + msm_mux_##f8, \ > + msm_mux_##f9 \ > + }, \ > + .nfuncs = 10, \ > + .ctl_reg = base + REG_SIZE * id, \ > + .io_reg = base + 0x4 + REG_SIZE * id, \ > + .intr_cfg_reg = base + 0x8 + REG_SIZE * id, \ > + .intr_status_reg = base + 0xc + REG_SIZE * id, \ > + .intr_target_reg = base + 0x8 + REG_SIZE * id, \ > + .mux_bit = 2, \ > + .pull_bit = 0, \ > + .drv_bit = 6, \ > + .oe_bit = 9, \ > + .in_bit = 0, \ > + .out_bit = 1, \ > + .intr_enable_bit = 0, \ > + .intr_status_bit = 0, \ > + .intr_target_bit = 5, \ > + .intr_target_kpss_val = 3, \ > + .intr_raw_status_bit = 4, \ > + .intr_polarity_bit = 1, \ > + .intr_detection_bit = 2, \ > + .intr_detection_width = 2, \ > + } > + > +#define SDC_QDSD_PINGROUP(pg_name, ctl, pull, drv) \ > + { \ > + .name = #pg_name, \ > + .pins = pg_name##_pins, \ > + .npins = (unsigned int)ARRAY_SIZE(pg_name##_pins), \ > + .ctl_reg = ctl, \ > + .io_reg = 0, \ > + .intr_cfg_reg = 0, \ > + .intr_status_reg = 0, \ > + .intr_target_reg = 0, \ > + .mux_bit = -1, \ > + .pull_bit = pull, \ > + .drv_bit = drv, \ > + .oe_bit = -1, \ > + .in_bit = -1, \ > + .out_bit = -1, \ > + .intr_enable_bit = -1, \ > + .intr_status_bit = -1, \ > + .intr_target_bit = -1, \ > + .intr_raw_status_bit = -1, \ > + .intr_polarity_bit = -1, \ > + .intr_detection_bit = -1, \ > + .intr_detection_width = -1, \ > + } > + > +#define UFS_RESET(pg_name, offset) \ > + { \ > + .name = #pg_name, \ > + .pins = pg_name##_pins, \ > + .npins = (unsigned int)ARRAY_SIZE(pg_name##_pins), \ > + .ctl_reg = offset, \ > + .io_reg = offset + 0x4, \ > + .intr_cfg_reg = 0, \ > + .intr_status_reg = 0, \ > + .intr_target_reg = 0, \ > + .mux_bit = -1, \ > + .pull_bit = 3, \ > + .drv_bit = 0, \ > + .oe_bit = -1, \ > + .in_bit = -1, \ > + .out_bit = 0, \ > + .intr_enable_bit = -1, \ > + .intr_status_bit = -1, \ > + .intr_target_bit = -1, \ > + .intr_raw_status_bit = -1, \ > + .intr_polarity_bit = -1, \ > + .intr_detection_bit = -1, \ > + .intr_detection_width = -1, \ > + } Please add an empty line here. > +static const struct pinctrl_pin_desc msm8998_pins[] = { [..] > +enum msm8998_functions { [..] > + msm_mux_phase_flag6, > + msm_mux_phase_flag29, > + msm_mux_phase_flag30, > + msm_mux_phase_flag31, With the Qualcomm pinctrl driver it's possible to specify configuration in DT for a subset of pins of a group. So I think you should squash the "phase_flag"s and "atest_char" into one group each. > + msm_mux_pa_indicator, > + msm_mux_ssbi1, > + msm_mux_isense_dbg, > + msm_mux_mss_lte, > + msm_mux_gpio, > + msm_mux_NA, > +}; [..] > +static const struct msm_pingroup msm8998_groups[] = { > + PINGROUP(0, EAST, blsp_spi1, blsp_uart1_a, blsp_uim1_a, NA, NA, NA, NA, > + NA, NA), Please do ignore the 80-char "rule" and skip the line break on these - it makes the table easier to read. [..] > +}; > + > +static const struct msm_pinctrl_soc_data msm8998_pinctrl = { > + .pins = msm8998_pins, > + .npins = ARRAY_SIZE(msm8998_pins), > + .functions = msm8998_functions, > + .nfunctions = ARRAY_SIZE(msm8998_functions), > + .groups = msm8998_groups, > + .ngroups = ARRAY_SIZE(msm8998_groups), > + .ngpios = 153, ngpios is 150 > +}; > + Regards, Bjorn From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750904AbdALQUq (ORCPT ); Thu, 12 Jan 2017 11:20:46 -0500 Received: from mail-pf0-f179.google.com ([209.85.192.179]:36192 "EHLO mail-pf0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750779AbdALQUn (ORCPT ); Thu, 12 Jan 2017 11:20:43 -0500 Date: Thu, 12 Jan 2017 08:20:37 -0800 From: Bjorn Andersson To: Imran Khan Cc: andy.gross@linaro.org, Linus Walleij , Rob Herring , Mark Rutland , David Brown , "open list:PIN CONTROL SUBSYSTEM" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list , "open list:ARM/QUALCOMM SUPPORT" , "open list:ARM/QUALCOMM SUPPORT" Subject: Re: [PATCH] pinctrl: qcom: Add msm8998 pinctrl driver Message-ID: <20170112162037.GA27322@minitux> References: <1483974019-8235-1-git-send-email-kimran@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1483974019-8235-1-git-send-email-kimran@codeaurora.org> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 09 Jan 07:00 PST 2017, Imran Khan wrote: > diff --git a/drivers/pinctrl/qcom/pinctrl-msm8998.c b/drivers/pinctrl/qcom/pinctrl-msm8998.c [..] > + > +#define NORTH 0x500000 > +#define WEST 0x100000 > +#define EAST 0x900000 So the control registers are now laid out in 3 non-contiguous ranges? Please move them to the top of the file. > +#define REG_SIZE 0x1000 Just inline 0x1000 in the macro below. > +#define PINGROUP(id, base, f1, f2, f3, f4, f5, f6, f7, f8, f9) \ > + { \ > + .name = "gpio" #id, \ > + .pins = gpio##id##_pins, \ > + .npins = (unsigned int)ARRAY_SIZE(gpio##id##_pins), \ > + .funcs = (int[]){ \ > + msm_mux_gpio, /* gpio mode */ \ > + msm_mux_##f1, \ > + msm_mux_##f2, \ > + msm_mux_##f3, \ > + msm_mux_##f4, \ > + msm_mux_##f5, \ > + msm_mux_##f6, \ > + msm_mux_##f7, \ > + msm_mux_##f8, \ > + msm_mux_##f9 \ > + }, \ > + .nfuncs = 10, \ > + .ctl_reg = base + REG_SIZE * id, \ > + .io_reg = base + 0x4 + REG_SIZE * id, \ > + .intr_cfg_reg = base + 0x8 + REG_SIZE * id, \ > + .intr_status_reg = base + 0xc + REG_SIZE * id, \ > + .intr_target_reg = base + 0x8 + REG_SIZE * id, \ > + .mux_bit = 2, \ > + .pull_bit = 0, \ > + .drv_bit = 6, \ > + .oe_bit = 9, \ > + .in_bit = 0, \ > + .out_bit = 1, \ > + .intr_enable_bit = 0, \ > + .intr_status_bit = 0, \ > + .intr_target_bit = 5, \ > + .intr_target_kpss_val = 3, \ > + .intr_raw_status_bit = 4, \ > + .intr_polarity_bit = 1, \ > + .intr_detection_bit = 2, \ > + .intr_detection_width = 2, \ > + } > + > +#define SDC_QDSD_PINGROUP(pg_name, ctl, pull, drv) \ > + { \ > + .name = #pg_name, \ > + .pins = pg_name##_pins, \ > + .npins = (unsigned int)ARRAY_SIZE(pg_name##_pins), \ > + .ctl_reg = ctl, \ > + .io_reg = 0, \ > + .intr_cfg_reg = 0, \ > + .intr_status_reg = 0, \ > + .intr_target_reg = 0, \ > + .mux_bit = -1, \ > + .pull_bit = pull, \ > + .drv_bit = drv, \ > + .oe_bit = -1, \ > + .in_bit = -1, \ > + .out_bit = -1, \ > + .intr_enable_bit = -1, \ > + .intr_status_bit = -1, \ > + .intr_target_bit = -1, \ > + .intr_raw_status_bit = -1, \ > + .intr_polarity_bit = -1, \ > + .intr_detection_bit = -1, \ > + .intr_detection_width = -1, \ > + } > + > +#define UFS_RESET(pg_name, offset) \ > + { \ > + .name = #pg_name, \ > + .pins = pg_name##_pins, \ > + .npins = (unsigned int)ARRAY_SIZE(pg_name##_pins), \ > + .ctl_reg = offset, \ > + .io_reg = offset + 0x4, \ > + .intr_cfg_reg = 0, \ > + .intr_status_reg = 0, \ > + .intr_target_reg = 0, \ > + .mux_bit = -1, \ > + .pull_bit = 3, \ > + .drv_bit = 0, \ > + .oe_bit = -1, \ > + .in_bit = -1, \ > + .out_bit = 0, \ > + .intr_enable_bit = -1, \ > + .intr_status_bit = -1, \ > + .intr_target_bit = -1, \ > + .intr_raw_status_bit = -1, \ > + .intr_polarity_bit = -1, \ > + .intr_detection_bit = -1, \ > + .intr_detection_width = -1, \ > + } Please add an empty line here. > +static const struct pinctrl_pin_desc msm8998_pins[] = { [..] > +enum msm8998_functions { [..] > + msm_mux_phase_flag6, > + msm_mux_phase_flag29, > + msm_mux_phase_flag30, > + msm_mux_phase_flag31, With the Qualcomm pinctrl driver it's possible to specify configuration in DT for a subset of pins of a group. So I think you should squash the "phase_flag"s and "atest_char" into one group each. > + msm_mux_pa_indicator, > + msm_mux_ssbi1, > + msm_mux_isense_dbg, > + msm_mux_mss_lte, > + msm_mux_gpio, > + msm_mux_NA, > +}; [..] > +static const struct msm_pingroup msm8998_groups[] = { > + PINGROUP(0, EAST, blsp_spi1, blsp_uart1_a, blsp_uim1_a, NA, NA, NA, NA, > + NA, NA), Please do ignore the 80-char "rule" and skip the line break on these - it makes the table easier to read. [..] > +}; > + > +static const struct msm_pinctrl_soc_data msm8998_pinctrl = { > + .pins = msm8998_pins, > + .npins = ARRAY_SIZE(msm8998_pins), > + .functions = msm8998_functions, > + .nfunctions = ARRAY_SIZE(msm8998_functions), > + .groups = msm8998_groups, > + .ngroups = ARRAY_SIZE(msm8998_groups), > + .ngpios = 153, ngpios is 150 > +}; > + Regards, Bjorn