Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1424872 > unrolled thread

[PATCH] gpio: sx150x: Update OF configuration

Back to article view | Back to linux.kernel

Contents

  [PATCH] gpio: sx150x: Update OF configuration Neil Armstrong <narmstrong@baylibre.com> - 2016-06-17 12:00 +0200
    Re: [PATCH] gpio: sx150x: Update OF configuration kbuild test robot <lkp@intel.com> - 2016-06-17 13:20 +0200
    Re: [PATCH] gpio: sx150x: Update OF configuration Linus Walleij <linus.walleij@linaro.org> - 2016-06-18 11:10 +0200
      Re: [PATCH] gpio: sx150x: Update OF configuration Neil Armstrong <narmstrong@baylibre.com> - 2016-06-20 11:20 +0200
    Re: [PATCH] gpio: sx150x: Update OF configuration Rob Herring <robh@kernel.org> - 2016-06-20 18:20 +0200
      Re: [PATCH] gpio: sx150x: Update OF configuration Linus Walleij <linus.walleij@linaro.org> - 2016-06-23 10:10 +0200
        Re: [PATCH] gpio: sx150x: Update OF configuration Neil Armstrong <narmstrong@baylibre.com> - 2016-06-23 10:20 +0200
          Re: [PATCH] gpio: sx150x: Update OF configuration Linus Walleij <linus.walleij@linaro.org> - 2016-06-23 11:10 +0200

#1424872 — [PATCH] gpio: sx150x: Update OF configuration

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

[toc] | [next] | [standalone]

#1424948

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1425679

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

[toc] | [prev] | [next] | [standalone]

#1426372

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
> 

[toc] | [prev] | [next] | [standalone]

#1426766

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.

[toc] | [prev] | [next] | [standalone]

#1429553

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

[toc] | [prev] | [next] | [standalone]

#1429562

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

[toc] | [prev] | [next] | [standalone]

#1429619

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web