Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966357AbbBCRks (ORCPT ); Tue, 3 Feb 2015 12:40:48 -0500 Received: from mail-ig0-f171.google.com ([209.85.213.171]:63955 "EHLO mail-ig0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965975AbbBCRkq (ORCPT ); Tue, 3 Feb 2015 12:40:46 -0500 Date: Tue, 3 Feb 2015 09:40:42 -0800 From: Dmitry Torokhov To: Ray Jui Cc: Linus Walleij , Stephen Warren , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Grant Likely , Christian Daudt , Matt Porter , Florian Fainelli , Russell King , Scott Branden , Anatol Pomazau , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, devicetree@vger.kernel.org Subject: Re: [PATCH v3 3/4] pinctrl: cygnus: add initial IOMUX driver support Message-ID: <20150203174042.GA15969@dtor-ws> References: <1422928894-20716-1-git-send-email-rjui@broadcom.com> <1422928894-20716-4-git-send-email-rjui@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1422928894-20716-4-git-send-email-rjui@broadcom.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15097 Lines: 502 Hi Ray, On Mon, Feb 02, 2015 at 06:01:33PM -0800, Ray Jui wrote: > This adds the initial driver support for the Broadcom Cygnus IOMUX > controller. The Cygnus IOMUX controller supports group based mux > configuration but allows certain pins to be muxed to GPIO individually > > Signed-off-by: Ray Jui > Reviewed-by: Scott Branden Just a few random nits/comments... > --- > drivers/pinctrl/bcm/Kconfig | 13 + > drivers/pinctrl/bcm/Makefile | 5 +- > drivers/pinctrl/bcm/pinctrl-cygnus-mux.c | 1087 ++++++++++++++++++++++++++++++ > 3 files changed, 1103 insertions(+), 2 deletions(-) > create mode 100644 drivers/pinctrl/bcm/pinctrl-cygnus-mux.c > > diff --git a/drivers/pinctrl/bcm/Kconfig b/drivers/pinctrl/bcm/Kconfig > index bc6d048..eb13201 100644 > --- a/drivers/pinctrl/bcm/Kconfig > +++ b/drivers/pinctrl/bcm/Kconfig > @@ -19,3 +19,16 @@ config PINCTRL_BCM2835 > bool > select PINMUX > select PINCONF > + > +config PINCTRL_CYGNUS_MUX > + bool "Broadcom Cygnus IOMUX driver" > + depends on (ARCH_BCM_CYGNUS || COMPILE_TEST) > + select PINMUX > + select GENERIC_PINCONF > + default ARCH_BCM_CYGNUS > + help > + Say yes here to enable the Broadcom Cygnus IOMUX driver. > + > + The Broadcom Cygnus IOMUX driver supports group based IOMUX > + configuration, with the exception that certain individual pins > + can be overrided to GPIO function > diff --git a/drivers/pinctrl/bcm/Makefile b/drivers/pinctrl/bcm/Makefile > index 7ba80a3..bb6beb6 100644 > --- a/drivers/pinctrl/bcm/Makefile > +++ b/drivers/pinctrl/bcm/Makefile > @@ -1,4 +1,5 @@ > # Broadcom pinctrl support > > -obj-$(CONFIG_PINCTRL_BCM281XX) += pinctrl-bcm281xx.o > -obj-$(CONFIG_PINCTRL_BCM2835) += pinctrl-bcm2835.o > +obj-$(CONFIG_PINCTRL_BCM281XX) += pinctrl-bcm281xx.o > +obj-$(CONFIG_PINCTRL_BCM2835) += pinctrl-bcm2835.o > +obj-$(CONFIG_PINCTRL_CYGNUS_MUX) += pinctrl-cygnus-mux.o > diff --git a/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c b/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c > new file mode 100644 > index 0000000..33565b4 > --- /dev/null > +++ b/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c > @@ -0,0 +1,1087 @@ > +/* Copyright (C) 2014-2015 Broadcom Corporation > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * This file contains the Cygnus IOMUX driver that supports group based PINMUX > + * configuration. Although PINMUX configuration is mainly group based, the > + * Cygnus IOMUX controller allows certain pins to be individually muxed to GPIO > + * function, and therefore be controlled by the Cygnus ASIU GPIO controller > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "../core.h" > +#include "../pinctrl-utils.h" > + > +#define CYGNUS_NUM_IOMUX_REGS 8 > +#define CYGNUS_NUM_MUX_PER_REG 8 > +#define CYGNUS_NUM_IOMUX (CYGNUS_NUM_IOMUX_REGS * \ > + CYGNUS_NUM_MUX_PER_REG) > + > +/* > + * Cygnus IOMUX register description > + * > + * @offset: register offset for mux configuration of a group > + * @shift: bit shift for mux configuration of a group > + * @alt: alternate function to set to > + */ > +struct cygnus_mux { > + unsigned int offset; > + unsigned int shift; > + unsigned int alt; > +}; > + > +/* > + * Keep track of Cygnus IOMUX configuration and prevent double configuration > + * > + * @cygnus_mux: Cygnus IOMUX register description > + * @is_configured: flag to indicate whether a mux setting has already been > + * configured > + */ > +struct cygnus_mux_log { > + struct cygnus_mux mux; > + bool is_configured; > +}; > + > +/* > + * Group based IOMUX configuration > + * > + * @name: name of the group > + * @pins: array of pins used by this group > + * @num_pins: total number of pins used by this group > + * @mux: Cygnus group based IOMUX configuration > + */ > +struct cygnus_pin_group { > + const char *name; > + const unsigned *pins; > + const unsigned num_pins; > + const struct cygnus_mux mux; Not: the last 2 consts are quite weird - if you want to make an instance of cygnus_pin_group immutable you declare it as a const (and I see you are already doing that below). With the structure as it laid out currently you can only do static initializers. > +}; > + > +/* > + * Cygnus mux function and supported pin groups > + * > + * @name: name of the function > + * @groups: array of groups that can be supported by this function > + * @num_groups: total number of groups that can be supported by this function > + */ > +struct cygnus_pin_function { > + const char *name; > + const char * const *groups; > + const unsigned num_groups; Here as well. ... > + > +/* > + * List of pins in Cygnus > + */ > +static struct cygnus_pin cygnus_pins[] = { const? > + CYGNUS_PIN_DESC(0, "ext_device_reset_n", 0, 0, 0), > + CYGNUS_PIN_DESC(1, "chip_mode0", 0, 0, 0), ... > +#define CYGNUS_PIN_GROUP(group_name, off, sh, al) \ > +{ \ > + .name = #group_name"""_grp", \ Why do we need extra pair of quotes? BTW we can also do .name = __stringify(group_name) "_grp", > + .pins = group_name ## _pins, \ > + .num_pins = ARRAY_SIZE(group_name ## _pins), \ > + .mux = { \ > + .offset = off, \ > + .shift = sh, \ > + .alt = al, \ > + } \ > +} ... > + > +static struct pinctrl_ops cygnus_pinctrl_ops = { const? > + .get_groups_count = cygnus_get_groups_count, > + .get_group_name = cygnus_get_group_name, > + .get_group_pins = cygnus_get_group_pins, > + .pin_dbg_show = cygnus_pin_dbg_show, > + .dt_node_to_map = cygnus_dt_node_to_map, > + .dt_free_map = pinctrl_utils_dt_free_map, > +}; > + > +static int cygnus_get_functions_count(struct pinctrl_dev *pctrl_dev) > +{ > + struct cygnus_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev); > + > + return pinctrl->num_functions; > +} > + > +static const char *cygnus_get_function_name(struct pinctrl_dev *pctrl_dev, > + unsigned selector) > +{ > + struct cygnus_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev); > + > + return pinctrl->functions[selector].name; > +} > + > +static int cygnus_get_function_groups(struct pinctrl_dev *pctrl_dev, > + unsigned selector, > + const char * const **groups, > + unsigned * const num_groups) > +{ > + struct cygnus_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev); > + > + *groups = pinctrl->functions[selector].groups; > + *num_groups = pinctrl->functions[selector].num_groups; > + > + return 0; > +} > + > +static int cygnus_pinmux_set(struct cygnus_pinctrl *pinctrl, > + const struct cygnus_pin_function *func, > + const struct cygnus_pin_group *grp, > + struct cygnus_mux_log *mux_log) > +{ > + const struct cygnus_mux *mux = &grp->mux; > + int i; > + u32 val, mask = 0x7; > + unsigned long flags; > + > + for (i = 0; i < CYGNUS_NUM_IOMUX; i++) { > + if (mux->offset != mux_log[i].mux.offset || > + mux->shift != mux_log[i].mux.shift) > + continue; > + > + /* match found if we reach here */ > + > + /* if this is a new configuration, just do it! */ > + if (!mux_log[i].is_configured) > + break; > + > + /* > + * IOMUX has been configured previously and one is trying to > + * configure it to a different function > + */ > + if (mux_log[i].mux.alt != mux->alt) { > + dev_err(pinctrl->dev, > + "double configuration error detected!\n"); > + dev_err(pinctrl->dev, "func:%s grp:%s\n", > + func->name, grp->name); > + return -EINVAL; > + } else { > + /* > + * One tries to configure it to the same function. > + * Just quit and don't bother > + */ > + return 0; > + } > + } > + > + mux_log[i].mux.alt = mux->alt; > + mux_log[i].is_configured = true; > + > + spin_lock_irqsave(&pinctrl->lock, flags); > + > + val = readl(pinctrl->base0 + grp->mux.offset); > + val &= ~(mask << grp->mux.shift); > + val |= grp->mux.alt << grp->mux.shift; > + writel(val, pinctrl->base0 + grp->mux.offset); > + > + spin_unlock_irqrestore(&pinctrl->lock, flags); > + > + return 0; > +} > + > +static int cygnus_pinmux_set_mux(struct pinctrl_dev *pctrl_dev, > + unsigned func_select, unsigned grp_select) > +{ > + struct cygnus_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev); > + const struct cygnus_pin_function *func = > + &pinctrl->functions[func_select]; > + const struct cygnus_pin_group *grp = &pinctrl->groups[grp_select]; > + > + dev_dbg(pctrl_dev->dev, "func:%u name:%s grp:%u name:%s\n", > + func_select, func->name, grp_select, grp->name); > + > + dev_dbg(pctrl_dev->dev, "offset:0x%08x shift:%u alt:%u\n", > + grp->mux.offset, grp->mux.shift, grp->mux.alt); > + > + return cygnus_pinmux_set(pinctrl, func, grp, pinctrl->mux_log); > +} > + > +static int cygnus_gpio_request_enable(struct pinctrl_dev *pctrl_dev, > + struct pinctrl_gpio_range *range, > + unsigned pin) > +{ > + struct cygnus_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev); > + struct cygnus_gpio_mux *mux = pctrl_dev->desc->pins[pin].drv_data; const? > + u32 val; > + unsigned long flags; > + > + /* not all pins support GPIO pinmux override */ > + if (!mux->is_supported) > + return -ENOTSUPP; > + > + spin_lock_irqsave(&pinctrl->lock, flags); > + > + val = readl(pinctrl->base1 + mux->offset); > + val |= 0x3 << mux->shift; > + writel(val, pinctrl->base1 + mux->offset); > + > + spin_unlock_irqrestore(&pinctrl->lock, flags); > + > + dev_dbg(pctrl_dev->dev, > + "gpio request enable pin=%u offset=0x%x shift=%u\n", > + pin, mux->offset, mux->shift); > + > + return 0; > +} > + > +static void cygnus_gpio_disable_free(struct pinctrl_dev *pctrl_dev, > + struct pinctrl_gpio_range *range, > + unsigned pin) > +{ > + struct cygnus_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev); > + struct cygnus_gpio_mux *mux = pctrl_dev->desc->pins[pin].drv_data; > + u32 val; > + unsigned long flags; > + > + if (!mux->is_supported) > + return; > + > + spin_lock_irqsave(&pinctrl->lock, flags); > + > + val = readl(pinctrl->base1 + mux->offset); > + val &= ~(0x3 << mux->shift); > + writel(val, pinctrl->base1 + mux->offset); > + > + spin_unlock_irqrestore(&pinctrl->lock, flags); > + > + dev_err(pctrl_dev->dev, > + "gpio disable free pin=%u offset=0x%x shift=%u\n", > + pin, mux->offset, mux->shift); > +} > + > +static struct pinmux_ops cygnus_pinmux_ops = { const? > + .get_functions_count = cygnus_get_functions_count, > + .get_function_name = cygnus_get_function_name, > + .get_function_groups = cygnus_get_function_groups, > + .set_mux = cygnus_pinmux_set_mux, > + .gpio_request_enable = cygnus_gpio_request_enable, > + .gpio_disable_free = cygnus_gpio_disable_free, > +}; > + > +static struct pinctrl_desc cygnus_pinctrl_desc = { > + .name = "cygnus-pinmux", > + .pctlops = &cygnus_pinctrl_ops, > + .pmxops = &cygnus_pinmux_ops, > +}; > + > +static int cygnus_mux_log_init(struct cygnus_pinctrl *pinctrl) > +{ > + struct cygnus_mux_log *log; > + unsigned int i, j; > + > + pinctrl->mux_log = devm_kcalloc(pinctrl->dev, CYGNUS_NUM_IOMUX, > + sizeof(struct cygnus_mux_log), > + GFP_KERNEL); > + if (!pinctrl->mux_log) > + return -ENOMEM; > + > + log = pinctrl->mux_log; > + for (i = 0; i < CYGNUS_NUM_IOMUX_REGS; i++) { > + for (j = 0; j < CYGNUS_NUM_MUX_PER_REG; j++) { > + log = &pinctrl->mux_log[i * CYGNUS_NUM_MUX_PER_REG > + + j]; > + log->mux.offset = i * 4; > + log->mux.shift = j * 4; > + log->mux.alt = 0; > + log->is_configured = false; > + } > + } > + > + return 0; > +} > + > +static int cygnus_pinmux_probe(struct platform_device *pdev) > +{ > + struct cygnus_pinctrl *pinctrl; > + struct resource *res; > + int i, ret; > + struct pinctrl_pin_desc *pins; > + unsigned num_pins = ARRAY_SIZE(cygnus_pins); > + > + pinctrl = devm_kzalloc(&pdev->dev, sizeof(*pinctrl), GFP_KERNEL); > + if (!pinctrl) > + return -ENOMEM; > + > + pinctrl->dev = &pdev->dev; > + platform_set_drvdata(pdev, pinctrl); > + spin_lock_init(&pinctrl->lock); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + pinctrl->base0 = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(pinctrl->base0)) { > + dev_err(&pdev->dev, "unable to map I/O space\n"); > + return PTR_ERR(pinctrl->base0); > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + pinctrl->base1 = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(pinctrl->base1)) { > + dev_err(&pdev->dev, "unable to map I/O space\n"); > + return PTR_ERR(pinctrl->base1); > + } > + > + ret = cygnus_mux_log_init(pinctrl); > + if (ret) { > + dev_err(&pdev->dev, "unable to initialize IOMUX log\n"); > + return ret; > + } > + > + pins = devm_kcalloc(&pdev->dev, num_pins, sizeof(*pins), GFP_KERNEL); > + if (!pins) > + return -ENOMEM; > + > + for (i = 0; i < num_pins; i++) { > + pins[i].number = cygnus_pins[i].pin; > + pins[i].name = cygnus_pins[i].name; > + pins[i].drv_data = &cygnus_pins[i].gpio_mux; > + } > + > + pinctrl->groups = cygnus_pin_groups; > + pinctrl->num_groups = ARRAY_SIZE(cygnus_pin_groups); > + pinctrl->functions = cygnus_pin_functions; > + pinctrl->num_functions = ARRAY_SIZE(cygnus_pin_functions); > + cygnus_pinctrl_desc.pins = pins; > + cygnus_pinctrl_desc.npins = num_pins; > + > + pinctrl->pctl = pinctrl_register(&cygnus_pinctrl_desc, &pdev->dev, > + pinctrl); > + if (!pinctrl->pctl) { > + dev_err(&pdev->dev, "unable to register Cygnus IOMUX pinctrl\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static struct of_device_id cygnus_pinmux_of_match[] = { > + { .compatible = "brcm,cygnus-pinmux" }, > + { } > +}; > + > +static struct platform_driver cygnus_pinmux_driver = { > + .driver = { > + .name = "cygnus-pinmux", > + .of_match_table = cygnus_pinmux_of_match, > + }, > + .probe = cygnus_pinmux_probe, You also need to either provide remove() method or disallow unbinding via sysfs by setting suppress_bind_attrs in platform driver. > +}; > + > +static int __init cygnus_pinmux_init(void) > +{ > + return platform_driver_register(&cygnus_pinmux_driver); > +} > +arch_initcall(cygnus_pinmux_init); > + > +MODULE_AUTHOR("Ray Jui "); > +MODULE_DESCRIPTION("Broadcom Cygnus IOMUX driver"); > +MODULE_LICENSE("GPL v2"); > -- > 1.7.9.5 > Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/