[PATCH v1 1/2] pinctrl: cherryview: Avoid duplicated I/O

[PATCH v1 1/2] pinctrl: cherryview: Avoid duplicated I/O

[Andy Shevchenko]

In some cases we already read the value from the register followed
by a reading of it again for other purposes, but the both reads
are under the lock and bits we are insterested in are not going
to change (they are not volatile from HW perspective). Hence, no
need to read the same registeer twice.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index f047f7bf4afb..9b9f18f50c1d 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -612,9 +612,14 @@ static void chv_writel(struct intel_pinctrl *pctrl, unsigned int pin, unsigned i
 }
 
 /* When Pad Cfg is locked, driver can only change GPIOTXState or GPIORXState */
+static bool chv_pad_is_locked(u32 ctrl1)
+{
+	return ctrl1 & CHV_PADCTRL1_CFGLOCK;
+}
+
 static bool chv_pad_locked(struct intel_pinctrl *pctrl, unsigned int offset)
 {
-	return chv_readl(pctrl, offset, CHV_PADCTRL1) & CHV_PADCTRL1_CFGLOCK;
+	return chv_pad_is_locked(chv_readl(pctrl, offset, CHV_PADCTRL1));
 }
 
 static void chv_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
@@ -623,13 +628,11 @@ static void chv_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	unsigned long flags;
 	u32 ctrl0, ctrl1;
-	bool locked;
 
 	raw_spin_lock_irqsave(&chv_lock, flags);
 
 	ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
 	ctrl1 = chv_readl(pctrl, offset, CHV_PADCTRL1);
-	locked = chv_pad_locked(pctrl, offset);
 
 	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
@@ -646,7 +649,7 @@ static void chv_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
 
 	seq_printf(s, "0x%08x 0x%08x", ctrl0, ctrl1);
 
-	if (locked)
+	if (chv_pad_is_locked(ctrl1))
 		seq_puts(s, " [LOCKED]");
 }
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 2/2] pinctrl: cherryview: Simplify code with cleanup helpers

[Andy Shevchenko]

Use macros defined in linux/cleanup.h to automate resource lifetime
control in the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 137 +++++++--------------
 1 file changed, 47 insertions(+), 90 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 9b9f18f50c1d..2ffeccc83ccd 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -12,6 +12,7 @@
 
 #include <linux/acpi.h>
 #include <linux/array_size.h>
+#include <linux/cleanup.h>
 #include <linux/dmi.h>
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
@@ -626,15 +627,12 @@ static void chv_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
 			     unsigned int offset)
 {
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	unsigned long flags;
 	u32 ctrl0, ctrl1;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
-
-	ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
-	ctrl1 = chv_readl(pctrl, offset, CHV_PADCTRL1);
-
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
+	scoped_guard(raw_spinlock_irqsave, &chv_lock) {
+		ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
+		ctrl1 = chv_readl(pctrl, offset, CHV_PADCTRL1);
+	}
 
 	if (ctrl0 & CHV_PADCTRL0_GPIOEN) {
 		seq_puts(s, "GPIO ");
@@ -666,17 +664,15 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	struct device *dev = pctrl->dev;
 	const struct intel_pingroup *grp;
-	unsigned long flags;
 	int i;
 
 	grp = &pctrl->soc->groups[group];
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
 	/* Check first that the pad is not locked */
 	for (i = 0; i < grp->grp.npins; i++) {
 		if (chv_pad_locked(pctrl, grp->grp.pins[i])) {
-			raw_spin_unlock_irqrestore(&chv_lock, flags);
 			dev_warn(dev, "unable to set mode for locked pin %u\n", grp->grp.pins[i]);
 			return -EBUSY;
 		}
@@ -716,8 +712,6 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev,
 			invert_oe ? "" : "not ");
 	}
 
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
-
 	return 0;
 }
 
