[PATCH v5 0/2] iio: humidity: hdc100x: add manufacturer and device ID check

[PATCH v5 0/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Potin Lai]

In this patch series, we move the callback from probe to probe_new, and add
manufacturer and device ID check for hdc100x driver.

LINK: [v1] https://lore.kernel.org/all/20220722172035.44977-1-potin.lai.pt@gmail.com/
LINK: [v2] https://lore.kernel.org/all/20220727064415.940690-1-potin.lai.pt@gmail.com/
LINK: [v3] https://lore.kernel.org/all/20220727164133.973930-1-potin.lai.pt@gmail.com/
LINK: [v4] https://lore.kernel.org/all/20220728003417.3083353-1-potin.lai.pt@gmail.com/

changes v1 --> v2:
- fix typo in commit message
- change to use probe_new
- use device_get_match_data instead of i2c_of_match_device

changes v2 --> v3:
- move probe_new part into a separate patch
- remove unsed variable
- remove redundant checking of matched data

changes v3 --> v4:
- move ID support checking to probe()
- add hdc100x_chip_data pointer into hdc100x_data for accessing in future

changes v4 --> v5:
- remove hdc100x_chip_data pointer from hdc100x_data structure

Potin Lai (2):
  iio: humidity: hdc100x: switch to probe_new callback
  iio: humidity: hdc100x: add manufacturer and device ID check

 drivers/iio/humidity/hdc100x.c | 72 +++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 14 deletions(-)

-- 
2.31.1

[PATCH v5 1/2] iio: humidity: hdc100x: switch to probe_new callback

[Potin Lai]

Switch to probe_new callback due to probe is deprecated soon.

Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/humidity/hdc100x.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
index 9e0fce917ce4c..0d514818635cb 100644
--- a/drivers/iio/humidity/hdc100x.c
+++ b/drivers/iio/humidity/hdc100x.c
@@ -351,8 +351,7 @@ static const struct iio_info hdc100x_info = {
 	.attrs = &hdc100x_attribute_group,
 };
 
-static int hdc100x_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id)
+static int hdc100x_probe(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev;
 	struct hdc100x_data *data;
@@ -422,7 +421,7 @@ static struct i2c_driver hdc100x_driver = {
 		.name	= "hdc100x",
 		.of_match_table = hdc100x_dt_ids,
 	},
-	.probe = hdc100x_probe,
+	.probe_new = hdc100x_probe,
 	.id_table = hdc100x_id,
 };
 module_i2c_driver(hdc100x_driver);
-- 
2.31.1

Re: [PATCH v5 1/2] iio: humidity: hdc100x: switch to probe_new callback

[Andy Shevchenko]

On Thu, Jul 28, 2022 at 3:32 PM Potin Lai <potin.lai.pt@gmail.com> wrote:
>
> Switch to probe_new callback due to probe is deprecated soon.

Just noticed that commit message is a bit not okay in a few ways:
1) we refer to the callbacks like ->probe_new();
2) we don't know when we deprecate it, the point here is not that, but
unused id parameter in the current code.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] iio: humidity: hdc100x: switch to probe_new callback

[Potin Lai]

On 7/29/22 04:41, Andy Shevchenko wrote:
> On Thu, Jul 28, 2022 at 3:32 PM Potin Lai <potin.lai.pt@gmail.com> wrote:
>> Switch to probe_new callback due to probe is deprecated soon.
> Just noticed that commit message is a bit not okay in a few ways:
> 1) we refer to the callbacks like ->probe_new();
> 2) we don't know when we deprecate it, the point here is not that, but
> unused id parameter in the current code.
>
>
Thanks for point it out, is it OK leave the message as it is? or you prefer to submit another version to fix it?
If new version required, I will also add another patch for struct device pointer you mentioned in the other reply.

Just want to confirm that is the new message looks OK?
New message:
Switch ->porbe() to new callback ->probe_new()

Thanks,
Potin

Re: [PATCH v5 1/2] iio: humidity: hdc100x: switch to probe_new callback

[Andy Shevchenko]

On Fri, Jul 29, 2022 at 2:45 AM Potin Lai <potin.lai.pt@gmail.com> wrote:
> On 7/29/22 04:41, Andy Shevchenko wrote:
> > On Thu, Jul 28, 2022 at 3:32 PM Potin Lai <potin.lai.pt@gmail.com> wrote:
> >> Switch to probe_new callback due to probe is deprecated soon.
> > Just noticed that commit message is a bit not okay in a few ways:
> > 1) we refer to the callbacks like ->probe_new();

> > 2) we don't know when we deprecate it, the point here is not that, but
> > unused id parameter in the current code.

^^^ read this again and follow below.

> Thanks for point it out, is it OK leave the message as it is? or you prefer to submit another version to fix it?
> If new version required, I will also add another patch for struct device pointer you mentioned in the other reply.
>
> Just want to confirm that is the new message looks OK?
> New message:
> Switch ->porbe() to new callback ->probe_new()

You need to answer the question "why?" you are doing this and that.
The above is just saying "what?". See above.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Potin Lai]

