Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 27 Mar 2023 14:42:24 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     William Breathitt Gray <william.gray@linaro.org>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/2] iio: addac: stx104: Migrate to the regmap API
Message-ID: <ZCGBIAvr7OQLwNXv@smile.fi.intel.com>
References: <cover.1679867815.git.william.gray@linaro.org>
 <4ebc1b6b609a086846420954b893e914fd395384.1679867815.git.william.gray@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4ebc1b6b609a086846420954b893e914fd395384.1679867815.git.william.gray@linaro.org>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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