@@ -748,16 +742,14 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 				   unsigned int offset)
 {
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	unsigned long flags;
 	u32 value;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
 	if (chv_pad_locked(pctrl, offset)) {
 		value = chv_readl(pctrl, offset, CHV_PADCTRL0);
 		if (!(value & CHV_PADCTRL0_GPIOEN)) {
 			/* Locked so cannot enable */
-			raw_spin_unlock_irqrestore(&chv_lock, flags);
 			return -EBUSY;
 		}
 	} else {
@@ -792,8 +784,6 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 		chv_writel(pctrl, offset, CHV_PADCTRL0, value);
 	}
 
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
-
 	return 0;
 }
 
@@ -802,14 +792,13 @@ static void chv_gpio_disable_free(struct pinctrl_dev *pctldev,
 				  unsigned int offset)
 {
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	unsigned long flags;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
-	if (!chv_pad_locked(pctrl, offset))
-		chv_gpio_clear_triggering(pctrl, offset);
+	if (chv_pad_locked(pctrl, offset))
+		return;
 
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
+	chv_gpio_clear_triggering(pctrl, offset);
 }
 
 static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
@@ -817,10 +806,9 @@ static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
 				  unsigned int offset, bool input)
 {
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	unsigned long flags;
 	u32 ctrl0;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
 	ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0) & ~CHV_PADCTRL0_GPIOCFG_MASK;
 	if (input)
@@ -829,8 +817,6 @@ static int chv_gpio_set_direction(struct pinctrl_dev *pctldev,
 		ctrl0 |= CHV_PADCTRL0_GPIOCFG_GPO << CHV_PADCTRL0_GPIOCFG_SHIFT;
 	chv_writel(pctrl, offset, CHV_PADCTRL0, ctrl0);
 
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
-
 	return 0;
 }
 
@@ -849,15 +835,14 @@ static int chv_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
 {
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	enum pin_config_param param = pinconf_to_config_param(*config);
-	unsigned long flags;
 	u32 ctrl0, ctrl1;
 	u16 arg = 0;
 	u32 term;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
-	ctrl0 = chv_readl(pctrl, pin, CHV_PADCTRL0);
-	ctrl1 = chv_readl(pctrl, pin, CHV_PADCTRL1);
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
+	scoped_guard(raw_spinlock_irqsave, &chv_lock) {
+		ctrl0 = chv_readl(pctrl, pin, CHV_PADCTRL0);
+		ctrl1 = chv_readl(pctrl, pin, CHV_PADCTRL1);
+	}
 
 	term = (ctrl0 & CHV_PADCTRL0_TERM_MASK) >> CHV_PADCTRL0_TERM_SHIFT;
 
@@ -932,10 +917,10 @@ static int chv_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
 static int chv_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 			       enum pin_config_param param, u32 arg)
 {
-	unsigned long flags;
 	u32 ctrl0, pull;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
+
 	ctrl0 = chv_readl(pctrl, pin, CHV_PADCTRL0);
 
 	switch (param) {
@@ -958,7 +943,6 @@ static int chv_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 			pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
 			break;
 		default:
-			raw_spin_unlock_irqrestore(&chv_lock, flags);
 			return -EINVAL;
 		}
 
@@ -976,7 +960,6 @@ static int chv_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 			pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
 			break;
 		default:
-			raw_spin_unlock_irqrestore(&chv_lock, flags);
 			return -EINVAL;
 		}
 
@@ -984,12 +967,10 @@ static int chv_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 		break;
 
 	default:
-		raw_spin_unlock_irqrestore(&chv_lock, flags);
 		return -EINVAL;
 	}
 
 	chv_writel(pctrl, pin, CHV_PADCTRL0, ctrl0);
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	return 0;
 }
