Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Andy Shevchenko]

+Cc: Mika

On Tue, Apr 25, 2017 at 8:44 AM, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
> On 2017-04-24 23:25, Andy Shevchenko wrote:
>> On Mon, Apr 24, 2017 at 11:32 PM, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
>>> On 2017-04-24 22:05, Andy Shevchenko wrote:
>>>> On Mon, 2017-04-24 at 21:28 +0200, Jan Kiszka wrote:
>>>>> This is an upstream port of an IIO driver for the TI ADC108S102 and
>>>>> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
>>>>> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
>>>>> included.

>>>>> +    chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
>>>>> +    spi->controller_data = chip_data;
>>>>> +    dev_info(&spi->dev, "setting GPIO CS value to %d\n",
>>>>> +             chip_data->gpio_cs);
>>>>> +    spi_setup(spi);
>>>>> +
>>>>> +    *pdata = &int3495_platform_data;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>
>>>> This is weird approach.
>>>
>>> Let me dig deeper if are allowed to pass a static struct here as well.
>>> But the struct is driver-defined.
>>
>> We have _DSD for ACPI, that's why I sent another email where I was
>> asking for DSDT excerpt and if it's already in the wild.
>
> I don't find any traces of "_DSD" in those DSDTs.

Yes, and looking into the DSDT you don't need them.

>>>> Moreover, please do not use platform data at all.
>>>
>>> That is just following pre-existing pattern, just look around in the
>>> iio/adc folder, not to speak of others. But I'm open to learn about any
>>> newer pattern there is.
>>
>> Unified Device Properties API is your friend. It makes driver to
>> consume resources in agnostic way.
>
> Is that ACPI-only or a generic solution?

It's generic as one may assume from the title.

> Where is a good example? Sorry,
> I still don't see how to make code out of your comments.

Mostly remove those ugly hacks and start over.

>> See above. We have nowadays mechanisms to provide device properties natively.
>> Without seeing DSDT I can't tell more.
>
> You've seen it, please tell me more now.

DSDT is wrong. So, it's another bug in the table. If you able to fix
it in your firmware, do it ASAP.

CS is a property of the host controller, not the slave devices.

Once I pointed to Mika's work for Galileo, perhaps you forgot. The
below is an example how to fix ACPI table using

https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/galileo/spi.asl

It's done for SPI1, but you easily can convert it to SPI0 and
corresponding properties.

Btw, we welcome any contribution to meta-acpi repository!

>> ...and I'm not talking about it at all.

> But I am: ACPI is not the center of the world (luckily), and this driver
> shall not be designed to only work with that way of defining resources.

Yes, that's correct.

-- 
With Best Regards,
Andy Shevchenko