RFC: gpio: mmio: add support for 3 direction regs

Message ID 32edfd70-95dc-26a1-2ea6-143344cb2384@linux.intel.com
State New
Headers show
Series
  • RFC: gpio: mmio: add support for 3 direction regs
Related show

Commit Message

Fried, Ramon Jan. 3, 2019, 7:36 a.m.
Hi.

I'm working on a driver for STA2X11 GPIO controller who seems to fit best to the generic mmio driver,
the only problem I have is with the dir register case.
The STA2X11 has 3 registers for dir, one for data, one for set and one for clear.
The generic-mmio driver has support for this fashion for the dat & set & clear registers but not for dirout/dirin registers.

I wonder if support for this is generic enough to deserve a patch, if so I'm willing to quickly add this support, if not, adding a flag such as below, will allow partly using the generic mmio driver only for set/get and the direction can be handled outside the driver.

Thanks,
Ramon

Comments

Vladimir Zapolskiy Jan. 3, 2019, 8:07 a.m. | #1
Hi Ramon,

On 01/03/2019 09:36 AM, Fried, Ramon wrote:
> Hi.
> 
> I'm working on a driver for STA2X11 GPIO controller who seems to fit
> best to the generic mmio driver,

I hope you have seen the existing driver drivers/gpio/gpio-sta2x11.c

> the only problem I have is with the dir register case. The STA2X11
> has 3 registers for dir, one for data, one for set and one for
> clear. The generic-mmio driver has support for this fashion for the
> dat & set & clear registers but not for dirout/dirin registers.
> 
> I wonder if support for this is generic enough to deserve a patch, if
> so I'm willing to quickly add this support, if not, adding a flag
> such as below, will allow partly using the generic mmio driver only
> for set/get and the direction can be handled outside the driver.
> 

If gpio-mmio fits well, then it might be simpler to set a flag
BGPIOF_UNREADABLE_REG_DIR, then call bgpio_init() and then overwrite
.direction_input, .direction_output and .get_direction callbacks,
as a reference you can take a look at gpio-74xx-mmio.c

--
Best wishes,
Vladimir
Fried, Ramon Jan. 3, 2019, 8:51 a.m. | #2
On 1/3/2019 10:07, Vladimir Zapolskiy wrote:
> Hi Ramon,
>
> On 01/03/2019 09:36 AM, Fried, Ramon wrote:
>> Hi.
>>
>> I'm working on a driver for STA2X11 GPIO controller who seems to fit
>> best to the generic mmio driver,
> I hope you have seen the existing driver drivers/gpio/gpio-sta2x11.c
I surely did. we have the same IP in our soc but it was changed a lot internally, don't want to litter the original.
>
>> the only problem I have is with the dir register case. The STA2X11
>> has 3 registers for dir, one for data, one for set and one for
>> clear. The generic-mmio driver has support for this fashion for the
>> dat & set & clear registers but not for dirout/dirin registers.
>>
>> I wonder if support for this is generic enough to deserve a patch, if
>> so I'm willing to quickly add this support, if not, adding a flag
>> such as below, will allow partly using the generic mmio driver only
>> for set/get and the direction can be handled outside the driver.
>>
> If gpio-mmio fits well, then it might be simpler to set a flag
> BGPIOF_UNREADABLE_REG_DIR, then call bgpio_init() and then overwrite
> .direction_input, .direction_output and .get_direction callbacks,
> as a reference you can take a look at gpio-74xx-mmio.c

Nice.
That's an option I didn't think of, better than setting the flag. what about adding the generic support ?

>
> --
> Best wishes,
> Vladimir
Vladimir Zapolskiy Jan. 3, 2019, 8:59 a.m. | #3
On 01/03/2019 10:51 AM, Fried, Ramon wrote:
> 
> On 1/3/2019 10:07, Vladimir Zapolskiy wrote:
>> Hi Ramon,
>>
>> On 01/03/2019 09:36 AM, Fried, Ramon wrote:
>>> Hi.
>>>
>>> I'm working on a driver for STA2X11 GPIO controller who seems to fit
>>> best to the generic mmio driver,
>> I hope you have seen the existing driver drivers/gpio/gpio-sta2x11.c
> I surely did. we have the same IP in our soc but it was changed a lot
> internally, don't want to litter the original.
>>
>>> the only problem I have is with the dir register case. The STA2X11
>>> has 3 registers for dir, one for data, one for set and one for
>>> clear. The generic-mmio driver has support for this fashion for the
>>> dat & set & clear registers but not for dirout/dirin registers.
>>>
>>> I wonder if support for this is generic enough to deserve a patch, if
>>> so I'm willing to quickly add this support, if not, adding a flag
>>> such as below, will allow partly using the generic mmio driver only
>>> for set/get and the direction can be handled outside the driver.
>>>
>> If gpio-mmio fits well, then it might be simpler to set a flag
>> BGPIOF_UNREADABLE_REG_DIR, then call bgpio_init() and then overwrite
>> .direction_input, .direction_output and .get_direction callbacks,
>> as a reference you can take a look at gpio-74xx-mmio.c
> 
> Nice.
> That's an option I didn't think of, better than setting the flag.
> what about adding the generic support ?

