[PATCH v3 0/7] gpio: exar: refactor the driver

[PATCH v3 0/7] gpio: exar: refactor the driver

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I just wanted to convert the driver to using simpler IDA API but ended up
quickly converting it to using regmap. Unfortunately I don't have the HW
to test it so marking the patches that introduce functional change as RFT
and Cc'ing the original author.

v1 -> v2:
- add new regmap helper: regmap_assign_bits()
- fix lvl vs sel register access
- set value in direction_output callback

v2 -> v3:
- drop the regmap helper from series

Note: I didn't use the fancy method of offset calculation Andy and David
suggested because this series broke the driver according to Jan - let's
get it right before we modify it any more. I found a couple problems that
could maybe cause the crash. Jan: could you give it another spin?

Bartosz Golaszewski (7):
  gpio: exar: add a newline after the copyright notice
  gpio: exar: include idr.h
  gpio: exar: switch to a simpler IDA interface
  gpio: exar: use a helper variable for &pdev->dev
  gpio: exar: unduplicate address and offset computation
  gpio: exar: switch to using regmap
  gpio: exar: use devm action for freeing the IDA and drop remove()

 drivers/gpio/Kconfig     |   1 +
 drivers/gpio/gpio-exar.c | 154 +++++++++++++++++++--------------------
 2 files changed, 77 insertions(+), 78 deletions(-)

-- 
2.29.1

[PATCH v3 1/7] gpio: exar: add a newline after the copyright notice

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

It's customary to have a newline between the copyright header and the
includes. Add one to gpio-exar.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index b1accfba017d..4202dd363a11 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2015 Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
  */
+
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/gpio/driver.h>
-- 
2.29.1

[PATCH v3 2/7] gpio: exar: include idr.h

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This driver uses IDA APIs but doesn't include the relevant header. This
fixes it.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 4202dd363a11..1941ae533418 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -8,6 +8,7 @@
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/gpio/driver.h>
+#include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-- 
2.29.1

[PATCH v3 3/7] gpio: exar: switch to a simpler IDA interface

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We don't need to specify any ranges when allocating IDs so we can switch
to ida_alloc() and ida_free() instead of the ida_simple_ counterparts.

ida_simple_get(ida, 0, 0, gfp) is equivalent to
ida_alloc_range(ida, 0, UINT_MAX, gfp) which is equivalent to
ida_alloc(ida, gfp). Note: IDR will never actually allocate an ID
larger than INT_MAX.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 1941ae533418..752e8437ff80 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -149,7 +149,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 
 	mutex_init(&exar_gpio->lock);
 
