Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
On Wed, Aug 15, 2012 at 12:08 PM, Opensource [Anthony Olech] wrote: > [Me] >> > + if (offset > 1) >> > + return -EINVAL; >> So there are two GPIO pins, 0 and 1? That seems odd, but OK. > > That is a feature of the hardware. I believe that calling them "0" and > "1" is the correct thing to do. Correct me if they should have been > called "1" and "2", or something else. It's correct, what I thought was odd was the fact that there were only two GPIO pins on this device. But some have only one even, just wanted to verify... > HANDLING NIBBLES > > > The handling of nibbles within a byte follows the rule that constants > for the nibble NOT being operated on have those bits set to zero, > and thus only bits being operated on may be non-zero. Thus to set, > for example, the value 0xB into the MSH the operation is: > byte &= ~0xF0 > byte |= 0xB0 > it being obvious that it is the upper nibble being operated on. > It seems that you are following a different rule for handling nibbles, > and I can't find any standard for doing so in the kernel, so could > you send me your reference documents? In this case as stated elsewhere, I'm happy that you do things this way, if you #define the magic values you're using in your bytes and nibbles, because else it's just hard to read. #define FOO_MASK 0xF0 #define BAR_FEATURE 0xB0 byte &= ~FOO_MASK; byte |= BAR_FEATURE; It's more readble. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [NEW DRIVER V2 5/7] DA9058 GPIO driver
> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: 13 August 2012 14:10
> To: Opensource [Anthony Olech]
> Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; David Dajun Chen; Samuel
> Ortiz; Lee Jones
> Subject: Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
> Hi Anthony, sorry for delayed reply...
> On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech
> wrote:
> > This is the GPIO component driver of the Dialog DA9058 PMIC.
> > This driver is just one component of the whole DA9058 PMIC driver.
> > It depends on the core DA9058 MFD driver.
> OK
> > +config GPIO_DA9058
> > + tristate "Dialog DA9058 GPIO"
> > + depends on MFD_DA9058
> select IRQ_DOMAIN, you're going to want to use it...
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index
> > 0f55662..209224a 100644
> (...)
> > +#include
> > +#include
> Really?
> > +#include
> Really?
> > +#include
> > +#include
> > +#include
> Really?
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> If you're using regmap you better select it in Kconfig too, but it appears you
> don't. You should be using regmap in the main MFD driver in this case (I
> haven't looked at it though.)
> This header set just looks like it was copied from some other file and never
> really proofread, so please go over it in detail.
for some reason against 2.6.2x some of those were required, I just totally
forgot to prune out the unwanted ones when rebasing forwards to 3.5.
Good that you spotted it! Sorry I will try to prune the includes in the future.
> > +#include #include
> > + #include
> > +#include #include
> > +#include
> Samuel will have to comment on this organization of headers, it seems a little
> much. DO you really need all of them?
One of them should have been stripped out by my submit script, but as for the
others you must bear in mind that the DA9058 PMIC is a multifunction device,
and thus some header files are common and some are specific to various
component drivers. The very reason that you picked up on non-relevant include
files surely has implications on the structure of the header files for the
DA9058,
in particular struct's and define's that only apply to one component driver
should
be in separate header files.
> > +struct da9058_gpio {
> > + struct da9058 *da9058;
> > + struct platform_device *pdev;
> > + struct gpio_chip gp;
> > + struct mutex lock;
> > + u8 inp_config;
> > + u8 out_config;
> > +};
> > +
> > +static struct da9058_gpio *gpio_chip_to_da9058_gpio(struct gpio_chip
> > +*chip) {
> > + return container_of(chip, struct da9058_gpio, gp); }
> static inline, or a #define, but the compile will probably optimize-inline it
> anyway.
The compiler should optimize it to in-line, but I will change it anyway.
> > +static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset) {
> > + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> > + struct da9058 *da9058 = gpio->da9058;
> > + unsigned int gpio_level;
> > + int ret;
> > +
> > + if (offset > 1)
> > + return -EINVAL;
> So there are two GPIO pins, 0 and 1? That seems odd, but OK.
That is a feature of the hardware. I believe that calling them "0" and
"1" is the correct thing to do. Correct me if they should have been
called "1" and "2", or something else.
> > + if (offset) {
> So this is for GPIO 1
Yes, it seemed the obvious thing to do.
> > + u8 value_bits = value ? 0x80 : 0x00;
>
> These "value_bits" are just confusing. Just delete this and use the direct
> value
> below.
Will do. It was done for diagnostics that have since been stripped out.
> > +
> > + gpio->out_config &= ~0x80;
> A better way of writing &= ~0x80; is &= 0x7F
> > + gpio->out_config |= value_bits;
> gpio->out_config = value ? 0x80 : 0x00;
> So, less confusing.
see HANDLING NIBBLES below
> > + if (!(gpio_cntrl & 0x20))
> > + goto exit;
> Please insert a comment explaining what this bit is doing and why you're just
> exiting if it's not set. I don't understand one thing.
I have explained why in the driver source in the next submission attempt
> Maybe this would be better if you didn't use so many magic values, what about:
> #include
> #define FOO_FLAG BIT(3) /* This is a flag for foo */
> > +
> > + g
RE: [NEW DRIVER V2 5/7] DA9058 GPIO driver
-Original Message-
From: Linus Walleij [mailto:linus.wall...@linaro.org]
Sent: 13 August 2012 14:10
To: Opensource [Anthony Olech]
Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; David Dajun Chen; Samuel
Ortiz; Lee Jones
Subject: Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
Hi Anthony, sorry for delayed reply...
On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech
anthony.olech.opensou...@diasemi.com wrote:
This is the GPIO component driver of the Dialog DA9058 PMIC.
This driver is just one component of the whole DA9058 PMIC driver.
It depends on the core DA9058 MFD driver.
OK
+config GPIO_DA9058
+ tristate Dialog DA9058 GPIO
+ depends on MFD_DA9058
select IRQ_DOMAIN, you're going to want to use it...
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index
0f55662..209224a 100644
(...)
+#include linux/module.h
+#include linux/fs.h
Really?
+#include linux/uaccess.h
Really?
+#include linux/platform_device.h
+#include linux/gpio.h
+#include linux/syscalls.h
Really?
+#include linux/seq_file.h
+#include linux/slab.h
+#include linux/interrupt.h
+#include linux/mfd/core.h
+#include linux/regmap.h
If you're using regmap you better select it in Kconfig too, but it appears you
don't. You should be using regmap in the main MFD driver in this case (I
haven't looked at it though.)
This header set just looks like it was copied from some other file and never
really proofread, so please go over it in detail.
for some reason against 2.6.2x some of those were required, I just totally
forgot to prune out the unwanted ones when rebasing forwards to 3.5.
Good that you spotted it! Sorry I will try to prune the includes in the future.
+#include linux/mfd/da9058/version.h #include
+linux/mfd/da9058/registers.h #include linux/mfd/da9058/core.h
+#include linux/mfd/da9058/gpio.h #include linux/mfd/da9058/irq.h
+#include linux/mfd/da9058/pdata.h
Samuel will have to comment on this organization of headers, it seems a little
much. DO you really need all of them?
One of them should have been stripped out by my submit script, but as for the
others you must bear in mind that the DA9058 PMIC is a multifunction device,
and thus some header files are common and some are specific to various
component drivers. The very reason that you picked up on non-relevant include
files surely has implications on the structure of the header files for the
DA9058,
in particular struct's and define's that only apply to one component driver
should
be in separate header files.
+struct da9058_gpio {
+ struct da9058 *da9058;
+ struct platform_device *pdev;
+ struct gpio_chip gp;
+ struct mutex lock;
+ u8 inp_config;
+ u8 out_config;
+};
+
+static struct da9058_gpio *gpio_chip_to_da9058_gpio(struct gpio_chip
+*chip) {
+ return container_of(chip, struct da9058_gpio, gp); }
static inline, or a #define, but the compile will probably optimize-inline it
anyway.
The compiler should optimize it to in-line, but I will change it anyway.
+static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset) {
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio-da9058;
+ unsigned int gpio_level;
+ int ret;
+
+ if (offset 1)
+ return -EINVAL;
So there are two GPIO pins, 0 and 1? That seems odd, but OK.
That is a feature of the hardware. I believe that calling them 0 and
1 is the correct thing to do. Correct me if they should have been
called 1 and 2, or something else.
+ if (offset) {
So this is for GPIO 1
Yes, it seemed the obvious thing to do.
+ u8 value_bits = value ? 0x80 : 0x00;
These value_bits are just confusing. Just delete this and use the direct
value
below.
Will do. It was done for diagnostics that have since been stripped out.
+
+ gpio-out_config = ~0x80;
A better way of writing = ~0x80; is = 0x7F
+ gpio-out_config |= value_bits;
gpio-out_config = value ? 0x80 : 0x00;
So, less confusing.
see HANDLING NIBBLES below
+ if (!(gpio_cntrl 0x20))
+ goto exit;
Please insert a comment explaining what this bit is doing and why you're just
exiting if it's not set. I don't understand one thing.
I have explained why in the driver source in the next submission attempt
Maybe this would be better if you didn't use so many magic values, what about:
#include linux/bitops.h
#define FOO_FLAG BIT(3) /* This is a flag for foo */
+
+ gpio_cntrl = ~0xF0;
A better way to write = ~F0 is to write = 0x0F;
If you don't #define the constants this way of negating numbers just get
confusing.
So this is OK:
foo = ~FOO_FLAG;
foo |= set ? FOO_FLAG : 0;
This is just hard to read:
foo = ~0x55;
foo |= set ? 0x55 : 0;
And is better off
foo = 0xAA;
foo
Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
On Wed, Aug 15, 2012 at 12:08 PM, Opensource [Anthony Olech] anthony.olech.opensou...@diasemi.com wrote: [Me] + if (offset 1) + return -EINVAL; So there are two GPIO pins, 0 and 1? That seems odd, but OK. That is a feature of the hardware. I believe that calling them 0 and 1 is the correct thing to do. Correct me if they should have been called 1 and 2, or something else. It's correct, what I thought was odd was the fact that there were only two GPIO pins on this device. But some have only one even, just wanted to verify... HANDLING NIBBLES The handling of nibbles within a byte follows the rule that constants for the nibble NOT being operated on have those bits set to zero, and thus only bits being operated on may be non-zero. Thus to set, for example, the value 0xB into the MSH the operation is: byte = ~0xF0 byte |= 0xB0 it being obvious that it is the upper nibble being operated on. It seems that you are following a different rule for handling nibbles, and I can't find any standard for doing so in the kernel, so could you send me your reference documents? In this case as stated elsewhere, I'm happy that you do things this way, if you #define the magic values you're using in your bytes and nibbles, because else it's just hard to read. #define FOO_MASK 0xF0 #define BAR_FEATURE 0xB0 byte = ~FOO_MASK; byte |= BAR_FEATURE; It's more readble. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
On Mon, Aug 13, 2012 at 03:09:31PM +0200, Linus Walleij wrote: > On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech > > +#include > If you're using regmap you better select it in Kconfig > too, but it appears you don't. You should be using regmap in the > main MFD driver in this case (I haven't looked at it though.) For MFDs the MFD core should already be ensuring that regmap is selected. > > + gpio_cntrl |= 0x0F & gpio->out_config; > > + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, > > gpio_cntrl); > Further, if you're checking that flag just in order to avoid doing this > write if it's not necessary, it's the wrong solution. The right solution > is to implement regmap in the MFD driver so it quickly sees that > the right value is already in the register and bounces off. Just using regmap_update_bits() will do the right thing. > > + if (offset) > > + return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI1); > > + else > > + return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI0); > > +} > Lee Jones and Mark Brown discussed these virtual IRQ mapping functions > recently, and I think the outcome was to patch irqdomain to do the work > and not sprinkle these custom interfaces to fetch virtual IRQs all over the > place. The easiest thing to do would be to pick the VIRQ numbers so that you can just do da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI0 + offset); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
Hi Anthony, sorry for delayed reply...
On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech
wrote:
> This is the GPIO component driver of the Dialog DA9058 PMIC.
> This driver is just one component of the whole DA9058 PMIC driver.
> It depends on the core DA9058 MFD driver.
OK
> +config GPIO_DA9058
> + tristate "Dialog DA9058 GPIO"
> + depends on MFD_DA9058
select IRQ_DOMAIN, you're going to want to use it...
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 0f55662..209224a 100644
(...)
> +#include
> +#include
Really?
> +#include
Really?
> +#include
> +#include
> +#include
Really?
> +#include
> +#include
> +#include
> +#include
> +#include
If you're using regmap you better select it in Kconfig
too, but it appears you don't. You should be using regmap in the
main MFD driver in this case (I haven't looked at it though.)
This header set just looks like it was copied from some other
file and never really proofread, so please go over it in detail.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Samuel will have to comment on this organization of headers, it seems a
little much. DO you really need all of them?
> +struct da9058_gpio {
> + struct da9058 *da9058;
> + struct platform_device *pdev;
> + struct gpio_chip gp;
> + struct mutex lock;
> + u8 inp_config;
> + u8 out_config;
> +};
> +
> +static struct da9058_gpio *gpio_chip_to_da9058_gpio(struct gpio_chip *chip)
> +{
> + return container_of(chip, struct da9058_gpio, gp);
> +}
static inline, or a #define, but the compile will probably optimize-inline
it anyway.
> +static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> + unsigned int gpio_level;
> + int ret;
> +
> + if (offset > 1)
> + return -EINVAL;
So there are two GPIO pins, 0 and 1? That seems odd, but OK.
> +
> + mutex_lock(>lock);
> + ret = da9058_reg_read(da9058, DA9058_STATUSC_REG, _level);
> + mutex_unlock(>lock);
> + if (ret < 0)
> + return ret;
> +
> + if (offset) {
> + if (gpio_level & DA9058_STATUSC_GPI1)
> + return 1;
> + else
> + return 0;
> + } else {
> + if (gpio_level & DA9058_STATUSC_GPI0)
> + return 1;
> + else
> + return 0;
> + }
> +}
> +
> +static void da9058_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> + unsigned int gpio_cntrl;
> + int ret;
> +
> + if (offset > 1) {
> + dev_err(da9058->dev,
> + "Failed to set GPIO%d output=%d because illegal
> GPIO\n",
> + offset, value);
> + return;
> + }
> +
> + mutex_lock(>lock);
> +
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, _cntrl);
> + if (ret)
> + goto exit;
> +
> + if (offset) {
So this is for GPIO 1
> + u8 value_bits = value ? 0x80 : 0x00;
These "value_bits" are just confusing. Just delete this and use the
direct value below.
> +
> + gpio->out_config &= ~0x80;
A better way of writing &= ~0x80; is &= 0x7F
> + gpio->out_config |= value_bits;
gpio->out_config = value ? 0x80 : 0x00;
So, less confusing.
> + if (!(gpio_cntrl & 0x20))
> + goto exit;
Please insert a comment explaining what this bit is doing and
why you're just exiting if it's not set. I don't understand one thing.
Maybe this would be better if you didn't use so many magic values,
what about:
#include
#define FOO_FLAG BIT(3) /* This is a flag for foo */
> +
> + gpio_cntrl &= ~0xF0;
A better way to write &= ~F0 is to write &= 0x0F;
If you don't #define the constants this way of negating numbers
just get confusing.
So this is OK:
foo &= ~FOO_FLAG;
foo |= set ? FOO_FLAG : 0;
This is just hard to read:
foo &= ~0x55;
foo |= set ? 0x55 : 0;
And is better off
foo &= 0xAA;
foo |= set ? 0x55 : 0;
I prefer that you #define the registers but it's your pick.
> + gpio_cntrl |= 0xF0 & gpio->out_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG,
> gpio_cntrl);
> + } else {
> + u8 value_bits = value ? 0x08 : 0x00;
Same here, delete this.
> + gpio->out_config &= ~0x08;
&= 0xF7;
or use some and #defines ...
> + gpio->out_config |= value_bits;
Just
gpio->out_config |= value ? 0x08 : 0x00;
> + if (!(gpio_cntrl & 0x02))
> + goto exit;
Same thing, explain that flag.
> +
> +
Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
Hi Anthony, sorry for delayed reply...
On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech
anthony.olech.opensou...@diasemi.com wrote:
This is the GPIO component driver of the Dialog DA9058 PMIC.
This driver is just one component of the whole DA9058 PMIC driver.
It depends on the core DA9058 MFD driver.
OK
+config GPIO_DA9058
+ tristate Dialog DA9058 GPIO
+ depends on MFD_DA9058
select IRQ_DOMAIN, you're going to want to use it...
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 0f55662..209224a 100644
(...)
+#include linux/module.h
+#include linux/fs.h
Really?
+#include linux/uaccess.h
Really?
+#include linux/platform_device.h
+#include linux/gpio.h
+#include linux/syscalls.h
Really?
+#include linux/seq_file.h
+#include linux/slab.h
+#include linux/interrupt.h
+#include linux/mfd/core.h
+#include linux/regmap.h
If you're using regmap you better select it in Kconfig
too, but it appears you don't. You should be using regmap in the
main MFD driver in this case (I haven't looked at it though.)
This header set just looks like it was copied from some other
file and never really proofread, so please go over it in detail.
+#include linux/mfd/da9058/version.h
+#include linux/mfd/da9058/registers.h
+#include linux/mfd/da9058/core.h
+#include linux/mfd/da9058/gpio.h
+#include linux/mfd/da9058/irq.h
+#include linux/mfd/da9058/pdata.h
Samuel will have to comment on this organization of headers, it seems a
little much. DO you really need all of them?
+struct da9058_gpio {
+ struct da9058 *da9058;
+ struct platform_device *pdev;
+ struct gpio_chip gp;
+ struct mutex lock;
+ u8 inp_config;
+ u8 out_config;
+};
+
+static struct da9058_gpio *gpio_chip_to_da9058_gpio(struct gpio_chip *chip)
+{
+ return container_of(chip, struct da9058_gpio, gp);
+}
static inline, or a #define, but the compile will probably optimize-inline
it anyway.
+static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio-da9058;
+ unsigned int gpio_level;
+ int ret;
+
+ if (offset 1)
+ return -EINVAL;
So there are two GPIO pins, 0 and 1? That seems odd, but OK.
+
+ mutex_lock(gpio-lock);
+ ret = da9058_reg_read(da9058, DA9058_STATUSC_REG, gpio_level);
+ mutex_unlock(gpio-lock);
+ if (ret 0)
+ return ret;
+
+ if (offset) {
+ if (gpio_level DA9058_STATUSC_GPI1)
+ return 1;
+ else
+ return 0;
+ } else {
+ if (gpio_level DA9058_STATUSC_GPI0)
+ return 1;
+ else
+ return 0;
+ }
+}
+
+static void da9058_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+ struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
+ struct da9058 *da9058 = gpio-da9058;
+ unsigned int gpio_cntrl;
+ int ret;
+
+ if (offset 1) {
+ dev_err(da9058-dev,
+ Failed to set GPIO%d output=%d because illegal
GPIO\n,
+ offset, value);
+ return;
+ }
+
+ mutex_lock(gpio-lock);
+
+ ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
+ if (ret)
+ goto exit;
+
+ if (offset) {
So this is for GPIO 1
+ u8 value_bits = value ? 0x80 : 0x00;
These value_bits are just confusing. Just delete this and use the
direct value below.
+
+ gpio-out_config = ~0x80;
A better way of writing = ~0x80; is = 0x7F
+ gpio-out_config |= value_bits;
gpio-out_config = value ? 0x80 : 0x00;
So, less confusing.
+ if (!(gpio_cntrl 0x20))
+ goto exit;
Please insert a comment explaining what this bit is doing and
why you're just exiting if it's not set. I don't understand one thing.
Maybe this would be better if you didn't use so many magic values,
what about:
#include linux/bitops.h
#define FOO_FLAG BIT(3) /* This is a flag for foo */
+
+ gpio_cntrl = ~0xF0;
A better way to write = ~F0 is to write = 0x0F;
If you don't #define the constants this way of negating numbers
just get confusing.
So this is OK:
foo = ~FOO_FLAG;
foo |= set ? FOO_FLAG : 0;
This is just hard to read:
foo = ~0x55;
foo |= set ? 0x55 : 0;
And is better off
foo = 0xAA;
foo |= set ? 0x55 : 0;
I prefer that you #define the registers but it's your pick.
+ gpio_cntrl |= 0xF0 gpio-out_config;
+
+ ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG,
gpio_cntrl);
+ } else {
+ u8 value_bits = value ? 0x08 : 0x00;
Same here, delete this.
+ gpio-out_config = ~0x08;
= 0xF7;
or use
Re: [NEW DRIVER V2 5/7] DA9058 GPIO driver
On Mon, Aug 13, 2012 at 03:09:31PM +0200, Linus Walleij wrote: On Sun, Aug 5, 2012 at 10:43 PM, Anthony Olech +#include linux/regmap.h If you're using regmap you better select it in Kconfig too, but it appears you don't. You should be using regmap in the main MFD driver in this case (I haven't looked at it though.) For MFDs the MFD core should already be ensuring that regmap is selected. + gpio_cntrl |= 0x0F gpio-out_config; + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl); Further, if you're checking that flag just in order to avoid doing this write if it's not necessary, it's the wrong solution. The right solution is to implement regmap in the MFD driver so it quickly sees that the right value is already in the register and bounces off. Just using regmap_update_bits() will do the right thing. + if (offset) + return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI1); + else + return da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI0); +} Lee Jones and Mark Brown discussed these virtual IRQ mapping functions recently, and I think the outcome was to patch irqdomain to do the work and not sprinkle these custom interfaces to fetch virtual IRQs all over the place. The easiest thing to do would be to pick the VIRQ numbers so that you can just do da9058_to_virt_irq_num(da9058, DA9058_IRQ_EGPI0 + offset); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
