linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gpio: sx150x: Update OF configuration
@ 2016-06-17  9:51 Neil Armstrong
  2016-06-17 11:11 ` kbuild test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Neil Armstrong @ 2016-06-17  9:51 UTC (permalink / raw)
  To: Linus Walleij, Alexandre Courbot
  Cc: Neil Armstrong, linux-gpio, devicetree, linux-kernel

In case of OF probing, the driver fails to initialize :
- gpio_chip.base must be -1
- irq_summary must be either -1 or valid
- There is no way to use the other configurations

Add OF parsing function to complete the HW configuration, make
OF configuration dynamic instead of static with #defines and
update the DT bindings.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 .../devicetree/bindings/gpio/gpio-sx150x.txt       | 17 ++++++
 drivers/gpio/gpio-sx150x.c                         | 65 +++++++++++++++++++---
 2 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-sx150x.txt b/Documentation/devicetree/bindings/gpio/gpio-sx150x.txt
index c809acb..d2b5bb3 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-sx150x.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-sx150x.txt
@@ -22,6 +22,17 @@ Required properties:
 
 - interrupt-controller: Marks the device as a interrupt controller.
 
+Optional Properties:
+- oscio-is-gpo: Boolean, Indicated the oscio pin can be used as additional
+		output gpo port.
+
+- pull-up-ports: Array of port numbers which must have pull-up enabled.
+- pull-down-ports: Array of port numbers which must have pull-down enabled.
+- open-drain-ports: Array of port numbers which must be configured as open-drain,
+			Push-Pull mode is default.
+- polarity-invert-ports: Array of port numbers whih port polarity must be inverted.
+- probe-reset: Boolean, Indicates the expander must be resetted.
+
 The GPIO expander can optionally be used as an interrupt controller, in
 which case it uses the default two cell specifier as described in
 Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.
@@ -38,4 +49,10 @@ Example:
 
 		gpio-controller;
 		interrupt-controller;
+
+		pull-up-ports = <0 12>;
+		pull-down-ports = <1 8>;
+		open-drain-ports = <6 7 11>;
+		polarity-invert-ports = <3>;
+		probe-reset;
 	};
diff --git a/drivers/gpio/gpio-sx150x.c b/drivers/gpio/gpio-sx150x.c
index a177ebd..f7bfde8 100644
--- a/drivers/gpio/gpio-sx150x.c
+++ b/drivers/gpio/gpio-sx150x.c
@@ -588,13 +588,14 @@ static void sx150x_init_chip(struct sx150x_chip *chip,
 	chip->gpio_chip.get              = sx150x_gpio_get;
 	chip->gpio_chip.set              = sx150x_gpio_set;
 	chip->gpio_chip.set_single_ended = sx150x_gpio_set_single_ended;
-	chip->gpio_chip.base             = pdata->gpio_base;
 	chip->gpio_chip.can_sleep        = true;
 	chip->gpio_chip.ngpio            = chip->dev_cfg->ngpios;
-#ifdef CONFIG_OF_GPIO
-	chip->gpio_chip.of_node          = client->dev.of_node;
-	chip->gpio_chip.of_gpio_n_cells  = 2;
-#endif
+	if (client->dev.of_node) {
+		chip->gpio_chip.of_node          = client->dev.of_node;
+		chip->gpio_chip.of_gpio_n_cells  = 2;
+		chip->gpio_chip.base             = -1;
+	} else
+		chip->gpio_chip.base             = pdata->gpio_base;
 	if (pdata->oscio_is_gpo)
 		++chip->gpio_chip.ngpio;
 
@@ -735,16 +736,66 @@ static int sx150x_install_irq_chip(struct sx150x_chip *chip,
 	return err;
 }
 
+static u16 sx150x_of_probe_pins(struct device *dev, char *attr_name)
+{
+	struct property *prop;
+	const __be32 *p;
+	u16 pins = 0;
+	u32 u;
+
+	if (!of_find_property(dev->of_node, attr_name, NULL))
+		return 0;
+
+	of_property_for_each_u32(dev->of_node, attr_name, prop, p, u)
+		if (u < 16)
+			pins |= BIT(u);
+
+	return pins;
+}
+
+static struct sx150x_platform_data *sx150x_of_probe(struct device *dev)
+{
+	struct sx150x_platform_data *pdata = devm_kzalloc(dev, sizeof(*pdata),
+								GFP_KERNEL);
+	/* gpio_base is not needed with OF */
+
+	pdata->oscio_is_gpo = of_property_read_bool(dev->of_node,
+						    "oscio-is-gpo");
+
+	pdata->io_pullup_ena = sx150x_of_probe_pins(dev,
+						    "pull-up-ports");
+
+	pdata->io_pulldn_ena = sx150x_of_probe_pins(dev,
+						    "pull-down-ports");
+
+	pdata->io_polarity = sx150x_of_probe_pins(dev,
+						  "polarity-invert-ports");
+
+	pdata->irq_summary = of_irq_get(dev->of_node, 0);
+
+	/* irq_base is not needed with OF */
+
+	pdata->reset_during_probe = of_property_read_bool(dev->of_node,
+							  "probe-reset");
+
+	return pdata;
+}
+
 static int sx150x_probe(struct i2c_client *client,
 				const struct i2c_device_id *id)
 {
 	static const u32 i2c_funcs = I2C_FUNC_SMBUS_BYTE_DATA |
 				     I2C_FUNC_SMBUS_WRITE_WORD_DATA;
-	struct sx150x_platform_data *pdata;
+	struct sx150x_platform_data *pdata = NULL;
 	struct sx150x_chip *chip;
 	int rc;
 
-	pdata = dev_get_platdata(&client->dev);
+	if (client->dev.of_node)
+		pdata = sx150x_of_probe(&client->dev);
+
+	if (!pdata)
+		pdata = dev_get_platdata(&client->dev);
+
 	if (!pdata)
 		return -EINVAL;
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: sx150x: Update OF configuration
  2016-06-17  9:51 [PATCH] gpio: sx150x: Update OF configuration Neil Armstrong
@ 2016-06-17 11:11 ` kbuild test robot
  2016-06-18  9:02 ` Linus Walleij
  2016-06-20 15:57 ` Rob Herring
  2 siblings, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2016-06-17 11:11 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: kbuild-all, Linus Walleij, Alexandre Courbot, Neil Armstrong,
	linux-gpio, devicetree, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3217 bytes --]

