I think we've sorted this out, but replying to finalise the conversation On Tue, 12 Jul 2022, at 11:27, Peter Delevoryas wrote: > On Mon, Jul 11, 2022 at 10:56:08PM +0930, Andrew Jeffery wrote: >> >> /* >> @@ -607,7 +608,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr >> offset, uint64_t data, >> data &= props->output; >> data = update_value_control_source(set, set->data_value, data); >> set->data_read = data; >> - aspeed_gpio_update(s, set, data); >> + aspeed_gpio_update(s, set, data, set->direction); >> return; >> case gpio_reg_direction: >> /* >> @@ -683,7 +684,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr >> offset, uint64_t data, >> HWADDR_PRIx"\n", __func__, offset); >> return; >> } >> - aspeed_gpio_update(s, set, set->data_value); >> + aspeed_gpio_update(s, set, set->data_value, UINT32_MAX); > > Looks great overall, but why is the mode_mask UINT32_MAX here? Shouldn't it be > set->direction? We only want to let the guest OS write to output pins, right? > Or is that not how the register works?
The set->direction case is handled in the top hunk which handles the data register write. Note that it performs an early return. The bottom hunk deals with making the value register consistent when we've updated any register that isn't the data register. Andrew