-	index = ida_simple_get(&ida_index, 0, 0, GFP_KERNEL);
+	index = ida_alloc(&ida_index, GFP_KERNEL);
 	if (index < 0) {
 		ret = index;
 		goto err_mutex_destroy;
@@ -179,7 +179,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	return 0;
 
 err_destroy:
-	ida_simple_remove(&ida_index, index);
+	ida_free(&ida_index, index);
 err_mutex_destroy:
 	mutex_destroy(&exar_gpio->lock);
 	return ret;
@@ -189,7 +189,7 @@ static int gpio_exar_remove(struct platform_device *pdev)
 {
 	struct exar_gpio_chip *exar_gpio = platform_get_drvdata(pdev);
 
-	ida_simple_remove(&ida_index, exar_gpio->index);
+	ida_free(&ida_index, exar_gpio->index);
 	mutex_destroy(&exar_gpio->lock);
 
 	return 0;
-- 
2.29.1

[PATCH v3 4/7] gpio: exar: use a helper variable for &pdev->dev

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

It's more elegant to use a helper local variable to store the address
of the underlying struct device than to dereference pdev everywhere. It
also has the benefit of avoiding unnecessary line breaks.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 752e8437ff80..db366d85b6b4 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -120,7 +120,8 @@ static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
 
 static int gpio_exar_probe(struct platform_device *pdev)
 {
-	struct pci_dev *pcidev = to_pci_dev(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct pci_dev *pcidev = to_pci_dev(dev->parent);
 	struct exar_gpio_chip *exar_gpio;
 	u32 first_pin, ngpios;
 	void __iomem *p;
@@ -134,16 +135,15 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	if (!p)
 		return -ENOMEM;
 
-	ret = device_property_read_u32(&pdev->dev, "exar,first-pin",
-				       &first_pin);
+	ret = device_property_read_u32(dev, "exar,first-pin", &first_pin);
 	if (ret)
 		return ret;
 
-	ret = device_property_read_u32(&pdev->dev, "ngpios", &ngpios);
+	ret = device_property_read_u32(dev, "ngpios", &ngpios);
 	if (ret)
 		return ret;
 
-	exar_gpio = devm_kzalloc(&pdev->dev, sizeof(*exar_gpio), GFP_KERNEL);
+	exar_gpio = devm_kzalloc(dev, sizeof(*exar_gpio), GFP_KERNEL);
 	if (!exar_gpio)
 		return -ENOMEM;
 
@@ -157,7 +157,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 
 	sprintf(exar_gpio->name, "exar_gpio%d", index);
 	exar_gpio->gpio_chip.label = exar_gpio->name;
-	exar_gpio->gpio_chip.parent = &pdev->dev;
+	exar_gpio->gpio_chip.parent = dev;
 	exar_gpio->gpio_chip.direction_output = exar_direction_output;
 	exar_gpio->gpio_chip.direction_input = exar_direction_input;
 	exar_gpio->gpio_chip.get_direction = exar_get_direction;
@@ -169,8 +169,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	exar_gpio->index = index;
 	exar_gpio->first_pin = first_pin;
 
-	ret = devm_gpiochip_add_data(&pdev->dev,
-				     &exar_gpio->gpio_chip, exar_gpio);
+	ret = devm_gpiochip_add_data(dev, &exar_gpio->gpio_chip, exar_gpio);
 	if (ret)
 		goto err_destroy;
 
-- 
2.29.1

[PATCH v3 5/7] gpio: exar: unduplicate address and offset computation

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Provide and use helpers for calculating the register address and bit
offset instead of hand coding it in every function.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index db366d85b6b4..28b0b4b5fa35 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -33,6 +33,26 @@ struct exar_gpio_chip {
 	unsigned int first_pin;
 };
 
+static unsigned int
+exar_offset_to_sel_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
+{
+	return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOSEL_HI
+						   : EXAR_OFFSET_MPIOSEL_LO;
+}
+
+static unsigned int
+exar_offset_to_lvl_addr(struct exar_gpio_chip *exar_gpio, unsigned int offset)
+{
+	return (offset + exar_gpio->first_pin) / 8 ? EXAR_OFFSET_MPIOLVL_HI
+						   : EXAR_OFFSET_MPIOLVL_LO;
+}
+
+static unsigned int
+exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset)
+{
+	return (offset + exar_gpio->first_pin) % 8;
+}
+
 static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
 			unsigned int offset)
 {
@@ -52,9 +72,8 @@ static int exar_set_direction(struct gpio_chip *chip, int direction,
 			      unsigned int offset)
 {
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
-		EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
-	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
+	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
 	exar_update(chip, addr, direction, bit);
 	return 0;
@@ -75,9 +94,8 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)
 static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
-		EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
-	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
+	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
 	if (exar_get(chip, addr) & BIT(bit))
 		return GPIO_LINE_DIRECTION_IN;
@@ -88,9 +106,8 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
 static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
 {
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
-		EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
-	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
+	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
 	return !!(exar_get(chip, addr) & BIT(bit));
 }
@@ -99,9 +116,8 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
 			   int value)
 {
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
-		EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
-	unsigned int bit  = (offset + exar_gpio->first_pin) % 8;
+	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
 	exar_update(chip, addr, value, bit);
 }
-- 
2.29.1

[PATCH v3 6/7] gpio: exar: switch to using regmap

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We can simplify the code in gpio-exar by using regmap. This allows us to
drop the mutex (regmap provides its own locking) and we can also reuse
regmap's bit operations instead of implementing our own update function.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/Kconfig     |  1 +
 drivers/gpio/gpio-exar.c | 90 +++++++++++++++++-----------------------
 2 files changed, 38 insertions(+), 53 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5d4de5cd6759..253a61ec9645 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -255,6 +255,7 @@ config GPIO_EP93XX
 config GPIO_EXAR
 	tristate "Support for GPIO pins on XR17V352/354/358"
 	depends on SERIAL_8250_EXAR
+	select REGMAP_MMIO
 	help
 	  Selecting this option will enable handling of GPIO pins present
 	  on Exar XR17V352/354/358 chips.
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 28b0b4b5fa35..a2d324c513f8 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 
 #define EXAR_OFFSET_MPIOLVL_LO 0x90
 #define EXAR_OFFSET_MPIOSEL_LO 0x93
@@ -26,9 +27,8 @@ static DEFINE_IDA(ida_index);
 
 struct exar_gpio_chip {
 	struct gpio_chip gpio_chip;
-	struct mutex lock;
+	struct regmap *regs;
 	int index;
-	void __iomem *regs;
 	char name[20];
 	unsigned int first_pin;
 };
@@ -53,51 +53,13 @@ exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset)
 	return (offset + exar_gpio->first_pin) % 8;
 }
 
