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
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
