Re: [PATCH v2 3/4] iio: ad5755: added full support for devicetree

["Sean Nyekjær" <sean.nyekjaer-rjjw5hvvQKZaa/9Udqfwiw@public.gmane.org>]

On 2016-02-09 21:30, Lars-Peter Clausen wrote:
> On 02/09/2016 09:22 AM, Sean Nyekjær wrote:
>> On 2016-02-03 15:31, Sean Nyekjaer wrote:
>>> Devicetree can provide platform data
>>>
>>> Signed-off-by: Sean Nyekjaer <sean.nyekjaer-rjjw5hvvQKZaa/9Udqfwiw@public.gmane.org>
>> Lars do you have time to look at this before i'm fixing the comments from
>> Jonathan and Rob :-)
> The obvious thing is to use '-' instead of '_' since that is the DT convention.
I'm removing the defines in from the DT doc.
>
> Other than that this non-trivial. This is the first binding for such a
> device and it is always hard to come up with a good ABI without knowing what
> other similar devices might need to be supported by the same bindings. And
> it is also not always clear what is hardware description and what is driver
> policy.
I was hoping we could cooperate on enabling this feature. It's not the
majority of linux devices that are using board files any more.
>
> The external resistor configuration should be clear, that's a description of
> the hardware whether the resistor exists or not.
>
> For the others it's not so clear. E.g. the DC-to-DC converter configuration,
> this is most certainly not a description of the hardware. But we could get
> away with it by saying this is a system level design decision and is made by
> the system designer based on the operating parameters of the hardware and
> hence is policy that belongs in the DT. It might make sense to follow what
> the regulator bindings do here, even though the regulator is not exposed
> externally.
>
> The configuration of the output pin (voltage level, current level, etc) is
> certainly influenced by the restrictions of the hardware itself. You don't
> want to allow a configuration that can damage the hardware. But on the other
> hand it is still a valid usecase for some applications to switch between two
> setups at runtime, which is something you said you need to be able to do.
I can't why enabling configuration of the DAC from the DT is posing a 
greater
risk of damaging hardware than the previous way of editing the board file.
> Here we might want to use something similar to pinctrl where the devicetree
> can offer a selection of valid configurations and the driver can switch
> between those configurations.
>
> - Lars
>
> [...]
I'll submit an PATCH v3 where i have removed the defines from the DT doc 
and introduce checks for the values passed from DT.

/Sean
>>> +static struct ad5755_platform_data *ad5755_parse_dt(struct device *dev)
>>> +{
>>> +    struct device_node *np = dev->of_node;
>>> +    struct device_node *pp;
>>> +    struct ad5755_platform_data *pdata;
>>> +    unsigned int tmp;
>>> +    unsigned int tmparray[3];
>>> +    int devnr;
>>> +
>>> +    pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> +    if (!pdata)
>>> +        return NULL;
>>> +
>>> +    pdata->ext_dc_dc_compenstation_resistor =
>>> +           of_property_read_bool(np,
>>> "adi,ext_dc_dc_compenstation_resistor");
>>> +
>>> +    if (!of_property_read_u32(np, "adi,dc_dc_phase", &tmp))
>>> +        pdata->dc_dc_phase = tmp;
>>> +    else
>>> +        pdata->dc_dc_phase = AD5755_DC_DC_PHASE_ALL_SAME_EDGE;
>>> +
>>> +    if (!of_property_read_u32(np, "adi,dc_dc_freq", &tmp))
>>> +        pdata->dc_dc_freq = tmp;
>>> +    else
>>> +        pdata->dc_dc_freq = AD5755_DC_DC_FREQ_410kHZ;
>>> +
>>> +    if (!of_property_read_u32(np, "adi,dc_dc_maxv", &tmp))
>>> +        pdata->dc_dc_maxv = tmp;
>>> +    else
>>> +        pdata->dc_dc_maxv = AD5755_DC_DC_MAXV_23V;
>>> +
>>> +    devnr = 0;
>>> +    for_each_child_of_node(np, pp) {
>>> +        if (devnr > AD5755_NUM_CHANNELS) {
>>> +            dev_err(dev, "There is to many channels defined in DT\n");
>>> +            goto error_out;
>>> +        }
>>> +
>>> +        if (!of_property_read_u32(pp, "adi,mode", &tmp))
>>> +            pdata->dac[devnr].mode = tmp;
>>> +        else
>>> +            pdata->dac[devnr].mode = AD5755_MODE_CURRENT_4mA_20mA;
>>> +
>>> +        pdata->dac[devnr].ext_current_sense_resistor =
>>> +               of_property_read_bool(pp, "adi,ext_current_sense_resistor");
>>> +
>>> +        pdata->dac[devnr].enable_voltage_overrange =
>>> +            of_property_read_bool(pp, "adi,enable_voltage_overrange");
>>> +        if (!of_property_read_u32_array(pp, "adi,slew", tmparray, 3)) {
>>> +            pdata->dac[devnr].slew.enable = tmparray[0];
>>> +            pdata->dac[devnr].slew.rate = tmparray[1];
>>> +            pdata->dac[devnr].slew.step_size = tmparray[2];
>>> +        } else {
>>> +            pdata->dac[devnr].slew.enable = false;
>>> +            pdata->dac[devnr].slew.rate = AD5755_SLEW_RATE_64k;
>>> +            pdata->dac[devnr].slew.step_size = AD5755_SLEW_STEP_SIZE_1;
>>> +        }
>>> +
>>> +        devnr++;
>>> +    }
>>> +
>>> +    return pdata;
> [...]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html