[PATCH 1/3] gpio: syscon: Add support for a custom get operation

[PATCH 1/3] gpio: syscon: Add support for a custom get operation

[Paul Kocialkowski]

Some drivers might need a custom get operation to match custom
behavior implemented in the set operation.

Add plumbing for supporting that.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpio/gpio-syscon.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 31f332074d7d..05c537ed73f1 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -43,8 +43,9 @@ struct syscon_gpio_data {
 	unsigned int	bit_count;
 	unsigned int	dat_bit_offset;
 	unsigned int	dir_bit_offset;
-	void		(*set)(struct gpio_chip *chip,
-			       unsigned offset, int value);
+	int		(*get)(struct gpio_chip *chip, unsigned offset);
+	void		(*set)(struct gpio_chip *chip, unsigned offset,
+			       int value);
 };
 
 struct syscon_gpio_priv {
@@ -252,7 +253,7 @@ static int syscon_gpio_probe(struct platform_device *pdev)
 	priv->chip.label = dev_name(dev);
 	priv->chip.base = -1;
 	priv->chip.ngpio = priv->data->bit_count;
-	priv->chip.get = syscon_gpio_get;
+	priv->chip.get = priv->data->get ? : syscon_gpio_get;
 	if (priv->data->flags & GPIO_SYSCON_FEAT_IN)
 		priv->chip.direction_input = syscon_gpio_dir_in;
 	if (priv->data->flags & GPIO_SYSCON_FEAT_OUT) {
-- 
2.23.0

[PATCH 2/3] dt-bindings: gpio: Add binding document for xylon logicvc-gpio

[Paul Kocialkowski]

The Xylon LogiCVC display controller exports some GPIOs, which are
exposed as a dedicated driver.

This introduces the associated device-tree bindings documentation.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 .../bindings/gpio/xylon,logicvc-gpio.txt      | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt

diff --git a/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt b/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt
new file mode 100644
index 000000000000..4835659cb90b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt
@@ -0,0 +1,48 @@
+Xylon LogiCVC GPIO controller
+
+The Xylon LogiCVC is a display controller that contains a number of GPIO pins,
+meant to be used for controlling display-related signals.
+
+In practice, the GPIOs can be used for any purpose they might be needed for.
+
+The controller exposes GPIOs from the display and power control registers,
+which are mapped by the driver as follows:
+- GPIO[4:0] (display control) mapped to index 0-4
+- EN_BLIGHT (power control) mapped to index 5
+- EN_VDD (power control) mapped to index 6
+- EN_VEE (power control) mapped to index 7
+- V_EN (power control) mapped to index 8
+
+The driver was implemented and tested for version 3.02.a of the controller,
+but should be compatible with version 4 as well.
+
+Required properties:
+- compatible: Should contain "xylon,logicvc-3.02.a-gpio".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be 2. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+    0 = Active high,
+    1 = Active low.
+- gpio,syscon-dev: Syscon phandle representing the logicvc instance.
+
+Example:
+
+	logicvc: logicvc@43c00000 {
+		compatible = "syscon", "simple-mfd";
+		reg = <0x43c00000 0x6000>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		logicvc_gpio: display-gpio@40 {
+			compatible = "xylon,logicvc-3.02.a-gpio";
+			reg = <0x40 0x40>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio,syscon-dev = <&logicvc>;
+		};
+	};
+
+Note: the device-tree node should either be declared as a child of the logicvc
+syscon node or the syscon node should be precised with the gpio,syscon-dev
+property. Both are shown in the example above.
-- 
2.23.0

[PATCH 3/3] gpio: syscon: Add support for the Xylon LogiCVC GPIOs

[Paul Kocialkowski]

The LogiCVC display hardware block comes with GPIO capabilities
that must be exposed separately from the main driver (as GPIOs) for
use with regulators and panels. A syscon is used to share the same
regmap across the two drivers.

Since the GPIO capabilities are pretty simple, add them to the syscon
GPIO driver.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpio/gpio-syscon.c | 68 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 05c537ed73f1..3d435187940b 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -190,6 +190,70 @@ static const struct syscon_gpio_data keystone_dsp_gpio = {
 	.set		= keystone_gpio_set,
 };
 
+#define LOGICVC_CTRL_REG		0x40
+#define LOGICVC_CTRL_GPIO_SHIFT		11
+#define LOGICVC_CTRL_GPIO_BITS		5
+
+#define LOGICVC_POWER_CTRL_REG		0x78
+#define LOGICVC_POWER_CTRL_GPIO_SHIFT	0
+#define LOGICVC_POWER_CTRL_GPIO_BITS	4
+
+static void logicvc_gpio_offset(struct syscon_gpio_priv *priv,
+				unsigned offset, unsigned int *reg,
+				unsigned int *bit)
+{
+	if (offset >= LOGICVC_CTRL_GPIO_BITS) {
+		*reg = LOGICVC_POWER_CTRL_REG;
+
+		/* To the (virtual) power ctrl offset. */
+		offset -= LOGICVC_CTRL_GPIO_BITS;
+		/* To the actual bit offset in reg. */
+		offset += LOGICVC_POWER_CTRL_GPIO_SHIFT;
+	} else {
+		*reg = LOGICVC_CTRL_REG;
+
+		/* To the actual bit offset in reg. */
+		offset += LOGICVC_CTRL_GPIO_SHIFT;
+	}
+
+	*bit = 1 << offset;
+}
+
+static int logicvc_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct syscon_gpio_priv *priv = gpiochip_get_data(chip);
+	unsigned int reg;
+	unsigned int bit;
+	unsigned int value;
+	int ret;
+
+	logicvc_gpio_offset(priv, offset, &reg, &bit);
+
+	ret = regmap_read(priv->syscon, reg, &value);
+	if (ret)
+		return ret;
+
+	return !!(value & bit);
+}
+
+static void logicvc_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
+{
+	struct syscon_gpio_priv *priv = gpiochip_get_data(chip);
+	unsigned int reg;
+	unsigned int bit;
+
+	logicvc_gpio_offset(priv, offset, &reg, &bit);
+
+	regmap_update_bits(priv->syscon, reg, bit, val ? bit : 0);
+}
+
+static const struct syscon_gpio_data logicvc_3_gpio = {
+	.flags		= GPIO_SYSCON_FEAT_OUT,
+	.bit_count	= LOGICVC_CTRL_GPIO_BITS + LOGICVC_POWER_CTRL_GPIO_BITS,
+	.get		= logicvc_gpio_get,
+	.set		= logicvc_gpio_set,
+};
+
 static const struct of_device_id syscon_gpio_ids[] = {
 	{
 		.compatible	= "cirrus,ep7209-mctrl-gpio",
@@ -203,6 +267,10 @@ static const struct of_device_id syscon_gpio_ids[] = {
 		.compatible	= "rockchip,rk3328-grf-gpio",
 		.data		= &rockchip_rk3328_gpio_mute,
 	},
+	{
+		.compatible	= "xylon,logicvc-3.02.a-gpio",
+		.data		= &logicvc_3_gpio,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, syscon_gpio_ids);
-- 
2.23.0

Re: [PATCH 3/3] gpio: syscon: Add support for the Xylon LogiCVC GPIOs

[Linus Walleij]

On Tue, Sep 10, 2019 at 4:29 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:

> The LogiCVC display hardware block comes with GPIO capabilities
> that must be exposed separately from the main driver (as GPIOs) for
> use with regulators and panels. A syscon is used to share the same
> regmap across the two drivers.
>
> Since the GPIO capabilities are pretty simple, add them to the syscon
> GPIO driver.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

I'm fine with this for now, but the gpio-syscon driver is now growing
big and when you use it you are getting support for a whole bunch
of systems you're not running on included in your binary.

We need to think about possibly creating drivers/gpio/syscon
and split subdrivers into separate files and config options
so that people can slim down to what they actually need.

> +       *bit = 1 << offset;

Please do this:

#include <linux/bits.h>

*bit = BIT(offset);

Yours,
Linus Walleij

Re: [PATCH 1/3] gpio: syscon: Add support for a custom get operation

[Linus Walleij]

On Tue, Sep 10, 2019 at 4:29 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:

> Some drivers might need a custom get operation to match custom
> behavior implemented in the set operation.
>
> Add plumbing for supporting that.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Looks OK but as noted in the other patch: we are accumulating stuff
in this driver, possibly this syscon part should just be a library
used by individual drivers that can be switched on/off with Kconfig.

Yours,
Linus Walleij

Re: [PATCH 2/3] dt-bindings: gpio: Add binding document for xylon logicvc-gpio

[Linus Walleij]

On Tue, Sep 10, 2019 at 4:29 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:

> The Xylon LogiCVC display controller exports some GPIOs, which are
> exposed as a dedicated driver.
>
> This introduces the associated device-tree bindings documentation.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
(...)
> +The controller exposes GPIOs from the display and power control registers,
> +which are mapped by the driver as follows:
> +- GPIO[4:0] (display control) mapped to index 0-4
> +- EN_BLIGHT (power control) mapped to index 5
> +- EN_VDD (power control) mapped to index 6
> +- EN_VEE (power control) mapped to index 7
> +- V_EN (power control) mapped to index 8

This should be reflected in the gpio-line-names in the example
and in your device trees.

Yours,
Linus Walleij

Re: [PATCH 2/3] dt-bindings: gpio: Add binding document for xylon logicvc-gpio

[Rob Herring]

On Tue, Sep 10, 2019 at 05:28:54PM +0200, Paul Kocialkowski wrote:
> The Xylon LogiCVC display controller exports some GPIOs, which are
> exposed as a dedicated driver.
> 
> This introduces the associated device-tree bindings documentation.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  .../bindings/gpio/xylon,logicvc-gpio.txt      | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt

Please consider using the new DT schema format.

> 
> diff --git a/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt b/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt
> new file mode 100644
> index 000000000000..4835659cb90b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt
> @@ -0,0 +1,48 @@
> +Xylon LogiCVC GPIO controller
> +
> +The Xylon LogiCVC is a display controller that contains a number of GPIO pins,
> +meant to be used for controlling display-related signals.
> +
> +In practice, the GPIOs can be used for any purpose they might be needed for.
> +
> +The controller exposes GPIOs from the display and power control registers,
> +which are mapped by the driver as follows:
> +- GPIO[4:0] (display control) mapped to index 0-4
> +- EN_BLIGHT (power control) mapped to index 5
> +- EN_VDD (power control) mapped to index 6
> +- EN_VEE (power control) mapped to index 7
> +- V_EN (power control) mapped to index 8
> +
> +The driver was implemented and tested for version 3.02.a of the controller,
> +but should be compatible with version 4 as well.
> +
> +Required properties:
> +- compatible: Should contain "xylon,logicvc-3.02.a-gpio".
> +- gpio-controller: Marks the device node as a gpio controller.
> +- #gpio-cells: Should be 2. The first cell is the pin number and
> +  the second cell is used to specify the gpio polarity:
> +    0 = Active high,
> +    1 = Active low.

No need to define these standard flags again here.

> +- gpio,syscon-dev: Syscon phandle representing the logicvc instance.

Don't need this. It's the parent.

> +
> +Example:
> +
> +	logicvc: logicvc@43c00000 {
> +		compatible = "syscon", "simple-mfd";

This device needs a device specific compatible. These 2 alone are not 
desired.

Please define everything that's in the chip as much as you can. 

> +		reg = <0x43c00000 0x6000>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		logicvc_gpio: display-gpio@40 {

Use standard node names: gpio@40

You may not even need a child node here. It depends on what other child 
nodes you have and whether they have their own DT resources.

> +			compatible = "xylon,logicvc-3.02.a-gpio";
> +			reg = <0x40 0x40>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio,syscon-dev = <&logicvc>;
> +		};
> +	};
> +
> +Note: the device-tree node should either be declared as a child of the logicvc
> +syscon node or the syscon node should be precised with the gpio,syscon-dev
> +property. Both are shown in the example above.

Why? Just pick one and a child node is the preference.

> -- 
> 2.23.0
> 

Re: [PATCH 3/3] gpio: syscon: Add support for the Xylon LogiCVC GPIOs

[Paul Kocialkowski]

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

Hi,

On Thu 12 Sep 19, 10:17, Linus Walleij wrote:
> On Tue, Sep 10, 2019 at 4:29 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> 
> > The LogiCVC display hardware block comes with GPIO capabilities
> > that must be exposed separately from the main driver (as GPIOs) for
> > use with regulators and panels. A syscon is used to share the same
> > regmap across the two drivers.
> >
> > Since the GPIO capabilities are pretty simple, add them to the syscon
> > GPIO driver.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> 
> I'm fine with this for now, but the gpio-syscon driver is now growing
> big and when you use it you are getting support for a whole bunch
> of systems you're not running on included in your binary.
> 
> We need to think about possibly creating drivers/gpio/syscon
> and split subdrivers into separate files and config options
> so that people can slim down to what they actually need.

Thanks for the review!

I understand your concern about more devices getting pulled-in and built
unconditionally. Though I do like the idea of having a single driver for only
exposing the GPIO part of bigger components instead of specific drivers with
< 100 lines of useful code.

Maybe a first step would be to introduce Kconfig options for each device and
ifdef around in the code, as to solve the "built unconditionally" aspect?

Either way, I'll send v2 still adding my driver to gpio-syscon but feel free to
have me in the loop when that driver needs to be changed.

> > +       *bit = 1 << offset;
> 
> Please do this:
> 
> #include <linux/bits.h>
> 
> *bit = BIT(offset);

Sure thing and sorry I missed that, thanks!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] gpio: syscon: Add support for a custom get operation

[Paul Kocialkowski]

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

Hi,

On Thu 12 Sep 19, 10:18, Linus Walleij wrote:
> On Tue, Sep 10, 2019 at 4:29 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> 
> > Some drivers might need a custom get operation to match custom
> > behavior implemented in the set operation.
> >
> > Add plumbing for supporting that.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> 
> Looks OK but as noted in the other patch: we are accumulating stuff
> in this driver, possibly this syscon part should just be a library
> used by individual drivers that can be switched on/off with Kconfig.

Looks like adding support for a custom get operation would be good to have
before moving to a library then :)

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] dt-bindings: gpio: Add binding document for xylon logicvc-gpio

