On Mon, Feb 16, 2015 at 12:06 PM, Arnd Bergmann <arnd@xxxxxxxx> 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?
>> + - 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.
>> diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
>> index d3dd7bf..317796b 100644
>> --- a/drivers/net/wireless/ti/wlcore/sdio.c
>> +++ b/drivers/net/wireless/ti/wlcore/sdio.c
>
> Please make this a two-patch series and keep the dt binding in a separate
> patch from the driver change.
>
sure.
>> +#ifdef CONFIG_OF
>> +static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
>> +{
>> + struct device_node *np = dev->of_node;
>> + struct wl12xx_platform_data *pdata;
>> +
>> + if (!np || !of_device_is_compatible(np, "ti,wlcore")) {
>> + dev_err(dev, "No platform data set\n");
>> + return NULL;
>> + }
>> +
>> + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata)
>> + return NULL;
>
> Your method seems overly complicated. While a lot of drivers do the same
> thing, allocating a platform_data structure at probe time is really
> just extra work compared to making the platform_data optional and
> adding the required fields into the driver-private structure.
i see your point here.
however, the driver already holds (and uses) a pointer describing the
platform_data (in the non-dt case), so this patch simply takes
leverage of the current behavior (and returns a similar pointer).
thanks for the review!
Eliad.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
