[PATCH v1 1/4] dt-bindings: sc16is7xx: Add alternative clock-frequence property

[PATCH v1 1/4] dt-bindings: sc16is7xx: Add alternative clock-frequence property

[Andy Shevchenko]

For the platforms which have no clock provider for the sc16is7xx type of UART,
introduce an alternative clock-frequency property which would be used instead.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
index e7921a8e276b..b8cf38a1e43c 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
@@ -12,6 +12,8 @@ Required properties:
 - reg: I2C address of the SC16IS7xx device.
 - interrupts: Should contain the UART interrupt
 - clocks: Reference to the IC source clock.
+	OR
+- clock-frequency: The source clock frequency for the IC.
 
 Optional properties:
 - gpio-controller: Marks the device node as a GPIO controller.
-- 
2.19.2

Re: [PATCH v1 1/4] dt-bindings: sc16is7xx: Add alternative clock-frequence property

[Vladimir Zapolskiy]

Hi Andy,

On 12/05/2018 04:11 PM, Andy Shevchenko wrote:
> For the platforms which have no clock provider for the sc16is7xx type of UART,
> introduce an alternative clock-frequency property which would be used instead.

the subject has a typo in 'clock-frequence', then can you please tell me more,
how is it possible that an SC16IS7xx IC has no clock provider connected to it?

And if there is one, then please just describe it in device tree as well.

> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> index e7921a8e276b..b8cf38a1e43c 100644
> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> @@ -12,6 +12,8 @@ Required properties:
>  - reg: I2C address of the SC16IS7xx device.
>  - interrupts: Should contain the UART interrupt
>  - clocks: Reference to the IC source clock.
> +	OR
> +- clock-frequency: The source clock frequency for the IC.
>  

I strongly dislike this change, I'm inclined to cast a NAK to the series.

1. 'clock-frequency' is a very specific device tree property, in my opinion
    its presence is justified on sort of clock provider devices only (like I2C
    controllers), unfortunately the property was added to a number of device
    tree bindings improperly, mainly it was done before introduction of
    "assigned-clocks" and "assigned-clock-parents" properties in CCF, and then
    it was blindly copied.

2. SC16IS7xx type of UARTs is a regular clock consumer, ICs always have a valid
   clock provider connected to XTAL1 (and XTAL2 in case of a connected
   crystal oscillator), thus, if needed, the driver can get input clock rate
   by calling standard clk_get_rate(), so the presence of the required 'clocks'
   property is sufficient.

3. In some very specific corner cases it might be needed to add another
   "assigned-clocks" and "assigned-clock-parents" properties to a particular
   device node on a particular board, but their explicit description in device
   tree bindings is not needed.

--
Best wishes,
Vladimir

>  Optional properties:
>  - gpio-controller: Marks the device node as a GPIO controller.
> 

Re: [PATCH v1 1/4] dt-bindings: sc16is7xx: Add alternative clock-frequence property

[Andy Shevchenko]

On Fri, Dec 07, 2018 at 10:18:26AM +0200, Vladimir Zapolskiy wrote:
> Hi Andy,
> 
> On 12/05/2018 04:11 PM, Andy Shevchenko wrote:
> > For the platforms which have no clock provider for the sc16is7xx type of UART,
> > introduce an alternative clock-frequency property which would be used instead.
> 
> the subject has a typo in 'clock-frequence', then can you please tell me more,
> how is it possible that an SC16IS7xx IC has no clock provider connected to it?

I better ask Grigorii about this, since I have no hardware at my possession.

> And if there is one, then please just describe it in device tree as well.

Tell me how to do this for ACPI?

> 
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > index e7921a8e276b..b8cf38a1e43c 100644
> > --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
> > @@ -12,6 +12,8 @@ Required properties:
> >  - reg: I2C address of the SC16IS7xx device.
> >  - interrupts: Should contain the UART interrupt
> >  - clocks: Reference to the IC source clock.
> > +	OR
> > +- clock-frequency: The source clock frequency for the IC.
> >  
> 
> I strongly dislike this change, I'm inclined to cast a NAK to the series.

