On 31.05.2010 18:17, Michael Karcher wrote:
> This patch unifies the code for different Winbond W83627-family chips,
> to be applied before I add another W83627 GPIO raise function.
>
> Pleas comment on whether this approach is too sophisticated or a good
> idea.
>
> Signed-off-by: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>
>
I think it's pretty good. There are a few stylistic changes I'd like to
see, but other than that I see a huge potential for code cleanup coming
from this.
> diff --git a/board_enable.c b/board_enable.c
> index b5eb63f..edba408 100644
> --- a/board_enable.c
> +++ b/board_enable.c
> @@ -138,41 +138,67 @@ static int fdc37b787_gpio50_raise_3f0(const char *name)
> return fdc37b787_gpio50_raise(0x3f0, name);
> }
>
> -/**
> - * Winbond W83627HF: Raise GPIO24.
> - *
> - * Suited for:
> - * - Agami Aruma
> - * - IWILL DK8-HTX
> - */
> -static int w83627hf_gpio24_raise(uint16_t port, const char *name)
> +struct w83627x_gpio {
> + uint8_t device_id; /* reg 0x20 of the expected w83626x */
> + uint8_t ldn; /* LDN this GPIO register is located in */
> +
> + uint8_t mux_reg; /* register that selects GPIO/special fn mux,
> + or 0 if none */
> + uint8_t enable_bit; /* bit in 0x30 of that LDN to enable
> + the GPIO port */
> + uint8_t base; /* base register in that LDN for the port */
> +};
> +
> +static const struct w83627x_gpio w83627hf_2 = {0x52, 0x08, 0x2B, 0, 0xF0};
> +static const struct w83627x_gpio w83627thf_4 = {0x82, 0x09, 0, 1, 0xF4};
> +
> +static int w83627x_gpio_set(uint16_t port, const struct w83627x_gpio * gpio,
>
Can you eliminate struct w83627x_gpio* from the parameter list? IMHO
this function should take a look at the device ID it reads from the
chip, and use that to pick the correct struct.
> + int bit, int raise)
> {
> w836xx_ext_enter(port);
>
> - /* Is this the W83627HF? */
> - if (sio_read(port, 0x20) != 0x52) { /* Super I/O device ID reg. */
> - msg_perr("\nERROR: %s: W83627HF: Wrong ID: 0x%02X.\n",
> - name, sio_read(port, 0x20));
> + /* Is this the right Super I/O chip? */
> + if (sio_read(port, 0x20) != gpio->device_id) {
> + msg_perr("\nERROR: W83627x: Wrong ID: 0x%02X, expecting 0x%02X.\n",
> + sio_read(port, 0x20), gpio->device_id);
> w836xx_ext_leave(port);
> return -1;
> }
>
> - /* PIN89S: WDTO/GP24 multiplex -> GPIO24 */
> - sio_mask(port, 0x2B, 0x10, 0x10);
> + /* Select logical device */
> + sio_write(port, 0x07, gpio->ldn);
>
> - /* Select logical device 8: GPIO port 2 */
> - sio_write(port, 0x07, 0x08);
> + /* Activate logical device. */
> + sio_mask(port, 0x30, 1 << gpio->enable_bit, 1 << gpio->enable_bit);
>
> - sio_mask(port, 0x30, 0x01, 0x01); /* Activate logical device. */
> - sio_mask(port, 0xF0, 0x00, 0x10); /* GPIO24 -> output */
> - sio_mask(port, 0xF2, 0x00, 0x10); /* Clear GPIO24 inversion */
> - sio_mask(port, 0xF1, 0x10, 0x10); /* Raise GPIO24 */
> + /* Select GPIO function of that pin */
> + if(gpio->mux_reg)
>
Space after if.
> + sio_mask(port, gpio->mux_reg, 1 << bit, 1 << bit);
>
If bit>7 this will invoke undefined behaviour, and I don't see any code
checking if bit is in the range 0..7.
> +
> + sio_mask(port, gpio->base + 0, 0, 1 << bit); /* make pin output */
> + sio_mask(port, gpio->base + 2, 0, 1 << bit); /* Clear inversion */
> + if(raise)
> + sio_mask(port, gpio->base + 1, 1 << bit, 1 << bit);
> + else
> + sio_mask(port, gpio->base + 1, 0, 1 << bit);
>
Could you replace the if/else statement with this:
sio_mask(port, gpio->base + 1, raise << bit, 1 << bit);
>
> w836xx_ext_leave(port);
>
> return 0;
> }
>
> +/**
> + * Winbond W83627HF: Raise GPIO24.
> + *
> + * Suited for:
> + * - Agami Aruma
> + * - IWILL DK8-HTX
> + */
> +static int w83627hf_gpio24_raise(uint16_t port, const char *name)
> +{
> + return w83627x_gpio_set(port, &w83627hf_2, 4, 1);
> +}
> +
> static int w83627hf_gpio24_raise_2e(const char *name)
> {
> return w83627hf_gpio24_raise(0x2e, name);
> @@ -187,27 +213,7 @@ static int w83627hf_gpio24_raise_2e(const char *name)
> */
> static int w83627thf_gpio4_4_raise(uint16_t port, const char *name)
> {
> - w836xx_ext_enter(port);
> -
> - /* Is this the W83627THF? */
> - if (sio_read(port, 0x20) != 0x82) { /* Super I/O device ID reg. */
> - msg_perr("\nERROR: %s: W83627THF: Wrong ID: 0x%02X.\n",
> - name, sio_read(port, 0x20));
> - w836xx_ext_leave(port);
> - return -1;
>
> - }
> -
> - /* PINxxxxS: GPIO4/bit 4 multiplex -> GPIOXXX */
> -
> - sio_write(port, 0x07, 0x09); /* Select LDN 9: GPIO port 4 */
> - sio_mask(port, 0x30, 0x02, 0x02); /* Activate logical device. */
> - sio_mask(port, 0xF4, 0x00, 0x10); /* GPIO4 bit 4 -> output */
> - sio_mask(port, 0xF6, 0x00, 0x10); /* Clear GPIO4 bit 4 inversion */
> - sio_mask(port, 0xF5, 0x10, 0x10); /* Raise GPIO4 bit 4 */
> -
> - w836xx_ext_leave(port);
> -
> - return 0;
> + return w83627x_gpio_set(port, &w83627thf_4, 4, 1);
> }
>
> static int w83627thf_gpio4_4_raise_2e(const char *name)
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/