-static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
-			unsigned int offset)
-{
-	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	int temp;
-
-	mutex_lock(&exar_gpio->lock);
-	temp = readb(exar_gpio->regs + reg);
-	temp &= ~BIT(offset);
-	if (val)
-		temp |= BIT(offset);
-	writeb(temp, exar_gpio->regs + reg);
-	mutex_unlock(&exar_gpio->lock);
-}
-
-static int exar_set_direction(struct gpio_chip *chip, int direction,
-			      unsigned int offset)
-{
-	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
-	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
-
-	exar_update(chip, addr, direction, bit);
-	return 0;
-}
-
-static int exar_get(struct gpio_chip *chip, unsigned int reg)
-{
-	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
-	int value;
-
-	mutex_lock(&exar_gpio->lock);
-	value = readb(exar_gpio->regs + reg);
-	mutex_unlock(&exar_gpio->lock);
-
-	return value;
-}
-
 static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
 	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
 	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
-	if (exar_get(chip, addr) & BIT(bit))
+	if (regmap_test_bits(exar_gpio->regs, addr, BIT(bit)))
 		return GPIO_LINE_DIRECTION_IN;
 
 	return GPIO_LINE_DIRECTION_OUT;
@@ -109,7 +71,7 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
 	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
 	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
-	return !!(exar_get(chip, addr) & BIT(bit));
+	return !!(regmap_test_bits(exar_gpio->regs, addr, BIT(bit)));
 }
 
 static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
@@ -119,21 +81,42 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
 	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
 	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
 
-	exar_update(chip, addr, value, bit);
+	if (value)
+		regmap_set_bits(exar_gpio->regs, addr, BIT(bit));
+	else
+		regmap_clear_bits(exar_gpio->regs, addr, BIT(bit));
 }
 
 static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
 				 int value)
 {
+	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
+
 	exar_set_value(chip, offset, value);
-	return exar_set_direction(chip, 0, offset);
+	regmap_clear_bits(exar_gpio->regs, addr, BIT(bit));
+
+	return 0;
 }
 
 static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
 {
-	return exar_set_direction(chip, 1, offset);
+	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
+	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
+
+	regmap_set_bits(exar_gpio->regs, addr, BIT(bit));
+
+	return 0;
 }
 
+static const struct regmap_config exar_regmap_config = {
+	.name		= "exar-gpio",
+	.reg_bits	= 8,
+	.val_bits	= 8,
+};
+
 static int gpio_exar_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -163,13 +146,17 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	if (!exar_gpio)
 		return -ENOMEM;
 
-	mutex_init(&exar_gpio->lock);
+	/*
+	 * We don't need to check the return values of mmio regmap operations (unless
+	 * the regmap has a clock attached which is not the case here).
+	 */
+	exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
+	if (IS_ERR(exar_gpio->regs))
+		return PTR_ERR(exar_gpio->regs);
 
 	index = ida_alloc(&ida_index, GFP_KERNEL);
-	if (index < 0) {
-		ret = index;
-		goto err_mutex_destroy;
-	}
+	if (index < 0)
+		return index;
 
 	sprintf(exar_gpio->name, "exar_gpio%d", index);
 	exar_gpio->gpio_chip.label = exar_gpio->name;
@@ -195,8 +182,6 @@ static int gpio_exar_probe(struct platform_device *pdev)
 
 err_destroy:
 	ida_free(&ida_index, index);
-err_mutex_destroy:
-	mutex_destroy(&exar_gpio->lock);
 	return ret;
 }
 
@@ -205,7 +190,6 @@ static int gpio_exar_remove(struct platform_device *pdev)
 	struct exar_gpio_chip *exar_gpio = platform_get_drvdata(pdev);
 
 	ida_free(&ida_index, exar_gpio->index);