To be productive, please propose the alternative, otherwise your NAK is nothing
to do with a real hardware and approach I proposed.

> 
> 1. 'clock-frequency' is a very specific device tree property, in my opinion
>     its presence is justified on sort of clock provider devices only (like I2C
>     controllers), unfortunately the property was added to a number of device
>     tree bindings improperly, mainly it was done before introduction of
>     "assigned-clocks" and "assigned-clock-parents" properties in CCF, and then
>     it was blindly copied.

OK, I will wait for your patch to remove such from, for example, 8250_dw.c
where same problem had been targeted in the same way.

> 
> 2. SC16IS7xx type of UARTs is a regular clock consumer, ICs always have a valid
>    clock provider connected to XTAL1 (and XTAL2 in case of a connected
>    crystal oscillator), thus, if needed, the driver can get input clock rate
>    by calling standard clk_get_rate(), so the presence of the required 'clocks'
>    property is sufficient.

So what?
There is a hardware, there is a clock provider hidden in it. How you would
describe it? Platform data? Why?

> 3. In some very specific corner cases it might be needed to add another
>    "assigned-clocks" and "assigned-clock-parents" properties to a particular
>    device node on a particular board, but their explicit description in device
>    tree bindings is not needed.

Can DT people once in life think outside the box?!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] dt-bindings: sc16is7xx: Add alternative clock-frequence property

[Andy Shevchenko]

On Fri, Dec 07, 2018 at 03:48:44PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 07, 2018 at 10:18:26AM +0200, Vladimir Zapolskiy wrote:
> > On 12/05/2018 04:11 PM, Andy Shevchenko wrote:
> > > For the platforms which have no clock provider for the sc16is7xx type of UART,
> > > introduce an alternative clock-frequency property which would be used instead.
> > 
> > the subject has a typo in 'clock-frequence', then can you please tell me more,
> > how is it possible that an SC16IS7xx IC has no clock provider connected to it?
> 
> I better ask Grigorii about this, since I have no hardware at my possession.

My apologizes, I meant Georgii!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] dt-bindings: sc16is7xx: Add alternative clock-frequence property

[Vladimir Zapolskiy]

On 12/07/2018 03:48 PM, Andy Shevchenko wrote:
> On Fri, Dec 07, 2018 at 10:18:26AM +0200, Vladimir Zapolskiy wrote:
>> Hi Andy,
>>
>> On 12/05/2018 04:11 PM, Andy Shevchenko wrote:
>>> For the platforms which have no clock provider for the sc16is7xx type of UART,
>>> introduce an alternative clock-frequency property which would be used instead.
>>
>> the subject has a typo in 'clock-frequence', then can you please tell me more,
>> how is it possible that an SC16IS7xx IC has no clock provider connected to it?
> 
> I better ask Grigorii about this, since I have no hardware at my possession.
> 
>> And if there is one, then please just describe it in device tree as well.
> 
> Tell me how to do this for ACPI?
> 

I didn't grasp the connection between your update of IC device tree bindings
and ACPI, please elaborate in the context of updating device tree bindings
documentation and supported properties.

I do care about purity of device tree bindings of SC16IS7xx IC, which is
found on some of my boards with description in DTB, that's why I object to
this series.

>>
>>>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: devicetree@vger.kernel.org
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> ---
>>>  Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>> index e7921a8e276b..b8cf38a1e43c 100644
>>> --- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>> +++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
>>> @@ -12,6 +12,8 @@ Required properties:
>>>  - reg: I2C address of the SC16IS7xx device.
>>>  - interrupts: Should contain the UART interrupt
>>>  - clocks: Reference to the IC source clock.
>>> +	OR
>>> +- clock-frequency: The source clock frequency for the IC.
>>>  
>>
>> I strongly dislike this change, I'm inclined to cast a NAK to the series.
> 
> To be productive, please propose the alternative, otherwise your NAK is nothing
> to do with a real hardware and approach I proposed.