Setting a GPIO direction over three different registers doesn't sound
as a suitable candidate for generalization, also your particular
implementation is partial, relies on external platform/driver code
to preset the directions and it does not allow to change direction
in runtime, so just renounce the idea.

--
Best wishes,
Vladimir
Fried, Ramon Jan. 3, 2019, 9:30 a.m. | #4
On 1/3/2019 10:59, Vladimir Zapolskiy wrote:
> On 01/03/2019 10:51 AM, Fried, Ramon wrote:
>> On 1/3/2019 10:07, Vladimir Zapolskiy wrote:
>>> Hi Ramon,
>>>
>>> On 01/03/2019 09:36 AM, Fried, Ramon wrote:
>>>> Hi.
>>>>
>>>> I'm working on a driver for STA2X11 GPIO controller who seems to fit
>>>> best to the generic mmio driver,
>>> I hope you have seen the existing driver drivers/gpio/gpio-sta2x11.c
>> I surely did. we have the same IP in our soc but it was changed a lot
>> internally, don't want to litter the original.
>>>> the only problem I have is with the dir register case. The STA2X11
>>>> has 3 registers for dir, one for data, one for set and one for
>>>> clear. The generic-mmio driver has support for this fashion for the
>>>> dat & set & clear registers but not for dirout/dirin registers.
>>>>
>>>> I wonder if support for this is generic enough to deserve a patch, if
>>>> so I'm willing to quickly add this support, if not, adding a flag
>>>> such as below, will allow partly using the generic mmio driver only
>>>> for set/get and the direction can be handled outside the driver.
>>>>
>>> If gpio-mmio fits well, then it might be simpler to set a flag
>>> BGPIOF_UNREADABLE_REG_DIR, then call bgpio_init() and then overwrite
>>> .direction_input, .direction_output and .get_direction callbacks,
>>> as a reference you can take a look at gpio-74xx-mmio.c
>> Nice.
>> That's an option I didn't think of, better than setting the flag.
>> what about adding the generic support ?
> Setting a GPIO direction over three different registers doesn't sound
> as a suitable candidate for generalization, also your particular
> implementation is partial, relies on external platform/driver code
> to preset the directions and it does not allow to change direction
> in runtime, so just renounce the idea.

Alrighty then, will overwrite the direction CBs.

Thanks.

>
> --
> Best wishes,
> Vladimir

Patch

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a4d5eb3..4f91526 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -435,6 +435,7 @@  int bgpio_init(struct gpio_chip *gc, struct device *dev,
 #define BGPIOF_BIG_ENDIAN_BYTE_ORDER   BIT(3)
 #define BGPIOF_READ_OUTPUT_REG_SET     BIT(4) /* reg_set stores output value */
 #define BGPIOF_NO_OUTPUT               BIT(5) /* only input */
+#define BGPIOF_NO_DIRECTION            BIT(6)
 
 #endif

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 935292a..66f6448 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -554,6 +554,8 @@  static int bgpio_setup_direction(struct gpio_chip *gc,
                gc->direction_input = bgpio_dir_in;
                gc->get_direction = bgpio_get_dir;
                gc->bgpio_dir_inverted = true;
+       } else if (flags & BGPIOF_NO_DIRECTION) {
+               return 0;
        } else {
                if (flags & BGPIOF_NO_OUTPUT)
                        gc->direction_output = bgpio_dir_out_err;
@@ -638,7 +640,7 @@  int bgpio_init(struct gpio_chip *gc, struct device *dev,
        if (gc->set == bgpio_set_set &&
                        !(flags & BGPIOF_UNREADABLE_REG_SET))
                gc->bgpio_data = gc->read_reg(gc->reg_set);
-       if (gc->reg_dir && !(flags & BGPIOF_UNREADABLE_REG_DIR))
+       if (gc->reg_dir && !(flags & (BGPIOF_UNREADABLE_REG_DIR | BGPIOF_NO_DIRECTION)))
                gc->bgpio_dir = gc->read_reg(gc->reg_dir);
 
        return ret;