linux-kernel - Re: [PATCH v2 1/2] iio: gyro: Add driver support for ADXRS290

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] [day] [month] [year] [list]

Date:   Wed, 22 Jul 2020 19:59:11 +0530
From:   Nishant Malpani <nish.malpani25@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        "Bogdan, Dragos" <dragos.bogdan@...log.com>,
        darius.berghe@...log.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-iio <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] iio: gyro: Add driver support for ADXRS290



On 22/07/20 3:53 pm, Andy Shevchenko wrote:
> On Wed, Jul 22, 2020 at 12:40 PM Nishant Malpani
> <nish.malpani25@...il.com> wrote:
>> On 22/07/20 3:08 am, Andy Shevchenko wrote:
>>> On Tue, Jul 21, 2020 at 11:35 PM Nishant Malpani
>>> <nish.malpani25@...il.com> wrote:
>>>> On 22/07/20 1:16 am, Andy Shevchenko wrote:
> 
> ...
> 
>>> Can't you declare table as const int?
>>>
>> I'm not sure I understand you completely here; do you mean const int *?
>> So, an array of alternate integer and fractional parts? I suppose that's
>> possible but we'd be introducing unwanted complexity I feel - for
>> example, currently the index of the 3db frequency in the table is used
>> to directly map & set bits in the filter register corresponding to that
>> frequency but with the approach you share, we'd have to apply a
>> transformation (div by 2) to set the same bits in the filter register.
>> Do you think the added complexity justifies the removal of the casting?
> 
> It was a question. If you think it is too much, don't change :-)
>  > ...
> 
>>>>>> +       /* max transition time to measurement mode */
>>>>>> +       msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS);
>>>>>
>>>>> I'm not sure what the point of interruptible variant here?
>>>>>
>>>> I referred Documentation/timers/timers-howto.rst for this.
>>>> My reasoning was shaped to use the interruptible variant because the
>>>> transition settles in a time *less than* 100ms and since 100ms is quite
>>>> a huge time to sleep, it should be interrupted in case a signal arrives.
>>>
>>> This is probe of the device,
>>> What are the expectations here?
>>>
>> I fail to understand why this can't be used in the probe() but perhaps
>> in a routine to standby/resume. Could you please elaborate?
> 
> I didn't say it can not be used, what I'm asking is what are the
> expectations of the interruptible part here.
> In other words what is the benefit that makes you choose this over
> plain msleep().
> 
Oh, sorry for I misunderstood.

I cannot think of anything more to add to the reasoning that I explained 
earlier; in that case, I'll fall back to msleep() (in v3) unless someone 
else comes with a strong point in msleep_interruptible()'s favor.

With regards,
Nishant Malpani