As I've said 'clock-frequency' property is not a hardware property of SC16IS7xx
IC, it is a hardware property of some external hardware component, it may
provide a volatile clock rate, which you miss, and it should be described
separately in DT.

The current approach with 'clocks' property addresses all cases perfectly,
even if your change is an attempt to solve some actual problem, you haven't
managed to describe it in the commit message.

NAK for the added property, which makes obtaining of the clock supplier
frequency equivocal.

>>
>> 1. 'clock-frequency' is a very specific device tree property, in my opinion
>>     its presence is justified on sort of clock provider devices only (like I2C
>>     controllers), unfortunately the property was added to a number of device
>>     tree bindings improperly, mainly it was done before introduction of
>>     "assigned-clocks" and "assigned-clock-parents" properties in CCF, and then
>>     it was blindly copied.
> 
> OK, I will wait for your patch to remove such from, for example, 8250_dw.c
> where same problem had been targeted in the same way.

I'm not interested to fix legacy device tree binding issues added in 2011,
equally I'm not going to close my eyes on right the same issues, when someone
attempts to spread them further today.

>>
>> 2. SC16IS7xx type of UARTs is a regular clock consumer, ICs always have a valid
>>    clock provider connected to XTAL1 (and XTAL2 in case of a connected
>>    crystal oscillator), thus, if needed, the driver can get input clock rate
>>    by calling standard clk_get_rate(), so the presence of the required 'clocks'
>>    property is sufficient.
> 
> So what?
> There is a hardware, there is a clock provider hidden in it. How you would
> describe it? Platform data? Why?
> 

What do you mean by 'hardware'? PCB, SC16IS7xx IC or something else?

What do you mean by 'a clock provider hidden in it'?

Please find the hidden clock provided and describe it in a proper way, for DTS
changes please reference to Documentation/devicetree/bindings/clock contents.

>> 3. In some very specific corner cases it might be needed to add another
>>    "assigned-clocks" and "assigned-clock-parents" properties to a particular
>>    device node on a particular board, but their explicit description in device
>>    tree bindings is not needed.
> 
> Can DT people once in life think outside the box?!
> 

The rhetorical question doesn't sound like a nice supporting argument of
your change.

Please don't slip into arrogance, and please concentrate on technical aspects.

NAK to the change.

--
Best wishes,
Vladimir

Re: [PATCH v1 1/4] dt-bindings: sc16is7xx: Add alternative clock-frequence property

[Andy Shevchenko]

(Cc: Rafael and linux-acpi. Rafael, a short context for you, there is a device
 connected externally to the IoT ACPI-enabled x86-board via I2C bus. The driver
 needs a clock frequency used inside that device to perform correctly, since
 ACPI has not yet concept of clock provider I proposed to add a property widely
 used for other IPs in a similar way, but DT people strongly reject my
 approach. If you may have a chance to look and maybe suggest the approach
 which satisfies both sides, it would be really nice!)

On Fri, Dec 07, 2018 at 06:05:14PM +0200, Vladimir Zapolskiy wrote:
> On 12/07/2018 03:48 PM, Andy Shevchenko wrote:
> > On Fri, Dec 07, 2018 at 10:18:26AM +0200, Vladimir Zapolskiy wrote:
> >> On 12/05/2018 04:11 PM, Andy Shevchenko wrote:
> >>> For the platforms which have no clock provider for the sc16is7xx type of UART,
> >>> introduce an alternative clock-frequency property which would be used instead.
> >>
> >> the subject has a typo in 'clock-frequence', then can you please tell me more,
> >> how is it possible that an SC16IS7xx IC has no clock provider connected to it?
> > 
> > I better ask Grigorii about this, since I have no hardware at my possession.
> > 
> >> And if there is one, then please just describe it in device tree as well.
> > 
> > Tell me how to do this for ACPI?
> > 
> 
> I didn't grasp the connection between your update of IC device tree bindings
> and ACPI, please elaborate in the context of updating device tree bindings
> documentation and supported properties.

OK.

> I do care about purity of device tree bindings of SC16IS7xx IC, which is
> found on some of my boards with description in DTB, that's why I object to
> this series.

