From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Subject: Re: [PATCH v1 1/1] pinctrl: intel: Add Intel Merrifield pin controller support Date: Tue, 21 Jun 2016 16:04:21 +0300 Message-ID: <1466514261.30123.221.camel@linux.intel.com> References: <1466351099-22282-1-git-send-email-andriy.shevchenko@linux.intel.com> <20160620092007.GB1739@lahna.fi.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com ([192.55.52.115]:32980 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751432AbcFUNFB (ORCPT ); Tue, 21 Jun 2016 09:05:01 -0400 In-Reply-To: <20160620092007.GB1739@lahna.fi.intel.com> Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Mika Westerberg Cc: Linus Walleij , linux-gpio@vger.kernel.org On Mon, 2016-06-20 at 12:20 +0300, Mika Westerberg wrote: > On Sun, Jun 19, 2016 at 06:44:59PM +0300, Andy Shevchenko wrote: > > This driver adds pinctrl support for Intel Merrifield. The IP block > > which is > > called Family-Level Interface Shim is a separate entity in SoC. The > > GPIO driver > > (gpio-intel-mid.c) will be updated accordingly to support pinctrl > > interface. > > > > Signed-off-by: Andy Shevchenko Mika, my answers below. (For the rest I agree with you and fixed that locally) > > + > > +/** > > + * struct mrfld_pinctrl_soc_data - Intel pin controller per-SoC > > configuration > > + * @name: Name of the family > > + * @pins: Array if pins this pinctrl controls > > + * @npins: Number of pins in the array > > + * @groups: Array of pin groups > > + * @ngroups: Number of groups in the array > > + * @functions: Array of functions > > + * @nfunctions: Number of functions in the array > > + * @families: Array of families this pinctrl handles > > + * @nfamilies: Number of families in the array > > + * > > + * The @families is used as a template by the core driver. It will > > make > > + * copy of all families and fill in rest of the information. > > + */ > > +struct mrfld_pinctrl_soc_data { > > + const char *name; > > + const struct pinctrl_pin_desc *pins; > > + size_t npins; > > + const struct intel_pingroup *groups; > > + size_t ngroups; > > + const struct intel_function *functions; > > + size_t nfunctions; > > + const struct mrfld_family *families; > > + size_t nfamilies; > > +}; > > + > > +static const struct mrfld_pinctrl_soc_data mrfld_soc_data = { > > + .pins = mrfld_pins, > > + .npins = ARRAY_SIZE(mrfld_pins), > > + .groups = mrfld_groups, > > + .ngroups = ARRAY_SIZE(mrfld_groups), > > + .functions = mrfld_functions, > > + .nfunctions = ARRAY_SIZE(mrfld_functions), > > + .families = mrfld_families, > > + .nfamilies = ARRAY_SIZE(mrfld_families), > > +}; > > + > > These are pointless. There can be only one kind of SoC data in > Merrifield so please drop these. See for example pinctrl-cherryview.c. I didn't quite get what should I look for? In mentioned driver we have struct chv_community which contains same fields. IIRC I took it from exactly that driver. > +static int mrfld_pinmux_set_mux(struct pinctrl_dev *pctldev, > > unsigned function, > > + unsigned group) > > +{ > > + struct mrfld_pinctrl *mp = > > pinctrl_dev_get_drvdata(pctldev); > > + const struct intel_pingroup *grp = &mp->soc->groups[group]; > > + unsigned long flags; > > + unsigned int i; > > + > > + spin_lock_irqsave(&mp->lock, flags); > > If you ever need to call the backing pinctrl functions from irqchip > driver, please use raw_spinlock instead. Frankly speaking I dunno. How may I check this? > +static int mrfld_pinctrl_remove(struct platform_device *pdev) > > +{ > > + return 0; > > +} > > Is this really needed? Not for now. I hope we still may remove module without this callback in place. At least that's how I read __device_release_driver(). -- Andy Shevchenko Intel Finland Oy