[Paul Kocialkowski]

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

Hi,

On Thu 12 Sep 19, 10:19, Linus Walleij wrote:
> On Tue, Sep 10, 2019 at 4:29 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> 
> > The Xylon LogiCVC display controller exports some GPIOs, which are
> > exposed as a dedicated driver.
> >
> > This introduces the associated device-tree bindings documentation.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> (...)
> > +The controller exposes GPIOs from the display and power control registers,
> > +which are mapped by the driver as follows:
> > +- GPIO[4:0] (display control) mapped to index 0-4
> > +- EN_BLIGHT (power control) mapped to index 5
> > +- EN_VDD (power control) mapped to index 6
> > +- EN_VEE (power control) mapped to index 7
> > +- V_EN (power control) mapped to index 8
> 
> This should be reflected in the gpio-line-names in the example
> and in your device trees.

Thanks, I didn't know about it until now!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] dt-bindings: gpio: Add binding document for xylon logicvc-gpio

[Paul Kocialkowski]

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

Hi,

On Fri 13 Sep 19, 15:36, Rob Herring wrote:
> On Tue, Sep 10, 2019 at 05:28:54PM +0200, Paul Kocialkowski wrote:
> > The Xylon LogiCVC display controller exports some GPIOs, which are
> > exposed as a dedicated driver.
> > 
> > This introduces the associated device-tree bindings documentation.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  .../bindings/gpio/xylon,logicvc-gpio.txt      | 48 +++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt
> 
> Please consider using the new DT schema format.