I also support the purity of many drivers and modules in the software (Linux
kernel), but because of existing hardware and customers I can't reject their
needs. That's why, unfortunately, the drivers' code is full of quirks.

If I would object on each of those cases, I would end up with the OS which
supports almost nothing.

> >>> +- clock-frequency: The source clock frequency for the IC.
> >>>  
> >>
> >> I strongly dislike this change, I'm inclined to cast a NAK to the series.
> > 
> > To be productive, please propose the alternative, otherwise your NAK is nothing
> > to do with a real hardware and approach I proposed.
> 
> As I've said 'clock-frequency' property is not a hardware property of SC16IS7xx
> IC, it is a hardware property of some external hardware component, it may
> provide a volatile clock rate, which you miss, and it should be described
> separately in DT.

I disagree with you.

Crystal or PLL or another clock source, even being external component, still is
internal to the blackbox called UART-I2C adapter.

> The current approach with 'clocks' property addresses all cases perfectly,
> even if your change is an attempt to solve some actual problem, you haven't
> managed to describe it in the commit message.

I will fix the commit message. I agree it's not perfect.

> NAK for the added property, which makes obtaining of the clock supplier
> frequency equivocal.

Your NAK is nothing w/o proposed alternative which would work in a case of ACPI
enabled platform.

> >> 1. 'clock-frequency' is a very specific device tree property, in my opinion
> >>     its presence is justified on sort of clock provider devices only (like I2C
> >>     controllers), unfortunately the property was added to a number of device
> >>     tree bindings improperly, mainly it was done before introduction of
> >>     "assigned-clocks" and "assigned-clock-parents" properties in CCF, and then
> >>     it was blindly copied.
> > 
> > OK, I will wait for your patch to remove such from, for example, 8250_dw.c
> > where same problem had been targeted in the same way.
> 
> I'm not interested to fix legacy device tree binding issues added in 2011,
> equally I'm not going to close my eyes on right the same issues, when someone
> attempts to spread them further today.

Any proposal, be constructive, please.

> >> 2. SC16IS7xx type of UARTs is a regular clock consumer, ICs always have a valid
> >>    clock provider connected to XTAL1 (and XTAL2 in case of a connected
> >>    crystal oscillator), thus, if needed, the driver can get input clock rate
> >>    by calling standard clk_get_rate(), so the presence of the required 'clocks'
> >>    property is sufficient.
> > 
> > So what?
> > There is a hardware, there is a clock provider hidden in it. How you would
> > describe it? Platform data? Why?
> > 
> 
> What do you mean by 'hardware'? PCB, SC16IS7xx IC or something else?
> 
> What do you mean by 'a clock provider hidden in it'?
> 
> Please find the hidden clock provided and describe it in a proper way, for DTS
> changes please reference to Documentation/devicetree/bindings/clock contents.

Again, try to think about the world not being just arm + dts.

> >> 3. In some very specific corner cases it might be needed to add another
> >>    "assigned-clocks" and "assigned-clock-parents" properties to a particular
> >>    device node on a particular board, but their explicit description in device
> >>    tree bindings is not needed.
> > 
> > Can DT people once in life think outside the box?!
> > 
> 
> The rhetorical question doesn't sound like a nice supporting argument of
> your change.
> 
> Please don't slip into arrogance, and please concentrate on technical aspects.

Please, read your sentence above and tell me what the alternative I have?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] dt-bindings: sc16is7xx: Add alternative clock-frequence property

[Rob Herring]

On Fri, Dec 07, 2018 at 07:09:56PM +0200, Andy Shevchenko wrote:
> (Cc: Rafael and linux-acpi. Rafael, a short context for you, there is a device
>  connected externally to the IoT ACPI-enabled x86-board via I2C bus. The driver
>  needs a clock frequency used inside that device to perform correctly, since
>  ACPI has not yet concept of clock provider I proposed to add a property widely
>  used for other IPs in a similar way, but DT people strongly reject my
>  approach. If you may have a chance to look and maybe suggest the approach
>  which satisfies both sides, it would be really nice!)