Add manufacturer and device ID checking during probe, and skip the
checking if chip model not supported.

Supported:
- HDC1000
- HDC1010
- HDC1050
- HDC1080

Not supported:
- HDC1008

Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
---
 drivers/iio/humidity/hdc100x.c | 67 ++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
index 0d514818635cb..31f18cc1cf63c 100644
--- a/drivers/iio/humidity/hdc100x.c
+++ b/drivers/iio/humidity/hdc100x.c
@@ -34,6 +34,23 @@
 #define HDC100X_REG_CONFIG_ACQ_MODE		BIT(12)
 #define HDC100X_REG_CONFIG_HEATER_EN		BIT(13)
 
+#define HDC100X_REG_MFR_ID	0xFE
+#define HDC100X_REG_DEV_ID	0xFF
+
+#define HDC100X_MFR_ID	0x5449
+
+struct hdc100x_chip_data {
+	bool support_mfr_check;
+};
+
+static const struct hdc100x_chip_data hdc100x_chip_data = {
+	.support_mfr_check	= true,
+};
+
+static const struct hdc100x_chip_data hdc1008_chip_data = {
+	.support_mfr_check	= false,
+};
+
 struct hdc100x_data {
 	struct i2c_client *client;
 	struct mutex lock;
@@ -351,8 +368,32 @@ static const struct iio_info hdc100x_info = {
 	.attrs = &hdc100x_attribute_group,
 };
 
+static int hdc100x_read_mfr_id(struct i2c_client *client)
+{
+	return i2c_smbus_read_word_swapped(client, HDC100X_REG_MFR_ID);
+}
+
+static int hdc100x_read_dev_id(struct i2c_client *client)
+{
+	return i2c_smbus_read_word_swapped(client, HDC100X_REG_DEV_ID);
+}
+
+static bool is_valid_hdc100x(struct i2c_client *client)
+{
+	int mfr_id, dev_id;
+
+	mfr_id = hdc100x_read_mfr_id(client);
+	dev_id = hdc100x_read_dev_id(client);
+	if (mfr_id == HDC100X_MFR_ID &&
+	   (dev_id == 0x1000 || dev_id == 0x1050))
+		return true;
+
+	return false;
+}
+
 static int hdc100x_probe(struct i2c_client *client)
 {
+	const struct hdc100x_chip_data *chip_data;
 	struct iio_dev *indio_dev;
 	struct hdc100x_data *data;
 	int ret;
@@ -361,6 +402,10 @@ static int hdc100x_probe(struct i2c_client *client)
 				     I2C_FUNC_SMBUS_BYTE | I2C_FUNC_I2C))
 		return -EOPNOTSUPP;
 
