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

[Andy Shevchenko]

On Wed, May 3, 2017 at 11:24 AM, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
> On 2017-05-03 10:06, Andy Shevchenko wrote:
>> On Wed, May 3, 2017 at 8:35 AM, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> wrote:
>>> On 2017-05-02 22:53, Andy Shevchenko wrote:

>>>>> +       ret = device_property_read_u32(&spi->dev, "ext-vin-microvolt", &val);
>>>>
>>>> Why not to read u16 here?
>>>>
>>>
>>> Can I read a property with arbitrary width? Then this would simplify
>>> things.
>>
>> device_property_read_u16();
>
> By now I found out that ACPI does not care about the property type
> width, only DT does. So reading it as u16 under ACPI would be fine. But
> with DT, we will need a correctly sized property as well - default is u32.

I'm a bit lost here. ACPI cares and DT cares about property size. You
need to agreed what to use with DT people I think based on two
aspects:
- what existing property is using, if any
- what is preferable size to use

API allows to read any size. If you stick with u32 then perhaps better
to change internal variable to be u32 as well.

>>> Or do I have to follow how it was defined in the ACPI or device
>>> tree world?
>>
>> For property by the way you have to either follow existing one (by
>> name and meaning), or you
>> you need get an Ack from DT people (Rob, for example) to introduce such.
>>
>> Existing one is called "vref-external". So, definitely you need to
>> figure out this with DT people.
>
> Good point... @Rob, @Jonathon, to avoid a different naming between the
> now to be introduced ACPI usage and a potential DT binding later on,
> really better define a DT one now? Which name to use for such an
> external vref, the one above used by spear-adc already?

I forgot to mention you also need to add a binding documentation for
this driver.

>>>>> +               .owner  = THIS_MODULE,
>>>>
>>>> This is redundant I'm pretty sure.
>>>
>>> Even in 2017, drivers keep being added that carry such assignments. Can
>>> you explain when it is needed and when not? Otherwise, I will leave it in.
>>
>> The above I'm 100% sure is not needed. It's needed only in cases when
>> framework / device core doesn't do this for ya.
>>
>> In the above case IIRC device core does it once for all.

> Then this is not consistently filtered out in new driver reviews. I can
> remove it, of course.

Please, remove.

I guess many of new drivers just lack of proper review :-(

-- 
With Best Regards,
Andy Shevchenko