Re: [PATCH v2 1/3] iio: Add driver for Infineon DPS310

[Jonathan Cameron]

On Wed, 15 May 2019 13:46:46 -0500
Eddie James <eajames@xxxxxxxxxxxxx> wrote:

> On 5/11/19 4:22 AM, Jonathan Cameron wrote:
> > On Wed,  8 May 2019 14:35:26 -0500
> > Eddie James <eajames@xxxxxxxxxxxxx> wrote:
> >  
> >> From: Joel Stanley <joel@xxxxxxxxx>
> >>
> >> The DPS310 is a temperature and pressure sensor. It can be accessed over
> >> i2c and SPI.
> >>
> >> Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>  
> > 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
Yes, there is a fallback path (which in theory will be removed one day)
in which the vendor is stripped off the dts entry and the rest matched
against the registered i2c device table.

However, it's less than ideal as it'll lead to a potential false match
if some other company uses dps310 as a part number.

Hence it is preferred to always provide and explicit dt binding which is
checked before this fallback path.

As a side note, this is a classic thing for people to pick up on and send
follow up patches for.  Makes everyone's life easier to do it early!

Thanks,

Jonathan

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