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

[Andy Shevchenko]

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.

>>> +#ifdef CONFIG_ACPI
>>> +typedef int (*acpi_setup_handler)(struct spi_device *,
>>> +                              const struct
>>> adc1x8s102_platform_data **);
>>> +
>>> +static const struct adc1x8s102_platform_data int3495_platform_data =
>>> {
>>> +    .ext_vin = 5000,        /* 5 V */
>>> +};
>>> +
>>
>>> +/* Galileo Gen 2 SPI setup */
>>> +static int
>>> +adc1x8s102_setup_int3495(struct spi_device *spi,
>>> +                     const struct adc1x8s102_platform_data
>>> **pdata)
>>> +{
>>
>>> +    struct pxa2xx_spi_chip *chip_data;
>>
>> This one is too big to waste memory on one member.
>>
>>> +
>>> +    chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data),
>>> GFP_KERNEL);
>>> +    if (!chip_data)
>>> +            return -ENOMEM;
>>> +
>>> +    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.

>
>> 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.

>>> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
>>> +    { "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
>>> +    { }
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
>>> +#endif
>>> +
>>> +static int adc1x8s102_probe(struct spi_device *spi)
>>> +{
>>> +    const struct adc1x8s102_platform_data *pdata = spi-
>>>> dev.platform_data;
>>> +    struct adc1x8s102_state *st;
>>> +    struct iio_dev *indio_dev;
>>> +    int ret;
>>> +
>>
>>> +#ifdef CONFIG_ACPI
>>
>> No.
>
> ...because?

Because in correctly written ->probe() all ACPI functions have stubs
for !CONFIG_ACPI case. Just no need.

>>> +            setup_handler = (acpi_setup_handler)id->driver_data;
>>> +            if (setup_handler) {
>>> +                    ret = setup_handler(spi, &pdata);
>>> +                    if (ret)
>>> +                            return ret;
>>> +            }
>>
>> No way.
>
> Constructive feedback, please.

See above. We have nowadays mechanisms to provide device properties natively.
Without seeing DSDT I can't tell more.

>>> +++ b/include/linux/platform_data/adc1x8s102.h
>>
>> It must be no such file at all!
>> Please, remove it completely.
>
> Not without explaining what the new style is. As I said, the existing
> driver use that as well.

See above.

> The fact that there is no OF binding yet
> exploiting this should be no excuse IMHO.

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

-- 
With Best Regards,
Andy Shevchenko