Sure, I will give it a try.

> > diff --git a/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt b/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt
> > new file mode 100644
> > index 000000000000..4835659cb90b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/xylon,logicvc-gpio.txt
> > @@ -0,0 +1,48 @@
> > +Xylon LogiCVC GPIO controller
> > +
> > +The Xylon LogiCVC is a display controller that contains a number of GPIO pins,
> > +meant to be used for controlling display-related signals.
> > +
> > +In practice, the GPIOs can be used for any purpose they might be needed for.
> > +
> > +The controller exposes GPIOs from the display and power control registers,
> > +which are mapped by the driver as follows:
> > +- GPIO[4:0] (display control) mapped to index 0-4
> > +- EN_BLIGHT (power control) mapped to index 5
> > +- EN_VDD (power control) mapped to index 6
> > +- EN_VEE (power control) mapped to index 7
> > +- V_EN (power control) mapped to index 8
> > +
> > +The driver was implemented and tested for version 3.02.a of the controller,
> > +but should be compatible with version 4 as well.
> > +
> > +Required properties:
> > +- compatible: Should contain "xylon,logicvc-3.02.a-gpio".
> > +- gpio-controller: Marks the device node as a gpio controller.
> > +- #gpio-cells: Should be 2. The first cell is the pin number and
> > +  the second cell is used to specify the gpio polarity:
> > +    0 = Active high,
> > +    1 = Active low.
> 
> No need to define these standard flags again here.
> 
> > +- gpio,syscon-dev: Syscon phandle representing the logicvc instance.
> 
> Don't need this. It's the parent.