@@ -997,10 +978,10 @@ static int chv_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 static int chv_config_set_oden(struct intel_pinctrl *pctrl, unsigned int pin,
 			       bool enable)
 {
-	unsigned long flags;
 	u32 ctrl1;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
+
 	ctrl1 = chv_readl(pctrl, pin, CHV_PADCTRL1);
 
 	if (enable)
@@ -1009,7 +990,6 @@ static int chv_config_set_oden(struct intel_pinctrl *pctrl, unsigned int pin,
 		ctrl1 &= ~CHV_PADCTRL1_ODEN;
 
 	chv_writel(pctrl, pin, CHV_PADCTRL1, ctrl1);
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
 	return 0;
 }
@@ -1119,28 +1099,26 @@ static struct pinctrl_desc chv_pinctrl_desc = {
 static int chv_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
-	unsigned long flags;
 	u32 ctrl0, cfg;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
-	ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
+	scoped_guard(raw_spinlock_irqsave, &chv_lock)
+		ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
 
 	cfg = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
 	cfg >>= CHV_PADCTRL0_GPIOCFG_SHIFT;
 
 	if (cfg == CHV_PADCTRL0_GPIOCFG_GPO)
 		return !!(ctrl0 & CHV_PADCTRL0_GPIOTXSTATE);
+
 	return !!(ctrl0 & CHV_PADCTRL0_GPIORXSTATE);
 }
 
 static void chv_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
-	unsigned long flags;
 	u32 ctrl0;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
 	ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
 
@@ -1150,19 +1128,15 @@ static void chv_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 		ctrl0 &= ~CHV_PADCTRL0_GPIOTXSTATE;
 
 	chv_writel(pctrl, offset, CHV_PADCTRL0, ctrl0);
-
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
 }
 
 static int chv_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
 	u32 ctrl0, direction;
-	unsigned long flags;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
-	ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
+	scoped_guard(raw_spinlock_irqsave, &chv_lock)
+		ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
 
 	direction = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
 	direction >>= CHV_PADCTRL0_GPIOCFG_SHIFT;
@@ -1203,23 +1177,20 @@ static void chv_gpio_irq_ack(struct irq_data *d)
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 	u32 intr_line;
 
-	raw_spin_lock(&chv_lock);
+	guard(raw_spinlock)(&chv_lock);
 
 	intr_line = chv_readl(pctrl, hwirq, CHV_PADCTRL0);
 	intr_line &= CHV_PADCTRL0_INTSEL_MASK;
 	intr_line >>= CHV_PADCTRL0_INTSEL_SHIFT;
 	chv_pctrl_writel(pctrl, CHV_INTSTAT, BIT(intr_line));
