* [PATCH 0/2] iio: ti-dac5571: Add TI DAC081C081 support @ 2021-07-23 18:31 Laurent Pinchart 2021-07-23 18:31 ` [PATCH 1/2] dt-bindings: iio: dac: ti,dac5571: " Laurent Pinchart 2021-07-23 18:31 ` [PATCH 2/2] iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart 0 siblings, 2 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-23 18:31 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c, Wolfram Sang Hello, This small patch series adds support for the TI DAC081C081 in the ti-dac5571 IIO driver. Patch 1/2 addresses the DT bindings, and patch 2/2 the driver. I've expanded the CC list to the I2C mailing list and Wolfram because I think the second patch may not be the best option. Instead of addressing this issue in all drivers, it wonder if it wouldn't be better for the I2C subsystem to handle this internally. One option would be to extend i2c_match_id to walk through the compatible values in case of an OF-based match, but I'm worried this could introduce regressions. Feedback would be welcome. The series could still be merged as-is in the meantime if desired. Jose Cazarin (1): iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart (1): dt-bindings: iio: dac: ti,dac5571: Add TI DAC081C081 support .../bindings/iio/dac/ti,dac5571.yaml | 24 ++++++++++------- drivers/iio/dac/ti-dac5571.c | 27 ++++++++++++------- 2 files changed, 31 insertions(+), 20 deletions(-) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] dt-bindings: iio: dac: ti,dac5571: Add TI DAC081C081 support 2021-07-23 18:31 [PATCH 0/2] iio: ti-dac5571: Add TI DAC081C081 support Laurent Pinchart @ 2021-07-23 18:31 ` Laurent Pinchart 2021-07-29 21:26 ` Rob Herring 2021-07-23 18:31 ` [PATCH 2/2] iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart 1 sibling, 1 reply; 20+ messages in thread From: Laurent Pinchart @ 2021-07-23 18:31 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c, Wolfram Sang The TI DAC081C081 is compatible with the TI DAC5571 from a software point of view. Add a device-specific compatible string value with a fallback to "ti,dac5571". Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- .../bindings/iio/dac/ti,dac5571.yaml | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/Documentation/devicetree/bindings/iio/dac/ti,dac5571.yaml b/Documentation/devicetree/bindings/iio/dac/ti,dac5571.yaml index 714191724f7c..cdbbb336a5b5 100644 --- a/Documentation/devicetree/bindings/iio/dac/ti,dac5571.yaml +++ b/Documentation/devicetree/bindings/iio/dac/ti,dac5571.yaml @@ -11,16 +11,20 @@ maintainers: properties: compatible: - enum: - - ti,dac5571 - - ti,dac6571 - - ti,dac7571 - - ti,dac5574 - - ti,dac6574 - - ti,dac7574 - - ti,dac5573 - - ti,dac6573 - - ti,dac7573 + oneOf: + - enum: + - ti,dac5571 + - ti,dac6571 + - ti,dac7571 + - ti,dac5574 + - ti,dac6574 + - ti,dac7574 + - ti,dac5573 + - ti,dac6573 + - ti,dac7573 + - items: + - const: ti,dac081c081 + - const: ti,dac5571 reg: maxItems: 1 -- Regards, Laurent Pinchart ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: dac: ti,dac5571: Add TI DAC081C081 support 2021-07-23 18:31 ` [PATCH 1/2] dt-bindings: iio: dac: ti,dac5571: " Laurent Pinchart @ 2021-07-29 21:26 ` Rob Herring 0 siblings, 0 replies; 20+ messages in thread From: Rob Herring @ 2021-07-29 21:26 UTC (permalink / raw) To: Laurent Pinchart Cc: Rob Herring, linux-iio, Sean Nyekjaer, Jonathan Cameron, devicetree, Lars-Peter Clausen, linux-i2c, Wolfram Sang, Jose Cazarin On Fri, 23 Jul 2021 21:31:13 +0300, Laurent Pinchart wrote: > The TI DAC081C081 is compatible with the TI DAC5571 from a software > point of view. Add a device-specific compatible string value with a > fallback to "ti,dac5571". > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../bindings/iio/dac/ti,dac5571.yaml | 24 +++++++++++-------- > 1 file changed, 14 insertions(+), 10 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-07-23 18:31 [PATCH 0/2] iio: ti-dac5571: Add TI DAC081C081 support Laurent Pinchart 2021-07-23 18:31 ` [PATCH 1/2] dt-bindings: iio: dac: ti,dac5571: " Laurent Pinchart @ 2021-07-23 18:31 ` Laurent Pinchart 2021-07-23 23:06 ` kernel test robot ` (2 more replies) 1 sibling, 3 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-23 18:31 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c, Wolfram Sang From: Jose Cazarin <joseespiriki@gmail.com> When matching an OF device, the match mechanism tries all components of the compatible property. This can result with a device matched with a compatible string that isn't the first in the compatible list. For instance, with a compatible property set to compatible = "ti,dac081c081", "ti,dac5571"; the driver will match the second compatible string, as the first one isn't listed in the of_device_id table. The device will however be named "dac081c081" by the I2C core. This causes an issue when identifying the chip. The probe function receives a i2c_device_id that comes from the module's I2C device ID table. There is no entry in that table for "dac081c081", which results in a NULL pointer passed to the probe function. To fix this, add chip_id information in the data field of the OF device ID table, and retrieve it with of_device_get_match_data() for OF devices. Signed-off-by: Jose Cazarin <joseespiriki@gmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/iio/dac/ti-dac5571.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c index 2a5ba1b08a1d..bd005b3a976b 100644 --- a/drivers/iio/dac/ti-dac5571.c +++ b/drivers/iio/dac/ti-dac5571.c @@ -311,6 +311,7 @@ static int dac5571_probe(struct i2c_client *client, const struct dac5571_spec *spec; struct dac5571_data *data; struct iio_dev *indio_dev; + enum chip_id chip_id; int ret, i; indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); @@ -326,7 +327,13 @@ static int dac5571_probe(struct i2c_client *client, indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = dac5571_channels; - spec = &dac5571_spec[id->driver_data]; + if (dev->of_node) + chip_id = (uintptr_t)of_device_get_match_data(dev); + else + chip_id = id->driver_data; + + spec = &dac5571_spec[chip_id]; + indio_dev->num_channels = spec->num_channels; data->spec = spec; @@ -384,15 +391,15 @@ static int dac5571_remove(struct i2c_client *i2c) } static const struct of_device_id dac5571_of_id[] = { - {.compatible = "ti,dac5571"}, - {.compatible = "ti,dac6571"}, - {.compatible = "ti,dac7571"}, - {.compatible = "ti,dac5574"}, - {.compatible = "ti,dac6574"}, - {.compatible = "ti,dac7574"}, - {.compatible = "ti,dac5573"}, - {.compatible = "ti,dac6573"}, - {.compatible = "ti,dac7573"}, + {.compatible = "ti,dac5571", .data = (void *)single_8bit}, + {.compatible = "ti,dac6571", .data = (void *)single_10bit}, + {.compatible = "ti,dac7571", .data = (void *)single_12bit}, + {.compatible = "ti,dac5574", .data = (void *)quad_8bit}, + {.compatible = "ti,dac6574", .data = (void *)quad_10bit}, + {.compatible = "ti,dac7574", .data = (void *)quad_12bit}, + {.compatible = "ti,dac5573", .data = (void *)quad_8bit}, + {.compatible = "ti,dac6573", .data = (void *)quad_10bit}, + {.compatible = "ti,dac7573", .data = (void *)quad_12bit}, {} }; MODULE_DEVICE_TABLE(of, dac5571_of_id); -- Regards, Laurent Pinchart ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-07-23 18:31 ` [PATCH 2/2] iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart @ 2021-07-23 23:06 ` kernel test robot 2021-07-23 23:06 ` kernel test robot 2021-07-24 0:06 ` [PATCH v1.1 " Laurent Pinchart 2 siblings, 0 replies; 20+ messages in thread From: kernel test robot @ 2021-07-23 23:06 UTC (permalink / raw) To: Laurent Pinchart, linux-iio Cc: kbuild-all, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c, Wolfram Sang [-- Attachment #1: Type: text/plain, Size: 4117 bytes --] Hi Laurent, Thank you for the patch! Yet something to improve: [auto build test ERROR on iio/togreg] [also build test ERROR on robh/for-next v5.14-rc2 next-20210723] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Laurent-Pinchart/iio-ti-dac5571-Add-TI-DAC081C081-support/20210724-023333 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg config: x86_64-randconfig-s022-20210723 (attached as .config) compiler: gcc-10 (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-341-g8af24329-dirty # https://github.com/0day-ci/linux/commit/aea544dbbcecf5d723ede9b42a2da945ac5038f9 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Laurent-Pinchart/iio-ti-dac5571-Add-TI-DAC081C081-support/20210724-023333 git checkout aea544dbbcecf5d723ede9b42a2da945ac5038f9 # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): drivers/iio/dac/ti-dac5571.c: In function 'dac5571_probe': >> drivers/iio/dac/ti-dac5571.c:331:24: error: implicit declaration of function 'of_device_get_match_data'; did you mean 'device_get_match_data'? [-Werror=implicit-function-declaration] 331 | chip_id = (uintptr_t)of_device_get_match_data(dev); | ^~~~~~~~~~~~~~~~~~~~~~~~ | device_get_match_data cc1: some warnings being treated as errors vim +331 drivers/iio/dac/ti-dac5571.c 306 307 static int dac5571_probe(struct i2c_client *client, 308 const struct i2c_device_id *id) 309 { 310 struct device *dev = &client->dev; 311 const struct dac5571_spec *spec; 312 struct dac5571_data *data; 313 struct iio_dev *indio_dev; 314 enum chip_id chip_id; 315 int ret, i; 316 317 indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); 318 if (!indio_dev) 319 return -ENOMEM; 320 321 data = iio_priv(indio_dev); 322 i2c_set_clientdata(client, indio_dev); 323 data->client = client; 324 325 indio_dev->info = &dac5571_info; 326 indio_dev->name = id->name; 327 indio_dev->modes = INDIO_DIRECT_MODE; 328 indio_dev->channels = dac5571_channels; 329 330 if (dev->of_node) > 331 chip_id = (uintptr_t)of_device_get_match_data(dev); 332 else 333 chip_id = id->driver_data; 334 335 spec = &dac5571_spec[chip_id]; 336 337 indio_dev->num_channels = spec->num_channels; 338 data->spec = spec; 339 340 data->vref = devm_regulator_get(dev, "vref"); 341 if (IS_ERR(data->vref)) 342 return PTR_ERR(data->vref); 343 344 ret = regulator_enable(data->vref); 345 if (ret < 0) 346 return ret; 347 348 mutex_init(&data->lock); 349 350 switch (spec->num_channels) { 351 case 1: 352 data->dac5571_cmd = dac5571_cmd_single; 353 data->dac5571_pwrdwn = dac5571_pwrdwn_single; 354 break; 355 case 4: 356 data->dac5571_cmd = dac5571_cmd_quad; 357 data->dac5571_pwrdwn = dac5571_pwrdwn_quad; 358 break; 359 default: 360 goto err; 361 } 362 363 for (i = 0; i < spec->num_channels; i++) { 364 ret = data->dac5571_cmd(data, i, 0); 365 if (ret) { 366 dev_err(dev, "failed to initialize channel %d to 0\n", i); 367 goto err; 368 } 369 } 370 371 ret = iio_device_register(indio_dev); 372 if (ret) 373 goto err; 374 375 return 0; 376 377 err: 378 regulator_disable(data->vref); 379 return ret; 380 } 381 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 33841 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-07-23 18:31 ` [PATCH 2/2] iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart 2021-07-23 23:06 ` kernel test robot @ 2021-07-23 23:06 ` kernel test robot 2021-07-24 0:06 ` [PATCH v1.1 " Laurent Pinchart 2 siblings, 0 replies; 20+ messages in thread From: kernel test robot @ 2021-07-23 23:06 UTC (permalink / raw) To: Laurent Pinchart, linux-iio Cc: kbuild-all, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c, Wolfram Sang [-- Attachment #1: Type: text/plain, Size: 4176 bytes --] Hi Laurent, Thank you for the patch! Yet something to improve: [auto build test ERROR on iio/togreg] [also build test ERROR on robh/for-next v5.14-rc2 next-20210723] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Laurent-Pinchart/iio-ti-dac5571-Add-TI-DAC081C081-support/20210724-023333 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg config: arc-randconfig-r012-20210723 (attached as .config) compiler: arceb-elf-gcc (GCC) 10.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/aea544dbbcecf5d723ede9b42a2da945ac5038f9 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Laurent-Pinchart/iio-ti-dac5571-Add-TI-DAC081C081-support/20210724-023333 git checkout aea544dbbcecf5d723ede9b42a2da945ac5038f9 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=arc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): drivers/iio/dac/ti-dac5571.c: In function 'dac5571_probe': >> drivers/iio/dac/ti-dac5571.c:331:24: error: implicit declaration of function 'of_device_get_match_data'; did you mean 'device_get_match_data'? [-Werror=implicit-function-declaration] 331 | chip_id = (uintptr_t)of_device_get_match_data(dev); | ^~~~~~~~~~~~~~~~~~~~~~~~ | device_get_match_data cc1: some warnings being treated as errors vim +331 drivers/iio/dac/ti-dac5571.c 306 307 static int dac5571_probe(struct i2c_client *client, 308 const struct i2c_device_id *id) 309 { 310 struct device *dev = &client->dev; 311 const struct dac5571_spec *spec; 312 struct dac5571_data *data; 313 struct iio_dev *indio_dev; 314 enum chip_id chip_id; 315 int ret, i; 316 317 indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); 318 if (!indio_dev) 319 return -ENOMEM; 320 321 data = iio_priv(indio_dev); 322 i2c_set_clientdata(client, indio_dev); 323 data->client = client; 324 325 indio_dev->info = &dac5571_info; 326 indio_dev->name = id->name; 327 indio_dev->modes = INDIO_DIRECT_MODE; 328 indio_dev->channels = dac5571_channels; 329 330 if (dev->of_node) > 331 chip_id = (uintptr_t)of_device_get_match_data(dev); 332 else 333 chip_id = id->driver_data; 334 335 spec = &dac5571_spec[chip_id]; 336 337 indio_dev->num_channels = spec->num_channels; 338 data->spec = spec; 339 340 data->vref = devm_regulator_get(dev, "vref"); 341 if (IS_ERR(data->vref)) 342 return PTR_ERR(data->vref); 343 344 ret = regulator_enable(data->vref); 345 if (ret < 0) 346 return ret; 347 348 mutex_init(&data->lock); 349 350 switch (spec->num_channels) { 351 case 1: 352 data->dac5571_cmd = dac5571_cmd_single; 353 data->dac5571_pwrdwn = dac5571_pwrdwn_single; 354 break; 355 case 4: 356 data->dac5571_cmd = dac5571_cmd_quad; 357 data->dac5571_pwrdwn = dac5571_pwrdwn_quad; 358 break; 359 default: 360 goto err; 361 } 362 363 for (i = 0; i < spec->num_channels; i++) { 364 ret = data->dac5571_cmd(data, i, 0); 365 if (ret) { 366 dev_err(dev, "failed to initialize channel %d to 0\n", i); 367 goto err; 368 } 369 } 370 371 ret = iio_device_register(indio_dev); 372 if (ret) 373 goto err; 374 375 return 0; 376 377 err: 378 regulator_disable(data->vref); 379 return ret; 380 } 381 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 33497 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-07-23 18:31 ` [PATCH 2/2] iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart 2021-07-23 23:06 ` kernel test robot 2021-07-23 23:06 ` kernel test robot @ 2021-07-24 0:06 ` Laurent Pinchart 2021-07-24 14:43 ` Jonathan Cameron 2021-07-24 14:44 ` Jonathan Cameron 2 siblings, 2 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-24 0:06 UTC (permalink / raw) To: linux-iio Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c, Wolfram Sang From: Jose Cazarin <joseespiriki@gmail.com> When matching an OF device, the match mechanism tries all components of the compatible property. This can result with a device matched with a compatible string that isn't the first in the compatible list. For instance, with a compatible property set to compatible = "ti,dac081c081", "ti,dac5571"; the driver will match the second compatible string, as the first one isn't listed in the of_device_id table. The device will however be named "dac081c081" by the I2C core. This causes an issue when identifying the chip. The probe function receives a i2c_device_id that comes from the module's I2C device ID table. There is no entry in that table for "dac081c081", which results in a NULL pointer passed to the probe function. To fix this, add chip_id information in the data field of the OF device ID table, and retrieve it with of_device_get_match_data() for OF devices. Signed-off-by: Jose Cazarin <joseespiriki@gmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v1: - Include linux/of_device.h --- drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c index 2a5ba1b08a1d..8ceb1b42b14e 100644 --- a/drivers/iio/dac/ti-dac5571.c +++ b/drivers/iio/dac/ti-dac5571.c @@ -19,6 +19,7 @@ #include <linux/i2c.h> #include <linux/module.h> #include <linux/mod_devicetable.h> +#include <linux/of_device.h> #include <linux/regulator/consumer.h> enum chip_id { @@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client, const struct dac5571_spec *spec; struct dac5571_data *data; struct iio_dev *indio_dev; + enum chip_id chip_id; int ret, i; indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); @@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client, indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = dac5571_channels; - spec = &dac5571_spec[id->driver_data]; + if (dev->of_node) + chip_id = (uintptr_t)of_device_get_match_data(dev); + else + chip_id = id->driver_data; + + spec = &dac5571_spec[chip_id]; + indio_dev->num_channels = spec->num_channels; data->spec = spec; @@ -384,15 +392,15 @@ static int dac5571_remove(struct i2c_client *i2c) } static const struct of_device_id dac5571_of_id[] = { - {.compatible = "ti,dac5571"}, - {.compatible = "ti,dac6571"}, - {.compatible = "ti,dac7571"}, - {.compatible = "ti,dac5574"}, - {.compatible = "ti,dac6574"}, - {.compatible = "ti,dac7574"}, - {.compatible = "ti,dac5573"}, - {.compatible = "ti,dac6573"}, - {.compatible = "ti,dac7573"}, + {.compatible = "ti,dac5571", .data = (void *)single_8bit}, + {.compatible = "ti,dac6571", .data = (void *)single_10bit}, + {.compatible = "ti,dac7571", .data = (void *)single_12bit}, + {.compatible = "ti,dac5574", .data = (void *)quad_8bit}, + {.compatible = "ti,dac6574", .data = (void *)quad_10bit}, + {.compatible = "ti,dac7574", .data = (void *)quad_12bit}, + {.compatible = "ti,dac5573", .data = (void *)quad_8bit}, + {.compatible = "ti,dac6573", .data = (void *)quad_10bit}, + {.compatible = "ti,dac7573", .data = (void *)quad_12bit}, {} }; MODULE_DEVICE_TABLE(of, dac5571_of_id); -- Regards, Laurent Pinchart ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-07-24 0:06 ` [PATCH v1.1 " Laurent Pinchart @ 2021-07-24 14:43 ` Jonathan Cameron 2021-07-24 23:14 ` Laurent Pinchart 2021-08-17 20:44 ` Wolfram Sang 2021-07-24 14:44 ` Jonathan Cameron 1 sibling, 2 replies; 20+ messages in thread From: Jonathan Cameron @ 2021-07-24 14:43 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c, Wolfram Sang On Sat, 24 Jul 2021 03:06:54 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > From: Jose Cazarin <joseespiriki@gmail.com> > > When matching an OF device, the match mechanism tries all components of > the compatible property. This can result with a device matched with a > compatible string that isn't the first in the compatible list. For > instance, with a compatible property set to > > compatible = "ti,dac081c081", "ti,dac5571"; > > the driver will match the second compatible string, as the first one > isn't listed in the of_device_id table. The device will however be named > "dac081c081" by the I2C core. > > This causes an issue when identifying the chip. The probe function > receives a i2c_device_id that comes from the module's I2C device ID > table. There is no entry in that table for "dac081c081", which results > in a NULL pointer passed to the probe function. > > To fix this, add chip_id information in the data field of the OF device > ID table, and retrieve it with of_device_get_match_data() for OF > devices. > > Signed-off-by: Jose Cazarin <joseespiriki@gmail.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Interesting problem that I hadn't previously realised could happen. One request though, can we use device_get_match_data() here rather than the of specific version? Include property.h as well for that. That should allow the same issue with compatible to work correctly when using PRP0001 based ACPI methods. https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/acpi/bus.c#L891 Will result in acpi_of_device_get_match_data() being called which will match to the of_device_id table. Thanks, Jonathan > --- > Changes since v1: > > - Include linux/of_device.h > --- > drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c > index 2a5ba1b08a1d..8ceb1b42b14e 100644 > --- a/drivers/iio/dac/ti-dac5571.c > +++ b/drivers/iio/dac/ti-dac5571.c > @@ -19,6 +19,7 @@ > #include <linux/i2c.h> > #include <linux/module.h> > #include <linux/mod_devicetable.h> > +#include <linux/of_device.h> > #include <linux/regulator/consumer.h> > > enum chip_id { > @@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client, > const struct dac5571_spec *spec; > struct dac5571_data *data; > struct iio_dev *indio_dev; > + enum chip_id chip_id; > int ret, i; > > indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > @@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client, > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = dac5571_channels; > > - spec = &dac5571_spec[id->driver_data]; > + if (dev->of_node) > + chip_id = (uintptr_t)of_device_get_match_data(dev); > + else > + chip_id = id->driver_data; > + > + spec = &dac5571_spec[chip_id]; > + > indio_dev->num_channels = spec->num_channels; > data->spec = spec; > > @@ -384,15 +392,15 @@ static int dac5571_remove(struct i2c_client *i2c) > } > > static const struct of_device_id dac5571_of_id[] = { > - {.compatible = "ti,dac5571"}, > - {.compatible = "ti,dac6571"}, > - {.compatible = "ti,dac7571"}, > - {.compatible = "ti,dac5574"}, > - {.compatible = "ti,dac6574"}, > - {.compatible = "ti,dac7574"}, > - {.compatible = "ti,dac5573"}, > - {.compatible = "ti,dac6573"}, > - {.compatible = "ti,dac7573"}, > + {.compatible = "ti,dac5571", .data = (void *)single_8bit}, > + {.compatible = "ti,dac6571", .data = (void *)single_10bit}, > + {.compatible = "ti,dac7571", .data = (void *)single_12bit}, > + {.compatible = "ti,dac5574", .data = (void *)quad_8bit}, > + {.compatible = "ti,dac6574", .data = (void *)quad_10bit}, > + {.compatible = "ti,dac7574", .data = (void *)quad_12bit}, > + {.compatible = "ti,dac5573", .data = (void *)quad_8bit}, > + {.compatible = "ti,dac6573", .data = (void *)quad_10bit}, > + {.compatible = "ti,dac7573", .data = (void *)quad_12bit}, > {} > }; > MODULE_DEVICE_TABLE(of, dac5571_of_id); ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-07-24 14:43 ` Jonathan Cameron @ 2021-07-24 23:14 ` Laurent Pinchart 2021-08-17 20:44 ` Wolfram Sang 1 sibling, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-07-24 23:14 UTC (permalink / raw) To: Jonathan Cameron Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c, Wolfram Sang Hi Jonathan, On Sat, Jul 24, 2021 at 03:43:08PM +0100, Jonathan Cameron wrote: > On Sat, 24 Jul 2021 03:06:54 +0300 Laurent Pinchart wrote: > > > From: Jose Cazarin <joseespiriki@gmail.com> > > > > When matching an OF device, the match mechanism tries all components of > > the compatible property. This can result with a device matched with a > > compatible string that isn't the first in the compatible list. For > > instance, with a compatible property set to > > > > compatible = "ti,dac081c081", "ti,dac5571"; > > > > the driver will match the second compatible string, as the first one > > isn't listed in the of_device_id table. The device will however be named > > "dac081c081" by the I2C core. > > > > This causes an issue when identifying the chip. The probe function > > receives a i2c_device_id that comes from the module's I2C device ID > > table. There is no entry in that table for "dac081c081", which results > > in a NULL pointer passed to the probe function. > > > > To fix this, add chip_id information in the data field of the OF device > > ID table, and retrieve it with of_device_get_match_data() for OF > > devices. > > > > Signed-off-by: Jose Cazarin <joseespiriki@gmail.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Interesting problem that I hadn't previously realised could happen. > > One request though, can we use device_get_match_data() here rather than > the of specific version? Include property.h as well for that. > > That should allow the same issue with compatible to work correctly when > using PRP0001 based ACPI methods. > https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/acpi/bus.c#L891 > Will result in acpi_of_device_get_match_data() being called which will > match to the of_device_id table. Good point. I wasn't aware of PRP0001. I'll submit a v2 with this fixed, after giving a bit of time for additional review, if any (I'm in particular interested in whether this issue should be fixed in individual drivers or in the I2C core, as explained in the cover letter)). > > --- > > Changes since v1: > > > > - Include linux/of_device.h > > --- > > drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++---------- > > 1 file changed, 18 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c > > index 2a5ba1b08a1d..8ceb1b42b14e 100644 > > --- a/drivers/iio/dac/ti-dac5571.c > > +++ b/drivers/iio/dac/ti-dac5571.c > > @@ -19,6 +19,7 @@ > > #include <linux/i2c.h> > > #include <linux/module.h> > > #include <linux/mod_devicetable.h> > > +#include <linux/of_device.h> > > #include <linux/regulator/consumer.h> > > > > enum chip_id { > > @@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client, > > const struct dac5571_spec *spec; > > struct dac5571_data *data; > > struct iio_dev *indio_dev; > > + enum chip_id chip_id; > > int ret, i; > > > > indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > > @@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client, > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->channels = dac5571_channels; > > > > - spec = &dac5571_spec[id->driver_data]; > > + if (dev->of_node) > > + chip_id = (uintptr_t)of_device_get_match_data(dev); > > + else > > + chip_id = id->driver_data; > > + > > + spec = &dac5571_spec[chip_id]; > > + > > indio_dev->num_channels = spec->num_channels; > > data->spec = spec; > > > > @@ -384,15 +392,15 @@ static int dac5571_remove(struct i2c_client *i2c) > > } > > > > static const struct of_device_id dac5571_of_id[] = { > > - {.compatible = "ti,dac5571"}, > > - {.compatible = "ti,dac6571"}, > > - {.compatible = "ti,dac7571"}, > > - {.compatible = "ti,dac5574"}, > > - {.compatible = "ti,dac6574"}, > > - {.compatible = "ti,dac7574"}, > > - {.compatible = "ti,dac5573"}, > > - {.compatible = "ti,dac6573"}, > > - {.compatible = "ti,dac7573"}, > > + {.compatible = "ti,dac5571", .data = (void *)single_8bit}, > > + {.compatible = "ti,dac6571", .data = (void *)single_10bit}, > > + {.compatible = "ti,dac7571", .data = (void *)single_12bit}, > > + {.compatible = "ti,dac5574", .data = (void *)quad_8bit}, > > + {.compatible = "ti,dac6574", .data = (void *)quad_10bit}, > > + {.compatible = "ti,dac7574", .data = (void *)quad_12bit}, > > + {.compatible = "ti,dac5573", .data = (void *)quad_8bit}, > > + {.compatible = "ti,dac6573", .data = (void *)quad_10bit}, > > + {.compatible = "ti,dac7573", .data = (void *)quad_12bit}, > > {} > > }; > > MODULE_DEVICE_TABLE(of, dac5571_of_id); > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-07-24 14:43 ` Jonathan Cameron 2021-07-24 23:14 ` Laurent Pinchart @ 2021-08-17 20:44 ` Wolfram Sang 2021-08-17 20:52 ` Laurent Pinchart 1 sibling, 1 reply; 20+ messages in thread From: Wolfram Sang @ 2021-08-17 20:44 UTC (permalink / raw) To: Jonathan Cameron Cc: Laurent Pinchart, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c [-- Attachment #1: Type: text/plain, Size: 1851 bytes --] > > When matching an OF device, the match mechanism tries all components of > > the compatible property. This can result with a device matched with a > > compatible string that isn't the first in the compatible list. For > > instance, with a compatible property set to > > > > compatible = "ti,dac081c081", "ti,dac5571"; > > > > the driver will match the second compatible string, as the first one > > isn't listed in the of_device_id table. The device will however be named > > "dac081c081" by the I2C core. > > > > This causes an issue when identifying the chip. The probe function > > receives a i2c_device_id that comes from the module's I2C device ID > > table. There is no entry in that table for "dac081c081", which results > > in a NULL pointer passed to the probe function. > > > > To fix this, add chip_id information in the data field of the OF device > > ID table, and retrieve it with of_device_get_match_data() for OF > > devices. > > > > Signed-off-by: Jose Cazarin <joseespiriki@gmail.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Interesting problem that I hadn't previously realised could happen. > > One request though, can we use device_get_match_data() here rather than > the of specific version? Include property.h as well for that. > > That should allow the same issue with compatible to work correctly when > using PRP0001 based ACPI methods. > https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/acpi/bus.c#L891 > Will result in acpi_of_device_get_match_data() being called which will > match to the of_device_id table. Couldn't you use the "new" probe_new() callback instead which will drop the i2c_device_id? Kieran was interested in such conversions IIRC. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-08-17 20:44 ` Wolfram Sang @ 2021-08-17 20:52 ` Laurent Pinchart 2021-08-17 20:58 ` Wolfram Sang 0 siblings, 1 reply; 20+ messages in thread From: Laurent Pinchart @ 2021-08-17 20:52 UTC (permalink / raw) To: Wolfram Sang Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c Hi Wolfram, On Tue, Aug 17, 2021 at 10:44:20PM +0200, Wolfram Sang wrote: > > > > When matching an OF device, the match mechanism tries all components of > > > the compatible property. This can result with a device matched with a > > > compatible string that isn't the first in the compatible list. For > > > instance, with a compatible property set to > > > > > > compatible = "ti,dac081c081", "ti,dac5571"; > > > > > > the driver will match the second compatible string, as the first one > > > isn't listed in the of_device_id table. The device will however be named > > > "dac081c081" by the I2C core. > > > > > > This causes an issue when identifying the chip. The probe function > > > receives a i2c_device_id that comes from the module's I2C device ID > > > table. There is no entry in that table for "dac081c081", which results > > > in a NULL pointer passed to the probe function. > > > > > > To fix this, add chip_id information in the data field of the OF device > > > ID table, and retrieve it with of_device_get_match_data() for OF > > > devices. > > > > > > Signed-off-by: Jose Cazarin <joseespiriki@gmail.com> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Interesting problem that I hadn't previously realised could happen. > > > > One request though, can we use device_get_match_data() here rather than > > the of specific version? Include property.h as well for that. > > > > That should allow the same issue with compatible to work correctly when > > using PRP0001 based ACPI methods. > > https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/acpi/bus.c#L891 > > Will result in acpi_of_device_get_match_data() being called which will > > match to the of_device_id table. > > Couldn't you use the "new" probe_new() callback instead which will drop > the i2c_device_id? Kieran was interested in such conversions IIRC. It's a bit unrelated to this patch, but I can add another patch to the series. While I have your attention, there's a question for you in the cover letter :-) Could you please have a look ? -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-08-17 20:52 ` Laurent Pinchart @ 2021-08-17 20:58 ` Wolfram Sang 2021-08-17 21:20 ` Laurent Pinchart 0 siblings, 1 reply; 20+ messages in thread From: Wolfram Sang @ 2021-08-17 20:58 UTC (permalink / raw) To: Laurent Pinchart Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c [-- Attachment #1: Type: text/plain, Size: 432 bytes --] > > Couldn't you use the "new" probe_new() callback instead which will drop > > the i2c_device_id? Kieran was interested in such conversions IIRC. > > It's a bit unrelated to this patch, but I can add another patch to the > series. > > While I have your attention, there's a question for you in the cover > letter :-) Could you please have a look ? ? This was the answer to that question. Unless I misunderstood. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-08-17 20:58 ` Wolfram Sang @ 2021-08-17 21:20 ` Laurent Pinchart 2022-03-24 23:25 ` Laurent Pinchart 2022-03-28 9:20 ` Wolfram Sang 0 siblings, 2 replies; 20+ messages in thread From: Laurent Pinchart @ 2021-08-17 21:20 UTC (permalink / raw) To: Wolfram Sang Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c On Tue, Aug 17, 2021 at 10:58:19PM +0200, Wolfram Sang wrote: > > > > Couldn't you use the "new" probe_new() callback instead which will drop > > > the i2c_device_id? Kieran was interested in such conversions IIRC. > > > > It's a bit unrelated to this patch, but I can add another patch to the > > series. > > > > While I have your attention, there's a question for you in the cover > > letter :-) Could you please have a look ? > > ? This was the answer to that question. Unless I misunderstood. My point is that this patch shouldn't be needed. I'd like if the I2C core could get the driver data from the i2c_device_id table instead of duplicating it in the of_device_id. This isn't possible today as i2c_match_id() doesn't have the fallback mechanism that OF matching has. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-08-17 21:20 ` Laurent Pinchart @ 2022-03-24 23:25 ` Laurent Pinchart 2022-03-28 9:20 ` Wolfram Sang 1 sibling, 0 replies; 20+ messages in thread From: Laurent Pinchart @ 2022-03-24 23:25 UTC (permalink / raw) To: Wolfram Sang Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c Hi Wolfram, On Wed, Aug 18, 2021 at 12:20:03AM +0300, Laurent Pinchart wrote: > On Tue, Aug 17, 2021 at 10:58:19PM +0200, Wolfram Sang wrote: > > > > > > Couldn't you use the "new" probe_new() callback instead which will drop > > > > the i2c_device_id? Kieran was interested in such conversions IIRC. > > > > > > It's a bit unrelated to this patch, but I can add another patch to the > > > series. > > > > > > While I have your attention, there's a question for you in the cover > > > letter :-) Could you please have a look ? > > > > ? This was the answer to that question. Unless I misunderstood. > > My point is that this patch shouldn't be needed. I'd like if the I2C > core could get the driver data from the i2c_device_id table instead of > duplicating it in the of_device_id. This isn't possible today as > i2c_match_id() doesn't have the fallback mechanism that OF matching has. Any thought on this ? -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-08-17 21:20 ` Laurent Pinchart 2022-03-24 23:25 ` Laurent Pinchart @ 2022-03-28 9:20 ` Wolfram Sang 2022-03-28 10:04 ` Laurent Pinchart 1 sibling, 1 reply; 20+ messages in thread From: Wolfram Sang @ 2022-03-28 9:20 UTC (permalink / raw) To: Laurent Pinchart Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c [-- Attachment #1: Type: text/plain, Size: 485 bytes --] > My point is that this patch shouldn't be needed. I'd like if the I2C > core could get the driver data from the i2c_device_id table instead of > duplicating it in the of_device_id. This isn't possible today as > i2c_match_id() doesn't have the fallback mechanism that OF matching has. I think the proper fix would be naming the I2C client after the actually matched compatible property, and not after the first one? I am a bit afraid of regressions when we change that, however... [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2022-03-28 9:20 ` Wolfram Sang @ 2022-03-28 10:04 ` Laurent Pinchart 2022-03-28 12:22 ` Wolfram Sang 0 siblings, 1 reply; 20+ messages in thread From: Laurent Pinchart @ 2022-03-28 10:04 UTC (permalink / raw) To: Wolfram Sang Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c On Mon, Mar 28, 2022 at 11:20:54AM +0200, Wolfram Sang wrote: > > > My point is that this patch shouldn't be needed. I'd like if the I2C > > core could get the driver data from the i2c_device_id table instead of > > duplicating it in the of_device_id. This isn't possible today as > > i2c_match_id() doesn't have the fallback mechanism that OF matching has. > > I think the proper fix would be naming the I2C client after the actually > matched compatible property, and not after the first one? I am a bit > afraid of regressions when we change that, however... That would be the right way indeed. I have the same concern regarding regressions though. Is it worth a try to see what could break ? -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2022-03-28 10:04 ` Laurent Pinchart @ 2022-03-28 12:22 ` Wolfram Sang 2022-03-29 0:44 ` Laurent Pinchart 0 siblings, 1 reply; 20+ messages in thread From: Wolfram Sang @ 2022-03-28 12:22 UTC (permalink / raw) To: Laurent Pinchart Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c [-- Attachment #1: Type: text/plain, Size: 439 bytes --] > > I think the proper fix would be naming the I2C client after the actually > > matched compatible property, and not after the first one? I am a bit > > afraid of regressions when we change that, however... > > That would be the right way indeed. I have the same concern regarding > regressions though. Is it worth a try to see what could break ? Sure! Only problem: Patches welcome(tm) or I put it on my to-do-list(tm) ;) [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2022-03-28 12:22 ` Wolfram Sang @ 2022-03-29 0:44 ` Laurent Pinchart 2022-03-29 8:59 ` Wolfram Sang 0 siblings, 1 reply; 20+ messages in thread From: Laurent Pinchart @ 2022-03-29 0:44 UTC (permalink / raw) To: Wolfram Sang Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c Hi Wolfram, On Mon, Mar 28, 2022 at 02:22:27PM +0200, Wolfram Sang wrote: > > > > I think the proper fix would be naming the I2C client after the actually > > > matched compatible property, and not after the first one? I am a bit > > > afraid of regressions when we change that, however... > > > > That would be the right way indeed. I have the same concern regarding > > regressions though. Is it worth a try to see what could break ? > > Sure! Only problem: Patches welcome(tm) or I put it on my to-do-list(tm) > ;) I've had a look, but it seems to be problematic. The name of the client is set in i2c_new_client_device(), way before we match with a driver. The name is used in the uevent sent to userspace, so changing it afterwards is likely not a good idea. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2022-03-29 0:44 ` Laurent Pinchart @ 2022-03-29 8:59 ` Wolfram Sang 0 siblings, 0 replies; 20+ messages in thread From: Wolfram Sang @ 2022-03-29 8:59 UTC (permalink / raw) To: Laurent Pinchart Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c [-- Attachment #1: Type: text/plain, Size: 398 bytes --] > I've had a look, but it seems to be problematic. The name of the client > is set in i2c_new_client_device(), way before we match with a driver. > The name is used in the uevent sent to userspace, so changing it > afterwards is likely not a good idea. Okay, that is a definitive no-go. Thanks for checking. Seems we really need to update i2c_match_id to handle the extra case as you suggested. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v1.1 2/2] iio: dac: dac5571: Fix chip id detection for OF devices 2021-07-24 0:06 ` [PATCH v1.1 " Laurent Pinchart 2021-07-24 14:43 ` Jonathan Cameron @ 2021-07-24 14:44 ` Jonathan Cameron 1 sibling, 0 replies; 20+ messages in thread From: Jonathan Cameron @ 2021-07-24 14:44 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-iio, Lars-Peter Clausen, Rob Herring, Sean Nyekjaer, devicetree, Jose Cazarin, linux-i2c, Wolfram Sang On Sat, 24 Jul 2021 03:06:54 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > From: Jose Cazarin <joseespiriki@gmail.com> > > When matching an OF device, the match mechanism tries all components of > the compatible property. This can result with a device matched with a > compatible string that isn't the first in the compatible list. For > instance, with a compatible property set to > > compatible = "ti,dac081c081", "ti,dac5571"; > > the driver will match the second compatible string, as the first one > isn't listed in the of_device_id table. The device will however be named > "dac081c081" by the I2C core. > > This causes an issue when identifying the chip. The probe function > receives a i2c_device_id that comes from the module's I2C device ID > table. There is no entry in that table for "dac081c081", which results > in a NULL pointer passed to the probe function. > > To fix this, add chip_id information in the data field of the OF device > ID table, and retrieve it with of_device_get_match_data() for OF > devices. > > Signed-off-by: Jose Cazarin <joseespiriki@gmail.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Side note I failed to put in the review email. I definitely prefer a whole new series even when a change is just to a single patch like this. Much easier to track and eventually pick up as one unit. Thanks, Jonathan > --- > Changes since v1: > > - Include linux/of_device.h > --- > drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c > index 2a5ba1b08a1d..8ceb1b42b14e 100644 > --- a/drivers/iio/dac/ti-dac5571.c > +++ b/drivers/iio/dac/ti-dac5571.c > @@ -19,6 +19,7 @@ > #include <linux/i2c.h> > #include <linux/module.h> > #include <linux/mod_devicetable.h> > +#include <linux/of_device.h> > #include <linux/regulator/consumer.h> > > enum chip_id { > @@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client, > const struct dac5571_spec *spec; > struct dac5571_data *data; > struct iio_dev *indio_dev; > + enum chip_id chip_id; > int ret, i; > > indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > @@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client, > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = dac5571_channels; > > - spec = &dac5571_spec[id->driver_data]; > + if (dev->of_node) > + chip_id = (uintptr_t)of_device_get_match_data(dev); > + else > + chip_id = id->driver_data; > + > + spec = &dac5571_spec[chip_id]; > + > indio_dev->num_channels = spec->num_channels; > data->spec = spec; > > @@ -384,15 +392,15 @@ static int dac5571_remove(struct i2c_client *i2c) > } > > static const struct of_device_id dac5571_of_id[] = { > - {.compatible = "ti,dac5571"}, > - {.compatible = "ti,dac6571"}, > - {.compatible = "ti,dac7571"}, > - {.compatible = "ti,dac5574"}, > - {.compatible = "ti,dac6574"}, > - {.compatible = "ti,dac7574"}, > - {.compatible = "ti,dac5573"}, > - {.compatible = "ti,dac6573"}, > - {.compatible = "ti,dac7573"}, > + {.compatible = "ti,dac5571", .data = (void *)single_8bit}, > + {.compatible = "ti,dac6571", .data = (void *)single_10bit}, > + {.compatible = "ti,dac7571", .data = (void *)single_12bit}, > + {.compatible = "ti,dac5574", .data = (void *)quad_8bit}, > + {.compatible = "ti,dac6574", .data = (void *)quad_10bit}, > + {.compatible = "ti,dac7574", .data = (void *)quad_12bit}, > + {.compatible = "ti,dac5573", .data = (void *)quad_8bit}, > + {.compatible = "ti,dac6573", .data = (void *)quad_10bit}, > + {.compatible = "ti,dac7573", .data = (void *)quad_12bit}, > {} > }; > MODULE_DEVICE_TABLE(of, dac5571_of_id); ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-03-29 8:59 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-23 18:31 [PATCH 0/2] iio: ti-dac5571: Add TI DAC081C081 support Laurent Pinchart 2021-07-23 18:31 ` [PATCH 1/2] dt-bindings: iio: dac: ti,dac5571: " Laurent Pinchart 2021-07-29 21:26 ` Rob Herring 2021-07-23 18:31 ` [PATCH 2/2] iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart 2021-07-23 23:06 ` kernel test robot 2021-07-23 23:06 ` kernel test robot 2021-07-24 0:06 ` [PATCH v1.1 " Laurent Pinchart 2021-07-24 14:43 ` Jonathan Cameron 2021-07-24 23:14 ` Laurent Pinchart 2021-08-17 20:44 ` Wolfram Sang 2021-08-17 20:52 ` Laurent Pinchart 2021-08-17 20:58 ` Wolfram Sang 2021-08-17 21:20 ` Laurent Pinchart 2022-03-24 23:25 ` Laurent Pinchart 2022-03-28 9:20 ` Wolfram Sang 2022-03-28 10:04 ` Laurent Pinchart 2022-03-28 12:22 ` Wolfram Sang 2022-03-29 0:44 ` Laurent Pinchart 2022-03-29 8:59 ` Wolfram Sang 2021-07-24 14:44 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).