Note that this is de-facto already supported by the gpio-syscon driver: the
driver supports either an explicit syscon dev with this property or having the
parent as syscon.

I assumed that mentioning both was good for the sake of diversity, but let's
stick to parent node then.

> > +
> > +Example:
> > +
> > +	logicvc: logicvc@43c00000 {
> > +		compatible = "syscon", "simple-mfd";
> 
> This device needs a device specific compatible. These 2 alone are not 
> desired.

Thanks for the heads-up. I'll introduce a new compatible, describing a mfd
device then.

> Please define everything that's in the chip as much as you can. 
> 
> > +		reg = <0x43c00000 0x6000>;
> > +
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +
> > +		logicvc_gpio: display-gpio@40 {
> 
> Use standard node names: gpio@40
> 
> You may not even need a child node here. It depends on what other child 
> nodes you have and whether they have their own DT resources.
> 
> > +			compatible = "xylon,logicvc-3.02.a-gpio";
> > +			reg = <0x40 0x40>;
> > +			gpio-controller;
> > +			#gpio-cells = <2>;
> > +			gpio,syscon-dev = <&logicvc>;
> > +		};
> > +	};
> > +
> > +Note: the device-tree node should either be declared as a child of the logicvc
> > +syscon node or the syscon node should be precised with the gpio,syscon-dev
> > +property. Both are shown in the example above.
> 
> Why? Just pick one and a child node is the preference.

That was for the sake of diversity, but having both at once could probably be
misleading too.

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] gpio: syscon: Add support for the Xylon LogiCVC GPIOs

[Linus Walleij]

On Mon, Sep 23, 2019 at 3:33 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:

> Maybe a first step would be to introduce Kconfig options for each device and
> ifdef around in the code, as to solve the "built unconditionally" aspect?

ifdefs is something we try to avoid using too much, better for things
to have their own files and use a library, usually, it's cleaner.

Yours,
Linus Walleij