+	chip_data = device_get_match_data(&client->dev);
+	if (chip_data->support_mfr_check && !is_valid_hdc100x(client))
+		return -EINVAL;
+
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
@@ -396,22 +441,22 @@ static int hdc100x_probe(struct i2c_client *client)
 }
 
 static const struct i2c_device_id hdc100x_id[] = {
-	{ "hdc100x", 0 },
-	{ "hdc1000", 0 },
-	{ "hdc1008", 0 },
-	{ "hdc1010", 0 },
-	{ "hdc1050", 0 },
-	{ "hdc1080", 0 },
+	{ "hdc100X", (kernel_ulong_t)&hdc100x_chip_data },
+	{ "hdc1000", (kernel_ulong_t)&hdc100x_chip_data },
+	{ "hdc1008", (kernel_ulong_t)&hdc1008_chip_data },
+	{ "hdc1010", (kernel_ulong_t)&hdc100x_chip_data },
+	{ "hdc1050", (kernel_ulong_t)&hdc100x_chip_data },
+	{ "hdc1080", (kernel_ulong_t)&hdc100x_chip_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, hdc100x_id);
 
 static const struct of_device_id hdc100x_dt_ids[] = {
-	{ .compatible = "ti,hdc1000" },
-	{ .compatible = "ti,hdc1008" },
-	{ .compatible = "ti,hdc1010" },
-	{ .compatible = "ti,hdc1050" },
-	{ .compatible = "ti,hdc1080" },
+	{ .compatible = "ti,hdc1000", .data = &hdc100x_chip_data },
+	{ .compatible = "ti,hdc1008", .data = &hdc1008_chip_data },
+	{ .compatible = "ti,hdc1010", .data = &hdc100x_chip_data },
+	{ .compatible = "ti,hdc1050", .data = &hdc100x_chip_data },
+	{ .compatible = "ti,hdc1080", .data = &hdc100x_chip_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, hdc100x_dt_ids);
-- 
2.31.1

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Andy Shevchenko]

On Thu, Jul 28, 2022 at 3:32 PM Potin Lai <potin.lai.pt@gmail.com> wrote:
>
> Add manufacturer and device ID checking during probe, and skip the
> checking if chip model not supported.
>
> Supported:
> - HDC1000
> - HDC1010
> - HDC1050
> - HDC1080
>
> Not supported:
> - HDC1008

This one is okay now, thanks!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
> ---
>  drivers/iio/humidity/hdc100x.c | 67 ++++++++++++++++++++++++++++------
>  1 file changed, 56 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
> index 0d514818635cb..31f18cc1cf63c 100644
> --- a/drivers/iio/humidity/hdc100x.c
> +++ b/drivers/iio/humidity/hdc100x.c
> @@ -34,6 +34,23 @@
>  #define HDC100X_REG_CONFIG_ACQ_MODE            BIT(12)
>  #define HDC100X_REG_CONFIG_HEATER_EN           BIT(13)
>
> +#define HDC100X_REG_MFR_ID     0xFE
> +#define HDC100X_REG_DEV_ID     0xFF
> +
> +#define HDC100X_MFR_ID 0x5449
> +
> +struct hdc100x_chip_data {
> +       bool support_mfr_check;
> +};
> +
> +static const struct hdc100x_chip_data hdc100x_chip_data = {
> +       .support_mfr_check      = true,
> +};
> +
> +static const struct hdc100x_chip_data hdc1008_chip_data = {
> +       .support_mfr_check      = false,
> +};
> +
>  struct hdc100x_data {
>         struct i2c_client *client;
>         struct mutex lock;
> @@ -351,8 +368,32 @@ static const struct iio_info hdc100x_info = {
>         .attrs = &hdc100x_attribute_group,
>  };
>
> +static int hdc100x_read_mfr_id(struct i2c_client *client)
> +{
> +       return i2c_smbus_read_word_swapped(client, HDC100X_REG_MFR_ID);
> +}
> +
> +static int hdc100x_read_dev_id(struct i2c_client *client)
> +{
> +       return i2c_smbus_read_word_swapped(client, HDC100X_REG_DEV_ID);
> +}
> +
> +static bool is_valid_hdc100x(struct i2c_client *client)
> +{
> +       int mfr_id, dev_id;
> +
> +       mfr_id = hdc100x_read_mfr_id(client);
> +       dev_id = hdc100x_read_dev_id(client);
> +       if (mfr_id == HDC100X_MFR_ID &&
> +          (dev_id == 0x1000 || dev_id == 0x1050))
> +               return true;
> +
> +       return false;
> +}
> +
>  static int hdc100x_probe(struct i2c_client *client)
>  {
> +       const struct hdc100x_chip_data *chip_data;
>         struct iio_dev *indio_dev;
>         struct hdc100x_data *data;
>         int ret;
> @@ -361,6 +402,10 @@ static int hdc100x_probe(struct i2c_client *client)
>                                      I2C_FUNC_SMBUS_BYTE | I2C_FUNC_I2C))
>                 return -EOPNOTSUPP;
>
> +       chip_data = device_get_match_data(&client->dev);
> +       if (chip_data->support_mfr_check && !is_valid_hdc100x(client))
> +               return -EINVAL;
> +
>         indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>         if (!indio_dev)
>                 return -ENOMEM;
> @@ -396,22 +441,22 @@ static int hdc100x_probe(struct i2c_client *client)
>  }
>
>  static const struct i2c_device_id hdc100x_id[] = {
> -       { "hdc100x", 0 },
> -       { "hdc1000", 0 },
> -       { "hdc1008", 0 },
> -       { "hdc1010", 0 },
> -       { "hdc1050", 0 },
> -       { "hdc1080", 0 },
> +       { "hdc100X", (kernel_ulong_t)&hdc100x_chip_data },
> +       { "hdc1000", (kernel_ulong_t)&hdc100x_chip_data },
> +       { "hdc1008", (kernel_ulong_t)&hdc1008_chip_data },
> +       { "hdc1010", (kernel_ulong_t)&hdc100x_chip_data },
> +       { "hdc1050", (kernel_ulong_t)&hdc100x_chip_data },
> +       { "hdc1080", (kernel_ulong_t)&hdc100x_chip_data },
>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, hdc100x_id);
>
>  static const struct of_device_id hdc100x_dt_ids[] = {
> -       { .compatible = "ti,hdc1000" },
> -       { .compatible = "ti,hdc1008" },
> -       { .compatible = "ti,hdc1010" },
> -       { .compatible = "ti,hdc1050" },
> -       { .compatible = "ti,hdc1080" },
> +       { .compatible = "ti,hdc1000", .data = &hdc100x_chip_data },
> +       { .compatible = "ti,hdc1008", .data = &hdc1008_chip_data },
> +       { .compatible = "ti,hdc1010", .data = &hdc100x_chip_data },
> +       { .compatible = "ti,hdc1050", .data = &hdc100x_chip_data },
> +       { .compatible = "ti,hdc1080", .data = &hdc100x_chip_data },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, hdc100x_dt_ids);
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Jonathan Cameron]

On Thu, 28 Jul 2022 12:54:35 +0000
Potin Lai <potin.lai.pt@gmail.com> wrote:

> Add manufacturer and device ID checking during probe, and skip the
> checking if chip model not supported.
> 
> Supported:
> - HDC1000
> - HDC1010
> - HDC1050
> - HDC1080
> 
> Not supported:
> - HDC1008
> 
> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>

I need some more information on the 'why' of this patch.  There are a number
of drivers that do a similar ID check but in recent times, that approach has
been considered wrong because it breaks potential use of multiple compatible
entries in device tree.   If a new device comes along and is backwards
compatible with an existing one (maybe has new features, but using them is
optional) then we want to have an entry that looks like

compatible = "ti,hdc1099", "ti,hdc1080"

If the new ID is not supported by the kernel that is being used, we still
want the driver to 'work' using the fallback compatible.

As such, we no generally do the following.

1) If we have a match to a device we know about but it's not the one the
   firmware tells us to expect, print a warning but operate as if the firmware
   had been correct - particularly if we know the parts aren't compatible
   with each other. (this bit is optional as we should be able to assume firmware
   doesn't do stupid things :)
2) If we don't match at all, print a warning about an unknown device but carry
   on with assumption that the firmware is correct and this new device ID is
   backwards compatible with the provided fallback compatible.

So if this is just a bit of defensive programming (rather than necessary for some
reason not yet explained) then change from returning an error on probe() to 
printing an warning message but continuing anyway. (which is part (2) of the
above)

> ---
>  drivers/iio/humidity/hdc100x.c | 67 ++++++++++++++++++++++++++++------
>  1 file changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
> index 0d514818635cb..31f18cc1cf63c 100644
> --- a/drivers/iio/humidity/hdc100x.c
> +++ b/drivers/iio/humidity/hdc100x.c
> @@ -34,6 +34,23 @@
>  #define HDC100X_REG_CONFIG_ACQ_MODE		BIT(12)
>  #define HDC100X_REG_CONFIG_HEATER_EN		BIT(13)
>  
> +#define HDC100X_REG_MFR_ID	0xFE
> +#define HDC100X_REG_DEV_ID	0xFF
> +
> +#define HDC100X_MFR_ID	0x5449
> +
> +struct hdc100x_chip_data {
> +	bool support_mfr_check;
> +};
> +
> +static const struct hdc100x_chip_data hdc100x_chip_data = {
> +	.support_mfr_check	= true,
> +};
> +
> +static const struct hdc100x_chip_data hdc1008_chip_data = {
> +	.support_mfr_check	= false,
> +};
> +
>  struct hdc100x_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
> @@ -351,8 +368,32 @@ static const struct iio_info hdc100x_info = {
>  	.attrs = &hdc100x_attribute_group,
>  };
>  
> +static int hdc100x_read_mfr_id(struct i2c_client *client)
> +{
> +	return i2c_smbus_read_word_swapped(client, HDC100X_REG_MFR_ID);
> +}
> +
> +static int hdc100x_read_dev_id(struct i2c_client *client)
> +{
> +	return i2c_smbus_read_word_swapped(client, HDC100X_REG_DEV_ID);
> +}
> +
> +static bool is_valid_hdc100x(struct i2c_client *client)
> +{
> +	int mfr_id, dev_id;
> +
> +	mfr_id = hdc100x_read_mfr_id(client);
> +	dev_id = hdc100x_read_dev_id(client);
> +	if (mfr_id == HDC100X_MFR_ID &&
> +	   (dev_id == 0x1000 || dev_id == 0x1050))
> +		return true;
> +
> +	return false;
> +}
> +
>  static int hdc100x_probe(struct i2c_client *client)
>  {
> +	const struct hdc100x_chip_data *chip_data;
>  	struct iio_dev *indio_dev;
>  	struct hdc100x_data *data;
>  	int ret;
> @@ -361,6 +402,10 @@ static int hdc100x_probe(struct i2c_client *client)
>  				     I2C_FUNC_SMBUS_BYTE | I2C_FUNC_I2C))
>  		return -EOPNOTSUPP;
>  
> +	chip_data = device_get_match_data(&client->dev);
> +	if (chip_data->support_mfr_check && !is_valid_hdc100x(client))
> +		return -EINVAL;
> +
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;
> @@ -396,22 +441,22 @@ static int hdc100x_probe(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id hdc100x_id[] = {
> -	{ "hdc100x", 0 },
> -	{ "hdc1000", 0 },
> -	{ "hdc1008", 0 },
> -	{ "hdc1010", 0 },
> -	{ "hdc1050", 0 },
> -	{ "hdc1080", 0 },
> +	{ "hdc100X", (kernel_ulong_t)&hdc100x_chip_data },
> +	{ "hdc1000", (kernel_ulong_t)&hdc100x_chip_data },
> +	{ "hdc1008", (kernel_ulong_t)&hdc1008_chip_data },
> +	{ "hdc1010", (kernel_ulong_t)&hdc100x_chip_data },
> +	{ "hdc1050", (kernel_ulong_t)&hdc100x_chip_data },
> +	{ "hdc1080", (kernel_ulong_t)&hdc100x_chip_data },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, hdc100x_id);
>  
>  static const struct of_device_id hdc100x_dt_ids[] = {
> -	{ .compatible = "ti,hdc1000" },
> -	{ .compatible = "ti,hdc1008" },
> -	{ .compatible = "ti,hdc1010" },
> -	{ .compatible = "ti,hdc1050" },
> -	{ .compatible = "ti,hdc1080" },
> +	{ .compatible = "ti,hdc1000", .data = &hdc100x_chip_data },
> +	{ .compatible = "ti,hdc1008", .data = &hdc1008_chip_data },
> +	{ .compatible = "ti,hdc1010", .data = &hdc100x_chip_data },
> +	{ .compatible = "ti,hdc1050", .data = &hdc100x_chip_data },
> +	{ .compatible = "ti,hdc1080", .data = &hdc100x_chip_data },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, hdc100x_dt_ids);

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Potin Lai]

