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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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