Re: [PATCH] net: rfkill-regulator: Add devicetree support.

[Belisko Marek]

On Mon, Feb 10, 2014 at 11:18 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Fri, Feb 07, 2014 at 07:48:49PM +0000, Marek Belisko wrote:
>> Signed-off-by: NeilBrown <neilb@xxxxxxx>
>> Signed-off-by: Marek Belisko <marek@xxxxxxxxxxxxx>
>> ---
>> Based on Neil's patch and extend for documentation and bindings include.
>>
>>  .../bindings/net/rfkill/rfkill-relugator.txt       | 28 ++++++++++++++++
>>  include/dt-bindings/net/rfkill-regulator.h         | 23 +++++++++++++
>>  net/rfkill/rfkill-regulator.c                      | 38 ++++++++++++++++++++++
>>  3 files changed, 89 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/net/rfkill/rfkill-relugator.txt
>>  create mode 100644 include/dt-bindings/net/rfkill-regulator.h
>>
>> diff --git a/Documentation/devicetree/bindings/net/rfkill/rfkill-relugator.txt b/Documentation/devicetree/bindings/net/rfkill/rfkill-relugator.txt
>> new file mode 100644
>> index 0000000..cdb7dd7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/rfkill/rfkill-relugator.txt
>> @@ -0,0 +1,28 @@
>> +Regulator consumer for rfkill devices
>
> What exactly is an "rfkill" device? How is it used? How does it relate
> to other devices in the DT?
>
> To me, this looks like a leak of a Linux abstraction.
>
>> +
>> +Required properties:
>> +- compatible   : Must be "rfkill-regulator".
>> +- label  : Name of rfkill device.
>
> What's this for? Why does this need a label in the DT? Surely this can
> be implied by the relationship to a particular radio device?
This label is used by rfkill (converted to pdata->name in probe
function) and used for displaying.
Maybe label isn't correct name for that purpose.
>
>> +- type  : Type of rfkill device.
>> +
>> +Possible values (defined in include/dt-bindings/net/rfkill-regulator.h):
>> +     RFKILL_TYPE_ALL
>> +     RFKILL_TYPE_WLAN
>> +     RFKILL_TYPE_BLUETOOTH
>> +     RFKILL_TYPE_UWB
>> +     RFKILL_TYPE_WIMAX
>> +     RFKILL_TYPE_WWAN
>> +     RFKILL_TYPE_GPS
>> +     RFKILL_TYPE_FM
>> +     RFKILL_TYPE_NFC
>
> What do these mean? Why can these not be implied by a relationship to
> any devices of these particular types?
I did platform data -> DT mapping 1 : 1. Maybe we don't need to export
those to separate
include file and only use raw number instead.
>
>> +
>> +- vrfkill-supply - regulator device.
>
> Why isn't this described on the radio revice node? It's a supply to the
> radio, not to the rfkill concept.
rfkill-regulator in probe check for vrfkill regulator so I've added it
to description as without that rfkill-regulator doesn't make sense.
>
>> +
>> +Example:
>> +     gps-rfkill {
>> +             compatible = "rfkill-regulator";
>> +             label = "GPS";
>> +             type = <RFKILL_TYPE_GPS>;
>> +             vrfkill-supply = <&reg>;
>> +     };
>
> Why is this not bound to the particular GPS device in some way?
We can do something like:
gps-device {
    compatible = "my-desired-gps";
    <other device properties>
    rfkill = <&gps-rfkill>;
};
>
> What if I have more than one of any of the types of device this
> supports, which device is this expected to control?
rfkill-regulator is linked with regulator so if you have another
device it is probably controlled
with another regulator.

>
> Why is it described as a separate device in the device tree at all?
>
> I do not think this binding is the right way to describe this.
Some time ago was posted rfkill-gpio DT binding conversion and was
using the nearly the same bindings as
we propose and there was no issue with that.

