Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 1/3] iio: Add driver for Infineon DPS310
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        joel@jms.id.au, pmeerw@pmeerw.net, lars@metafoo.de, knaack.h@gmx.de
References: <1557344128-690-1-git-send-email-eajames@linux.ibm.com>
 <1557344128-690-2-git-send-email-eajames@linux.ibm.com>
 <20190511102236.4c5f9585@archlinux>
From:   Eddie James <eajames@linux.ibm.com>
Date:   Wed, 15 May 2019 13:46:46 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <20190511102236.4c5f9585@archlinux>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Message-Id: <cf108058-dd50-4651-6c97-a5c7e82ef085@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 5/11/19 4:22 AM, Jonathan Cameron wrote:
> On Wed,  8 May 2019 14:35:26 -0500
> Eddie James <eajames@linux.ibm.com> wrote:
>
>> From: Joel Stanley <joel@jms.id.au>
>>
>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>> i2c and SPI.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Hi Eddie,
>
> Ideally we'll get a sign off form Joel as well on this.
>
> A few comments inline.
>
> I 'think' this is probably fine without any locking to prevent simultaneous reads
> and /or writes to the registers because the few functions that do multiple reads
> and writes look fine.  Please do take another look at that though to confirm there
> are no corner cases.
>
> Otherwise there is a race in the remove path that needs fixing.
> Various minor bits and bobs inline.
>
> thanks,
>
> Jonathan
>
>
>
>> ---
>>   MAINTAINERS                   |   6 +
>>   drivers/iio/pressure/Kconfig  |  10 +
>>   drivers/iio/pressure/Makefile |   1 +
>>   drivers/iio/pressure/dps310.c | 429 ++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 446 insertions(+)
>>   create mode 100644 drivers/iio/pressure/dps310.c
>>

>> +};
>> +MODULE_DEVICE_TABLE(i2c, dps310_id);
>> +
>> +static const unsigned short normal_i2c[] = {
>> +	0x77, 0x76, I2C_CLIENT_END
>> +};
>> +
>> +static struct i2c_driver dps310_driver = {
>> +	.driver = {
>> +		.name = "dps310",
>> +	},
>> +	.probe = dps310_probe,
>> +	.remove = dps310_remove,
>> +	.address_list = normal_i2c,
> I'm fairly sure the address list is only used along with the detection
> infrastructure.  As such it doesn't actually provide any value unless
> you have a detect callback.  Please remove.
>
> I would like to see a DT and/or ACPI binding though as that is the
> means most people will use to find the device.


Somehow the device is already present in the witherspoon device tree 
where it's currently being used, so I don't have anything to add. 
arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts

Thanks,

Eddie


>
>> +	.id_table = dps310_id,
>> +};
>> +module_i2c_driver(dps310_driver);
>> +
>> +MODULE_AUTHOR("Joel Stanley <joel@jms.id.au>");
>> +MODULE_DESCRIPTION("Infineon DPS310 pressure and temperature sensor");
>> +MODULE_LICENSE("GPL v2");