On 7/31/22 20:09, Jonathan Cameron wrote:
> On Thu, 28 Jul 2022 12:54:35 +0000
> Potin Lai <potin.lai.pt@gmail.com> wrote:
>
>> Add manufacturer and device ID checking during probe, and skip the
>> checking if chip model not supported.
>>
>> Supported:
>> - HDC1000
>> - HDC1010
>> - HDC1050
>> - HDC1080
>>
>> Not supported:
>> - HDC1008
>>
>> Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
> I need some more information on the 'why' of this patch.  There are a number
> of drivers that do a similar ID check but in recent times, that approach has
> been considered wrong because it breaks potential use of multiple compatible
> entries in device tree.   If a new device comes along and is backwards
> compatible with an existing one (maybe has new features, but using them is
> optional) then we want to have an entry that looks like
>
> compatible = "ti,hdc1099", "ti,hdc1080"
>
> If the new ID is not supported by the kernel that is being used, we still
> want the driver to 'work' using the fallback compatible.
>
> As such, we no generally do the following.
>
> 1) If we have a match to a device we know about but it's not the one the
>    firmware tells us to expect, print a warning but operate as if the firmware
>    had been correct - particularly if we know the parts aren't compatible
>    with each other. (this bit is optional as we should be able to assume firmware
>    doesn't do stupid things :)
> 2) If we don't match at all, print a warning about an unknown device but carry
>    on with assumption that the firmware is correct and this new device ID is
>    backwards compatible with the provided fallback compatible.
>
> So if this is just a bit of defensive programming (rather than necessary for some
> reason not yet explained) then change from returning an error on probe() to 
> printing an warning message but continuing anyway. (which is part (2) of the
> above)
Hi Jonathan,
In our hardware board, we have "ti,hdc1080" as main source, and "silabs,si7020"
for 2nd source. This two chip are locate at same bus and same slave address,
and we want to use multiple compatibles to support both chips with single device
node in device tree.
 
Ex:
compatible = "ti,hdc1099", "silabs,si7020";
 
In order to support this, I need to add ID checking mechanism into the current
hdc100x driver, so the si7020 chip will fail to probe with hdc100x driver
(because the ID checking is not failed), then success probe with si7020.
 
Base on you explanation, it looks multiple compatibles is not suitable in this
case? Would you mind advise us what would be the better approach for our case?

Thanks,
Potin

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Andy Shevchenko]

On Mon, Aug 1, 2022 at 3:52 AM Potin Lai <potin.lai.pt@gmail.com> wrote:
> On 7/31/22 20:09, Jonathan Cameron wrote:

...

> In our hardware board, we have "ti,hdc1080" as main source, and "silabs,si7020"
> for 2nd source. This two chip are locate at same bus and same slave address,
> and we want to use multiple compatibles to support both chips with single device
> node in device tree.
>
> Ex:
> compatible = "ti,hdc1099", "silabs,si7020";

This is simply broken DT, you must not put incompatible hardware on
the same compatible string. DT is by definition the description of a
certain platform. What you showed is a combination of incompatible
chips in a single DT.

> In order to support this, I need to add ID checking mechanism into the current
> hdc100x driver, so the si7020 chip will fail to probe with hdc100x driver
> (because the ID checking is not failed), then success probe with si7020.
>
> Base on you explanation, it looks multiple compatibles is not suitable in this
> case? Would you mind advise us what would be the better approach for our case?

If I may advise... fix your DT by dropping the wrong compatible item.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

On Mon, Aug 01, 2022 at 10:22:16AM +0200, Andy Shevchenko wrote:
> On Mon, Aug 1, 2022 at 3:52 AM Potin Lai <potin.lai.pt@gmail.com> wrote:
> > On 7/31/22 20:09, Jonathan Cameron wrote:
> > In our hardware board, we have "ti,hdc1080" as main source, and "silabs,si7020"
> > for 2nd source. This two chip are locate at same bus and same slave address,
> > and we want to use multiple compatibles to support both chips with single device
> > node in device tree.
> >
> > Ex:
> > compatible = "ti,hdc1099", "silabs,si7020";
> 
> This is simply broken DT, you must not put incompatible hardware on
> the same compatible string. DT is by definition the description of a
> certain platform. What you showed is a combination of incompatible
> chips in a single DT.

