This patch includes the following changes:
- Using irqsave/irqrestore spinlock variants to avoid any potential
deadlock in case of GPIO as IRQ
- Introducing a new device tree option 'pin-cfg' which can be used to setup
mode of one or several GPIO pins. It can be used to choose pin's
function, filtes, polarity and direction.
- Extending PIN_SEL_MASK to cover for otx2 platform
- In case of level irq, the current handle is not unmasking the GPIO
interrupt, so using handle_level_irq, which will allow next interrupt
to be handled.
All the changes have been internally verified on Marvell otx2 platforms.
Piyush Malgujar (5):
gpio: thunderx: avoid potential deadlock
dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option
gpio: thunderx: Configure GPIO pins at probe
gpio: thunderx: extend PIN_SEL_MASK to cover otx2 platform
gpio: thunderx: change handler for level interrupt
.../bindings/gpio/gpio-thunderx.txt | 4 ++
drivers/gpio/gpio-thunderx.c | 53 +++++++++++++++----
2 files changed, 47 insertions(+), 10 deletions(-)
--
2.17.1
Using irqsave/irqrestore locking variants to avoid any deadlock.
Signed-off-by: Piyush Malgujar <[email protected]>
---
drivers/gpio/gpio-thunderx.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index 9f66deab46eaa99d05413a996b585284c433574d..bb2b40e4033b00134af35592b6b7c7f83cf6c737 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -104,16 +104,17 @@ static int thunderx_gpio_request(struct gpio_chip *chip, unsigned int line)
static int thunderx_gpio_dir_in(struct gpio_chip *chip, unsigned int line)
{
struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
+ unsigned long flags;
if (!thunderx_gpio_is_gpio(txgpio, line))
return -EIO;
- raw_spin_lock(&txgpio->lock);
+ raw_spin_lock_irqsave(&txgpio->lock, flags);
clear_bit(line, txgpio->invert_mask);
clear_bit(line, txgpio->od_mask);
writeq(txgpio->line_entries[line].fil_bits,
txgpio->register_base + bit_cfg_reg(line));
- raw_spin_unlock(&txgpio->lock);
+ raw_spin_unlock_irqrestore(&txgpio->lock, flags);
return 0;
}
@@ -135,11 +136,12 @@ static int thunderx_gpio_dir_out(struct gpio_chip *chip, unsigned int line,
{
struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
u64 bit_cfg = txgpio->line_entries[line].fil_bits | GPIO_BIT_CFG_TX_OE;
+ unsigned long flags;
if (!thunderx_gpio_is_gpio(txgpio, line))
return -EIO;
- raw_spin_lock(&txgpio->lock);
+ raw_spin_lock_irqsave(&txgpio->lock, flags);
thunderx_gpio_set(chip, line, value);
@@ -151,7 +153,7 @@ static int thunderx_gpio_dir_out(struct gpio_chip *chip, unsigned int line,
writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(line));
- raw_spin_unlock(&txgpio->lock);
+ raw_spin_unlock_irqrestore(&txgpio->lock, flags);
return 0;
}
@@ -188,11 +190,12 @@ static int thunderx_gpio_set_config(struct gpio_chip *chip,
int ret = -ENOTSUPP;
struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
void __iomem *reg = txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET;
+ unsigned long flags;
if (!thunderx_gpio_is_gpio(txgpio, line))
return -EIO;
- raw_spin_lock(&txgpio->lock);
+ raw_spin_lock_irqsave(&txgpio->lock, flags);
orig_invert = test_bit(line, txgpio->invert_mask);
new_invert = orig_invert;
orig_od = test_bit(line, txgpio->od_mask);
@@ -243,7 +246,7 @@ static int thunderx_gpio_set_config(struct gpio_chip *chip,
default:
break;
}
- raw_spin_unlock(&txgpio->lock);
+ raw_spin_unlock_irqrestore(&txgpio->lock, flags);
/*
* If currently output and OPEN_DRAIN changed, install the new
@@ -329,6 +332,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d,
struct thunderx_gpio *txgpio = gpiochip_get_data(gc);
struct thunderx_line *txline =
&txgpio->line_entries[irqd_to_hwirq(d)];
+ unsigned long flags;
u64 bit_cfg;
irqd_set_trigger_type(d, flow_type);
@@ -342,7 +346,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d,
irq_set_handler_locked(d, handle_fasteoi_mask_irq);
}
- raw_spin_lock(&txgpio->lock);
+ raw_spin_lock_irqsave(&txgpio->lock, flags);
if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW)) {
bit_cfg |= GPIO_BIT_CFG_PIN_XOR;
set_bit(txline->line, txgpio->invert_mask);
@@ -351,7 +355,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d,
}
clear_bit(txline->line, txgpio->od_mask);
writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(txline->line));
- raw_spin_unlock(&txgpio->lock);
+ raw_spin_unlock_irqrestore(&txgpio->lock, flags);
return IRQ_SET_MASK_OK;
}
--
2.17.1
Add support for pin-cfg to configure GPIO Pins
Signed-off-by: Piyush Malgujar <[email protected]>
---
Documentation/devicetree/bindings/gpio/gpio-thunderx.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt b/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
index 3f883ae29d116887e702ead20b26a25f9d2349d5..05f0be98afdcae941ff8a24c3fdabd8af83ccb87 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-thunderx.txt
@@ -14,6 +14,9 @@ Optional Properties:
"interrupt-controller" is present.
- First cell is the GPIO pin number relative to the controller.
- Second cell is triggering flags as defined in interrupts.txt.
+- pin-cfg: Configuration of pin's function, filters, XOR and output mode.
+ - First cell is the GPIO pin number
+ - Second cell is a value written to GPIO_BIT_CFG register at driver probe.
Example:
@@ -24,4 +27,5 @@ gpio_6_0: gpio@6,0 {
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
+ pin-cfg = <57 0x2300000>, <58 0x2500000>;
};
--
2.17.1
The current level interrupt handler is masking the GPIO interrupt
and not unmasking it, to resolve that, handle_level_irq is used.
Signed-off-by: Witold Sadowski <[email protected]>
Signed-off-by: Piyush Malgujar <[email protected]>
---
drivers/gpio/gpio-thunderx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index 87ab1ad7e652347a67b7747ea497b944498a8b41..b1063e53ceb8edf26ca1a6ecab8035aad62128a1 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -343,7 +343,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d,
irq_set_handler_locked(d, handle_fasteoi_ack_irq);
bit_cfg |= GPIO_BIT_CFG_INT_TYPE;
} else {
- irq_set_handler_locked(d, handle_fasteoi_mask_irq);
+ irq_set_handler_locked(d, handle_level_irq);
}
raw_spin_lock_irqsave(&txgpio->lock, flags);
--
2.17.1
Add support to configure GPIO pins using DTS 'pin-cfg'
Signed-off-by: Piyush Malgujar <[email protected]>
---
drivers/gpio/gpio-thunderx.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index bb2b40e4033b00134af35592b6b7c7f83cf6c737..451c412512450fea717937376002d2ba35d1c508 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -426,6 +426,32 @@ static void *thunderx_gpio_populate_parent_alloc_info(struct gpio_chip *chip,
return info;
}
+static void thunderx_gpio_pinsel(struct device *dev,
+ struct thunderx_gpio *txgpio)
+{
+ struct device_node *node;
+ const __be32 *pinsel;
+ int npins, rlen, i;
+ u32 pin, sel;
+
+ node = dev_of_node(dev);
+ if (!node)
+ return;
+
+ pinsel = of_get_property(node, "pin-cfg", &rlen);
+ if (!pinsel || rlen % 2)
+ return;
+
+ npins = rlen / sizeof(__be32) / 2;
+
+ for (i = 0; i < npins; i++) {
+ pin = of_read_number(pinsel++, 1);
+ sel = of_read_number(pinsel++, 1);
+ dev_dbg(dev, "Set GPIO pin %d CFG register to %x\n", pin, sel);
+ writeq(sel, txgpio->register_base + bit_cfg_reg(pin));
+ }
+}
+
static int thunderx_gpio_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
@@ -548,6 +574,9 @@ static int thunderx_gpio_probe(struct pci_dev *pdev,
if (err)
goto out;
+ /* Configure default functions of GPIO pins */
+ thunderx_gpio_pinsel(dev, txgpio);
+
/* Push on irq_data and the domain for each line. */
for (i = 0; i < ngpio; i++) {
struct irq_fwspec fwspec;
--
2.17.1
Extending the PIN_SEL_MASK to support otx2 platform which was
earlier RAZ.
Signed-off-by: Piyush Malgujar <[email protected]>
---
drivers/gpio/gpio-thunderx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index 451c412512450fea717937376002d2ba35d1c508..87ab1ad7e652347a67b7747ea497b944498a8b41 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -32,7 +32,7 @@
#define GPIO_BIT_CFG_FIL_CNT_SHIFT 4
#define GPIO_BIT_CFG_FIL_SEL_SHIFT 8
#define GPIO_BIT_CFG_TX_OD BIT(12)
-#define GPIO_BIT_CFG_PIN_SEL_MASK GENMASK(25, 16)
+#define GPIO_BIT_CFG_PIN_SEL_MASK GENMASK(26, 16)
#define GPIO_INTR 0x800
#define GPIO_INTR_INTR BIT(0)
#define GPIO_INTR_INTR_W1S BIT(1)
--
2.17.1
Hi Piyush,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on linusw-gpio/for-next linux/master v5.18-rc4 next-20220427]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Piyush-Malgujar/gpio-thunderx-Marvell-GPIO-changes/20220427-225001
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 46cf2c613f4b10eb12f749207b0fd2c1bfae3088
config: alpha-randconfig-r005-20220427 (https://download.01.org/0day-ci/archive/20220428/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/31a85ad65112e3ed61aa418772670eb96a881a4f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Piyush-Malgujar/gpio-thunderx-Marvell-GPIO-changes/20220427-225001
git checkout 31a85ad65112e3ed61aa418772670eb96a881a4f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/gpio/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/gpio/gpio-thunderx.c: In function 'thunderx_gpio_pinsel':
>> drivers/gpio/gpio-thunderx.c:448:23: error: implicit declaration of function 'of_read_number' [-Werror=implicit-function-declaration]
448 | pin = of_read_number(pinsel++, 1);
| ^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/of_read_number +448 drivers/gpio/gpio-thunderx.c
428
429 static void thunderx_gpio_pinsel(struct device *dev,
430 struct thunderx_gpio *txgpio)
431 {
432 struct device_node *node;
433 const __be32 *pinsel;
434 int npins, rlen, i;
435 u32 pin, sel;
436
437 node = dev_of_node(dev);
438 if (!node)
439 return;
440
441 pinsel = of_get_property(node, "pin-cfg", &rlen);
442 if (!pinsel || rlen % 2)
443 return;
444
445 npins = rlen / sizeof(__be32) / 2;
446
447 for (i = 0; i < npins; i++) {
> 448 pin = of_read_number(pinsel++, 1);
449 sel = of_read_number(pinsel++, 1);
450 dev_dbg(dev, "Set GPIO pin %d CFG register to %x\n", pin, sel);
451 writeq(sel, txgpio->register_base + bit_cfg_reg(pin));
452 }
453 }
454
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Wed, Apr 27, 2022 at 4:47 PM Piyush Malgujar <[email protected]> wrote:
> The current level interrupt handler is masking the GPIO interrupt
> and not unmasking it, to resolve that, handle_level_irq is used.
>
> Signed-off-by: Witold Sadowski <[email protected]>
> Signed-off-by: Piyush Malgujar <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, Apr 27, 2022 at 4:46 PM Piyush Malgujar <[email protected]> wrote:
>
> Using irqsave/irqrestore locking variants to avoid any deadlock.
>
I see you'll be resending this anyway so would you mind providing an
example of a deadlock that is possible with no-irqsave variants?
Thanks.
Bart
> Signed-off-by: Piyush Malgujar <[email protected]>
> ---
On Wed, Apr 27, 2022 at 4:47 PM Piyush Malgujar <[email protected]> wrote:
> Add support for pin-cfg to configure GPIO Pins
>
> Signed-off-by: Piyush Malgujar <[email protected]>
> ---
> Documentation/devicetree/bindings/gpio/gpio-thunderx.txt | 4 ++++
Would be nice to rewrite this binding in YAML
> - First cell is the GPIO pin number relative to the controller.
> - Second cell is triggering flags as defined in interrupts.txt.
> +- pin-cfg: Configuration of pin's function, filters, XOR and output mode.
> + - First cell is the GPIO pin number
> + - Second cell is a value written to GPIO_BIT_CFG register at driver probe.
Just poking magic hex values into some random register as
part of a binding is not a good idea.
This looks like trying to reinvent the pin config subsystem.
GPIO is using the standard pin configurations in the second cell of
the handle, use them in this driver as well and add new ones if we
need.
You find the existing flags here:
include/dt-bindings/gpio/gpio.h
If you need something more sophisticated than a simple flag, I think
you need to implement proper pin config.
Yours,
Linus Walleij
On Mon, May 02, 2022 at 01:18:49PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 27, 2022 at 4:46 PM Piyush Malgujar <[email protected]> wrote:
> >
> > Using irqsave/irqrestore locking variants to avoid any deadlock.
> >
>
> I see you'll be resending this anyway so would you mind providing an
> example of a deadlock that is possible with no-irqsave variants?
> Thanks.
>
> Bart
>
Hi Bartosz,
Thanks for the review.
Please find below the issue scenario:
In the case when HARDIRQ-safe -> HARDIRQ-unsafe lock order is detected
and interrupt occurs, deadlock could occur.
========================================================
WARNING: possible irq lock inversion dependency detected
5.18.0-rc6 #4 Not tainted
--------------------------------------------------------
swapper/3/0 just changed the state of lock:
ffff000110904cd8 (lock_class){-...}-{2:2}, at: handle_fasteoi_ack_irq+0x2c/0x1b0
but this lock took another, HARDIRQ-unsafe lock in the past:
(&txgpio->lock){+.+.}-{2:2}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&txgpio->lock);
local_irq_disable();
lock(lock_class);
lock(&txgpio->lock);
<Interrupt>
lock(lock_class);
*** DEADLOCK ***
==========================================================
Thanks,
Piyush
> > Signed-off-by: Piyush Malgujar <[email protected]>
> > ---
On Wed, May 25, 2022 at 3:17 PM Piyush Malgujar <[email protected]> wrote:
>
> On Mon, May 02, 2022 at 01:18:49PM +0200, Bartosz Golaszewski wrote:
> > On Wed, Apr 27, 2022 at 4:46 PM Piyush Malgujar <[email protected]> wrote:
> > >
> > > Using irqsave/irqrestore locking variants to avoid any deadlock.
> > >
> >
> > I see you'll be resending this anyway so would you mind providing an
> > example of a deadlock that is possible with no-irqsave variants?
> > Thanks.
> >
> > Bart
> >
> Hi Bartosz,
>
> Thanks for the review.
>
> Please find below the issue scenario:
> In the case when HARDIRQ-safe -> HARDIRQ-unsafe lock order is detected
> and interrupt occurs, deadlock could occur.
>
> ========================================================
> WARNING: possible irq lock inversion dependency detected
> 5.18.0-rc6 #4 Not tainted
> --------------------------------------------------------
> swapper/3/0 just changed the state of lock:
> ffff000110904cd8 (lock_class){-...}-{2:2}, at: handle_fasteoi_ack_irq+0x2c/0x1b0
> but this lock took another, HARDIRQ-unsafe lock in the past:
> (&txgpio->lock){+.+.}-{2:2}
>
>
> and interrupts could create inverse lock ordering between them.
>
>
> other info that might help us debug this:
> Possible interrupt unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&txgpio->lock);
> local_irq_disable();
> lock(lock_class);
> lock(&txgpio->lock);
> <Interrupt>
> lock(lock_class);
>
> *** DEADLOCK ***
>
> ==========================================================
>
> Thanks,
> Piyush
> > > Signed-off-by: Piyush Malgujar <[email protected]>
> > > ---
Thanks. What I meant exactly was: resend it with that info in the
commit message.
Bart
Hi Linus,
Thanks for reviewing.
On Mon, May 02, 2022 at 12:15:34AM +0200, Linus Walleij wrote:
> On Wed, Apr 27, 2022 at 4:47 PM Piyush Malgujar <[email protected]> wrote:
>
> > Add support for pin-cfg to configure GPIO Pins
> >
> > Signed-off-by: Piyush Malgujar <[email protected]>
> > ---
> > Documentation/devicetree/bindings/gpio/gpio-thunderx.txt | 4 ++++
>
> Would be nice to rewrite this binding in YAML
>
Sure, will take care in V2.
> > - First cell is the GPIO pin number relative to the controller.
> > - Second cell is triggering flags as defined in interrupts.txt.
> > +- pin-cfg: Configuration of pin's function, filters, XOR and output mode.
> > + - First cell is the GPIO pin number
> > + - Second cell is a value written to GPIO_BIT_CFG register at driver probe.
>
> Just poking magic hex values into some random register as
> part of a binding is not a good idea.
>
> This looks like trying to reinvent the pin config subsystem.
>
> GPIO is using the standard pin configurations in the second cell of
> the handle, use them in this driver as well and add new ones if we
> need.
>
> You find the existing flags here:
> include/dt-bindings/gpio/gpio.h
>
> If you need something more sophisticated than a simple flag, I think
> you need to implement proper pin config.
>
> Yours,
> Linus Walleij
The purpose of this pin-cfg entry is different than the standard GPIO pin config usage.
It is to write a value to GPIO_BIT_CFG register which is used to configure fields like
pin function, selecting which signal is reported to GPIO output or which signal GPIO
input need to connect, filters, XOR and output mode.
We will define new entry specific to thunderx GPIO usage, instead of pin-cfg.
Thanks,
Piyush
On Fri, Jun 3, 2022 at 11:06 AM Piyush Malgujar <[email protected]> wrote:
> The purpose of this pin-cfg entry is different than the standard GPIO pin config usage.
> It is to write a value to GPIO_BIT_CFG register which is used to configure fields like
> pin function, selecting which signal is reported to GPIO output or which signal GPIO
> input need to connect, filters, XOR and output mode.
Then implement pin control for this driver instead of inventing a custom hack?
https://docs.kernel.org/driver-api/pin-control.html
Several drivers implement pin control with a GPIO front-end, for example:
drivers/gpio/gpio-pl061.c is used as a front end with
drivers/pinctrl/pinctrl-single.c
There are also composite drivers in drivers/pinctrl that implement both
pincontrol (incl muxing) and GPIO, such as drivers/pinctrl/pinctrl-sx150x.c
Yours,
Linus Walleij
On Fri, Jun 03, 2022 at 12:35:57PM +0200, Linus Walleij wrote:
> On Fri, Jun 3, 2022 at 11:06 AM Piyush Malgujar <[email protected]> wrote:
>
> > The purpose of this pin-cfg entry is different than the standard GPIO pin config usage.
> > It is to write a value to GPIO_BIT_CFG register which is used to configure fields like
> > pin function, selecting which signal is reported to GPIO output or which signal GPIO
> > input need to connect, filters, XOR and output mode.
>
> Then implement pin control for this driver instead of inventing a custom hack?
> https://docs.kernel.org/driver-api/pin-control.html
>
> Several drivers implement pin control with a GPIO front-end, for example:
> drivers/gpio/gpio-pl061.c is used as a front end with
> drivers/pinctrl/pinctrl-single.c
>
> There are also composite drivers in drivers/pinctrl that implement both
> pincontrol (incl muxing) and GPIO, such as drivers/pinctrl/pinctrl-sx150x.c
>
> Yours,
> Linus Walleij
Hi Linus,
Thanks for the reply.
But as in this case, we expect a 32 bit reg value via DTS for this driver
only from user with internal understanding of marvell soc and this reg bit
value can have many different combinations as the register fields can vary
for different marvell SoCs.
This patch just reads the reg value from DTS and writes it to the register.
Thanks,
Piyush
On Mon, Jun 13, 2022 at 10:04 AM Piyush Malgujar <[email protected]> wrote:
> Thanks for the reply.
> But as in this case, we expect a 32 bit reg value via DTS for this driver
> only from user with internal understanding of marvell soc and this reg bit
> value can have many different combinations as the register fields can vary
> for different marvell SoCs.
> This patch just reads the reg value from DTS and writes it to the register.
I understand that this is convenient but it does not use the right kernel
abstractions and it does not use device tree bindings the right way
either.
Rewrite the patches using definitions and fine control and move away
from magic numbers to be poked into registers.
Yours,
Linus Walleij
On 26/06/2022 00:59, Linus Walleij wrote:
> On Mon, Jun 13, 2022 at 10:04 AM Piyush Malgujar <[email protected]> wrote:
>
>> Thanks for the reply.
>> But as in this case, we expect a 32 bit reg value via DTS for this driver
>> only from user with internal understanding of marvell soc and this reg bit
>> value can have many different combinations as the register fields can vary
>> for different marvell SoCs.
>> This patch just reads the reg value from DTS and writes it to the register.
>
> I understand that this is convenient but it does not use the right kernel
> abstractions and it does not use device tree bindings the right way
> either.
>
> Rewrite the patches using definitions and fine control and move away
> from magic numbers to be poked into registers.
+1
Let's don't repeat the same pattern Samsung pinctrl has.
Best regards,
Krzysztof