-	mutex_destroy(&exar_gpio->lock);
 
 	return 0;
 }
-- 
2.29.1

[PATCH v3 7/7] gpio: exar: use devm action for freeing the IDA and drop remove()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We can simplify the error path in probe() and drop remove() entirely if
we provide a devm action for freeing the device ID.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-exar.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index a2d324c513f8..ae3ead033de4 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -111,6 +111,13 @@ static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
 	return 0;
 }
 
+static void exar_devm_ida_free(void *data)
+{
+	struct exar_gpio_chip *exar_gpio = data;
+
+	ida_free(&ida_index, exar_gpio->index);
+}
+
 static const struct regmap_config exar_regmap_config = {
 	.name		= "exar-gpio",
 	.reg_bits	= 8,
@@ -158,6 +165,10 @@ static int gpio_exar_probe(struct platform_device *pdev)
 	if (index < 0)
 		return index;
 
+	ret = devm_add_action_or_reset(dev, exar_devm_ida_free, exar_gpio);
+	if (ret)
+		return ret;
+
 	sprintf(exar_gpio->name, "exar_gpio%d", index);
 	exar_gpio->gpio_chip.label = exar_gpio->name;
 	exar_gpio->gpio_chip.parent = dev;
@@ -174,29 +185,15 @@ static int gpio_exar_probe(struct platform_device *pdev)
 
 	ret = devm_gpiochip_add_data(dev, &exar_gpio->gpio_chip, exar_gpio);
 	if (ret)
-		goto err_destroy;
+		return ret;
 
 	platform_set_drvdata(pdev, exar_gpio);
 
-	return 0;
-
-err_destroy:
-	ida_free(&ida_index, index);
-	return ret;
-}
-
-static int gpio_exar_remove(struct platform_device *pdev)
-{
-	struct exar_gpio_chip *exar_gpio = platform_get_drvdata(pdev);
-
-	ida_free(&ida_index, exar_gpio->index);
-
 	return 0;
 }
 
 static struct platform_driver gpio_exar_driver = {
 	.probe	= gpio_exar_probe,
-	.remove	= gpio_exar_remove,
 	.driver	= {
 		.name = DRIVER_NAME,
 	},
-- 
2.29.1

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Jan Kiszka]