We were mistaken that this is the appropriate way to specify this
behavior, partially because it works as long as the probe functions
return an error the next matching driver from the compatible will probe.
It does seem that specifying two different compatibles like this would
violate the intention of the DT spec:

    The property value consists of a concatenated list of null terminated
    strings, from most specific to most general. They allow a device to
    express its compatibility with a family of similar devices, potentially
    allowing a single device driver to match against several devices.

> 
> > In order to support this, I need to add ID checking mechanism into the current
> > hdc100x driver, so the si7020 chip will fail to probe with hdc100x driver
> > (because the ID checking is not failed), then success probe with si7020.
> >
> > Base on you explanation, it looks multiple compatibles is not suitable in this
> > case? Would you mind advise us what would be the better approach for our case?
> 
> If I may advise... fix your DT by dropping the wrong compatible item.

This doesn't really give any helpful advice.

The reality is that these two chips are pin compatible and function
compatible but not driver compatible.  Boards have been manufactured
which are identical except for this chip replaced, due various to chip
shortages.

Making probe fail so that the next 'compatible' is chosen sounds like it
isn't desired.  I'm pretty sure you can't have two DT entries for the
same i2c address, but with different 'compatible" properties, and even
if we did you'd still need probe to fail on one of them.

Are there any other suggestions for being able to inform the kernel that
one of two chips might be present?

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Andy Shevchenko]

On Mon, Aug 1, 2022 at 6:12 PM Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Mon, Aug 01, 2022 at 10:22:16AM +0200, Andy Shevchenko wrote:
> > On Mon, Aug 1, 2022 at 3:52 AM Potin Lai <potin.lai.pt@gmail.com> wrote:
> > > On 7/31/22 20:09, Jonathan Cameron wrote:
> > > In our hardware board, we have "ti,hdc1080" as main source, and "silabs,si7020"
> > > for 2nd source. This two chip are locate at same bus and same slave address,
> > > and we want to use multiple compatibles to support both chips with single device
> > > node in device tree.
> > >
> > > Ex:
> > > compatible = "ti,hdc1099", "silabs,si7020";
> >
> > This is simply broken DT, you must not put incompatible hardware on
> > the same compatible string. DT is by definition the description of a
> > certain platform. What you showed is a combination of incompatible
> > chips in a single DT.
>
> We were mistaken that this is the appropriate way to specify this
> behavior, partially because it works as long as the probe functions
> return an error the next matching driver from the compatible will probe.
> It does seem that specifying two different compatibles like this would
> violate the intention of the DT spec:
>
>     The property value consists of a concatenated list of null terminated
>     strings, from most specific to most general. They allow a device to
>     express its compatibility with a family of similar devices, potentially
>     allowing a single device driver to match against several devices.
>
> >
> > > In order to support this, I need to add ID checking mechanism into the current
> > > hdc100x driver, so the si7020 chip will fail to probe with hdc100x driver
> > > (because the ID checking is not failed), then success probe with si7020.
> > >
> > > Base on you explanation, it looks multiple compatibles is not suitable in this
> > > case? Would you mind advise us what would be the better approach for our case?
> >
> > If I may advise... fix your DT by dropping the wrong compatible item.
>
> This doesn't really give any helpful advice.

Sorry to hear this, but it's the best and correct solution to your
problem. Believe me, many Linux people will tell you the same.

> The reality is that these two chips are pin compatible and function
> compatible but not driver compatible.  Boards have been manufactured
> which are identical except for this chip replaced, due various to chip
> shortages.
>
> Making probe fail so that the next 'compatible' is chosen sounds like it
> isn't desired.  I'm pretty sure you can't have two DT entries for the
> same i2c address, but with different 'compatible" properties, and even
> if we did you'd still need probe to fail on one of them.
>
> Are there any other suggestions for being able to inform the kernel that
> one of two chips might be present?

I guess there is a gap in understanding what DT is. DT is the
description of the *platform*. Changing any discrete component on the
platform is changing the platform.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Andy Shevchenko]

