| Message ID | 20191213160312.1834-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] iio: gyro: adi16136: construct adis data on probe vs static on driver | expand |
On Fri, 13 Dec 2019 18:03:08 +0200 Alexandru Ardelean <alexandru.ardelean@analog.com> 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@analog.com> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> 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; >
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@analog.com> 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@analog.com> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > 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; > >
Hi all, On Mon, 2019-12-16 at 07:49 +0000, Ardelean, Alexandru wrote: > On Sun, 2019-12-15 at 16:18 +0000, Jonathan Cameron wrote: > > > > On Fri, 13 Dec 2019 18:03:08 +0200 > > Alexandru Ardelean <alexandru.ardelean@analog.com> 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@analog.com> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > 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 I guess we also need to prepare/send the following patches to show Jonathan why we need to dynamically allocate the data structure in some drivers. In the end is because some devices require different timeouts (handled by the adis core library) than the others and, in some cases these differences are quite significative. It was even happening that in some cases, we were not sleeping enough time (eg: after a reset command). In the next patches, a timeout structure is included that needs to be filled for each device. Alex, maybe we should include more patches in this series to show the "big picture" and then we can discuss if this is the best approach. Nuno Sá
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;