Re: [PATCH 2/5] iio: at91: Use different prescal, startup mask in MR for different IP

Re: [PATCH 2/5] iio: at91: Use different prescal, startup mask in MR for different IP

[Thomas Petazzoni]

Dear Nicolas Ferre,

On Tue, 16 Jul 2013 10:46:28 +0200, Nicolas Ferre wrote:

> > Ok, that make sense. I will use compatible names for the capabilities in
> > next version. Thanks.
> 
> Hold on a little bit Josh, I know that Jean-Christophe is not in favor 
> of the use of multiple compatible strings. So, as the code is already 
> there, let's wait and see if we find another argument...

I've asked exactly this question last week at Linaro Connect during the
ARM SoC consolidation panel/discussion, where Grant Likely, Arnd
Bergmann, Olof and others were answering Device Tree related questions.

My question, which precisely had the at91-adc DT binding in mind was
precisely whether we should use different compatible properties to
identify different revisions of an IP block and let the driver handle
those differences, or whether the DT binding should provide sufficient
properties (register offsets, bit numbers, etc.) to make the driver
independent of the IP revisions. And clearly, the answer was that
different compatible properties should be used to identify the
different versions of the IP block, and the driver should abstract out
the differences. I.e, was has been done for at91-adc is completely the
opposite of the best practices for Device Tree on ARM.

See
http://www.youtube.com/watch?v=zF_AXLgkFy4&feature=player_detailpage#t=1581s
where I ask exactly this question, and get answers from Olof Johansson
and Grant Likely. They clearly say that the solution of having separate
compatible properties and a driver that handles the differences is the
way to go. So the way at91-adc (and possibly other at91 drivers) is
using the Device Tree is wrong, there should have been multiple
compatible properties. It's a shame because this is something we did say
when we submitted at91-adc and during the reviews, but the maintainer
wasn't listening to our comments...

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 2/5] iio: at91: Use different prescal, startup mask in MR for different IP

[Jonathan Cameron]

On 07/16/2013 12:30 PM, Thomas Petazzoni wrote:
> Dear Nicolas Ferre,
> 
> On Tue, 16 Jul 2013 10:46:28 +0200, Nicolas Ferre wrote:
> 
>>> Ok, that make sense. I will use compatible names for the capabilities in
>>> next version. Thanks.
>>
>> Hold on a little bit Josh, I know that Jean-Christophe is not in favor 
>> of the use of multiple compatible strings. So, as the code is already 
>> there, let's wait and see if we find another argument...
> 
> I've asked exactly this question last week at Linaro Connect during the
> ARM SoC consolidation panel/discussion, where Grant Likely, Arnd
> Bergmann, Olof and others were answering Device Tree related questions.
> 
> My question, which precisely had the at91-adc DT binding in mind was
> precisely whether we should use different compatible properties to
> identify different revisions of an IP block and let the driver handle
> those differences, or whether the DT binding should provide sufficient
> properties (register offsets, bit numbers, etc.) to make the driver
> independent of the IP revisions. And clearly, the answer was that
> different compatible properties should be used to identify the
> different versions of the IP block, and the driver should abstract out
> the differences. I.e, was has been done for at91-adc is completely the
> opposite of the best practices for Device Tree on ARM.
> 
> See
> http://www.youtube.com/watch?v=zF_AXLgkFy4&feature=player_detailpage#t=1581s
> where I ask exactly this question, and get answers from Olof Johansson
> and Grant Likely. They clearly say that the solution of having separate
> compatible properties and a driver that handles the differences is the
> way to go. So the way at91-adc (and possibly other at91 drivers) is
> using the Device Tree is wrong, there should have been multiple
> compatible properties. It's a shame because this is something we did say
> when we submitted at91-adc and during the reviews, but the maintainer
> wasn't listening to our comments...
> 

Thanks for getting some clarity on this Thomas.  So I'll ask the somewhat obvious
question - how do we unwind from where we are to where we want to be wrt to the
bindings?

Re: [PATCH 2/5] iio: at91: Use different prescal, startup mask in MR for different IP

[Thomas Petazzoni]

Dear Jonathan Cameron,

On Tue, 16 Jul 2013 20:03:38 +0100, Jonathan Cameron wrote:

