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

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