Hi,

[auto build test ERROR on gpio/for-next]
[also build test ERROR on v4.7-rc3 next-20160617]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Neil-Armstrong/gpio-sx150x-Update-OF-configuration/20160617-175750
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: x86_64-randconfig-s0-06171829 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-sx150x.c: In function 'sx150x_init_chip':
>> drivers/gpio/gpio-sx150x.c:594:18: error: 'struct gpio_chip' has no member named 'of_node'
      chip->gpio_chip.of_node          = client->dev.of_node;
                     ^
>> drivers/gpio/gpio-sx150x.c:595:18: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
      chip->gpio_chip.of_gpio_n_cells  = 2;
                     ^
--
   /kbuild/src/smoke/drivers/gpio/gpio-sx150x.c: In function 'sx150x_init_chip':
>> /kbuild/src/smoke/drivers/gpio/gpio-sx150x.c:594:18: error: 'struct gpio_chip' has no member named 'of_node'
      chip->gpio_chip.of_node          = client->dev.of_node;
                     ^
>> /kbuild/src/smoke/drivers/gpio/gpio-sx150x.c:595:18: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
      chip->gpio_chip.of_gpio_n_cells  = 2;
                     ^

vim +594 drivers/gpio/gpio-sx150x.c

c34f16b7 drivers/gpio/sx150x.c      Gregory Bean   2010-08-10  588  	chip->gpio_chip.get              = sx150x_gpio_get;
c34f16b7 drivers/gpio/sx150x.c      Gregory Bean   2010-08-10  589  	chip->gpio_chip.set              = sx150x_gpio_set;
04b86956 drivers/gpio/gpio-sx150x.c Linus Walleij  2016-04-09  590  	chip->gpio_chip.set_single_ended = sx150x_gpio_set_single_ended;
9fb1f39e drivers/gpio/gpio-sx150x.c Linus Walleij  2013-12-04  591  	chip->gpio_chip.can_sleep        = true;
c34f16b7 drivers/gpio/sx150x.c      Gregory Bean   2010-08-10  592  	chip->gpio_chip.ngpio            = chip->dev_cfg->ngpios;
193ca3f8 drivers/gpio/gpio-sx150x.c Neil Armstrong 2016-06-17  593  	if (client->dev.of_node) {
04d2264c drivers/gpio/gpio-sx150x.c Wei Chen       2015-01-15 @594  		chip->gpio_chip.of_node          = client->dev.of_node;
04d2264c drivers/gpio/gpio-sx150x.c Wei Chen       2015-01-15 @595  		chip->gpio_chip.of_gpio_n_cells  = 2;
193ca3f8 drivers/gpio/gpio-sx150x.c Neil Armstrong 2016-06-17  596  		chip->gpio_chip.base             = -1;
193ca3f8 drivers/gpio/gpio-sx150x.c Neil Armstrong 2016-06-17  597  	} else
193ca3f8 drivers/gpio/gpio-sx150x.c Neil Armstrong 2016-06-17  598  		chip->gpio_chip.base             = pdata->gpio_base;

