Re: [PATCH v3 2/5] iio: sensorhub: Add sensorhub bindings

Re: [PATCH v3 2/5] iio: sensorhub: Add sensorhub bindings

[Jonathan Cameron]

On 05/12/14 19:54, Karol Wrona wrote:
> Add sensorhub bindings for sensorhub on Galaxy Gear 2.
> 
> Change-Id: I4ee25aef33c21a4662de230841de9a8684f2c26b
> Signed-off-by: Karol Wrona <k.wrona-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Looks good to me.  Comments inline.  Note I either need a device tree
ack or a long delay before I can take this though.  Unlikely to get the
device tree ack as you haven't cc'd the maintainers or list ;)

I've added the cc's. Please make sure future versions have them or you'll
just be slowing the process down (particularly if no one notices they 
are missing!)

> ---
>  .../devicetree/bindings/iio/sensorhub.txt          |   46 ++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/sensorhub.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/sensorhub.txt b/Documentation/devicetree/bindings/iio/sensorhub.txt
> new file mode 100644
> index 0000000..2aca0c3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/sensorhub.txt
> @@ -0,0 +1,46 @@
> +Samsung Sensorhub driver
> +
> +Sensorhub is a MCU which manages several sensors and also plays the role
> +of a virtual sensor device.
> +
> +Required properties:
> +- compatible: "samsung,sensorhub-rinato" or "samsung,sensorhub-thermostat"
> +- spi-max-frequency: max SPI clock frequency
> +- interrupt-parent: interrupt parent
> +- interrupts: communication interrupt
> +- ap-mcu-gpio: [out] ap to sensorhub line - used during communication
> +- mcu-ap-gpio: [in] sensorhub to ap - used during communication
> +- mcu-reset: [out] sensorhub reset
as commented in previous patch, why do the first two have -gpio and the
third not?
> +
> +Optional properties:
> +- sensor's nodes:
> +  compatible = "samsung,mpu6500-accel"
> +  compatible = "samsung,mpu6500-gyro"
> +  compatible = "samsung,adpd142"
> +
> +Sensor compatibles are used to match proper sensor driver to real sensor on
> +the board. The firmware does not give such information, so it helps to specify
> +some sensors properties. Sensors have "samsung" prefixes because frequently
> +they will not have much in common with sensors used without sensorhub because
> +it can do some data processing.
We'll keep that under review.  Might make sense, sometimes, to unify the
drivers.  The different compatible will probably still be needed, but if
these devices proliferate I don't want two drivers for everything that
gets stuck behind them.
> +
> +Example:
> +
> +	shub_spi: shub {
> +		compatible = "samsung,sensorhub-rinato";
> +		spi-max-frequency = <5000000>;
> +		interrupt-parent = <&gpx0>;
> +		interrupts = <2 0>;
> +		ap-mcu-gpio = <&gpx0 0 0>;
> +		mcu-ap-gpio = <&gpx0 4 0>;
> +		mcu-reset = <&gpx0 5 0>;
> +		sensor@0 {
> +			compatible = "samsung,mpu6500-accel";
> +		};
> +		sensor@1 {
> +			compatible = "samsung,mpu6500-gyro";
> +		};
> +		sensor@2 {
> +			compatible = "samsung,adpd142";
> +		};
> +	};
> 

Re: [PATCH v3 2/5] iio: sensorhub: Add sensorhub bindings

[Karol Wrona]

On 12/06/2014 03:29 PM, Jonathan Cameron wrote:
> On 05/12/14 19:54, Karol Wrona wrote:
>> Add sensorhub bindings for sensorhub on Galaxy Gear 2.
>>
>> Change-Id: I4ee25aef33c21a4662de230841de9a8684f2c26b
>> Signed-off-by: Karol Wrona <k.wrona-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Looks good to me.  Comments inline.  Note I either need a device tree
> ack or a long delay before I can take this though.  Unlikely to get the
> device tree ack as you haven't cc'd the maintainers or list ;)
I am curious about DT community opinion and I prefer to know it before
v4 sending.