One person does not equate to 'DT people'.

Actually, I'm fine with this change. I don't think we should necessarily 
require using the clock binding in simple cases just needing to know 
what a particular clock frequency is.

However, ACPI not having a clock provider is not an argument for DT 
bindings. The whole notion of sharing them in the first place is flawed.

Rob

> 
> On Fri, Dec 07, 2018 at 06:05:14PM +0200, Vladimir Zapolskiy wrote:
> > On 12/07/2018 03:48 PM, Andy Shevchenko wrote:
> > > On Fri, Dec 07, 2018 at 10:18:26AM +0200, Vladimir Zapolskiy wrote:
> > >> On 12/05/2018 04:11 PM, Andy Shevchenko wrote:
> > >>> For the platforms which have no clock provider for the sc16is7xx type of UART,
> > >>> introduce an alternative clock-frequency property which would be used instead.
> > >>
> > >> the subject has a typo in 'clock-frequence', then can you please tell me more,
> > >> how is it possible that an SC16IS7xx IC has no clock provider connected to it?
> > > 
> > > I better ask Grigorii about this, since I have no hardware at my possession.
> > > 
> > >> And if there is one, then please just describe it in device tree as well.
> > > 
> > > Tell me how to do this for ACPI?
> > > 
> > 
> > I didn't grasp the connection between your update of IC device tree bindings
> > and ACPI, please elaborate in the context of updating device tree bindings
> > documentation and supported properties.
> 
> OK.
> 
> > I do care about purity of device tree bindings of SC16IS7xx IC, which is
> > found on some of my boards with description in DTB, that's why I object to
> > this series.
> 
> I also support the purity of many drivers and modules in the software (Linux
> kernel), but because of existing hardware and customers I can't reject their
> needs. That's why, unfortunately, the drivers' code is full of quirks.
> 
> If I would object on each of those cases, I would end up with the OS which
> supports almost nothing.
> 
> > >>> +- clock-frequency: The source clock frequency for the IC.
> > >>>  
> > >>
> > >> I strongly dislike this change, I'm inclined to cast a NAK to the series.
> > > 
> > > To be productive, please propose the alternative, otherwise your NAK is nothing
> > > to do with a real hardware and approach I proposed.
> > 
> > As I've said 'clock-frequency' property is not a hardware property of SC16IS7xx
> > IC, it is a hardware property of some external hardware component, it may
> > provide a volatile clock rate, which you miss, and it should be described
> > separately in DT.
> 
> I disagree with you.
> 
> Crystal or PLL or another clock source, even being external component, still is
> internal to the blackbox called UART-I2C adapter.
> 
> > The current approach with 'clocks' property addresses all cases perfectly,
> > even if your change is an attempt to solve some actual problem, you haven't
> > managed to describe it in the commit message.
> 
> I will fix the commit message. I agree it's not perfect.
> 
> > NAK for the added property, which makes obtaining of the clock supplier
> > frequency equivocal.
> 
> Your NAK is nothing w/o proposed alternative which would work in a case of ACPI
> enabled platform.
> 
> > >> 1. 'clock-frequency' is a very specific device tree property, in my opinion
> > >>     its presence is justified on sort of clock provider devices only (like I2C
> > >>     controllers), unfortunately the property was added to a number of device
> > >>     tree bindings improperly, mainly it was done before introduction of
> > >>     "assigned-clocks" and "assigned-clock-parents" properties in CCF, and then
> > >>     it was blindly copied.
> > > 
> > > OK, I will wait for your patch to remove such from, for example, 8250_dw.c
> > > where same problem had been targeted in the same way.
> > 
> > I'm not interested to fix legacy device tree binding issues added in 2011,
> > equally I'm not going to close my eyes on right the same issues, when someone
> > attempts to spread them further today.
> 
> Any proposal, be constructive, please.
> 
> > >> 2. SC16IS7xx type of UARTs is a regular clock consumer, ICs always have a valid
> > >>    clock provider connected to XTAL1 (and XTAL2 in case of a connected
> > >>    crystal oscillator), thus, if needed, the driver can get input clock rate
> > >>    by calling standard clk_get_rate(), so the presence of the required 'clocks'
> > >>    property is sufficient.
> > > 
> > > So what?
> > > There is a hardware, there is a clock provider hidden in it. How you would
> > > describe it? Platform data? Why?
> > > 
> > 
> > What do you mean by 'hardware'? PCB, SC16IS7xx IC or something else?
> > 
> > What do you mean by 'a clock provider hidden in it'?
> > 
> > Please find the hidden clock provided and describe it in a proper way, for DTS
> > changes please reference to Documentation/devicetree/bindings/clock contents.
> 
> Again, try to think about the world not being just arm + dts.
> 
> > >> 3. In some very specific corner cases it might be needed to add another
> > >>    "assigned-clocks" and "assigned-clock-parents" properties to a particular
> > >>    device node on a particular board, but their explicit description in device
> > >>    tree bindings is not needed.
> > > 
> > > Can DT people once in life think outside the box?!
> > > 
> > 
> > The rhetorical question doesn't sound like a nice supporting argument of
> > your change.
> > 
> > Please don't slip into arrogance, and please concentrate on technical aspects.
> 
> Please, read your sentence above and tell me what the alternative I have?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v1 1/4] dt-bindings: sc16is7xx: Add alternative clock-frequence property