On Mon, Aug 1, 2022 at 6:26 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Aug 1, 2022 at 6:12 PM Patrick Williams <patrick@stwcx.xyz> wrote:
> > On Mon, Aug 01, 2022 at 10:22:16AM +0200, Andy Shevchenko wrote:
> > > On Mon, Aug 1, 2022 at 3:52 AM Potin Lai <potin.lai.pt@gmail.com> wrote:
> > > > On 7/31/22 20:09, Jonathan Cameron wrote:
> > > > In our hardware board, we have "ti,hdc1080" as main source, and "silabs,si7020"
> > > > for 2nd source. This two chip are locate at same bus and same slave address,
> > > > and we want to use multiple compatibles to support both chips with single device
> > > > node in device tree.
> > > >
> > > > Ex:
> > > > compatible = "ti,hdc1099", "silabs,si7020";
> > >
> > > This is simply broken DT, you must not put incompatible hardware on
> > > the same compatible string. DT is by definition the description of a
> > > certain platform. What you showed is a combination of incompatible
> > > chips in a single DT.
> >
> > We were mistaken that this is the appropriate way to specify this
> > behavior, partially because it works as long as the probe functions
> > return an error the next matching driver from the compatible will probe.
> > It does seem that specifying two different compatibles like this would
> > violate the intention of the DT spec:
> >
> >     The property value consists of a concatenated list of null terminated
> >     strings, from most specific to most general. They allow a device to
> >     express its compatibility with a family of similar devices, potentially
> >     allowing a single device driver to match against several devices.
> >
> > >
> > > > In order to support this, I need to add ID checking mechanism into the current
> > > > hdc100x driver, so the si7020 chip will fail to probe with hdc100x driver
> > > > (because the ID checking is not failed), then success probe with si7020.
> > > >
> > > > Base on you explanation, it looks multiple compatibles is not suitable in this
> > > > case? Would you mind advise us what would be the better approach for our case?
> > >
> > > If I may advise... fix your DT by dropping the wrong compatible item.
> >
> > This doesn't really give any helpful advice.
>
> Sorry to hear this, but it's the best and correct solution to your
> problem. Believe me, many Linux people will tell you the same.
>
> > The reality is that these two chips are pin compatible and function
> > compatible but not driver compatible.  Boards have been manufactured
> > which are identical except for this chip replaced, due various to chip
> > shortages.
> >
> > Making probe fail so that the next 'compatible' is chosen sounds like it
> > isn't desired.  I'm pretty sure you can't have two DT entries for the
> > same i2c address, but with different 'compatible" properties, and even
> > if we did you'd still need probe to fail on one of them.
> >
> > Are there any other suggestions for being able to inform the kernel that
> > one of two chips might be present?

Btw, how would it be solved in ACPI is the playing status bits by
firmware, depending on the run-time detected environment (straps,
other means). So, you may fix it on bootloader / firmware level by
patching DTB with status okay / disabled. I believe in DTB is the
number, which can be easily binary patched.

> I guess there is a gap in understanding what DT is. DT is the
> description of the *platform*. Changing any discrete component on the
> platform is changing the platform.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Jonathan Cameron]

On Mon, 1 Aug 2022 18:30:16 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Aug 1, 2022 at 6:26 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Aug 1, 2022 at 6:12 PM Patrick Williams <patrick@stwcx.xyz> wrote:  
> > > On Mon, Aug 01, 2022 at 10:22:16AM +0200, Andy Shevchenko wrote:  
> > > > On Mon, Aug 1, 2022 at 3:52 AM Potin Lai <potin.lai.pt@gmail.com> wrote:  
> > > > > On 7/31/22 20:09, Jonathan Cameron wrote:
> > > > > In our hardware board, we have "ti,hdc1080" as main source, and "silabs,si7020"
> > > > > for 2nd source. This two chip are locate at same bus and same slave address,
> > > > > and we want to use multiple compatibles to support both chips with single device
> > > > > node in device tree.
> > > > >
> > > > > Ex:
> > > > > compatible = "ti,hdc1099", "silabs,si7020";  
> > > >
> > > > This is simply broken DT, you must not put incompatible hardware on
> > > > the same compatible string. DT is by definition the description of a
> > > > certain platform. What you showed is a combination of incompatible
> > > > chips in a single DT.  
> > >
> > > We were mistaken that this is the appropriate way to specify this
> > > behavior, partially because it works as long as the probe functions
> > > return an error the next matching driver from the compatible will probe.
> > > It does seem that specifying two different compatibles like this would
> > > violate the intention of the DT spec:
> > >
> > >     The property value consists of a concatenated list of null terminated
> > >     strings, from most specific to most general. They allow a device to
> > >     express its compatibility with a family of similar devices, potentially
> > >     allowing a single device driver to match against several devices.
> > >  
> > > >  
> > > > > In order to support this, I need to add ID checking mechanism into the current
> > > > > hdc100x driver, so the si7020 chip will fail to probe with hdc100x driver
> > > > > (because the ID checking is not failed), then success probe with si7020.
> > > > >
> > > > > Base on you explanation, it looks multiple compatibles is not suitable in this
> > > > > case? Would you mind advise us what would be the better approach for our case?  
> > > >
> > > > If I may advise... fix your DT by dropping the wrong compatible item.  
> > >
> > > This doesn't really give any helpful advice.  
> >
> > Sorry to hear this, but it's the best and correct solution to your
> > problem. Believe me, many Linux people will tell you the same.
> >  
> > > The reality is that these two chips are pin compatible and function
> > > compatible but not driver compatible.  Boards have been manufactured
> > > which are identical except for this chip replaced, due various to chip
> > > shortages.
> > >
> > > Making probe fail so that the next 'compatible' is chosen sounds like it
> > > isn't desired.  I'm pretty sure you can't have two DT entries for the
> > > same i2c address, but with different 'compatible" properties, and even
> > > if we did you'd still need probe to fail on one of them.
> > >
> > > Are there any other suggestions for being able to inform the kernel that
> > > one of two chips might be present?  
> 
> Btw, how would it be solved in ACPI is the playing status bits by
> firmware, depending on the run-time detected environment (straps,
> other means). So, you may fix it on bootloader / firmware level by
> patching DTB with status okay / disabled. I believe in DTB is the
> number, which can be easily binary patched.
> 