> 
> I've added the cc's. Please make sure future versions have them or you'll
> just be slowing the process down (particularly if no one notices they 
> are missing!)
Thanks, I will remember that.
> 
>> ---
>>  .../devicetree/bindings/iio/sensorhub.txt          |   46 ++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/sensorhub.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/sensorhub.txt b/Documentation/devicetree/bindings/iio/sensorhub.txt
>> new file mode 100644
>> index 0000000..2aca0c3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/sensorhub.txt
>> @@ -0,0 +1,46 @@
>> +Samsung Sensorhub driver
>> +
>> +Sensorhub is a MCU which manages several sensors and also plays the role
>> +of a virtual sensor device.
>> +
>> +Required properties:
>> +- compatible: "samsung,sensorhub-rinato" or "samsung,sensorhub-thermostat"
>> +- spi-max-frequency: max SPI clock frequency
>> +- interrupt-parent: interrupt parent
>> +- interrupts: communication interrupt
>> +- ap-mcu-gpio: [out] ap to sensorhub line - used during communication
>> +- mcu-ap-gpio: [in] sensorhub to ap - used during communication
>> +- mcu-reset: [out] sensorhub reset
> as commented in previous patch, why do the first two have -gpio and the
> third not?
Ok, all 3 will have "gpios" ending
>> +
>> +Optional properties:
>> +- sensor's nodes:
>> +  compatible = "samsung,mpu6500-accel"
>> +  compatible = "samsung,mpu6500-gyro"
>> +  compatible = "samsung,adpd142"
>> +
>> +Sensor compatibles are used to match proper sensor driver to real sensor on
>> +the board. The firmware does not give such information, so it helps to specify
>> +some sensors properties. Sensors have "samsung" prefixes because frequently
>> +they will not have much in common with sensors used without sensorhub because
>> +it can do some data processing.
> We'll keep that under review.  Might make sense, sometimes, to unify the
> drivers.  The different compatible will probably still be needed, but if
> these devices proliferate I don't want two drivers for everything that
> gets stuck behind them.
I think that sensorhub is very similar hid-sensor (without hotpluging) where
every sensor type has its own representation.  The real sensors are not driven
directly but by external MCU so very often these sensors will not have much
common with real ones.  So I wonder if it could be better to resign from these
sensor compatibles and represent each sensor as mfd cell so optional properties
probably will dropped (?)
Generally I intended to have i.e. one ssp-accel driver if they use another
accelerometer then mpu6500 the buffering manner and sysfs will be handled in the
same way.  I think I was mistaken with these compatibles.  There is also some
probability that the firmware will be fixed someday and it will be able to tell
more about sensor then the type.

>> +
>> +Example:
>> +
>> +	shub_spi: shub {
>> +		compatible = "samsung,sensorhub-rinato";
>> +		spi-max-frequency = <5000000>;
>> +		interrupt-parent = <&gpx0>;
>> +		interrupts = <2 0>;
>> +		ap-mcu-gpio = <&gpx0 0 0>;
>> +		mcu-ap-gpio = <&gpx0 4 0>;
>> +		mcu-reset = <&gpx0 5 0>;
>> +		sensor@0 {
>> +			compatible = "samsung,mpu6500-accel";
>> +		};
>> +		sensor@1 {
>> +			compatible = "samsung,mpu6500-gyro";
>> +		};
>> +		sensor@2 {
>> +			compatible = "samsung,adpd142";
>> +		};
>> +	};
>>
> 
> 

Re: [PATCH v3 2/5] iio: sensorhub: Add sensorhub bindings

[Jonathan Cameron]