[Andy Shevchenko]

On Wed, Dec 19, 2018 at 02:27:34PM -0600, Rob Herring wrote:
> On Fri, Dec 07, 2018 at 07:09:56PM +0200, Andy Shevchenko wrote:
> > (Cc: Rafael and linux-acpi. Rafael, a short context for you, there is a device
> >  connected externally to the IoT ACPI-enabled x86-board via I2C bus. The driver
> >  needs a clock frequency used inside that device to perform correctly, since
> >  ACPI has not yet concept of clock provider I proposed to add a property widely
> >  used for other IPs in a similar way, but DT people strongly reject my
> >  approach. If you may have a chance to look and maybe suggest the approach
> >  which satisfies both sides, it would be really nice!)
> 
> One person does not equate to 'DT people'.

Sorry for that.

> Actually, I'm fine with this change. I don't think we should necessarily 
> require using the clock binding in simple cases just needing to know 
> what a particular clock frequency is.
> 
> However, ACPI not having a clock provider is not an argument for DT 
> bindings. The whole notion of sharing them in the first place is flawed.

Yeah, I can't object this. However we have a real customer with real needs.
Perhaps some additional warning should be added to code / device binding to
make clear that this is temporary and exclusively to fill the gap of ACPI
coverage?

> Rob

Thanks for review!
I will rebase and resend later this series.

