Re: [PATCH 1/5] iio: gyro: adi16136: construct adis data on probe vs static on driver

[Jonathan Cameron]

On Fri, 13 Dec 2019 18:03:08 +0200
Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:

> This change is done in preparation of adding an `struct adis_timeout` type.
> Some ADIS drivers support multiple drivers, with various combinations of
> timeouts. Creating static tables for each driver is possible, but that also
> creates quite a lot of duplication.
> 
> Signed-off-by: Nuno SÃ <nuno.sa@xxxxxxxxxx>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>

There are considerable advantages to using constant structures,
(security - not that relevant here probably, XiP, general readability)

So to take a series like this I want to see evidence that it makes
a significant difference.  So far you just have cases where we end up
with a worse result.  More code, harder to read...

Hence it will take a lot to persuade me to take this series without
the follow up patches where I assume significant advantages are seen.

Jonathan


> ---
>  drivers/iio/gyro/adis16136.c | 49 ++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
> index f10c4f173898..129de2bd5845 100644
> --- a/drivers/iio/gyro/adis16136.c
> +++ b/drivers/iio/gyro/adis16136.c
> @@ -465,24 +465,6 @@ static const char * const adis16136_status_error_msgs[] = {
>  	[ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL] = "Flash checksum error",
>  };
>  
> -static const struct adis_data adis16136_data = {
> -	.diag_stat_reg = ADIS16136_REG_DIAG_STAT,
> -	.glob_cmd_reg = ADIS16136_REG_GLOB_CMD,
> -	.msc_ctrl_reg = ADIS16136_REG_MSC_CTRL,
> -
> -	.self_test_mask = ADIS16136_MSC_CTRL_SELF_TEST,
> -	.startup_delay = 80,
> -
> -	.read_delay = 10,
> -	.write_delay = 10,
> -
> -	.status_error_msgs = adis16136_status_error_msgs,
> -	.status_error_mask = BIT(ADIS16136_DIAG_STAT_FLASH_UPDATE_FAIL) |
> -		BIT(ADIS16136_DIAG_STAT_SPI_FAIL) |
> -		BIT(ADIS16136_DIAG_STAT_SELF_TEST_FAIL) |
> -		BIT(ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL),
> -};
> -
>  enum adis16136_id {
>  	ID_ADIS16133,
>  	ID_ADIS16135,
> @@ -509,11 +491,36 @@ static const struct adis16136_chip_info adis16136_chip_info[] = {
>  	},
>  };
>  
> +static struct adis_data *adis16136_adis_data_alloc(struct adis16136 *st,
> +						   struct device *dev)
> +{
> +	struct adis_data *data;
> +
> +	data = devm_kzalloc(dev, sizeof(struct adis_data), GFP_KERNEL);
> +	if (!data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	data->msc_ctrl_reg = ADIS16136_REG_MSC_CTRL;
> +	data->glob_cmd_reg = ADIS16136_REG_GLOB_CMD;
> +	data->diag_stat_reg = ADIS16136_REG_DIAG_STAT;
> +	data->self_test_mask = ADIS16136_MSC_CTRL_SELF_TEST;
> +	data->read_delay = 10;
> +	data->write_delay = 10;
> +	data->status_error_msgs = adis16136_status_error_msgs;
> +	data->status_error_mask = BIT(ADIS16136_DIAG_STAT_FLASH_UPDATE_FAIL) |
> +				BIT(ADIS16136_DIAG_STAT_SPI_FAIL) |
> +				BIT(ADIS16136_DIAG_STAT_SELF_TEST_FAIL) |
> +				BIT(ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL);
> +
> +	return data;
> +}
> +
>  static int adis16136_probe(struct spi_device *spi)
>  {
>  	const struct spi_device_id *id = spi_get_device_id(spi);
>  	struct adis16136 *adis16136;
>  	struct iio_dev *indio_dev;
> +	const struct adis_data *adis16136_data;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adis16136));
> @@ -532,7 +539,11 @@ static int adis16136_probe(struct spi_device *spi)
>  	indio_dev->info = &adis16136_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = adis_init(&adis16136->adis, indio_dev, spi, &adis16136_data);
> +	adis16136_data = adis16136_adis_data_alloc(adis16136, &spi->dev);
> +	if (IS_ERR(adis16136_data))
> +		return PTR_ERR(adis16136_data);
> +
> +	ret = adis_init(&adis16136->adis, indio_dev, spi, adis16136_data);
>  	if (ret)
>  		return ret;
>