Re: [PATCH v2] wlcore: add basic device-tree support

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

On Tue, Feb 17, 2015 at 03:39:03PM +0000, Eliad Peller wrote:
> On Mon, Feb 16, 2015 at 12:06 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Sunday 15 February 2015 13:09:10 Eliad Peller wrote:
> > s
> >> +
> >> +This node provides properties for controlling the wilink wireless device. The
> >> +node is expected to be specified as a child node to the SDIO controller that
> >> +connects the device to the system.
> >> +
> >> +Required properties:
> >> +
> >> + - compatible : Should be "ti,wlcore".
> >
> > I think you should use the specific model number here. If I understand
> > correctly, wlcore is the name of the driver that is used for multiple
> > device implementation.
> >
> right, wlcore is the common driver part of wl12xx and wl18xx device drivers.
> these DT properties are common for both.
> can't we use a common binding as well in this case?

Just have a string for each; the driver can easily match them all and
handle them identically for now until we decide/realise we need to
handle them differently later.

Regardless, the compatible string should match some real part number
and/or standard rather than the Linux driver name.

> 
> >> + - interrupt-parent : the phandle for the interrupt controller to which the
> >> +     device interrupts are connected.
> >
> > interrupt-parent should not be required
> >
> sure. i'll make it optional.
> 
> >> +&mmc3 {
> >> +     status = "okay";
> >> +     vmmc-supply = <&wlan_en_reg>;
> >> +     bus-width = <4>;
> >> +     cap-power-off-card;
> >> +     keep-power-in-suspend;
> >> +
> >> +     #address-cells = <1>;
> >> +     #size-cells = <0>;
> >> +     wlcore: wlcore@0 {
> >> +             compatible = "ti,wlcore";
> >> +             reg = <2>;
> >> +             interrupt-parent = <&gpio0>;
> >> +             interrupts = <19 IRQ_TYPE_NONE>;
> >> +     };
> >> +};
> >
> > It could make sense to specify a few extra properties here:
> >
> > - The platform data lists two clocks. How about adding them
> >   here as optional clocks so we don't need to change the binding
> >   again.
> >
> There were some very long threads previously regarding the correct way
> to describe these clocks.
> I prefer starting a working basic implementation and add the
> controversial parts later on, as needed.

This could be problematic. Elsewhere we've later added properties that
should have been mandatory from the start but were masked by some
platform data somewhere. It would be very good to avoid that.

What is problematic about describing the clock inputs to this block? Is
the problem specific to this block or to the specific clock
controller(s) it happens to be wired to?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html