lkml.org 
[lkml]   [2023]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2023-03-27 13:43    [W:0.064 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site