On 16/12/14 07:30, Karol Wrona wrote:
> On 12/06/2014 03:29 PM, Jonathan Cameron wrote:
>> On 05/12/14 19:54, Karol Wrona wrote:
>>> Add sensorhub bindings for sensorhub on Galaxy Gear 2.
>>>
>>> Change-Id: I4ee25aef33c21a4662de230841de9a8684f2c26b
>>> Signed-off-by: Karol Wrona <k.wrona-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Looks good to me.  Comments inline.  Note I either need a device tree
>> ack or a long delay before I can take this though.  Unlikely to get the
>> device tree ack as you haven't cc'd the maintainers or list ;)
> I am curious about DT community opinion and I prefer to know it before
> v4 sending.
They have a lot of patches to review, so might take a while ;)
> 
>>
>> I've added the cc's. Please make sure future versions have them or you'll
>> just be slowing the process down (particularly if no one notices they 
>> are missing!)
> Thanks, I will remember that.
>>
>>> ---
>>>  .../devicetree/bindings/iio/sensorhub.txt          |   46 ++++++++++++++++++++
>>>  1 file changed, 46 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/sensorhub.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/sensorhub.txt b/Documentation/devicetree/bindings/iio/sensorhub.txt
>>> new file mode 100644
>>> index 0000000..2aca0c3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/sensorhub.txt
>>> @@ -0,0 +1,46 @@
>>> +Samsung Sensorhub driver
>>> +
>>> +Sensorhub is a MCU which manages several sensors and also plays the role
>>> +of a virtual sensor device.
>>> +
>>> +Required properties:
>>> +- compatible: "samsung,sensorhub-rinato" or "samsung,sensorhub-thermostat"
>>> +- spi-max-frequency: max SPI clock frequency
>>> +- interrupt-parent: interrupt parent
>>> +- interrupts: communication interrupt
>>> +- ap-mcu-gpio: [out] ap to sensorhub line - used during communication
>>> +- mcu-ap-gpio: [in] sensorhub to ap - used during communication
>>> +- mcu-reset: [out] sensorhub reset
>> as commented in previous patch, why do the first two have -gpio and the
>> third not?
> Ok, all 3 will have "gpios" ending
>>> +
>>> +Optional properties:
>>> +- sensor's nodes:
>>> +  compatible = "samsung,mpu6500-accel"
>>> +  compatible = "samsung,mpu6500-gyro"
>>> +  compatible = "samsung,adpd142"
>>> +
>>> +Sensor compatibles are used to match proper sensor driver to real sensor on
>>> +the board. The firmware does not give such information, so it helps to specify
>>> +some sensors properties. Sensors have "samsung" prefixes because frequently
>>> +they will not have much in common with sensors used without sensorhub because
>>> +it can do some data processing.
>> We'll keep that under review.  Might make sense, sometimes, to unify the
>> drivers.  The different compatible will probably still be needed, but if
>> these devices proliferate I don't want two drivers for everything that
>> gets stuck behind them.
> I think that sensorhub is very similar hid-sensor (without hotpluging) where
> every sensor type has its own representation.  The real sensors are not driven
> directly but by external MCU so very often these sensors will not have much
> common with real ones.  So I wonder if it could be better to resign from these
> sensor compatibles and represent each sensor as mfd cell so optional properties
> probably will dropped (?)
Might do.
> Generally I intended to have i.e. one ssp-accel driver if they use another
> accelerometer then mpu6500 the buffering manner and sysfs will be handled in the
> same way.  I think I was mistaken with these compatibles.  There is also some
> probability that the firmware will be fixed someday and it will be able to tell
> more about sensor then the type.
Would be good - particularly if we start to get lots of variants out in the wild.
> 
>>> +
>>> +Example:
>>> +
>>> +	shub_spi: shub {
>>> +		compatible = "samsung,sensorhub-rinato";
>>> +		spi-max-frequency = <5000000>;
>>> +		interrupt-parent = <&gpx0>;
>>> +		interrupts = <2 0>;
>>> +		ap-mcu-gpio = <&gpx0 0 0>;
>>> +		mcu-ap-gpio = <&gpx0 4 0>;
>>> +		mcu-reset = <&gpx0 5 0>;
>>> +		sensor@0 {
>>> +			compatible = "samsung,mpu6500-accel";
>>> +		};
>>> +		sensor@1 {
>>> +			compatible = "samsung,mpu6500-gyro";
>>> +		};
>>> +		sensor@2 {
>>> +			compatible = "samsung,adpd142";
>>> +		};
>>> +	};
>>>
>>
>>
>