When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
assignment:
'indio_dev->name = spi_get_device_id(spi)->name;'
After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
during probe with null de-ref.
This change fixes this by introducing an ad7192_chip_info struct, and
defines all part-names [that should be assigned to indio_dev->name] in a
'ad7192_chip_info_tbl' table.
With this change, the old 'st->devid' is also moved to be a
'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
values in the new 'ad7192_chip_info_tbl'.
This should fix the bug, and maintain the ABI for the 'indio_dev->name'
field.
Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
Signed-off-by: Alexandru Ardelean <[email protected]>
---
Changelog v2 -> v3:
* reworked patch to introduce a chip_info struct for the part-name
* added 2nd patch to move of-table closer to the end of the file; this
patch is more cosmetic; has no fixes tag, but is on top of the previous
Changelog v1 -> v2:
* fix colon for Fixes tag
* updated commit title a bit; to make it longer
drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
1 file changed, 46 insertions(+), 15 deletions(-)
diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 8ec28aa8fa8a..7e8662c5cb0e 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -125,10 +125,10 @@
#define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
/* ID Register Bit Designations (AD7192_REG_ID) */
-#define ID_AD7190 0x4
-#define ID_AD7192 0x0
-#define ID_AD7193 0x2
-#define ID_AD7195 0x6
+#define CHIPID_AD7190 0x4
+#define CHIPID_AD7192 0x0
+#define CHIPID_AD7193 0x2
+#define CHIPID_AD7195 0x6
#define AD7192_ID_MASK 0x0F
/* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
@@ -161,7 +161,20 @@ enum {
AD7192_SYSCALIB_FULL_SCALE,
};
+enum {
+ ID_AD7190,
+ ID_AD7192,
+ ID_AD7193,
+ ID_AD7195,
+};
+
+struct ad7192_chip_info {
+ unsigned int chip_id;
+ const char *name;
+};
+
struct ad7192_state {
+ const struct ad7192_chip_info *chip_info;
struct regulator *avdd;
struct regulator *dvdd;
struct clk *mclk;
@@ -172,7 +185,6 @@ struct ad7192_state {
u32 conf;
u32 scale_avail[8][2];
u8 gpocon;
- u8 devid;
u8 clock_sel;
struct mutex lock; /* protect sensor state */
u8 syscalib_mode[8];
@@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
id &= AD7192_ID_MASK;
- if (id != st->devid)
+ if (id != st->chip_info->chip_id)
dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
id);
@@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
st->mode |= AD7192_MODE_REJ60;
refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
- if (refin2_en && st->devid != ID_AD7195)
+ if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
st->conf |= AD7192_CONF_REFSEL;
st->conf &= ~AD7192_CONF_CHOP;
@@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(14),
};
+static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
+ [ID_AD7190] = {
+ .chip_id = CHIPID_AD7190,
+ .name = "ad7190",
+ },
+ [ID_AD7192] = {
+ .chip_id = CHIPID_AD7192,
+ .name = "ad7192",
+ },
+ [ID_AD7193] = {
+ .chip_id = CHIPID_AD7193,
+ .name = "ad7193",
+ },
+ [ID_AD7195] = {
+ .chip_id = CHIPID_AD7195,
+ .name = "ad7195",
+ },
+};
+
static int ad7192_channels_config(struct iio_dev *indio_dev)
{
struct ad7192_state *st = iio_priv(indio_dev);
- switch (st->devid) {
+ switch (st->chip_info->chip_id) {
case ID_AD7193:
indio_dev->channels = ad7193_channels;
indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
@@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
}
static const struct of_device_id ad7192_of_match[] = {
- { .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
- { .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
- { .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
- { .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
+ { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
+ { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
+ { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
+ { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
{}
};
MODULE_DEVICE_TABLE(of, ad7192_of_match);
@@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
}
spi_set_drvdata(spi, indio_dev);
- st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
+ st->chip_info = of_device_get_match_data(&spi->dev);
indio_dev->dev.parent = &spi->dev;
- indio_dev->name = spi_get_device_id(spi)->name;
+ indio_dev->name = st->chip_info->name;
indio_dev->modes = INDIO_DIRECT_MODE;
ret = ad7192_channels_config(indio_dev);
if (ret < 0)
goto error_disable_dvdd;
- if (st->devid == ID_AD7195)
+ if (st->chip_info->chip_id == CHIPID_AD7195)
indio_dev->info = &ad7195_info;
else
indio_dev->info = &ad7192_info;
--
2.17.1
On Mon, 13 Apr 2020 11:20:43 +0300
Alexandru Ardelean <[email protected]> wrote:
> When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
> assignment:
> 'indio_dev->name = spi_get_device_id(spi)->name;'
>
> After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
> during probe with null de-ref.
>
> This change fixes this by introducing an ad7192_chip_info struct, and
> defines all part-names [that should be assigned to indio_dev->name] in a
> 'ad7192_chip_info_tbl' table.
>
> With this change, the old 'st->devid' is also moved to be a
> 'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
> 'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
> values in the new 'ad7192_chip_info_tbl'.
>
> This should fix the bug, and maintain the ABI for the 'indio_dev->name'
> field.
>
> Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
> Signed-off-by: Alexandru Ardelean <[email protected]>
Applied to the fixes-togreg branch of iio.git.
Thanks,
Jonathan
> ---
>
> Changelog v2 -> v3:
> * reworked patch to introduce a chip_info struct for the part-name
> * added 2nd patch to move of-table closer to the end of the file; this
> patch is more cosmetic; has no fixes tag, but is on top of the previous
>
> Changelog v1 -> v2:
> * fix colon for Fixes tag
> * updated commit title a bit; to make it longer
>
> drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
> 1 file changed, 46 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 8ec28aa8fa8a..7e8662c5cb0e 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -125,10 +125,10 @@
> #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
>
> /* ID Register Bit Designations (AD7192_REG_ID) */
> -#define ID_AD7190 0x4
> -#define ID_AD7192 0x0
> -#define ID_AD7193 0x2
> -#define ID_AD7195 0x6
> +#define CHIPID_AD7190 0x4
> +#define CHIPID_AD7192 0x0
> +#define CHIPID_AD7193 0x2
> +#define CHIPID_AD7195 0x6
> #define AD7192_ID_MASK 0x0F
>
> /* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
> @@ -161,7 +161,20 @@ enum {
> AD7192_SYSCALIB_FULL_SCALE,
> };
>
> +enum {
> + ID_AD7190,
> + ID_AD7192,
> + ID_AD7193,
> + ID_AD7195,
> +};
> +
> +struct ad7192_chip_info {
> + unsigned int chip_id;
> + const char *name;
> +};
> +
> struct ad7192_state {
> + const struct ad7192_chip_info *chip_info;
> struct regulator *avdd;
> struct regulator *dvdd;
> struct clk *mclk;
> @@ -172,7 +185,6 @@ struct ad7192_state {
> u32 conf;
> u32 scale_avail[8][2];
> u8 gpocon;
> - u8 devid;
> u8 clock_sel;
> struct mutex lock; /* protect sensor state */
> u8 syscalib_mode[8];
> @@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
>
> id &= AD7192_ID_MASK;
>
> - if (id != st->devid)
> + if (id != st->chip_info->chip_id)
> dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
> id);
>
> @@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> st->mode |= AD7192_MODE_REJ60;
>
> refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
> - if (refin2_en && st->devid != ID_AD7195)
> + if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
> st->conf |= AD7192_CONF_REFSEL;
>
> st->conf &= ~AD7192_CONF_CHOP;
> @@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] = {
> IIO_CHAN_SOFT_TIMESTAMP(14),
> };
>
> +static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> + [ID_AD7190] = {
> + .chip_id = CHIPID_AD7190,
> + .name = "ad7190",
> + },
> + [ID_AD7192] = {
> + .chip_id = CHIPID_AD7192,
> + .name = "ad7192",
> + },
> + [ID_AD7193] = {
> + .chip_id = CHIPID_AD7193,
> + .name = "ad7193",
> + },
> + [ID_AD7195] = {
> + .chip_id = CHIPID_AD7195,
> + .name = "ad7195",
> + },
> +};
> +
> static int ad7192_channels_config(struct iio_dev *indio_dev)
> {
> struct ad7192_state *st = iio_priv(indio_dev);
>
> - switch (st->devid) {
> + switch (st->chip_info->chip_id) {
> case ID_AD7193:
> indio_dev->channels = ad7193_channels;
> indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
> @@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
> }
>
> static const struct of_device_id ad7192_of_match[] = {
> - { .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
> - { .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
> - { .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
> - { .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
> + { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> + { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> + { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> + { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
> {}
> };
> MODULE_DEVICE_TABLE(of, ad7192_of_match);
> @@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
> }
>
> spi_set_drvdata(spi, indio_dev);
> - st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
> + st->chip_info = of_device_get_match_data(&spi->dev);
> indio_dev->dev.parent = &spi->dev;
> - indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->name = st->chip_info->name;
> indio_dev->modes = INDIO_DIRECT_MODE;
>
> ret = ad7192_channels_config(indio_dev);
> if (ret < 0)
> goto error_disable_dvdd;
>
> - if (st->devid == ID_AD7195)
> + if (st->chip_info->chip_id == CHIPID_AD7195)
> indio_dev->info = &ad7195_info;
> else
> indio_dev->info = &ad7192_info;
On Mon, Apr 13, 2020 at 11:20:43AM +0300, Alexandru Ardelean wrote:
> When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
> assignment:
> 'indio_dev->name = spi_get_device_id(spi)->name;'
>
> After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
> during probe with null de-ref.
>
> This change fixes this by introducing an ad7192_chip_info struct, and
> defines all part-names [that should be assigned to indio_dev->name] in a
> 'ad7192_chip_info_tbl' table.
>
> With this change, the old 'st->devid' is also moved to be a
> 'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
> 'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
> values in the new 'ad7192_chip_info_tbl'.
>
> This should fix the bug, and maintain the ABI for the 'indio_dev->name'
> field.
>
> Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
> Signed-off-by: Alexandru Ardelean <[email protected]>
> ---
>
> Changelog v2 -> v3:
> * reworked patch to introduce a chip_info struct for the part-name
> * added 2nd patch to move of-table closer to the end of the file; this
> patch is more cosmetic; has no fixes tag, but is on top of the previous
>
> Changelog v1 -> v2:
> * fix colon for Fixes tag
> * updated commit title a bit; to make it longer
>
> drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
> 1 file changed, 46 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 8ec28aa8fa8a..7e8662c5cb0e 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -125,10 +125,10 @@
> #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
>
> /* ID Register Bit Designations (AD7192_REG_ID) */
> -#define ID_AD7190 0x4
> -#define ID_AD7192 0x0
> -#define ID_AD7193 0x2
> -#define ID_AD7195 0x6
> +#define CHIPID_AD7190 0x4
> +#define CHIPID_AD7192 0x0
> +#define CHIPID_AD7193 0x2
> +#define CHIPID_AD7195 0x6
> #define AD7192_ID_MASK 0x0F
>
> /* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
> @@ -161,7 +161,20 @@ enum {
> AD7192_SYSCALIB_FULL_SCALE,
> };
>
> +enum {
> + ID_AD7190,
> + ID_AD7192,
> + ID_AD7193,
> + ID_AD7195,
> +};
> +
> +struct ad7192_chip_info {
> + unsigned int chip_id;
> + const char *name;
> +};
> +
> struct ad7192_state {
> + const struct ad7192_chip_info *chip_info;
> struct regulator *avdd;
> struct regulator *dvdd;
> struct clk *mclk;
> @@ -172,7 +185,6 @@ struct ad7192_state {
> u32 conf;
> u32 scale_avail[8][2];
> u8 gpocon;
> - u8 devid;
> u8 clock_sel;
> struct mutex lock; /* protect sensor state */
> u8 syscalib_mode[8];
> @@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
>
> id &= AD7192_ID_MASK;
>
> - if (id != st->devid)
> + if (id != st->chip_info->chip_id)
> dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
> id);
>
> @@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> st->mode |= AD7192_MODE_REJ60;
>
> refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
> - if (refin2_en && st->devid != ID_AD7195)
> + if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
> st->conf |= AD7192_CONF_REFSEL;
>
> st->conf &= ~AD7192_CONF_CHOP;
> @@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] = {
> IIO_CHAN_SOFT_TIMESTAMP(14),
> };
>
> +static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> + [ID_AD7190] = {
> + .chip_id = CHIPID_AD7190,
> + .name = "ad7190",
> + },
> + [ID_AD7192] = {
> + .chip_id = CHIPID_AD7192,
> + .name = "ad7192",
> + },
> + [ID_AD7193] = {
> + .chip_id = CHIPID_AD7193,
> + .name = "ad7193",
> + },
> + [ID_AD7195] = {
> + .chip_id = CHIPID_AD7195,
> + .name = "ad7195",
> + },
> +};
> +
> static int ad7192_channels_config(struct iio_dev *indio_dev)
> {
> struct ad7192_state *st = iio_priv(indio_dev);
>
> - switch (st->devid) {
> + switch (st->chip_info->chip_id) {
> case ID_AD7193:
Would it be better to change this to CHIPID_AD7193? It looks like it works
the way you have it, but only by coincidence since it happens to be true
that ID_AD7193 == CHIPID_AD7193.
Jeremy
> indio_dev->channels = ad7193_channels;
> indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
> @@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
> }
>
> static const struct of_device_id ad7192_of_match[] = {
> - { .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
> - { .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
> - { .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
> - { .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
> + { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> + { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> + { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> + { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
> {}
> };
> MODULE_DEVICE_TABLE(of, ad7192_of_match);
> @@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
> }
>
> spi_set_drvdata(spi, indio_dev);
> - st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
> + st->chip_info = of_device_get_match_data(&spi->dev);
> indio_dev->dev.parent = &spi->dev;
> - indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->name = st->chip_info->name;
> indio_dev->modes = INDIO_DIRECT_MODE;
>
> ret = ad7192_channels_config(indio_dev);
> if (ret < 0)
> goto error_disable_dvdd;
>
> - if (st->devid == ID_AD7195)
> + if (st->chip_info->chip_id == CHIPID_AD7195)
> indio_dev->info = &ad7195_info;
> else
> indio_dev->info = &ad7192_info;
> --
> 2.17.1
>
On Mon, 13 Apr 2020 16:06:31 -0600
Jeremy Fertic <[email protected]> wrote:
> On Mon, Apr 13, 2020 at 11:20:43AM +0300, Alexandru Ardelean wrote:
> > When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
> > assignment:
> > 'indio_dev->name = spi_get_device_id(spi)->name;'
> >
> > After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
> > during probe with null de-ref.
> >
> > This change fixes this by introducing an ad7192_chip_info struct, and
> > defines all part-names [that should be assigned to indio_dev->name] in a
> > 'ad7192_chip_info_tbl' table.
> >
> > With this change, the old 'st->devid' is also moved to be a
> > 'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
> > 'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
> > values in the new 'ad7192_chip_info_tbl'.
> >
> > This should fix the bug, and maintain the ABI for the 'indio_dev->name'
> > field.
> >
> > Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
> > Signed-off-by: Alexandru Ardelean <[email protected]>
> > ---
> >
> > Changelog v2 -> v3:
> > * reworked patch to introduce a chip_info struct for the part-name
> > * added 2nd patch to move of-table closer to the end of the file; this
> > patch is more cosmetic; has no fixes tag, but is on top of the previous
> >
> > Changelog v1 -> v2:
> > * fix colon for Fixes tag
> > * updated commit title a bit; to make it longer
> >
> > drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
> > 1 file changed, 46 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 8ec28aa8fa8a..7e8662c5cb0e 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -125,10 +125,10 @@
> > #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
> >
> > /* ID Register Bit Designations (AD7192_REG_ID) */
> > -#define ID_AD7190 0x4
> > -#define ID_AD7192 0x0
> > -#define ID_AD7193 0x2
> > -#define ID_AD7195 0x6
> > +#define CHIPID_AD7190 0x4
> > +#define CHIPID_AD7192 0x0
> > +#define CHIPID_AD7193 0x2
> > +#define CHIPID_AD7195 0x6
> > #define AD7192_ID_MASK 0x0F
> >
> > /* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
> > @@ -161,7 +161,20 @@ enum {
> > AD7192_SYSCALIB_FULL_SCALE,
> > };
> >
> > +enum {
> > + ID_AD7190,
> > + ID_AD7192,
> > + ID_AD7193,
> > + ID_AD7195,
> > +};
> > +
> > +struct ad7192_chip_info {
> > + unsigned int chip_id;
> > + const char *name;
> > +};
> > +
> > struct ad7192_state {
> > + const struct ad7192_chip_info *chip_info;
> > struct regulator *avdd;
> > struct regulator *dvdd;
> > struct clk *mclk;
> > @@ -172,7 +185,6 @@ struct ad7192_state {
> > u32 conf;
> > u32 scale_avail[8][2];
> > u8 gpocon;
> > - u8 devid;
> > u8 clock_sel;
> > struct mutex lock; /* protect sensor state */
> > u8 syscalib_mode[8];
> > @@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> >
> > id &= AD7192_ID_MASK;
> >
> > - if (id != st->devid)
> > + if (id != st->chip_info->chip_id)
> > dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
> > id);
> >
> > @@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> > st->mode |= AD7192_MODE_REJ60;
> >
> > refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
> > - if (refin2_en && st->devid != ID_AD7195)
> > + if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
> > st->conf |= AD7192_CONF_REFSEL;
> >
> > st->conf &= ~AD7192_CONF_CHOP;
> > @@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] = {
> > IIO_CHAN_SOFT_TIMESTAMP(14),
> > };
> >
> > +static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> > + [ID_AD7190] = {
> > + .chip_id = CHIPID_AD7190,
> > + .name = "ad7190",
> > + },
> > + [ID_AD7192] = {
> > + .chip_id = CHIPID_AD7192,
> > + .name = "ad7192",
> > + },
> > + [ID_AD7193] = {
> > + .chip_id = CHIPID_AD7193,
> > + .name = "ad7193",
> > + },
> > + [ID_AD7195] = {
> > + .chip_id = CHIPID_AD7195,
> > + .name = "ad7195",
> > + },
> > +};
> > +
> > static int ad7192_channels_config(struct iio_dev *indio_dev)
> > {
> > struct ad7192_state *st = iio_priv(indio_dev);
> >
> > - switch (st->devid) {
> > + switch (st->chip_info->chip_id) {
> > case ID_AD7193:
>
> Would it be better to change this to CHIPID_AD7193? It looks like it works
> the way you have it, but only by coincidence since it happens to be true
> that ID_AD7193 == CHIPID_AD7193.
>
dropped patch from my fixes branch whilst this is resolved!
Thanks,
Jonathan
> Jeremy
>
> > indio_dev->channels = ad7193_channels;
> > indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
> > @@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev *indio_dev)
> > }
> >
> > static const struct of_device_id ad7192_of_match[] = {
> > - { .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
> > - { .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
> > - { .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
> > - { .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
> > + { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> > + { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> > + { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> > + { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
> > {}
> > };
> > MODULE_DEVICE_TABLE(of, ad7192_of_match);
> > @@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
> > }
> >
> > spi_set_drvdata(spi, indio_dev);
> > - st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
> > + st->chip_info = of_device_get_match_data(&spi->dev);
> > indio_dev->dev.parent = &spi->dev;
> > - indio_dev->name = spi_get_device_id(spi)->name;
> > + indio_dev->name = st->chip_info->name;
> > indio_dev->modes = INDIO_DIRECT_MODE;
> >
> > ret = ad7192_channels_config(indio_dev);
> > if (ret < 0)
> > goto error_disable_dvdd;
> >
> > - if (st->devid == ID_AD7195)
> > + if (st->chip_info->chip_id == CHIPID_AD7195)
> > indio_dev->info = &ad7195_info;
> > else
> > indio_dev->info = &ad7192_info;
> > --
> > 2.17.1
> >
On Mon, 2020-04-13 at 16:06 -0600, Jeremy Fertic wrote:
> [External]
>
> On Mon, Apr 13, 2020 at 11:20:43AM +0300, Alexandru Ardelean wrote:
> > When the 'spi_device_id' table was removed, it omitted to cleanup/fix the
> > assignment:
> > 'indio_dev->name = spi_get_device_id(spi)->name;'
> >
> > After that patch 'spi_get_device_id(spi)' returns NULL, so this crashes
> > during probe with null de-ref.
> >
> > This change fixes this by introducing an ad7192_chip_info struct, and
> > defines all part-names [that should be assigned to indio_dev->name] in a
> > 'ad7192_chip_info_tbl' table.
> >
> > With this change, the old 'st->devid' is also moved to be a
> > 'chip_info->chip_id'. And the old 'ID_AD719X' macros have been renamed to
> > 'CHIPID_AD719X'. Tld identifiers have been re-purposed to be enum/index
> > values in the new 'ad7192_chip_info_tbl'.
> >
> > This should fix the bug, and maintain the ABI for the 'indio_dev->name'
> > field.
> >
> > Fixes: 66614ab2be38 ("staging: iio: adc: ad7192: removed spi_device_id")
> > Signed-off-by: Alexandru Ardelean <[email protected]>
> > ---
> >
> > Changelog v2 -> v3:
> > * reworked patch to introduce a chip_info struct for the part-name
> > * added 2nd patch to move of-table closer to the end of the file; this
> > patch is more cosmetic; has no fixes tag, but is on top of the previous
> >
> > Changelog v1 -> v2:
> > * fix colon for Fixes tag
> > * updated commit title a bit; to make it longer
> >
> > drivers/iio/adc/ad7192.c | 61 ++++++++++++++++++++++++++++++----------
> > 1 file changed, 46 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 8ec28aa8fa8a..7e8662c5cb0e 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -125,10 +125,10 @@
> > #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
> >
> > /* ID Register Bit Designations (AD7192_REG_ID) */
> > -#define ID_AD7190 0x4
> > -#define ID_AD7192 0x0
> > -#define ID_AD7193 0x2
> > -#define ID_AD7195 0x6
> > +#define CHIPID_AD7190 0x4
> > +#define CHIPID_AD7192 0x0
> > +#define CHIPID_AD7193 0x2
> > +#define CHIPID_AD7195 0x6
> > #define AD7192_ID_MASK 0x0F
> >
> > /* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
> > @@ -161,7 +161,20 @@ enum {
> > AD7192_SYSCALIB_FULL_SCALE,
> > };
> >
> > +enum {
> > + ID_AD7190,
> > + ID_AD7192,
> > + ID_AD7193,
> > + ID_AD7195,
> > +};
> > +
> > +struct ad7192_chip_info {
> > + unsigned int chip_id;
> > + const char *name;
> > +};
> > +
> > struct ad7192_state {
> > + const struct ad7192_chip_info *chip_info;
> > struct regulator *avdd;
> > struct regulator *dvdd;
> > struct clk *mclk;
> > @@ -172,7 +185,6 @@ struct ad7192_state {
> > u32 conf;
> > u32 scale_avail[8][2];
> > u8 gpocon;
> > - u8 devid;
> > u8 clock_sel;
> > struct mutex lock; /* protect sensor state */
> > u8 syscalib_mode[8];
> > @@ -348,7 +360,7 @@ static int ad7192_setup(struct ad7192_state *st, struct
> > device_node *np)
> >
> > id &= AD7192_ID_MASK;
> >
> > - if (id != st->devid)
> > + if (id != st->chip_info->chip_id)
> > dev_warn(&st->sd.spi->dev, "device ID query failed (0x%X)\n",
> > id);
> >
> > @@ -363,7 +375,7 @@ static int ad7192_setup(struct ad7192_state *st, struct
> > device_node *np)
> > st->mode |= AD7192_MODE_REJ60;
> >
> > refin2_en = of_property_read_bool(np, "adi,refin2-pins-enable");
> > - if (refin2_en && st->devid != ID_AD7195)
> > + if (refin2_en && st->chip_info->chip_id != CHIPID_AD7195)
> > st->conf |= AD7192_CONF_REFSEL;
> >
> > st->conf &= ~AD7192_CONF_CHOP;
> > @@ -859,11 +871,30 @@ static const struct iio_chan_spec ad7193_channels[] =
> > {
> > IIO_CHAN_SOFT_TIMESTAMP(14),
> > };
> >
> > +static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> > + [ID_AD7190] = {
> > + .chip_id = CHIPID_AD7190,
> > + .name = "ad7190",
> > + },
> > + [ID_AD7192] = {
> > + .chip_id = CHIPID_AD7192,
> > + .name = "ad7192",
> > + },
> > + [ID_AD7193] = {
> > + .chip_id = CHIPID_AD7193,
> > + .name = "ad7193",
> > + },
> > + [ID_AD7195] = {
> > + .chip_id = CHIPID_AD7195,
> > + .name = "ad7195",
> > + },
> > +};
> > +
> > static int ad7192_channels_config(struct iio_dev *indio_dev)
> > {
> > struct ad7192_state *st = iio_priv(indio_dev);
> >
> > - switch (st->devid) {
> > + switch (st->chip_info->chip_id) {
> > case ID_AD7193:
>
> Would it be better to change this to CHIPID_AD7193? It looks like it works
> the way you have it, but only by coincidence since it happens to be true
> that ID_AD7193 == CHIPID_AD7193.
Thanks for catching this.
I'll fix up in a V3.
>
> Jeremy
>
> > indio_dev->channels = ad7193_channels;
> > indio_dev->num_channels = ARRAY_SIZE(ad7193_channels);
> > @@ -878,10 +909,10 @@ static int ad7192_channels_config(struct iio_dev
> > *indio_dev)
> > }
> >
> > static const struct of_device_id ad7192_of_match[] = {
> > - { .compatible = "adi,ad7190", .data = (void *)ID_AD7190 },
> > - { .compatible = "adi,ad7192", .data = (void *)ID_AD7192 },
> > - { .compatible = "adi,ad7193", .data = (void *)ID_AD7193 },
> > - { .compatible = "adi,ad7195", .data = (void *)ID_AD7195 },
> > + { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190]
> > },
> > + { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192]
> > },
> > + { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193]
> > },
> > + { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195]
> > },
> > {}
> > };
> > MODULE_DEVICE_TABLE(of, ad7192_of_match);
> > @@ -938,16 +969,16 @@ static int ad7192_probe(struct spi_device *spi)
> > }
> >
> > spi_set_drvdata(spi, indio_dev);
> > - st->devid = (unsigned long)of_device_get_match_data(&spi->dev);
> > + st->chip_info = of_device_get_match_data(&spi->dev);
> > indio_dev->dev.parent = &spi->dev;
> > - indio_dev->name = spi_get_device_id(spi)->name;
> > + indio_dev->name = st->chip_info->name;
> > indio_dev->modes = INDIO_DIRECT_MODE;
> >
> > ret = ad7192_channels_config(indio_dev);
> > if (ret < 0)
> > goto error_disable_dvdd;
> >
> > - if (st->devid == ID_AD7195)
> > + if (st->chip_info->chip_id == CHIPID_AD7195)
> > indio_dev->info = &ad7195_info;
> > else
> > indio_dev->info = &ad7192_info;
> > --
> > 2.17.1
> >