On Sun, Mar 26, 2023 at 06:05:57PM -0400, William Breathitt Gray wrote:
> The regmap API supports IO port accessors so we can take advantage of
> regmap abstractions rather than handling access to the device registers
> directly in the driver.
...
> +static const struct regmap_config aio_ctl_regmap_config = {
> + .name = "aio_ctl",
> + .reg_bits = 8,
> + .reg_stride = 1,
> + .reg_base = STX104_AIO_BASE,
> + .val_bits = 8,
> + .io_port = true,
> + .max_register = 0x11,
Not sure if define would be better for this, so it will be grouped with
register offset definitions. (Same for the other configs)
> + .wr_table = &aio_ctl_wr_table,
> + .rd_table = &aio_ctl_rd_table,
> + .volatile_table = &aio_ctl_volatile_table,
> + .cache_type = REGCACHE_FLAT,
> +};
Do we need regmap lock?
...
> +static const struct regmap_config aio_data_regmap_config = {
> + .name = "aio_data",
> + .reg_bits = 16,
> + .reg_stride = STX104_AIO_DATA_STRIDE,
> + .reg_base = STX104_AIO_BASE,
> + .val_bits = 16,
> + .io_port = true,
> + .max_register = 0x6,
> + .wr_table = &aio_data_wr_table,
> + .rd_table = &aio_data_rd_table,
> + .volatile_table = &aio_data_rd_table,
> + .cache_type = REGCACHE_FLAT,
> +};
Ditto.
> +static const struct regmap_config dio_regmap_config = {
> + .name = "dio",
> + .reg_bits = 8,
> + .reg_stride = 1,
> + .reg_base = STX104_DIO_REG,
> + .val_bits = 8,
> + .io_port = true,
> + .max_register = 0x0,
> };
Ditto.
...
> + err = regmap_read(priv->aio_ctl_map, STX104_ADC_CONFIGURATION, &adc_config);
> + if (err)
> + return err;
>
> - *val = 1 << gain;
> + *val = 1 << u8_get_bits(adc_config, STX104_GAIN);
Maybe not for this change, but why not BIT()?
...
> + do {
> + err = regmap_read(priv->aio_ctl_map, STX104_ADC_STATUS, &adc_status);
> + if (err)
> + return err;
> + } while (u8_get_bits(adc_status, STX104_CNV));
Hmm... Isn't it a potential infinite loop (e.g., ther hardware / firmware
is broken)?
Why not using regmap_read_poll_timeout() (or its atomic variant, depends on
the case)?
...
> case IIO_CHAN_INFO_RAW:
> if (chan->output) {
You can decrease indentation by
if (!chan->output)
return -EINVAL;
here.
> /* DAC can only accept up to a 16-bit value */
> if ((unsigned int)val > 65535)
> return -EINVAL;
>
> - priv->chan_out_states[chan->channel] = val;
> - iowrite16(val, &priv->reg->dac[chan->channel]);
> -
> - return 0;
> + return regmap_write(priv->aio_data_map, STX104_DAC_OFFSET(chan->channel),
> + val);
> }
> return -EINVAL;
> }
...
> + gpio_config = (struct gpio_regmap_config) {
> + .parent = dev,
> + .regmap = dio_map,
> + .ngpio = STX104_NGPIO,
> + .names = stx104_names,
> + .reg_dat_base = GPIO_REGMAP_ADDR(STX104_DIO_REG),
> + .reg_set_base = GPIO_REGMAP_ADDR(STX104_DIO_REG),
> + .ngpio_per_reg = STX104_NGPIO,
> + .reg_mask_xlate = stx104_reg_mask_xlate,
> + .drvdata = dio_map,
> + };
Not sure of compound literal is good to have in such case, but if
Jonathan asked for that...
--
With Best Regards,
Andy Shevchenko
