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

[Ardelean, Alexandru]

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