-
-	raw_spin_unlock(&chv_lock);
 }
 
 static void chv_gpio_irq_mask_unmask(struct gpio_chip *gc, irq_hw_number_t hwirq, bool mask)
 {
 	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
 	u32 value, intr_line;
-	unsigned long flags;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
 	intr_line = chv_readl(pctrl, hwirq, CHV_PADCTRL0);
 	intr_line &= CHV_PADCTRL0_INTSEL_MASK;
@@ -1231,8 +1202,6 @@ static void chv_gpio_irq_mask_unmask(struct gpio_chip *gc, irq_hw_number_t hwirq
 	else
 		value |= BIT(intr_line);
 	chv_pctrl_writel(pctrl, CHV_INTMASK, value);
-
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
 }
 
 static void chv_gpio_irq_mask(struct irq_data *d)
@@ -1257,7 +1226,15 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 {
 	/*
 	 * Check if the interrupt has been requested with 0 as triggering
-	 * type. In that case it is assumed that the current values
+	 * type. If not, bail out, ...
+	 */
+	if (irqd_get_trigger_type(d) != IRQ_TYPE_NONE) {
+		chv_gpio_irq_unmask(d);
+		return 0;
+	}
+
+	/*
+	 * ...otherwise it is assumed that the current values
 	 * programmed to the hardware are used (e.g BIOS configured
 	 * defaults).
 	 *
@@ -1265,17 +1242,15 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 	 * read back the values from hardware now, set correct flow handler
 	 * and update mappings before the interrupt is being used.
 	 */
-	if (irqd_get_trigger_type(d) == IRQ_TYPE_NONE) {
+	scoped_guard(raw_spinlock_irqsave, &chv_lock) {
 		struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 		struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
 		struct device *dev = pctrl->dev;
 		struct intel_community_context *cctx = &pctrl->context.communities[0];
 		irq_hw_number_t hwirq = irqd_to_hwirq(d);
 		irq_flow_handler_t handler;
-		unsigned long flags;
 		u32 intsel, value;
 
-		raw_spin_lock_irqsave(&chv_lock, flags);
 		intsel = chv_readl(pctrl, hwirq, CHV_PADCTRL0);
 		intsel &= CHV_PADCTRL0_INTSEL_MASK;
 		intsel >>= CHV_PADCTRL0_INTSEL_SHIFT;
@@ -1292,7 +1267,6 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 				intsel, hwirq);
 			cctx->intr_lines[intsel] = hwirq;
 		}
-		raw_spin_unlock_irqrestore(&chv_lock, flags);
 	}
 
 	chv_gpio_irq_unmask(d);
@@ -1357,17 +1331,14 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
-	unsigned long flags;
 	u32 value;
 	int ret;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
 	ret = chv_gpio_set_intr_line(pctrl, hwirq);
-	if (ret) {
-		raw_spin_unlock_irqrestore(&chv_lock, flags);
+	if (ret)
 		return ret;
-	}
 
 	/*
 	 * Pins which can be used as shared interrupt are configured in
@@ -1408,8 +1379,6 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
 	else if (type & IRQ_TYPE_LEVEL_MASK)
 		irq_set_handler_locked(d, handle_level_irq);
 
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
-
 	return 0;
 }
 
@@ -1433,14 +1402,12 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
 	struct intel_community_context *cctx = &pctrl->context.communities[0];
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	unsigned long pending;
-	unsigned long flags;
 	u32 intr_line;
 
 	chained_irq_enter(chip, desc);
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
-	pending = chv_pctrl_readl(pctrl, CHV_INTSTAT);
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
+	scoped_guard(raw_spinlock_irqsave, &chv_lock)
+		pending = chv_pctrl_readl(pctrl, CHV_INTSTAT);
 
 	for_each_set_bit(intr_line, &pending, community->nirqs) {
 		unsigned int offset;
@@ -1629,21 +1596,17 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function,
 	void *handler_context, void *region_context)
 {
 	struct intel_pinctrl *pctrl = region_context;
-	unsigned long flags;
-	acpi_status ret = AE_OK;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
 	if (function == ACPI_WRITE)
 		chv_pctrl_writel(pctrl, address, *value);
 	else if (function == ACPI_READ)
 		*value = chv_pctrl_readl(pctrl, address);
 	else
-		ret = AE_BAD_PARAMETER;
+		return AE_BAD_PARAMETER;
 
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
-
-	return ret;
+	return AE_OK;
 }
 
 static int chv_pinctrl_probe(struct platform_device *pdev)
@@ -1751,10 +1714,9 @@ static int chv_pinctrl_suspend_noirq(struct device *dev)
 {
 	struct intel_pinctrl *pctrl = dev_get_drvdata(dev);
 	struct intel_community_context *cctx = &pctrl->context.communities[0];
-	unsigned long flags;
 	int i;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
 	cctx->saved_intmask = chv_pctrl_readl(pctrl, CHV_INTMASK);
 
@@ -1772,8 +1734,6 @@ static int chv_pinctrl_suspend_noirq(struct device *dev)
 		ctx->padctrl1 = chv_readl(pctrl, desc->number, CHV_PADCTRL1);
 	}
 
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
-
 	return 0;
 }
 
@@ -1781,10 +1741,9 @@ static int chv_pinctrl_resume_noirq(struct device *dev)
 {
 	struct intel_pinctrl *pctrl = dev_get_drvdata(dev);
 	struct intel_community_context *cctx = &pctrl->context.communities[0];
-	unsigned long flags;
 	int i;
 
-	raw_spin_lock_irqsave(&chv_lock, flags);
+	guard(raw_spinlock_irqsave)(&chv_lock);
 
 	/*
 	 * Mask all interrupts before restoring per-pin configuration
@@ -1826,8 +1785,6 @@ static int chv_pinctrl_resume_noirq(struct device *dev)
 	chv_pctrl_writel(pctrl, CHV_INTSTAT, 0xffff);
 	chv_pctrl_writel(pctrl, CHV_INTMASK, cctx->saved_intmask);
 
-	raw_spin_unlock_irqrestore(&chv_lock, flags);
-
 	return 0;
 }
 
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 2/2] pinctrl: cherryview: Simplify code with cleanup helpers

[Mika Westerberg]

On Fri, Oct 06, 2023 at 12:40:33PM +0300, Andy Shevchenko wrote:
> @@ -1119,28 +1099,26 @@ static struct pinctrl_desc chv_pinctrl_desc = {
>  static int chv_gpio_get(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
> -	unsigned long flags;
>  	u32 ctrl0, cfg;
>  
> -	raw_spin_lock_irqsave(&chv_lock, flags);
> -	ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
> -	raw_spin_unlock_irqrestore(&chv_lock, flags);
> +	scoped_guard(raw_spinlock_irqsave, &chv_lock)
> +		ctrl0 = chv_readl(pctrl, offset, CHV_PADCTRL0);
>  
>  	cfg = ctrl0 & CHV_PADCTRL0_GPIOCFG_MASK;
>  	cfg >>= CHV_PADCTRL0_GPIOCFG_SHIFT;
>  
>  	if (cfg == CHV_PADCTRL0_GPIOCFG_GPO)
>  		return !!(ctrl0 & CHV_PADCTRL0_GPIOTXSTATE);
> +

Unrelated whitespace change.

Otherwise looks good,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 2/2] pinctrl: cherryview: Simplify code with cleanup helpers

[Andy Shevchenko]

On Fri, Oct 06, 2023 at 01:26:41PM +0300, Mika Westerberg wrote:
> On Fri, Oct 06, 2023 at 12:40:33PM +0300, Andy Shevchenko wrote:

...

> >  	if (cfg == CHV_PADCTRL0_GPIOCFG_GPO)
> >  		return !!(ctrl0 & CHV_PADCTRL0_GPIOTXSTATE);
> > +
> 
> Unrelated whitespace change.

Will drop it.

> Otherwise looks good,
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] pinctrl: cherryview: Simplify code with cleanup helpers

[Andy Shevchenko]

On Fri, Oct 06, 2023 at 01:26:41PM +0300, Mika Westerberg wrote:
> On Fri, Oct 06, 2023 at 12:40:33PM +0300, Andy Shevchenko wrote:

> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] pinctrl: cherryview: Avoid duplicated I/O

[Mika Westerberg]

On Fri, Oct 06, 2023 at 12:40:32PM +0300, Andy Shevchenko wrote:
> In some cases we already read the value from the register followed
> by a reading of it again for other purposes, but the both reads
> are under the lock and bits we are insterested in are not going
> to change (they are not volatile from HW perspective). Hence, no
> need to read the same registeer twice.

Typo "registeer" -> "register"

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 1/2] pinctrl: cherryview: Avoid duplicated I/O

[Andy Shevchenko]

On Fri, Oct 06, 2023 at 01:24:12PM +0300, Mika Westerberg wrote:
> On Fri, Oct 06, 2023 at 12:40:32PM +0300, Andy Shevchenko wrote:
> > In some cases we already read the value from the register followed
> > by a reading of it again for other purposes, but the both reads
> > are under the lock and bits we are insterested in are not going
> > to change (they are not volatile from HW perspective). Hence, no
> > need to read the same registeer twice.
> 
> Typo "registeer" -> "register"

Will fix when applying.

...

> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] pinctrl: cherryview: Avoid duplicated I/O

[Andy Shevchenko]

On Fri, Oct 06, 2023 at 01:24:12PM +0300, Mika Westerberg wrote:
> On Fri, Oct 06, 2023 at 12:40:32PM +0300, Andy Shevchenko wrote:

...

> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko