Re: [RFC PATCH 1/2] iio: hmc5843 Add channel attribute for bias configuration

[Jonathan Cameron]

On 05/02/16 09:58, Daniel Baluta wrote:
> On Thu, Feb 4, 2016 at 7:45 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
>> On 02/04/2016 03:50 PM, Cristina Moraru wrote:
>>> Replace non standard meas_conf attribute with the standard IIO
>>> calibbias attribute.
>>>
>>> API for setting bias measurement configuration:
>>>
>>> 0 - Normal measurement configuration (default): In normal measurement
>>>     configuration the device follows normal measurement flow. Pins BP
>>>     and BN are left floating and high impedance.
>>>
>>> 1 - Positive bias configuration: In positive bias configuration, a
>>>     positive current is forced across the resistive load on pins BP
>>>     and BN.
>>>
>>> 2 - Negative bias configuration. In negative bias configuration, a
>>>     negative current is forced across the resistive load on pins BP
>>>     and BN.
>>>
>>> 3 - Only available on HMC5983. Magnetic sensor is disabled.
>>>     Temperature sensor is enabled.
>>>
>>> With this in place, we can think of moving this driver out of staging.
>>
>> Using a standard attribute, but overloading it with custom semantics doesn't
>> do any good either. calibbias is supposed to be a integer that gets added to
>> measurements internally in the device (unit is device specific though).
>>
>> This attribute seems to do something else. In that case it might be better
>> to stay with a custom attribute (as long as it is documented) or come up
>> with a better way to map the device configuration onto standard attributes.
> 
> We missed the RFC tag. So, with this in mind we still have some minor
> issues to fix and then we can move the code out of staging.
> 
> 1) checkpatch.pl -- strict warnings
> 2) suspend/resume functions in hmc5843_core.c.
> 
> int hmc5843_common_suspend(struct device *dev)
> {
>         return hmc5843_set_mode(iio_priv(dev_get_drvdata(dev)),
>                         HMC5843_MODE_CONVERSION_CONTINUOUS);
> }
> 
> int hmc5843_common_resume(struct device *dev)
> {
>         return hmc5843_set_mode(iio_priv(dev_get_drvdata(dev)),
>                                 HMC5843_MODE_SLEEP);
> }
> 
> I think the function names or implementations were swapped.
> The device should go to sleep at suspend and back to continuous
> mode on resume.
Yup, from a quick glance - it otherwise looks sensible to me.
Will as ever attract more reviews once the move out of staging
patch shows up.

Jonathan

> 
> thanks,
> Daniel.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>