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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VfzH922Z5FfGQ7gS-+FLR7H_Q3mKm_58xmchZn5KEjiqQ@mail.gmail.com>
Date:   Tue, 25 Apr 2017 00:25:00 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Jan Kiszka <jan.kiszka@...mens.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Sascha Weisenberger <sascha.weisenberger@...mens.com>
Subject: Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

On Mon, Apr 24, 2017 at 11:32 PM, Jan Kiszka <jan.kiszka@...mens.com> 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