:::::: The code at line 594 was first introduced by commit
:::::: 04d2264c3bf07f5c3d18165ba78de0a93360c6c0 gpio: sx150x: add dts support for sx150x driver

:::::: TO: Wei Chen <Wei.Chen@csr.com>
:::::: CC: Linus Walleij <linus.walleij@linaro.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 26731 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: sx150x: Update OF configuration
  2016-06-17  9:51 [PATCH] gpio: sx150x: Update OF configuration Neil Armstrong
  2016-06-17 11:11 ` kbuild test robot
@ 2016-06-18  9:02 ` Linus Walleij
  2016-06-20  9:03   ` Neil Armstrong
  2016-06-20 15:57 ` Rob Herring
  2 siblings, 1 reply; 8+ messages in thread
From: Linus Walleij @ 2016-06-18  9:02 UTC (permalink / raw)
  To: Neil Armstrong; +Cc: Alexandre Courbot, linux-gpio, devicetree, linux-kernel

On Fri, Jun 17, 2016 at 11:51 AM, Neil Armstrong
<narmstrong@baylibre.com> wrote:

> In case of OF probing, the driver fails to initialize :
> - gpio_chip.base must be -1
> - irq_summary must be either -1 or valid
> - There is no way to use the other configurations
>
> Add OF parsing function to complete the HW configuration, make
> OF configuration dynamic instead of static with #defines and
> update the DT bindings.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

As you can see from the result of the build robot, the removal
of #ifdef CONFIG_OF_GPIO does not play well with non-DT
configurarions.

You need to figure something out here so that it builds for
both.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: sx150x: Update OF configuration
  2016-06-18  9:02 ` Linus Walleij
@ 2016-06-20  9:03   ` Neil Armstrong
  0 siblings, 0 replies; 8+ messages in thread
From: Neil Armstrong @ 2016-06-20  9:03 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Alexandre Courbot, linux-gpio, devicetree, linux-kernel

