Some electronics do not allow the data regsister to be read without
corrupting the existing data on the port. It a quirk of the board
design.
e.g. I have a couple of boards where the electronics engineer decided
to only use the chip select line, so no read/write signal is connected.
This means that reading the address activates the chip select and drives
the contents of the data bus to the port.This makes it impossible to
read the last data written to the port.
This solution is to use the existing shadow data register 'bgpio_data'.
It can be used to return the last value written to the port by the read
operation.
This is enabled for a particular port using a new flag and a new
device tree property "no-input" to allow it to be selected on a board by
board basis. This means it will only effect hardware that requests it.
Signed-off-by: Niall Leonard <[email protected]>
---
Changes in v2:
- Description of change updated to clarify why it is needed.
- Patches squashed as per request during review.
- wd,mbl-gpio bindings updated to yaml format.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Niall Leonard (2):
dt-bindings: improve wb,mbl-gpio binding documentation.
gpio: mmio: Use new flag BGPIOF_NO_INPUT.
.../devicetree/bindings/gpio/wd,mbl-gpio.txt | 38 -----------
.../devicetree/bindings/gpio/wd,mbl-gpio.yaml | 78 ++++++++++++++++++++++
drivers/gpio/gpio-mmio.c | 24 ++++++-
include/linux/gpio/driver.h | 1 +
4 files changed, 100 insertions(+), 41 deletions(-)
---
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20230126-gpio-mmio-fix-1a69d03ec9e7
Best regards,
--
Niall Leonard <[email protected]>
From: Niall Leonard <[email protected]>
Add new flag BGPIOF_NO_INPUT to header file.
Use the existing shadow data register 'bgpio_data' to allow
the last written value to be returned by the read operation
when BGPIOF_NO_INPUT flag is set.
Ensure this change only applies to the specific binding "wd,mbl-gpio".
Signed-off-by: Niall Leonard <[email protected]>
---
drivers/gpio/gpio-mmio.c | 24 +++++++++++++++++++++---
include/linux/gpio/driver.h | 1 +
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index d9dff3dc92ae..9939fdbf6345 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -164,6 +164,11 @@ static int bgpio_get_set_multiple(struct gpio_chip *gc, unsigned long *mask,
return 0;
}
+static int bgpio_get_shadow(struct gpio_chip *gc, unsigned int gpio)
+{
+ return !!(gc->bgpio_data & bgpio_line2mask(gc, gpio));
+}
+
static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
{
return !!(gc->read_reg(gc->reg_dat) & bgpio_line2mask(gc, gpio));
@@ -526,7 +531,10 @@ static int bgpio_setup_io(struct gpio_chip *gc,
* reading each line individually in that fringe case.
*/
} else {
- gc->get = bgpio_get;
+ if (flags & BGPIOF_NO_INPUT)
+ gc->get = bgpio_get_shadow;
+ else
+ gc->get = bgpio_get;
if (gc->be_bits)
gc->get_multiple = bgpio_get_multiple_be;
else
@@ -630,7 +638,11 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
if (ret)
return ret;
- gc->bgpio_data = gc->read_reg(gc->reg_dat);
+ if (flags & BGPIOF_NO_INPUT)
+ gc->bgpio_data = 0;
+ else
+ gc->bgpio_data = gc->read_reg(gc->reg_dat);
+
if (gc->set == bgpio_set_set &&
!(flags & BGPIOF_UNREADABLE_REG_SET))
gc->bgpio_data = gc->read_reg(gc->reg_set);
@@ -694,8 +706,9 @@ static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
unsigned long *flags)
{
struct bgpio_pdata *pdata;
+ const struct of_device_id *of_id;
- if (!of_match_device(bgpio_of_match, &pdev->dev))
+ if (!(of_id = of_match_device(bgpio_of_match, &pdev->dev)))
return NULL;
pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata),
@@ -711,6 +724,11 @@ static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
if (of_property_read_bool(pdev->dev.of_node, "no-output"))
*flags |= BGPIOF_NO_OUTPUT;
+ if (!strcmp(of_id->compatible, "wd,mbl-gpio")) {
+ if (of_property_read_bool(pdev->dev.of_node, "no-input"))
+ *flags |= BGPIOF_NO_INPUT;
+ }
+
return pdata;
}
#else
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 44783fc16125..e8e57310e3b8 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -682,6 +682,7 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
#define BGPIOF_READ_OUTPUT_REG_SET BIT(4) /* reg_set stores output value */
#define BGPIOF_NO_OUTPUT BIT(5) /* only input */
#define BGPIOF_NO_SET_ON_INPUT BIT(6)
+#define BGPIOF_NO_INPUT BIT(7) /* only output */
int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hwirq);
--
2.34.1
On Tue, Jan 31, 2023 at 01:49:38PM +0000, Niall Leonard wrote:
> Add new flag BGPIOF_NO_INPUT to header file.
> Use the existing shadow data register 'bgpio_data' to allow
> the last written value to be returned by the read operation
> when BGPIOF_NO_INPUT flag is set.
> Ensure this change only applies to the specific binding "wd,mbl-gpio".
I'm wondering why do we need that.
I mean the reading back the (possible cached) output value is the right thing
to do by default for GPIO (in output mode) or GPO. So, instead you can simply
check the current direction of the pin and return (cached) value.
Or did I miss something?
--
With Best Regards,
Andy Shevchenko
On 12/02/2023 12:38, Andy Shevchenko wrote:
> *External Message* - Use caution before opening links or attachments
>
> On Tue, Jan 31, 2023 at 01:49:38PM +0000, Niall Leonard wrote:
>> Add new flag BGPIOF_NO_INPUT to header file.
>> Use the existing shadow data register 'bgpio_data' to allow
>> the last written value to be returned by the read operation
>> when BGPIOF_NO_INPUT flag is set.
>> Ensure this change only applies to the specific binding "wd,mbl-gpio".
>
> I'm wondering why do we need that.
>
> I mean the reading back the (possible cached) output value is the right thing
> to do by default for GPIO (in output mode) or GPO. So, instead you can simply
> check the current direction of the pin and return (cached) value.
>
> Or did I miss something?
>
My thinking here was that I don't want to break any existing code which
relies on the read always reading the physical port.
I'm going to rethink my approach here as I'm starting to think the
better approach would be to modify the gpio-74xx-mmio.c driver to cater
for this hardware.
Regards,
Niall.
On Mon, Mar 13, 2023 at 01:56:24PM +0000, Leonard, Niall wrote:
> On 12/02/2023 12:38, Andy Shevchenko wrote:
> > On Tue, Jan 31, 2023 at 01:49:38PM +0000, Niall Leonard wrote:
> >> Add new flag BGPIOF_NO_INPUT to header file.
> >> Use the existing shadow data register 'bgpio_data' to allow
> >> the last written value to be returned by the read operation
> >> when BGPIOF_NO_INPUT flag is set.
> >> Ensure this change only applies to the specific binding "wd,mbl-gpio".
> >
> > I'm wondering why do we need that.
> >
> > I mean the reading back the (possible cached) output value is the right thing
> > to do by default for GPIO (in output mode) or GPO. So, instead you can simply
> > check the current direction of the pin and return (cached) value.
> >
> > Or did I miss something?
> >
> My thinking here was that I don't want to break any existing code which
> relies on the read always reading the physical port.
> I'm going to rethink my approach here as I'm starting to think the
> better approach would be to modify the gpio-74xx-mmio.c driver to cater
> for this hardware.
That's why we need a greatest denominator here, right?
Currently we have a full inconsistency on how drivers are implementing
all this. What I'm suggesting is to always have the following:
1) if pin is input or OS/OD/OE/OC -- read input buffer;
2) if pin is output, always read (cached) value.
Yes, there is an opinion to find a short circuit by reading input in
the output mode, but I consider that impractical complication.
Note, that above will also satisfy all (common) hardware limitations.
--
With Best Regards,
Andy Shevchenko