LKML Archive on lore.kernel.org
 help / color / Atom feed
From: "Tanwar, Rahul" <rahul.tanwar@linux.intel.com>
To: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: linus.walleij@linaro.org, robh+dt@kernel.org,
	mark.rutland@arm.com, linux-gpio@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	robh@kernel.org, qi-ming.wu@intel.com, yixin.zhu@linux.intel.com,
	cheol.yong.kim@intel.com
Subject: Re: [PATCH v5 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC
Date: Mon, 11 Nov 2019 13:59:42 +0800
Message-ID: <b6283998-fc3f-678d-1647-384f2c7749ca@linux.intel.com> (raw)
In-Reply-To: <20191108114058.GE32742@smile.fi.intel.com>


Hi Andy,

On 8/11/2019 7:40 PM, Andy Shevchenko wrote:
> On Fri, Nov 08, 2019 at 05:42:22PM +0800, Rahul Tanwar wrote:
>> Intel Lightning Mountain SoC has a pinmux controller & GPIO controller IP which
>> controls pin multiplexing & configuration including GPIO functions selection &
>> GPIO attributes configuration.
>>
>> This IP is not based on & does not have anything in common with Chassis
>> specification. The pinctrl drivers under pinctrl/intel/* are all based upon
>> Chassis spec compliant pinctrl IPs. So this driver doesn't fit & can not use
>> pinctrl framework under pinctrl/intel/* and it requires a separate new driver.
>>
>> Add a new GPIO & pin control framework based driver for this IP.
>> +static void eqbr_gpio_enable_irq(struct irq_data *d)
>> +{
>> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +	struct eqbr_gpio_ctrl *gctrl = gpiochip_get_data(gc);
>> +	unsigned int offset = irqd_to_hwirq(d);
>> +	unsigned long flags;
>> +
>> +	gc->direction_input(gc, offset);
> Does this any IO?
> If so, between above and below a window of possible race.
> Ditto for all other functions that do something similar.

gpio-mmio lib uses its own spin lock when it does IO in gc->direction_input()
And that would set pin direction as input (hw ensures that the pin is not
driven high when set direction) while below will enable interrupton that
pin. I do not see any possible race condition in that..

>> +	raw_spin_lock_irqsave(&gctrl->lock, flags);
>> +	writel(BIT(offset), gctrl->membase + GPIO_IRNRNSET);
>> +	raw_spin_unlock_irqrestore(&gctrl->lock, flags);
>> +}
>> +		ret = bgpio_init(&gctrl->chip, dev, gctrl->bank->nr_pins / 8,
>> +				 gctrl->membase + GPIO_IN,
>> +				 gctrl->membase + GPIO_OUTSET,
>> +				 gctrl->membase + GPIO_OUTCLR,
>> +				 gctrl->membase + GPIO_DIR,
>> +				 NULL,
>> +				 0);
> One line?

Sure, missed it.

>> +static int get_drv_cur(void __iomem *mem, unsigned int offset)
>> +{
>> +	unsigned int idx = offset / DRV_CUR_PINS; /* 0-15, 16-31 per register*/
>> +	unsigned int val;
>> +
>> +	val = readl(mem + REG_DRCC(idx));
>> +	offset %= DRV_CUR_PINS;
> From style point of view is better to have
> 	... foo = offset / X;
> 	... bar = offset % X;
>
> directly in definition block. Moreover, for example, on x86 it might be
> converted by compiler to single idiv call in assembly that returns in
> (eax, edx) both values at once.

Ok, i will change it like that but will have to introduce one more variable.

>> +	val = PARSE_DRV_CURRENT(val, offset);
>> +
>> +	return val;
>> +}
>> +	if (!(bank->aval_pinmap & BIT(offset))) {
>> +		dev_err(pctl->dev,
>> +			"PIN: %u is not valid, pinbase: %u, bitmap: %u\n",
>> +			pin, bank->pin_base, bank->aval_pinmap);
>> +		return -ENODEV;
>> +	}
> Looks like aval_pinmap is NIH of valid_mask bitmap in GPIO library.
> Can you check if it suits your purposes?

I did check about it earlier & now as well. It doesn't suit our purpose.
aval_pinmapin the driver indicates whether pad control of this pad is
available or not. It's for all pins irrespective of if it is used as a
gpio or not. Whereas, valid_mask bitmap of GPIO library is just to
indicate if that gpio line is valid or not. valid_mask would have been
useful if this driver was purely a GPIO driver.

>> +static bool is_func_exist(struct eqbr_pmx_func *funcs, const char *name,
>> +			 unsigned int nr_funcs, unsigned int *idx)
>> +{
>> +	int i;
>> +
>> +	if (!funcs || !nr_funcs)
>> +		return false;
> nr_funcs check is a dup of the one in for loop.

Sure, noted.

>> +	for (i = 0; i < nr_funcs; i++) {
>> +		if (funcs[i].name && (strcmp(funcs[i].name, name) == 0) ) {
> An extra space, but you may use !strcmp() and make it shorter without redundant
> parentheses.

Well noted, thanks.

>> +			*idx = i;
>> +			return true;
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
>> +		switch (op) {
>> +			case OP_COUNT_NR_FUNCS:
> case goes usually on the same column as switch.

Sure, will change, didn't know about it. Thanks.

>> +				if (!is_func_exist(funcs, fn_name,
>> +						   *nr_funcs, &fid))
>> +					*nr_funcs = *nr_funcs + 1;
>> +				break;
>> +
>> +			case OP_ADD_FUNCS:
>> +				if (!is_func_exist(funcs, fn_name,
>> +						   *nr_funcs, &fid))
>> +					funcs[i].name = fn_name;
>> +				break;
>> +
>> +			case OP_COUNT_NR_FUNC_GRPS:
>> +				if (is_func_exist(funcs, fn_name,
>> +						  *nr_funcs, &fid))
>> +					funcs[fid].nr_groups++;
>> +				break;
>> +
>> +			case OP_ADD_FUNC_GRPS:
>> +				if (is_func_exist(funcs, fn_name,
>> +						  *nr_funcs, &fid)) {
>> +					for(j=0;
> Other style issueS.

Will fix in v6, thanks.

>> +		}
>> +	for (i = 0; i < nr_funcs; i++) {
>> +		if (funcs[i].nr_groups) {
> 	if (!foo)
> 		continue;
> ?

Sure, will change. Thanks.

Regards,
Rahul


  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-08  9:42 [PATCH v5 0/2] pinctrl: Add new pinctrl/GPIO driver Rahul Tanwar
2019-11-08  9:42 ` [PATCH v5 1/2] pinctrl: Add pinmux & GPIO controller driver for a new SoC Rahul Tanwar
2019-11-08 11:40   ` Andy Shevchenko
2019-11-11  5:59     ` Tanwar, Rahul [this message]
2019-11-08  9:42 ` [PATCH v5 2/2] dt-bindings: pinctrl: intel: Add for " Rahul Tanwar

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b6283998-fc3f-678d-1647-384f2c7749ca@linux.intel.com \
    --to=rahul.tanwar@linux.intel.com \
    --cc=andriy.shevchenko@intel.com \
    --cc=cheol.yong.kim@intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=qi-ming.wu@intel.com \
    --cc=robh+dt@kernel.org \
    --cc=robh@kernel.org \
    --cc=yixin.zhu@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git