New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
a7150: move video card to slot device #11647
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks good. Just a few things to change to make debugging practical.
required_device<z80_device> m_cpu; | ||
required_device<z80ctc_device> m_ctc; | ||
required_device<z80sio_device> m_sio; | ||
required_device_array<rs232_port_device, 2> m_serial; | ||
required_device<palette_device> m_palette; | ||
required_shared_ptr<u8> m_ram; | ||
|
||
memory_view m_view_lo; | ||
memory_view m_view_hi; | ||
|
||
u16 m_start; | ||
|
||
uint8_t kgs_host_r(offs_t offset); | ||
void kgs_host_w(offs_t offset, uint8_t data); | ||
void abg_addr_w(offs_t offset, uint8_t data); | ||
void abg_func_w(offs_t offset, uint8_t data); | ||
void abg_split_w(offs_t offset, uint8_t data); | ||
void abg_misc_w(offs_t offset, uint8_t data); | ||
void kgs_memory_remap(); | ||
|
||
bool m_abg_msel = 0, m_kgs_iml = 0; | ||
uint8_t m_abg_func = 0, m_abg_split = 0; | ||
uint16_t m_abg_addr = 0; | ||
uint8_t m_kgs_datao = 0, m_kgs_datai = 0, m_kgs_ctrl = 0; | ||
bool m_nmi_enable = 0; | ||
|
||
uint32_t screen_update_k7072(screen_device &screen, bitmap_ind16 &bitmap, const rectangle &cliprect); | ||
void palette_init(palette_device &palette); | ||
|
||
uint16_t io_r(offs_t offset, uint16_t mem_mask); | ||
void io_w(offs_t offset, uint16_t data, uint16_t mem_mask); | ||
|
||
void k7070_cpu_io(address_map &map); | ||
void k7070_cpu_mem(address_map &map); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep data members together and member functions together within an access level section. Don’t intermix them like this.
bool m_abg_msel = 0, m_kgs_iml = 0; | ||
uint8_t m_abg_func = 0, m_abg_split = 0; | ||
uint16_t m_abg_addr = 0; | ||
uint8_t m_kgs_datao = 0, m_kgs_datai = 0, m_kgs_ctrl = 0; | ||
bool m_nmi_enable = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please initialise members in the constructor initialiser list. It’s confusing when there are member initialisations split between the header and source file.
case 6: | ||
data = ioport("DSEL0")->read(); | ||
break; | ||
|
||
case 7: | ||
data = ioport("DSEL1")->read(); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a required_ioport_array<2>
to access these so they don’t cause tag lookups in a read handler.
case 0: | ||
data = m_kgs_datao; | ||
m_kgs_ctrl &= ~(KGS_ST_ERR | KGS_ST_IBF); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check machine().side_effects_disabled()
before doing things with side effects in a read handler, or it will be impossible to debug.
case 1: | ||
data = m_kgs_datai; | ||
m_kgs_ctrl &= ~KGS_ST_OBF; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check machine().side_effects_disabled()
before doing things with side effects in a read handler, or it will be impossible to debug.
if (offset) | ||
LOG("%s: KGS %d == %02x '%c'\n", machine().describe_context(), offset, data, | ||
(data > 0x1f && data < 0x7f) ? data : 0x20); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please indent controlled statements by one level. Also, please check machine().side_effects_disabled()
when logging in a read handler to avoid spam when a debugger window is open.
if (offset) | ||
LOG("%s: KGS %d <- %02x '%c', ctrl %02x\n", machine().describe_context(), offset, data, | ||
(data > 0x1f && data < 0x7f) ? data : 0x20, m_kgs_ctrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please indent controlled statements by one level.
if (offset != 2 && offset != 5) | ||
LOG("%s: kgs %d == %02x '%c'\n", machine().describe_context(), offset, data, | ||
(data > 0x1f && data < 0x7f) ? data : ' '); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check machine().side_effects_disabled()
when logging in a read handler to avoid spam when a debugger window is open.
Thanks. |
…070 KGS a Multibus card. (mamedev#11647)
…070 KGS a Multibus card. (mamedev#11647)
No description provided.