linux-kernel - Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver

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: <017031cf-6092-0c33-339b-5c6b6b5ac6c7@xilinx.com>
Date:   Thu, 18 Nov 2021 09:07:30 +0100
From:   Michal Simek <michal.simek@...inx.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Anand Ashok Dumbre <anand.ashok.dumbre@...inx.com>
CC:     <linux-kernel@...r.kernel.org>, <jic23@...nel.org>,
        <lars@...afoo.de>, <linux-iio@...r.kernel.org>, <git@...inx.com>,
        <michal.simek@...inx.com>, <gregkh@...uxfoundation.org>,
        <rafael@...nel.org>, <linux-acpi@...r.kernel.org>,
        <heikki.krogerus@...ux.intel.com>,
        "Manish Narani" <manish.narani@...inx.com>
Subject: Re: [PATCH v10 3/5] iio: adc: Add Xilinx AMS driver



On 11/17/21 21:03, Andy Shevchenko wrote:
> On Wed, Nov 17, 2021 at 04:10:26PM +0000, Anand Ashok Dumbre wrote:
>> The AMS includes an ADC as well as on-chip sensors that can be used to
>> sample external voltages and monitor on-die operating conditions, such
>> as temperature and supply voltage levels. The AMS has two SYSMON blocks.
>> PL-SYSMON block is capable of monitoring off chip voltage and
>> temperature.
>>
>> PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring
>> from an external master. Out of these interfaces currently only DRP is
>> supported. Other block PS-SYSMON is memory mapped to PS.
>>
>> The AMS can use internal channels to monitor voltage and temperature as
>> well as one primary and up to 16 auxiliary channels for measuring
>> external voltages.
>>
>> The voltage and temperature monitoring channels also have event capability
>> which allows to generate an interrupt when their value falls below or
>> raises above a set threshold.
> 
> Thanks for an update, my comments below.
> 
> ...
> 
> Missed bitfields.h as kbuild bot noticed.
> 
>> +#include <linux/bits.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/overflow.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/slab.h>
> 
> ...
> 
>> +#define AMS_ALARM_THR_DIRECT_MASK	BIT(1)
> 
>> +#define AMS_ALARM_THR_MIN		~(BIT(16) - 1)
> 
> This is wrong. I already said it.
> 
>> +#define AMS_ALARM_THR_MAX		(BIT(16) - 1)
> 
> ...
> 
>> +enum ams_alarm_bit {
>> +	AMS_ALARM_BIT_TEMP,
>> +	AMS_ALARM_BIT_SUPPLY1,
>> +	AMS_ALARM_BIT_SUPPLY2,
>> +	AMS_ALARM_BIT_SUPPLY3,
>> +	AMS_ALARM_BIT_SUPPLY4,
>> +	AMS_ALARM_BIT_SUPPLY5,
>> +	AMS_ALARM_BIT_SUPPLY6,
>> +	AMS_ALARM_BIT_RESERVED,
>> +	AMS_ALARM_BIT_SUPPLY7,
>> +	AMS_ALARM_BIT_SUPPLY8,
>> +	AMS_ALARM_BIT_SUPPLY9,
>> +	AMS_ALARM_BIT_SUPPLY10,
>> +	AMS_ALARM_BIT_VCCAMS,
>> +	AMS_ALARM_BIT_TEMP_REMOTE
> 
> +Comma, same to the rest where the last item is not a terminator one.

And where you are touching this it will be good if you can initiate all 
values. That's recommendation we got from Greg some time ago.

Thanks,
Michal