On Sat, 26 Jan 2019 15:39:16 -0800
Martin Kelly <martin@xxxxxxxxxxxxxxxx> wrote:
> On 1/26/19 12:17 PM, Jonathan Cameron wrote:
> > On Mon, 21 Jan 2019 18:04:27 -0800
> > Martin Kelly <martin@xxxxxxxxxxxxxxxx> wrote:
> >
> >> From: Martin Kelly <martin@xxxxxxxxxxxxxxxx>
> >>
> >> Add interrupt support for the data ready signal on the BMI160, which fires
> >> an interrupt whenever new accelerometer/gyroscope data is ready to read.
> >>
> >> Signed-off-by: Martin Kelly <martin@xxxxxxxxxxxxxxxx>
> >
> > Various minor bits inline. I would also, if possible like Daniel to
> > take a glance at this series before we apply it. I don't know this hardware
> > at all well!
> >
> > Jonathan
> >
...
> >> +
> >> +static int bmi160_config_pin(struct regmap *regmap, enum bmi160_int_pin pin,
> >> + bool open_drain, u8 irq_mask,
> >> + unsigned long write_usleep)
> >> +{
> >> + int ret;
> >> + u8 int_out_ctrl_shift;
> >> + u8 int_latch_mask;
> >> + u8 int_map_mask;
> >> + u8 int_out_ctrl_mask;
> >> + u8 int_out_ctrl_bits;
> >> +
> >> + switch (pin) {
> >> + case BMI160_PIN_INT1:
> >> + int_out_ctrl_shift = BMI160_INT1_OUT_CTRL_SHIFT;
> >> + int_latch_mask = BMI160_INT1_LATCH_MASK;
> >> + int_map_mask = BMI160_INT1_MAP_DRDY_EN;
> >> + break;
> >> + case BMI160_PIN_INT2:
> >> + int_out_ctrl_shift = BMI160_INT2_OUT_CTRL_SHIFT;
> >> + int_latch_mask = BMI160_INT2_LATCH_MASK;
> >> + int_map_mask = BMI160_INT2_MAP_DRDY_EN;
> >> + break;
> >> + }
> >> + int_out_ctrl_mask = BMI160_INT_OUT_CTRL_MASK << int_out_ctrl_shift;
> >> +
> >> + /*
> >> + * Enable the requested pin with the right settings:
> >> + * - Push-pull/open-drain
> >> + * - Active low/high
> >> + * - Edge/level triggered
> >> + */
> >> + int_out_ctrl_bits = BMI160_OUTPUT_EN;
> >> + if (open_drain)
> >> + /* Default is push-pull. */
> >> + int_out_ctrl_bits |= BMI160_OPEN_DRAIN;
> >> + int_out_ctrl_bits |= irq_mask;
> >> + int_out_ctrl_bits <<= int_out_ctrl_shift;
> >> +
> >> + ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_OUT_CTRL,
> >> + int_out_ctrl_mask, int_out_ctrl_bits,
> >> + write_usleep);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* Set the pin to input mode with no latching. */
> >> + ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_LATCH,
> >> + int_latch_mask, int_latch_mask,
> >> + write_usleep);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* Map interrupts to the requested pin. */
> >> + ret = bmi160_write_conf_reg(regmap, BMI160_REG_INT_MAP,
> >> + int_map_mask, int_map_mask,
> >> + write_usleep);
> >> + if (ret)
> >> + return ret;
> > return bmi160...
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +int bmi160_enable_irq(struct regmap *regmap, bool enable)
> >> +{
> >> + unsigned int enable_bit = 0;
> >> +
> >> + if (enable)
> >> + enable_bit = BMI160_DRDY_INT_EN;
> >> +
> >> + return bmi160_write_conf_reg(regmap, BMI160_REG_INT_EN,
> >> + BMI160_DRDY_INT_EN, enable_bit,
> >> + BMI160_NORMAL_WRITE_USLEEP);
> >> +}
> >> +EXPORT_SYMBOL(bmi160_enable_irq);
> >> +
> >> +static bool bmi160_parse_irqname(struct device_node *of_node, int irq,
> >> + enum bmi160_int_pin *pin)
> >> +{
> >> + int ret;
> >> +
> >> + /* of_irq_get_byname returns the IRQ number if the entry is found. */
> >> + ret = of_irq_get_byname(of_node, "INT1");
> >> + if (ret == irq) {
> >> + *pin = BMI160_PIN_INT1;
> >> + return true;
> >> + }
> >
> > Given both could be provided, and we are implying a preference,
> > why not just get INT1 first by name and if it's not there try for
> > INT2? No need for this separate does the name match query.
> >
>
> I actually didn't mean to imply a preference; I just figured that a
> given IRQ can have only one name, and therefore at most one of the names
> "INT1" and "INT2" will match the passed-in IRQ. Is this a bad assumption?
I'm saying that you should express a preference. It makes
things predictable. But not by matching the 'first' one (which is
currently what happens) but rather just asking for INT1 by name
(don't use the spi->irq at all) and use that if present.
>
> >> +
> >> + ret = of_irq_get_byname(of_node, "INT2");
> >> + if (ret == irq) {
> >> + *pin = BMI160_PIN_INT2;
> >> + return true;
> >> + }
> >> +
> >> + return false;
> >> +}
>
