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;
>