On 10.11.20 13:34, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We can simplify the code in gpio-exar by using regmap. This allows us to
> drop the mutex (regmap provides its own locking) and we can also reuse
> regmap's bit operations instead of implementing our own update function.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/Kconfig     |  1 +
>  drivers/gpio/gpio-exar.c | 90 +++++++++++++++++-----------------------
>  2 files changed, 38 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 5d4de5cd6759..253a61ec9645 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -255,6 +255,7 @@ config GPIO_EP93XX
>  config GPIO_EXAR
>  	tristate "Support for GPIO pins on XR17V352/354/358"
>  	depends on SERIAL_8250_EXAR
> +	select REGMAP_MMIO
>  	help
>  	  Selecting this option will enable handling of GPIO pins present
>  	  on Exar XR17V352/354/358 chips.
> diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
> index 28b0b4b5fa35..a2d324c513f8 100644
> --- a/drivers/gpio/gpio-exar.c
> +++ b/drivers/gpio/gpio-exar.c
> @@ -14,6 +14,7 @@
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  
>  #define EXAR_OFFSET_MPIOLVL_LO 0x90
>  #define EXAR_OFFSET_MPIOSEL_LO 0x93
> @@ -26,9 +27,8 @@ static DEFINE_IDA(ida_index);
>  
>  struct exar_gpio_chip {
>  	struct gpio_chip gpio_chip;
> -	struct mutex lock;
> +	struct regmap *regs;
>  	int index;
> -	void __iomem *regs;
>  	char name[20];
>  	unsigned int first_pin;
>  };
> @@ -53,51 +53,13 @@ exar_offset_to_bit(struct exar_gpio_chip *exar_gpio, unsigned int offset)
>  	return (offset + exar_gpio->first_pin) % 8;
>  }
>  
> -static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
> -			unsigned int offset)
> -{
> -	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> -	int temp;
> -
> -	mutex_lock(&exar_gpio->lock);
> -	temp = readb(exar_gpio->regs + reg);
> -	temp &= ~BIT(offset);
> -	if (val)
> -		temp |= BIT(offset);
> -	writeb(temp, exar_gpio->regs + reg);
> -	mutex_unlock(&exar_gpio->lock);
> -}
> -
> -static int exar_set_direction(struct gpio_chip *chip, int direction,
> -			      unsigned int offset)
> -{
> -	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> -	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
> -	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
> -
> -	exar_update(chip, addr, direction, bit);
> -	return 0;
> -}
> -
> -static int exar_get(struct gpio_chip *chip, unsigned int reg)
> -{
> -	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> -	int value;
> -
> -	mutex_lock(&exar_gpio->lock);
> -	value = readb(exar_gpio->regs + reg);
> -	mutex_unlock(&exar_gpio->lock);
> -
> -	return value;
> -}
> -
>  static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
>  	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
>  	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
>  
> -	if (exar_get(chip, addr) & BIT(bit))
> +	if (regmap_test_bits(exar_gpio->regs, addr, BIT(bit)))
>  		return GPIO_LINE_DIRECTION_IN;
>  
>  	return GPIO_LINE_DIRECTION_OUT;
> @@ -109,7 +71,7 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
>  	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
>  	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
>  
> -	return !!(exar_get(chip, addr) & BIT(bit));
> +	return !!(regmap_test_bits(exar_gpio->regs, addr, BIT(bit)));
>  }
>  
>  static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
> @@ -119,21 +81,42 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
>  	unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset);
>  	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
>  
> -	exar_update(chip, addr, value, bit);
> +	if (value)
> +		regmap_set_bits(exar_gpio->regs, addr, BIT(bit));
> +	else
> +		regmap_clear_bits(exar_gpio->regs, addr, BIT(bit));
>  }
>  
>  static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
>  				 int value)
>  {
> +	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> +	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
> +	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
> +
>  	exar_set_value(chip, offset, value);
> -	return exar_set_direction(chip, 0, offset);
> +	regmap_clear_bits(exar_gpio->regs, addr, BIT(bit));
> +
> +	return 0;
>  }
>  
>  static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
>  {
> -	return exar_set_direction(chip, 1, offset);
> +	struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
> +	unsigned int addr = exar_offset_to_sel_addr(exar_gpio, offset);
> +	unsigned int bit = exar_offset_to_bit(exar_gpio, offset);
> +
> +	regmap_set_bits(exar_gpio->regs, addr, BIT(bit));
> +
> +	return 0;
>  }
>  
> +static const struct regmap_config exar_regmap_config = {
> +	.name		= "exar-gpio",
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +};
> +
>  static int gpio_exar_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -163,13 +146,17 @@ static int gpio_exar_probe(struct platform_device *pdev)
>  	if (!exar_gpio)
>  		return -ENOMEM;
>  
> -	mutex_init(&exar_gpio->lock);
> +	/*
> +	 * We don't need to check the return values of mmio regmap operations (unless
> +	 * the regmap has a clock attached which is not the case here).
> +	 */
> +	exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
> +	if (IS_ERR(exar_gpio->regs))
> +		return PTR_ERR(exar_gpio->regs);
>  
>  	index = ida_alloc(&ida_index, GFP_KERNEL);
> -	if (index < 0) {
> -		ret = index;
> -		goto err_mutex_destroy;
> -	}
> +	if (index < 0)
> +		return index;
>  
>  	sprintf(exar_gpio->name, "exar_gpio%d", index);
>  	exar_gpio->gpio_chip.label = exar_gpio->name;
> @@ -195,8 +182,6 @@ static int gpio_exar_probe(struct platform_device *pdev)
>  
>  err_destroy:
>  	ida_free(&ida_index, index);
> -err_mutex_destroy:
> -	mutex_destroy(&exar_gpio->lock);
>  	return ret;
>  }
>  
> @@ -205,7 +190,6 @@ static int gpio_exar_remove(struct platform_device *pdev)
>  	struct exar_gpio_chip *exar_gpio = platform_get_drvdata(pdev);
>  
>  	ida_free(&ida_index, exar_gpio->index);
> -	mutex_destroy(&exar_gpio->lock);
>  
>  	return 0;
>  }
> 

Unfortunately, this one still crashes:

...
0000:02:00.0: ttyS2 at MMIO 0x90000000 (irq = 44, base_baud = 7812500) is a XR17V35X
0000:02:00.0: ttyS3 at MMIO 0x90000400 (irq = 44, base_baud = 7812500) is a XR17V35X
BUG: kernel NULL pointer dereference, address: 00000000
#PF: supervisor instruction fetch in kernel mode
#PF: error_code(0xc3300010) - not-present page
*pde = 00000000 
Oops: 0010 [#1] PREEMPT
CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.10.0-rc2+ #440
Hardware name: Intel Corp. QUARK/SIMATIC IOT2000, BIOS V24.02.01 10/30/2018
Workqueue: events deferred_probe_work_func
EIP: 0x0
Code: Unable to access opcode bytes at RIP 0xffffffd6.
EAX: 00000000 EBX: f7c74000 ECX: 00000004 EDX: 00000096
ESI: 00000000 EDI: 00000000 EBP: c1157d88 ESP: c1157d70
DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010286
CR0: 80050033 CR2: ffffffd6 CR3: 03778000 CR4: 00100010
Call Trace:
 regmap_update_bits_base+0x22/0x60
 exar_set_value+0x4e/0x70 [gpio_exar]
 exar_direction_output+0x3a/0x80 [gpio_exar]
 ? exar_set_value+0x70/0x70 [gpio_exar]
 ? exar_set_value+0x70/0x70 [gpio_exar]
 gpiod_direction_output_raw_commit+0x74/0x270
 ? exar_direction_input+0x50/0x50 [gpio_exar]
 ? exar_set_value+0x70/0x70 [gpio_exar]
 gpiod_direction_output+0xf0/0x160
 create_gpio_led+0xea/0x180
 gpio_led_probe+0x22c/0x460
 ? device_pm_check_callbacks+0x4c/0x100
 platform_drv_probe+0x2d/0x80
 really_probe+0xcb/0x330
 driver_probe_device+0x49/0xa0
 __device_attach_driver+0x61/0x80
 ? driver_allows_async_probing+0x60/0x60
 bus_for_each_drv+0x4f/0x90
 __device_attach+0xbb/0x120
 ? driver_allows_async_probing+0x60/0x60
 device_initial_probe+0x12/0x20
 bus_probe_device+0x6f/0x80
 deferred_probe_work_func+0x56/0x80
 process_one_work+0x1ce/0x390
 worker_thread+0x37/0x420
 kthread+0x115/0x130
 ? process_one_work+0x390/0x390
 ? kthread_create_on_node+0x20/0x20
 ret_from_fork+0x19/0x30
Modules linked in: gpio_exar(+) spi_pxa2xx_platform(+) 8250_exar spi_pxa2xx_pci ti_adc108s102 industrialio_triggered_buffer kfifo_buf industrialio at24
CR2: 0000000000000000
---[ end trace 8ea7faca198463b8 ]---
EIP: 0x0
Code: Unable to access opcode bytes at RIP 0xffffffd6.
EAX: 00000000 EBX: f7c74000 ECX: 00000004 EDX: 00000096
ESI: 00000000 EDI: 00000000 EBP: c1157d88 ESP: c1157d70
DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068 EFLAGS: 00010286
CR0: 80050033 CR2: ffffffd6 CR3: 03778000 CR4: 00100010

Jan

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Bartosz Golaszewski]

On Tue, Nov 10, 2020 at 2:23 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
>
> Unfortunately, this one still crashes:
>

Just to confirm: does patch 5/7 alone work?

Bartosz

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Jan Kiszka]

On 10.11.20 14:29, Bartosz Golaszewski wrote:
> On Tue, Nov 10, 2020 at 2:23 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>>
>> Unfortunately, this one still crashes:
>>
> 
> Just to confirm: does patch 5/7 alone work?
> 

Yes, I've bisected.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 2:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

...

>  struct exar_gpio_chip {
>         struct gpio_chip gpio_chip;
> -       struct mutex lock;

> +       struct regmap *regs;

Leaving the same name is a call for potential troubles.

>         int index;
> -       void __iomem *regs;
>         char name[20];
>         unsigned int first_pin;
>  };

...

> +static const struct regmap_config exar_regmap_config = {
> +       .name           = "exar-gpio",
> +       .reg_bits       = 8,
> +       .val_bits       = 8,
> +};

Looking at the crash, are you sure this is a comprehensive description?
Maybe it requires something like stride or so?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Bartosz Golaszewski]