>
> Thanks,
> Mark.
>
>> +
>> diff --git a/include/dt-bindings/net/rfkill-regulator.h b/include/dt-bindings/net/rfkill-regulator.h
>> new file mode 100644
>> index 0000000..ae32273
>> --- /dev/null
>> +++ b/include/dt-bindings/net/rfkill-regulator.h
>> @@ -0,0 +1,23 @@
>> +/*
>> + * This header provides macros for rfkill-regulator bindings.
>> + *
>> + * Copyright (C) 2014 Marek Belisko <marek@xxxxxxxxxxxxx>
>> + *
>> + * GPLv2 only
>> + */
>> +
>> +#ifndef __DT_BINDINGS_RFKILL_REGULATOR_H__
>> +#define __DT_BINDINGS_RFKILL_REGULATOR_H__
>> +
>> +
>> +#define RFKILL_TYPE_ALL              (0)
>> +#define RFKILL_TYPE_WLAN     (1)
>> +#define RFKILL_TYPE_BLUETOOTH        (2)
>> +#define RFKILL_TYPE_UWB              (3)
>> +#define RFKILL_TYPE_WIMAX    (4)
>> +#define RFKILL_TYPE_WWAN     (5)
>> +#define RFKILL_TYPE_GPS              (6)
>> +#define RFKILL_TYPE_FM               (7)
>> +#define RFKILL_TYPE_NFC              (8)
>> +
>> +#endif /* __DT_BINDINGS_RFKILL_REGULATOR_H__ */
>> diff --git a/net/rfkill/rfkill-regulator.c b/net/rfkill/rfkill-regulator.c
>> index cf5b145..a04aff8 100644
>> --- a/net/rfkill/rfkill-regulator.c
>> +++ b/net/rfkill/rfkill-regulator.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/rfkill.h>
>>  #include <linux/rfkill-regulator.h>
>> +#include <linux/of_platform.h>
>>
>>  struct rfkill_regulator_data {
>>       struct rfkill *rf_kill;
>> @@ -57,6 +58,31 @@ static struct rfkill_ops rfkill_regulator_ops = {
>>       .set_block = rfkill_regulator_set_block,
>>  };
>>
>> +#ifdef CONFIG_OF
>> +static struct rfkill_regulator_platform_data *
>> +rfkill_regulator_parse_pdata(struct device *dev)
>> +{
>> +     struct rfkill_regulator_platform_data *pdata;
>> +     struct device_node *np = dev->of_node;
>> +     u32 num;
>> +     if (!np)
>> +             return NULL;
>> +     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +     if (!pdata)
>> +             return NULL;
>> +     if (of_property_read_u32(np, "type", &num) == 0)
>> +             pdata->type = num;
>> +     of_property_read_string(np, "label", &pdata->name);
>> +     return pdata;
>> +}
>> +#else
>> +static inline struct rfkill_regulator_platform_data *
>> +rfkill_regulator_parse_pdata(struct device *dev)
>> +{
>> +     return NULL;
>> +}
>> +#endif
>> +
>>  static int rfkill_regulator_probe(struct platform_device *pdev)
>>  {
>>       struct rfkill_regulator_platform_data *pdata = pdev->dev.platform_data;
>> @@ -65,6 +91,9 @@ static int rfkill_regulator_probe(struct platform_device *pdev)
>>       struct rfkill *rf_kill;
>>       int ret = 0;
>>
>> +     if (!pdata)
>> +             pdata = rfkill_regulator_parse_pdata(&pdev->dev);
>> +
>>       if (pdata == NULL) {
>>               dev_err(&pdev->dev, "no platform data\n");
>>               return -ENODEV;
>> @@ -137,12 +166,21 @@ static int rfkill_regulator_remove(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id rfkill_regulator_match[] = {
>> +     {.compatible = "rfkill-regulator"},
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(of, rfkill_regulator_match);
>> +#endif
>> +
>>  static struct platform_driver rfkill_regulator_driver = {
>>       .probe = rfkill_regulator_probe,
>>       .remove = rfkill_regulator_remove,
>>       .driver = {
>>               .name = "rfkill-regulator",
>>               .owner = THIS_MODULE,
>> +             .of_match_table = of_match_ptr(rfkill_regulator_match),
>>       },
>>  };
>>
>> --
>> 1.8.3.2
>>
>>

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/