Indeed, it's common to have boot firmware prelinux modify the DT.

That firmware can do probing if necessary to find out which device is present
and by the time Linux loads the DT should be correct for the particular
hardware.  Often this is done from a high level 'board ID' but nothing
stops you doing it this case.

I've cc'd the device tree binding maintainers and list, who may be able
to give you some useful pointers to examples of people doing this
in their boot loaders etc.

Thanks,

Jonathan


> > I guess there is a gap in understanding what DT is. DT is the
> > description of the *platform*. Changing any discrete component on the
> > platform is changing the platform.  
> 
> 

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Krzysztof Kozlowski]

On 06/08/2022 20:12, Jonathan Cameron wrote:
> On Mon, 1 Aug 2022 18:30:16 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
>> On Mon, Aug 1, 2022 at 6:26 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Mon, Aug 1, 2022 at 6:12 PM Patrick Williams <patrick@stwcx.xyz> wrote:  
>>>> On Mon, Aug 01, 2022 at 10:22:16AM +0200, Andy Shevchenko wrote:  
>>>>> On Mon, Aug 1, 2022 at 3:52 AM Potin Lai <potin.lai.pt@gmail.com> wrote:  
>>>>>> On 7/31/22 20:09, Jonathan Cameron wrote:
>>>>>> In our hardware board, we have "ti,hdc1080" as main source, and "silabs,si7020"
>>>>>> for 2nd source. This two chip are locate at same bus and same slave address,
>>>>>> and we want to use multiple compatibles to support both chips with single device
>>>>>> node in device tree.
>>>>>>
>>>>>> Ex:
>>>>>> compatible = "ti,hdc1099", "silabs,si7020";  
>>>>>
>>>>> This is simply broken DT, you must not put incompatible hardware on
>>>>> the same compatible string. DT is by definition the description of a
>>>>> certain platform. What you showed is a combination of incompatible
>>>>> chips in a single DT.  
>>>>
>>>> We were mistaken that this is the appropriate way to specify this
>>>> behavior, partially because it works as long as the probe functions
>>>> return an error the next matching driver from the compatible will probe.
>>>> It does seem that specifying two different compatibles like this would
>>>> violate the intention of the DT spec:
>>>>
>>>>     The property value consists of a concatenated list of null terminated
>>>>     strings, from most specific to most general. They allow a device to
>>>>     express its compatibility with a family of similar devices, potentially
>>>>     allowing a single device driver to match against several devices.
>>>>  
>>>>>  
>>>>>> In order to support this, I need to add ID checking mechanism into the current
>>>>>> hdc100x driver, so the si7020 chip will fail to probe with hdc100x driver
>>>>>> (because the ID checking is not failed), then success probe with si7020.
>>>>>>
>>>>>> Base on you explanation, it looks multiple compatibles is not suitable in this
>>>>>> case? Would you mind advise us what would be the better approach for our case?  
>>>>>
>>>>> If I may advise... fix your DT by dropping the wrong compatible item.  
>>>>
>>>> This doesn't really give any helpful advice.  
>>>
>>> Sorry to hear this, but it's the best and correct solution to your
>>> problem. Believe me, many Linux people will tell you the same.
>>>  
>>>> The reality is that these two chips are pin compatible and function
>>>> compatible but not driver compatible.

There is no such thing as driver compatible, in the terms of Devicetree.
Implementation does not matter. The compatibles and binding should
reflect the hardware (and its programming model).

>  Boards have been manufactured
>>>> which are identical except for this chip replaced, due various to chip
>>>> shortages.

The question is - whether the programming model (e.g. all I2C registers)
are similar or exactly the same?

>>>>
>>>> Making probe fail so that the next 'compatible' is chosen sounds like it
>>>> isn't desired. 

Yes, it is not desired because any probe failure is indication of test
failures in automated systems, so you do not develop a system which in
normal conditions has a failure.

I don't understand why you cannot include in this driver support for
second device?
Or if second device is so different, why you want to support different
hardware with the same device node. This contradicts the very basic of
Devicetree - description of hardware.

Best regards,
Krzysztof

Re: [PATCH v5 2/2] iio: humidity: hdc100x: add manufacturer and device ID check

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 11:40 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 06/08/2022 20:12, Jonathan Cameron wrote:

...

> >>>>>> compatible = "ti,hdc1099", "silabs,si7020";

...

> Or if second device is so different, why you want to support different
> hardware with the same device node. This contradicts the very basic of
> Devicetree - description of hardware.

Briefly looking into the above mentioned drivers points to the above
case, broken DT principles ==> broken DT ==> nothing to fix in the
Linux kernel.

-- 
With Best Regards,
Andy Shevchenko