On Tue, Nov 10, 2020 at 3:11 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Nov 10, 2020 at 2:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> ...
>
> >  struct exar_gpio_chip {
> >         struct gpio_chip gpio_chip;
> > -       struct mutex lock;
>
> > +       struct regmap *regs;
>
> Leaving the same name is a call for potential troubles.
>
> >         int index;
> > -       void __iomem *regs;
> >         char name[20];
> >         unsigned int first_pin;
> >  };
>
> ...
>
> > +static const struct regmap_config exar_regmap_config = {
> > +       .name           = "exar-gpio",
> > +       .reg_bits       = 8,
> > +       .val_bits       = 8,
> > +};
>
> Looking at the crash, are you sure this is a comprehensive description?
> Maybe it requires something like stride or so?
>

This is what I'm looking at ATM. Looking at the datasheet[1], there
are no breaks in the registers so the default stride of 1 should be
fine as is the value bits width of 8. I think that I got the address
width wrong though. Should be 16 bits probably.

Jan: could you change reg_bits to 16 and try again?

Bartosz

[1] https://www.maxlinear.com/ds/xr17v352.pdf

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 01:34:05PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We can simplify the code in gpio-exar by using regmap. This allows us to
> drop the mutex (regmap provides its own locking) and we can also reuse
> regmap's bit operations instead of implementing our own update function.

...

> +	/*
> +	 * We don't need to check the return values of mmio regmap operations (unless
> +	 * the regmap has a clock attached which is not the case here).
> +	 */
> +	exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
> +	if (IS_ERR(exar_gpio->regs))
> +		return PTR_ERR(exar_gpio->regs);
>  
>  	index = ida_alloc(&ida_index, GFP_KERNEL);
> -	if (index < 0) {
> -		ret = index;
> -		goto err_mutex_destroy;
> -	}
> +	if (index < 0)
> +		return index;

And below you effectively use p as regmap!
That's what renaming of variable regs -> regmap or map can easily reveal.

	exar_gpio->regs = p;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 04:26:24PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 10, 2020 at 01:34:05PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > 
> > We can simplify the code in gpio-exar by using regmap. This allows us to
> > drop the mutex (regmap provides its own locking) and we can also reuse
> > regmap's bit operations instead of implementing our own update function.
> 
> ...
> 
> > +	/*
> > +	 * We don't need to check the return values of mmio regmap operations (unless
> > +	 * the regmap has a clock attached which is not the case here).
> > +	 */
> > +	exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
> > +	if (IS_ERR(exar_gpio->regs))
> > +		return PTR_ERR(exar_gpio->regs);
> >  
> >  	index = ida_alloc(&ida_index, GFP_KERNEL);
> > -	if (index < 0) {
> > -		ret = index;
> > -		goto err_mutex_destroy;
> > -	}
> > +	if (index < 0)
> > +		return index;
> 
> And below you effectively use p as regmap!
> That's what renaming of variable regs -> regmap or map can easily reveal.
> 
> 	exar_gpio->regs = p;

Jan, if you remove this line, does it help?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Bartosz Golaszewski]

On Tue, Nov 10, 2020 at 3:26 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 10, 2020 at 04:26:24PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 10, 2020 at 01:34:05PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > We can simplify the code in gpio-exar by using regmap. This allows us to
> > > drop the mutex (regmap provides its own locking) and we can also reuse
> > > regmap's bit operations instead of implementing our own update function.
> >
> > ...
> >
> > > +   /*
> > > +    * We don't need to check the return values of mmio regmap operations (unless
> > > +    * the regmap has a clock attached which is not the case here).
> > > +    */
> > > +   exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
> > > +   if (IS_ERR(exar_gpio->regs))
> > > +           return PTR_ERR(exar_gpio->regs);
> > >
> > >     index = ida_alloc(&ida_index, GFP_KERNEL);
> > > -   if (index < 0) {
> > > -           ret = index;
> > > -           goto err_mutex_destroy;
> > > -   }
> > > +   if (index < 0)
> > > +           return index;
> >
> > And below you effectively use p as regmap!
> > That's what renaming of variable regs -> regmap or map can easily reveal.
> >
> >       exar_gpio->regs = p;
>
> Jan, if you remove this line, does it help?
>

Ha! I guess you were right saying that keeping the name is asking for
trouble then. :)

I think that may be it but address width should still be changed to 16.

Bartosz

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Jan Kiszka]