> 
> > 
> > On Fri, Dec 07, 2018 at 06:05:14PM +0200, Vladimir Zapolskiy wrote:
> > > On 12/07/2018 03:48 PM, Andy Shevchenko wrote:
> > > > On Fri, Dec 07, 2018 at 10:18:26AM +0200, Vladimir Zapolskiy wrote:
> > > >> On 12/05/2018 04:11 PM, Andy Shevchenko wrote:
> > > >>> For the platforms which have no clock provider for the sc16is7xx type of UART,
> > > >>> introduce an alternative clock-frequency property which would be used instead.
> > > >>
> > > >> the subject has a typo in 'clock-frequence', then can you please tell me more,
> > > >> how is it possible that an SC16IS7xx IC has no clock provider connected to it?
> > > > 
> > > > I better ask Grigorii about this, since I have no hardware at my possession.
> > > > 
> > > >> And if there is one, then please just describe it in device tree as well.
> > > > 
> > > > Tell me how to do this for ACPI?
> > > > 
> > > 
> > > I didn't grasp the connection between your update of IC device tree bindings
> > > and ACPI, please elaborate in the context of updating device tree bindings
> > > documentation and supported properties.
> > 
> > OK.
> > 
> > > I do care about purity of device tree bindings of SC16IS7xx IC, which is
> > > found on some of my boards with description in DTB, that's why I object to
> > > this series.
> > 
> > I also support the purity of many drivers and modules in the software (Linux
> > kernel), but because of existing hardware and customers I can't reject their
> > needs. That's why, unfortunately, the drivers' code is full of quirks.
> > 
> > If I would object on each of those cases, I would end up with the OS which
> > supports almost nothing.
> > 
> > > >>> +- clock-frequency: The source clock frequency for the IC.
> > > >>>  
> > > >>
> > > >> I strongly dislike this change, I'm inclined to cast a NAK to the series.
> > > > 
> > > > To be productive, please propose the alternative, otherwise your NAK is nothing
> > > > to do with a real hardware and approach I proposed.
> > > 
> > > As I've said 'clock-frequency' property is not a hardware property of SC16IS7xx
> > > IC, it is a hardware property of some external hardware component, it may
> > > provide a volatile clock rate, which you miss, and it should be described
> > > separately in DT.
> > 
> > I disagree with you.
> > 
> > Crystal or PLL or another clock source, even being external component, still is
> > internal to the blackbox called UART-I2C adapter.
> > 
> > > The current approach with 'clocks' property addresses all cases perfectly,
> > > even if your change is an attempt to solve some actual problem, you haven't
> > > managed to describe it in the commit message.
> > 
> > I will fix the commit message. I agree it's not perfect.
> > 
> > > NAK for the added property, which makes obtaining of the clock supplier
> > > frequency equivocal.
> > 
> > Your NAK is nothing w/o proposed alternative which would work in a case of ACPI
> > enabled platform.
> > 
> > > >> 1. 'clock-frequency' is a very specific device tree property, in my opinion
> > > >>     its presence is justified on sort of clock provider devices only (like I2C
> > > >>     controllers), unfortunately the property was added to a number of device
> > > >>     tree bindings improperly, mainly it was done before introduction of
> > > >>     "assigned-clocks" and "assigned-clock-parents" properties in CCF, and then
> > > >>     it was blindly copied.
> > > > 
> > > > OK, I will wait for your patch to remove such from, for example, 8250_dw.c
> > > > where same problem had been targeted in the same way.
> > > 
> > > I'm not interested to fix legacy device tree binding issues added in 2011,
> > > equally I'm not going to close my eyes on right the same issues, when someone
> > > attempts to spread them further today.
> > 
> > Any proposal, be constructive, please.
> > 
> > > >> 2. SC16IS7xx type of UARTs is a regular clock consumer, ICs always have a valid
> > > >>    clock provider connected to XTAL1 (and XTAL2 in case of a connected
> > > >>    crystal oscillator), thus, if needed, the driver can get input clock rate
> > > >>    by calling standard clk_get_rate(), so the presence of the required 'clocks'
> > > >>    property is sufficient.
> > > > 
> > > > So what?
> > > > There is a hardware, there is a clock provider hidden in it. How you would
> > > > describe it? Platform data? Why?
> > > > 
> > > 
> > > What do you mean by 'hardware'? PCB, SC16IS7xx IC or something else?
> > > 
> > > What do you mean by 'a clock provider hidden in it'?
> > > 
> > > Please find the hidden clock provided and describe it in a proper way, for DTS
> > > changes please reference to Documentation/devicetree/bindings/clock contents.
> > 
> > Again, try to think about the world not being just arm + dts.
> > 
> > > >> 3. In some very specific corner cases it might be needed to add another
> > > >>    "assigned-clocks" and "assigned-clock-parents" properties to a particular
> > > >>    device node on a particular board, but their explicit description in device
> > > >>    tree bindings is not needed.
> > > > 
> > > > Can DT people once in life think outside the box?!
> > > > 
> > > 
> > > The rhetorical question doesn't sound like a nice supporting argument of
> > > your change.
> > > 
> > > Please don't slip into arrogance, and please concentrate on technical aspects.
> > 
> > Please, read your sentence above and tell me what the alternative I have?
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> > 

-- 
With Best Regards,
Andy Shevchenko