Re: [LinuxBIOS] r2811 - trunk/util/flashrom

[Peter Stuge]

So, I should have reviewed sooner.

On Thu, Sep 27, 2007 at 04:29:57PM +0200, [e-mail] wrote:
> +/* Generic Super I/O helper functions */
> +uint8_t regval(uint16_t port, uint8_t reg)
> +{
> outb(reg, port);
> return inb(port + 1);
>
> +}
>
> +void regwrite(uint16_t port, uint8_t reg, uint8_t val)
> +{
> outb(reg, port);
> outb(val, port + 1);
>
> +}

I would have liked better names for these functions.
> +/* Helper functions for most recent ITE IT87xx Super I/O chips */
> +#define CHIP_ID_BYTE1_REG	0x20
> +#define CHIP_ID_BYTE2_REG	0x21
> +static void enter_conf_mode_ite(uint16_t port)
> +{
> outb(0x87, port);
> outb(0x01, port);
> outb(0x55, port);
> if (port == 0x2e)
> outb(0x55, port);
> else
> outb(0xaa, port);
>
> +}
>
> +static void exit_conf_mode_ite(uint16_t port)
> +{
> regwrite(port, 0x02, 0x02);
>
> +}
>
> +static uint16_t find_ite_serial_flash_port(uint16_t port)
> +static void it8716_serial_rdid(uint16_t port)
> +static int it87xx_probe_serial_flash(const char *name)

This is still in a generic file, right? Shouldn't these be in an ITE
file?

//Peter

[LinuxBIOS] r2811 - trunk/util/flashrom

[Carl-Daniel Hailfinger]

On 27.09.2007 17:53, Peter Stuge wrote:
> So, I should have reviewed sooner.

Thanks for reviewing now. I will take your comments into account for the
ongoing rewrite/restructuring.
> On Thu, Sep 27, 2007 at 04:29:57PM +0200, [e-mail] wrote:
> > +/* Generic Super I/O helper functions */
> > +uint8_t regval(uint16_t port, uint8_t reg)
> > +{
> > outb(reg, port);
> > return inb(port + 1);
> >
> > +}
> >
> > +void regwrite(uint16_t port, uint8_t reg, uint8_t val)
> > +{
> > outb(reg, port);
> > outb(val, port + 1);
> >
> > +}
>
> I would have liked better names for these functions.

Straight copy from flashrom. If we can agree to a new name, we have to
patch flashrom as well.
> > +/* Helper functions for most recent ITE IT87xx Super I/O chips */
> > +#define CHIP_ID_BYTE1_REG	0x20
> > +#define CHIP_ID_BYTE2_REG	0x21
> > +static void enter_conf_mode_ite(uint16_t port)
> > +{
> > outb(0x87, port);
> > outb(0x01, port);
> > outb(0x55, port);
> > if (port == 0x2e)
> > outb(0x55, port);
> > else
> > outb(0xaa, port);
> >
> > +}
> >
> > +static void exit_conf_mode_ite(uint16_t port)
> > +{
> > regwrite(port, 0x02, 0x02);
> >
> > +}
> >
> > +static uint16_t find_ite_serial_flash_port(uint16_t port)
> > +static void it8716_serial_rdid(uint16_t port)
> > +static int it87xx_probe_serial_flash(const char *name)
>
> This is still in a generic file, right? Shouldn't these be in an ITE
> file?

The functions (except it8716_serial_rdid, which is splitted) are
somewhat mainboard-specific and belong to board_enable.c IMHO. However,
the SPI specific stuff will be moved to spi.c.

Carl-Daniel