| Message ID | 20230126-gpio-mmio-fix-v2-2-38397aace340@ncr.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce new optional property to mark port as write only. | expand |
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?
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.
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);