> On 07/16/2013 12:30 PM, Thomas Petazzoni wrote:
> > I've asked exactly this question last week at Linaro Connect during the
> > ARM SoC consolidation panel/discussion, where Grant Likely, Arnd
> > Bergmann, Olof and others were answering Device Tree related questions.
> > 
> > My question, which precisely had the at91-adc DT binding in mind was
> > precisely whether we should use different compatible properties to
> > identify different revisions of an IP block and let the driver handle
> > those differences, or whether the DT binding should provide sufficient
> > properties (register offsets, bit numbers, etc.) to make the driver
> > independent of the IP revisions. And clearly, the answer was that
> > different compatible properties should be used to identify the
> > different versions of the IP block, and the driver should abstract out
> > the differences. I.e, was has been done for at91-adc is completely the
> > opposite of the best practices for Device Tree on ARM.
> > 
> > See
> > http://www.youtube.com/watch?v=zF_AXLgkFy4&feature=player_detailpage#t=1581s
> > where I ask exactly this question, and get answers from Olof Johansson
> > and Grant Likely. They clearly say that the solution of having separate
> > compatible properties and a driver that handles the differences is the
> > way to go. So the way at91-adc (and possibly other at91 drivers) is
> > using the Device Tree is wrong, there should have been multiple
> > compatible properties. It's a shame because this is something we did say
> > when we submitted at91-adc and during the reviews, but the maintainer
> > wasn't listening to our comments...
> > 
> 
> Thanks for getting some clarity on this Thomas.  So I'll ask the somewhat obvious
> question - how do we unwind from where we are to where we want to be wrt to the
> bindings?

During Linaro Connect last week, there was some discussion about
marking DT bindings as unstable for a little while, once they get
reviewed by a group of DT "experts" that mark them as stable. Until
they are stable, the kernel does not offer any ABI guarantees, and we
are free to change the DT bindings as needed.

Now, since this unstable/stable thing is not in place at the moment,
deciding whether to break or not existing bindings is something to be
decided by the maintainer of this platform, judging what is the best
option depending on whether there are already many users of the DT for
this platform or not, for example.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 2/5] iio: at91: Use different prescal, startup mask in MR for different IP

[Nicolas Ferre]

On 16/07/2013 21:17, Thomas Petazzoni :
> Dear Jonathan Cameron,
>
> On Tue, 16 Jul 2013 20:03:38 +0100, Jonathan Cameron wrote:
>
>> On 07/16/2013 12:30 PM, Thomas Petazzoni wrote:
>>> I've asked exactly this question last week at Linaro Connect during the
>>> ARM SoC consolidation panel/discussion, where Grant Likely, Arnd
>>> Bergmann, Olof and others were answering Device Tree related questions.
>>>
>>> My question, which precisely had the at91-adc DT binding in mind was
>>> precisely whether we should use different compatible properties to
>>> identify different revisions of an IP block and let the driver handle
>>> those differences, or whether the DT binding should provide sufficient
>>> properties (register offsets, bit numbers, etc.) to make the driver
>>> independent of the IP revisions. And clearly, the answer was that
>>> different compatible properties should be used to identify the
>>> different versions of the IP block, and the driver should abstract out
>>> the differences. I.e, was has been done for at91-adc is completely the
>>> opposite of the best practices for Device Tree on ARM.
>>>
>>> See
>>> http://www.youtube.com/watch?v=zF_AXLgkFy4&feature=player_detailpage#t=1581s
>>> where I ask exactly this question, and get answers from Olof Johansson
>>> and Grant Likely. They clearly say that the solution of having separate
>>> compatible properties and a driver that handles the differences is the
>>> way to go. So the way at91-adc (and possibly other at91 drivers) is
>>> using the Device Tree is wrong, there should have been multiple
>>> compatible properties. It's a shame because this is something we did say
>>> when we submitted at91-adc and during the reviews, but the maintainer
>>> wasn't listening to our comments...
>>>
>>
>> Thanks for getting some clarity on this Thomas.  So I'll ask the somewhat obvious
>> question - how do we unwind from where we are to where we want to be wrt to the
>> bindings?
>
> During Linaro Connect last week, there was some discussion about
> marking DT bindings as unstable for a little while, once they get
> reviewed by a group of DT "experts" that mark them as stable. Until
> they are stable, the kernel does not offer any ABI guarantees, and we
> are free to change the DT bindings as needed.
>
> Now, since this unstable/stable thing is not in place at the moment,
> deciding whether to break or not existing bindings is something to be
> decided by the maintainer of this platform, judging what is the best
> option depending on whether there are already many users of the DT for
> this platform or not, for example.

I didn't had in mind that the current discussion about the addition of 
some properties could cast doubt on the entire at91-adc binding!

The binding itself has several drawbacks and is kind of over engineered, 
I agree with that. Some register offsets in particular have nothing to 
do in a DT binding.
On the other hand, some values are highly dependent on the SoC process 
itself and can't be stored in the driver because it would require to 
change the driver for each new SoC, depending on the electrical 
characteristics.
In conclusion, we have to be cautious with this binding and make sure 
that we don't throw the baby out with the bath water.

Moreover, at the time we are just beginning to be comfortable with DT on 
AT91 and beginning to overcome the difficulty of converting our 
platforms, I see this new step on the path to "mainline + DT stable" as 
another slowdown.

Bye,
-- 
Nicolas Ferre