On 06/18/2016 11:02 AM, Linus Walleij wrote:
> On Fri, Jun 17, 2016 at 11:51 AM, Neil Armstrong
> <narmstrong@baylibre.com> wrote:
> 
>> In case of OF probing, the driver fails to initialize :
>> - gpio_chip.base must be -1
>> - irq_summary must be either -1 or valid
>> - There is no way to use the other configurations
>>
>> Add OF parsing function to complete the HW configuration, make
>> OF configuration dynamic instead of static with #defines and
>> update the DT bindings.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> As you can see from the result of the build robot, the removal
> of #ifdef CONFIG_OF_GPIO does not play well with non-DT
> configurarions.
> 
> You need to figure something out here so that it builds for
> both.

OK, I'll find out.

Thanks,
Neil

> Yours,
> Linus Walleij
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: sx150x: Update OF configuration
  2016-06-17  9:51 [PATCH] gpio: sx150x: Update OF configuration Neil Armstrong
  2016-06-17 11:11 ` kbuild test robot
  2016-06-18  9:02 ` Linus Walleij
@ 2016-06-20 15:57 ` Rob Herring
  2016-06-23  8:08   ` Linus Walleij
  2 siblings, 1 reply; 8+ messages in thread
From: Rob Herring @ 2016-06-20 15:57 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Linus Walleij, Alexandre Courbot, linux-gpio, devicetree, linux-kernel

On Fri, Jun 17, 2016 at 11:51:03AM +0200, Neil Armstrong wrote:
> In case of OF probing, the driver fails to initialize :
> - gpio_chip.base must be -1
> - irq_summary must be either -1 or valid
> - There is no way to use the other configurations
> 
> Add OF parsing function to complete the HW configuration, make
> OF configuration dynamic instead of static with #defines and
> update the DT bindings.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  .../devicetree/bindings/gpio/gpio-sx150x.txt       | 17 ++++++
>  drivers/gpio/gpio-sx150x.c                         | 65 +++++++++++++++++++---
>  2 files changed, 75 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-sx150x.txt b/Documentation/devicetree/bindings/gpio/gpio-sx150x.txt
> index c809acb..d2b5bb3 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-sx150x.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-sx150x.txt
> @@ -22,6 +22,17 @@ Required properties:
>  
>  - interrupt-controller: Marks the device as a interrupt controller.
>  
> +Optional Properties:
> +- oscio-is-gpo: Boolean, Indicated the oscio pin can be used as additional
> +		output gpo port.
> +

> +- pull-up-ports: Array of port numbers which must have pull-up enabled.
> +- pull-down-ports: Array of port numbers which must have pull-down enabled.
> +- open-drain-ports: Array of port numbers which must be configured as open-drain,
> +			Push-Pull mode is default.
> +- polarity-invert-ports: Array of port numbers whih port polarity must be inverted.

Seems like these should be done in a common way.

If not, they all need a vendor prefix.

> +- probe-reset: Boolean, Indicates the expander must be resetted.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: sx150x: Update OF configuration
  2016-06-20 15:57 ` Rob Herring
@ 2016-06-23  8:08   ` Linus Walleij
  2016-06-23  8:18     ` Neil Armstrong
  0 siblings, 1 reply; 8+ messages in thread
From: Linus Walleij @ 2016-06-23  8:08 UTC (permalink / raw)
  To: Rob Herring
  Cc: Neil Armstrong, Alexandre Courbot, linux-gpio, devicetree, linux-kernel

On Mon, Jun 20, 2016 at 5:57 PM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jun 17, 2016 at 11:51:03AM +0200, Neil Armstrong wrote:

>> +Optional Properties:
>> +- oscio-is-gpo: Boolean, Indicated the oscio pin can be used as additional
>> +             output gpo port.
>> +
>
>> +- pull-up-ports: Array of port numbers which must have pull-up enabled.
>> +- pull-down-ports: Array of port numbers which must have pull-down enabled.
>> +- open-drain-ports: Array of port numbers which must be configured as open-drain,
>> +                     Push-Pull mode is default.
>> +- polarity-invert-ports: Array of port numbers whih port polarity must be inverted.
>
> Seems like these should be done in a common way.
>
> If not, they all need a vendor prefix.

Actually on second look, this takes the sx150 to pin control territory.

I am starting to feel like a move to drivers/pinctrl/* might be warranted.

Neil you worked on other pin controllers IIRC, do you think it would
be much work to make a combined GPIO+pinctrl driver and move
this over to drivers/pinctrl?

I know refactoring across subsystems can be a pain, but at least
I'm maintaining both and happy to help out.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: sx150x: Update OF configuration
  2016-06-23  8:08   ` Linus Walleij
