linux-kernel - Re: [PATCH v2] 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: <CAHp75VeBa8wUX6U+S=QmrLmqrwdMuFg3TCw3nyY62NraZaDrNg@mail.gmail.com>
Date:   Wed, 3 May 2017 11:43:38 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Jan Kiszka <jan.kiszka@...mens.com>
Cc:     Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
        linux-iio@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Sascha Weisenberger <sascha.weisenberger@...mens.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>
Subject: Re: [PATCH v2] iio: adc: Add support for TI ADC1x8s102

On Wed, May 3, 2017 at 11:24 AM, Jan Kiszka <jan.kiszka@...mens.com> wrote:
> On 2017-05-03 10:06, Andy Shevchenko wrote:
>> On Wed, May 3, 2017 at 8:35 AM, Jan Kiszka <jan.kiszka@...mens.com> 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