On Sat, May 28, 2016 at 12:11 AM, Kevin Tsai <capellamicro@xxxxxxxxx> wrote:
> Added Vishay Capella CM36672 Proximity Sensor IIO driver. Support both
> ACPI and Device Tree.
>
> Signed-off-by: Kevin Tsai <capellamicro@xxxxxxxxx>
Aha OK...
> +Required properties:
> +- compatible: must be "capella,cm36672"
> +- reg: the I2C address of the device
> +- interrupts: interrupt-specifier for the sole interrupt generated by the device
> +
> +Optionoal properties:
> +- capella,reg_prx_conf: PS_CONF1 and PS_CONF2 registers
> +- capella,reg_prx_conf3: PS_CONF3 and PS_MS registers
This doesn't work at all, you are just taking magic values and
poking into registers.
Make real configurations for each and every option in the registers
that you want to support.
We already have standardized config options in DT for
a lot of stuff. If one bit means "open drain IRQ line" or so,
it should be its own config option.
See:
Documentation/devicetree/bindings/iio/light/us5182d.txt
For a good example.
> +- capella,reg_prx_canc: PS cancellation level setting
> +- capella,reg_prx_thdl: PS low interrupt threshold setting
> +- capella,reg_prx_thdh: PS high interrupt threshold setting
This looks similar to magic, what unit(s) are these levels
set in? Are they really 32 bit?
> +- capella,name_prx_int_rising: enable/disable rising interrupt
> +- capella,name_prx_int_falling: enable/disable falling interrupt
NACK this should be handled by flags from the irqchip.
If this client has falling edge assigned in the device tree
it looks something like so:
interrupt-parent = <&gpio2>;
interrupts = <18 IRQ_TYPE_EDGE_FALLING>,
Then your driver should just react to that and set up rising/falling
edge accordingly, see e.g.
drivers/iio/common/st_sensors/st_sensors_trigger.c:
unsigned long irq_trig;
irq_trig = irqd_get_trigger_type(irq_get_irq_data(irq));
if (irq_trig == IRQF_TRIGGER_FALLING) {
...
> +- capella,name_prx_duty: PS duty ratio setting
> +- capella,name_prx_pers: persistence setting
> +- capella,name_prx_it: integration time setting
Unit? kiloohms? megajoule?
Also: are they really 32bit?
> +- capella,name_prx_led_current: LED current selection
Isn't this rather something that should be modelled
by a regulator in the device tree?
Look at the other examples for light sensor
bindings, e.g.
Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
> +- capella,name_prx_threshold_falling: interrupt falling threshold
> +- capella,name_prx_threshold_rising: interrupt rising threshold
Units?
+ cm36672@60 {
> + compatible = "capella,cm36672";
> + reg = <0x60>;
> + interrupt-parent = <&gpio0>;
> + interrupts = <30 0>;
I understand until here.
> + /* Initialized by register */
> + capella,reg_prx_conf = <0x0322>;
> + capella,reg_prx_conf3 = <0x0000>;
> + capella,reg_prx_canc = <0x0000>;
> + capella,reg_prx_thdl = <0x0007>;
> + capella,reg_prx_thdh = <0x000A>;
> +
> + /* Or, initialized by name */
> + capella,name_prx_int_rising = <1>;
> + capella,name_prx_int_falling = <1>;
> + capella,name_prx_duty = <0>;
> + capella,name_prx_pers = <2>;
> + capella,name_prx_it = <1>;
> + capella,name_prx_led_current = <0>;
> + capella,name_prx_threshold_falling = <7>;
> + capella,name_prx_threshold_rising = <10>;
I don't understand any of this. The purpose of the
bindings is to make a detailed explanantion of all options.
> +/* RESERVED00 */
> +#define CM36672_RESERVED00_DEFAULT 0x0001
> +
> +/* RESERVED01 */
> +#define CM36672_RESERVED01_DEFAULT 0x0000
> +
> +/* RESERVED02 */
> +#define CM36672_RESERVED02_DEFAULT 0x0000
I don't understand these. Why are they even here?
> +/* PRX_CONF */
> +#define CM36672_PRX_DISABLE 0x0001
> +#define CM36672_PRX_INT_RISING BIT(8)
> +#define CM36672_PRX_INT_FALLING BIT(9)
> +#define CM36672_PRX_INT_MASK (CM36672_PRX_INT_RISING | \
> + CM36672_PRX_INT_FALLING)
So you do know what the different bits are doing.
> +/* PRX_CONF: duty ratio */
> +#define CM36672_PRX_DR_MASK (BIT(6) | BIT(7))
> +#define CM36672_PRX_DR_SHIFT 6
> +#define CM36672_PRX_DR1 0x0000 /* Duty ratio 1/40 */
> +#define CM36672_PRX_DR2 0x0040 /* Duty ratio 1/80 */
> +#define CM36672_PRX_DR3 0x0080 /* Duty ratio 1/160 */
> +#define CM36672_PRX_DR4 0x00C0 /* Duty ratio 1/320 */
> +
> +/* PRX_CONF: persistence */
> +#define CM36672_PRX_PERS_MASK (BIT(4) | BIT(5))
> +#define CM36672_PRX_PERS_SHIFT 4
> +#define CM36672_PRX_PERS2 0x0010
> +#define CM36672_PRX_PERS3 0x0020
> +#define CM36672_PRX_PERS4 0x0030
And these things are then doubly-defined in the device tree as
it seems.
There is more stuff but fixing these will change the driver a bit
so I prefer to review the rest later.
Yours,
Linus Walleij
--
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
