On Sun, 2019-12-15 at 16:18 +0000, Jonathan Cameron wrote:
> [External]
>
> 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.
>
Well, we've have some discussion about this, and how to do it.
There are several alternatives.
Some of the ideas were:
1. Keep the static data and clone it + populate the adis_timeout data as
needed during probe [based on each device's chip-info]
2. Rework all the chip-info data to include the adis_data types/info
2. may require more work ; 1. require fewer patches
This implementation [in this series] is 1. but without keeping the static
data and template.
I guess the idea was to reduce memory usage [by keeping the static data]. I
admit the memory usage is not that big.
I'll take a look at this again, and see if 2. can work more nicely.
It might be that 1. would be the end-result, but who knows?
Thanks
Alex
> 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;
> >
