Messages in this thread | | | Date | Mon, 27 Mar 2023 14:42:24 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 1/2] iio: addac: stx104: Migrate to the regmap API |
| |
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
| |