On 10.11.20 15:30, Bartosz Golaszewski wrote:
> On Tue, Nov 10, 2020 at 3:26 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Tue, Nov 10, 2020 at 04:26:24PM +0200, Andy Shevchenko wrote:
>>> On Tue, Nov 10, 2020 at 01:34:05PM +0100, Bartosz Golaszewski wrote:
>>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>>
>>>> We can simplify the code in gpio-exar by using regmap. This allows us to
>>>> drop the mutex (regmap provides its own locking) and we can also reuse
>>>> regmap's bit operations instead of implementing our own update function.
>>>
>>> ...
>>>
>>>> +   /*
>>>> +    * We don't need to check the return values of mmio regmap operations (unless
>>>> +    * the regmap has a clock attached which is not the case here).
>>>> +    */
>>>> +   exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
>>>> +   if (IS_ERR(exar_gpio->regs))
>>>> +           return PTR_ERR(exar_gpio->regs);
>>>>
>>>>     index = ida_alloc(&ida_index, GFP_KERNEL);
>>>> -   if (index < 0) {
>>>> -           ret = index;
>>>> -           goto err_mutex_destroy;
>>>> -   }
>>>> +   if (index < 0)
>>>> +           return index;
>>>
>>> And below you effectively use p as regmap!
>>> That's what renaming of variable regs -> regmap or map can easily reveal.
>>>
>>>       exar_gpio->regs = p;
>>
>> Jan, if you remove this line, does it help?
>>
> 
> Ha! I guess you were right saying that keeping the name is asking for
> trouble then. :)
> 
> I think that may be it but address width should still be changed to 16.
> 

Removing the line that Andy found made things work here. And switching
to 16 for reg_bits didn't make things worse again.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Bartosz Golaszewski]

On Tue, Nov 10, 2020 at 3:50 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
>
> On 10.11.20 15:30, Bartosz Golaszewski wrote:
> > On Tue, Nov 10, 2020 at 3:26 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >>
> >> On Tue, Nov 10, 2020 at 04:26:24PM +0200, Andy Shevchenko wrote:
> >>> On Tue, Nov 10, 2020 at 01:34:05PM +0100, Bartosz Golaszewski wrote:
> >>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>>>
> >>>> We can simplify the code in gpio-exar by using regmap. This allows us to
> >>>> drop the mutex (regmap provides its own locking) and we can also reuse
> >>>> regmap's bit operations instead of implementing our own update function.
> >>>
> >>> ...
> >>>
> >>>> +   /*
> >>>> +    * We don't need to check the return values of mmio regmap operations (unless
> >>>> +    * the regmap has a clock attached which is not the case here).
> >>>> +    */
> >>>> +   exar_gpio->regs = devm_regmap_init_mmio(dev, p, &exar_regmap_config);
> >>>> +   if (IS_ERR(exar_gpio->regs))
> >>>> +           return PTR_ERR(exar_gpio->regs);
> >>>>
> >>>>     index = ida_alloc(&ida_index, GFP_KERNEL);
> >>>> -   if (index < 0) {
> >>>> -           ret = index;
> >>>> -           goto err_mutex_destroy;
> >>>> -   }
> >>>> +   if (index < 0)
> >>>> +           return index;
> >>>
> >>> And below you effectively use p as regmap!
> >>> That's what renaming of variable regs -> regmap or map can easily reveal.
> >>>
> >>>       exar_gpio->regs = p;
> >>
> >> Jan, if you remove this line, does it help?
> >>
> >
> > Ha! I guess you were right saying that keeping the name is asking for
> > trouble then. :)
> >
> > I think that may be it but address width should still be changed to 16.
> >
>
> Removing the line that Andy found made things work here. And switching
> to 16 for reg_bits didn't make things worse again.
>
> Jan

Alright! I'll send a v4 with these things fixed then.

Bartosz

Re: [PATCH v3 6/7] gpio: exar: switch to using regmap

[Andy Shevchenko]

On Tue, Nov 10, 2020 at 03:52:00PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 10, 2020 at 3:50 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > On 10.11.20 15:30, Bartosz Golaszewski wrote:

...

> > Removing the line that Andy found made things work here. And switching
> > to 16 for reg_bits didn't make things worse again.
> >
> > Jan
> 
> Alright! I'll send a v4 with these things fixed then.

Hold on, the registers are 16-bit wide, but their halves are sparsed!
So, I guess 8 and 8 with helpers to get hi and lo parts are essential.


-- 
With Best Regards,
Andy Shevchenko