@ 2016-06-23  8:18     ` Neil Armstrong
  2016-06-23  9:08       ` Linus Walleij
  0 siblings, 1 reply; 8+ messages in thread
From: Neil Armstrong @ 2016-06-23  8:18 UTC (permalink / raw)
  To: Linus Walleij, Rob Herring
  Cc: Alexandre Courbot, linux-gpio, devicetree, linux-kernel

On 06/23/2016 10:08 AM, Linus Walleij wrote:
> On Mon, Jun 20, 2016 at 5:57 PM, Rob Herring <robh@kernel.org> wrote:
>> On Fri, Jun 17, 2016 at 11:51:03AM +0200, Neil Armstrong wrote:
> 
>>> +Optional Properties:
>>> +- oscio-is-gpo: Boolean, Indicated the oscio pin can be used as additional
>>> +             output gpo port.
>>> +
>>
>>> +- pull-up-ports: Array of port numbers which must have pull-up enabled.
>>> +- pull-down-ports: Array of port numbers which must have pull-down enabled.
>>> +- open-drain-ports: Array of port numbers which must be configured as open-drain,
>>> +                     Push-Pull mode is default.
>>> +- polarity-invert-ports: Array of port numbers whih port polarity must be inverted.
>>
>> Seems like these should be done in a common way.
>>
>> If not, they all need a vendor prefix.
> 
> Actually on second look, this takes the sx150 to pin control territory.
> 
> I am starting to feel like a move to drivers/pinctrl/* might be warranted.
> 
> Neil you worked on other pin controllers IIRC, do you think it would
> be much work to make a combined GPIO+pinctrl driver and move
> this over to drivers/pinctrl?
> 
> I know refactoring across subsystems can be a pain, but at least
> I'm maintaining both and happy to help out.
> 
> Yours,
> Linus Walleij
> 

Hi Linus,

Yes, it would be good to have it as a gpio+pinctrl with only pinconf, but it would show
the way on how to support external GPIO expanders using the pinctrl framework.

But it is quite challenging and needs quite some work, and actually the current state of the driver is that the OF is broken.

Would you agree to :
 - Push the minimal code to make OF work again, at least for 4.7 ?
 - Engage complete refactoring to transform it in a real gpio+pinctrl driver ?

For the dt-bindings properties, I can add the vendor prefixes ASAP.

Thanks,
Neil

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] gpio: sx150x: Update OF configuration
  2016-06-23  8:18     ` Neil Armstrong
@ 2016-06-23  9:08       ` Linus Walleij
  0 siblings, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2016-06-23  9:08 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Rob Herring, Alexandre Courbot, linux-gpio, devicetree, linux-kernel

On Thu, Jun 23, 2016 at 10:18 AM, Neil Armstrong
<narmstrong@baylibre.com> wrote:

> Would you agree to :
>  - Push the minimal code to make OF work again, at least for 4.7 ?
>  - Engage complete refactoring to transform it in a real gpio+pinctrl driver ?

Sounds like a plan, as long as the DT maintainers agree.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-06-23  9:08 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-17  9:51 [PATCH] gpio: sx150x: Update OF configuration Neil Armstrong
2016-06-17 11:11 ` kbuild test robot
2016-06-18  9:02 ` Linus Walleij
2016-06-20  9:03   ` Neil Armstrong
2016-06-20 15:57 ` Rob Herring
2016-06-23  8:08   ` Linus Walleij
2016-06-23  8:18     ` Neil Armstrong
2016-06-23  9:08       ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).