Re: [PATCH 2/2] iio: pressure: driver for Honeywell HSC/SSC series pressure sensors

[Jonathan Cameron]

On Wed, 22 Nov 2023 08:08:27 +0200
Petre Rodan <petre.rodan@xxxxxxxxxxxxxxx> wrote:

> hello,
> 
> first of all, thank you for the code review.
> in the interest of brevity I will skip all comments where I simply remove the block, blankline, or fix indentation.
> 
> On Mon, Nov 20, 2023 at 02:35:39PM +0200, Andy Shevchenko wrote:
> > > +	select HSC030PA_I2C if (I2C)
> > > +	select HSC030PA_SPI if (SPI_MASTER)  
> > 
> > Unneeded parentheses  
> 
> ack
Where you agree, just crop it out. Saves on scrolling!

> > > +	case IIO_CHAN_INFO_RAW:
> > > +		mutex_lock(&data->lock);
> > > +		ret = hsc_get_measurement(data);
> > > +		mutex_unlock(&data->lock);  
> > 
> > Use guard() operator from cleanup.h.  
> 
> I'm not familiar with that, for the time being I'll stick to mutex_lock/unlock if you don't mind.
> 

It's simple and worth taking a look for new drivers as it makes some error paths much much simpler.
I'm sitting on a big set that applies it to quite few IIO drivers.



> > > +	ret = devm_regulator_get_enable_optional(dev, "vdd");
> > > +	if (ret == -EPROBE_DEFER)
> > > +		return -EPROBE_DEFER;  
> > 
> > Oh, boy, this should check for ENODEV or so, yeah, regulator APIs a bit
> > interesting.  
> 
> since I'm unable to test this I'd rather remove the block altogether.
> if I go the ENODEV route my module will never load since I can't see any vdd-supply support on my devboard.
Problem here is why do you think that regulator is optional? Does your device
work with out power?  What is optional is whether the regulator is fixed and
on and hence doesn't need to be in DT or whether it is specified there.
That's unconnected to the enabling in driver.

The call you have here is for when the power supply really is optional.
That is the driver does something different if nothing is supplied on the pin.
Typically this is used when we have option of either an internal reference voltage
or supplying an external one. The absence on an external one means we fallback
to only enabling the internal one.


Jonathan