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:2.339 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site