On Fri, Aug 23, 2024 at 11:06:28PM +0300, Andy Shevchenko wrote: > On Fri, Aug 23, 2024 at 08:17:13PM +0200, Vasileios Amoiridis wrote: > > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as > > a trigger for when there are data ready in the sensor for pick up. > > > > This use case is used along with NORMAL_MODE in the sensor, which allows > > the sensor to do consecutive measurements depending on the ODR rate value. > > > > The trigger pin can be configured to be open-drain or push-pull and either > > rising or falling edge. > > > > No support is added yet for interrupts for FIFO, WATERMARK and out of range > > values. > > ... > > > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev, > > + const struct iio_trigger_ops *trigger_ops, > > + int (*int_config)(struct bmp280_data *data), > > > + irqreturn_t (*irq_thread_handler)(int irq, void *p)) > > irq_handler_t > But the function returns an irqreturn_t type, no? > ... > > > + fwnode = dev_fwnode(data->dev); > > + if (!fwnode) > > + return -ENODEV; > > Why do you need this? The below will fail anyway. Because If I don't make this check then fwnode might be garbage and I will pass garbage to the fwnode_irq_get() function. Or do I miss something? > > > + irq = fwnode_irq_get(fwnode, 0); > > + if (!irq) > > Are you sure this is correct check? > Well, I think yes, because the function return either the Linux IRQ number on success or a negative errno on failure. https://elixir.bootlin.com/linux/v6.10.6/source/drivers/base/property.c#L987 > > + return dev_err_probe(data->dev, -ENODEV, > > Shadowed error code. > I am not sure I understand what you mean here. You mean that there is no chance that the first one will pass and this one will fail? > > + "No interrupt found.\n"); > > > + desc = irq_get_irq_data(irq); > > + if (!desc) > > + return -EINVAL; > > When may this fail? > I think that this will fail when Linux were not able to actually register that interrupt. > > + irq_type = irqd_get_trigger_type(desc); > > + switch (irq_type) { > > + case IRQF_TRIGGER_RISING: > > + data->trig_active_high = true; > > + break; > > + case IRQF_TRIGGER_FALLING: > > + data->trig_active_high = false; > > + break; > > + default: > > + return dev_err_probe(data->dev, -EINVAL, > > + "Invalid interrupt type specified.\n"); > > + } > > > + data->trig_open_drain = fwnode_property_read_bool(fwnode, > > + "int-open-drain"); > > Better > > data->trig_open_drain = > fwnode_property_read_bool(fwnode, "int-open-drain"); > Indeed, thanks! > ... > > > +static int bmp380_data_rdy_trigger_set_state(struct iio_trigger *trig, > > + bool state) > > +{ > > + struct bmp280_data *data = iio_trigger_get_drvdata(trig); > > + int ret; > > + > > + guard(mutex)(&data->lock); > > + > > + ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CONTROL, > > + BMP380_INT_CTRL_DRDY_EN, > > + FIELD_PREP(BMP380_INT_CTRL_DRDY_EN, > > + state ? 1 : 0)); > > FIELD_PREP(BMP380_INT_CTRL_DRDY_EN, !!state)); > > ? ( Even <= 80 characters) Well, that's true. > > > + if (ret) { > > + dev_err(data->dev, "Could not enable/disable interrupt\n"); > > + return ret; > > + } > > + > > + return 0; > > if (ret) > dev_err(data->dev, "Could not enable/disable interrupt\n"); > > return ret; > > ? All the other if statements follow the style that I typed. If I follow yours, will make it different just for this one, does it make sense? Cheers, Vasilis > > > +} > > ... > > > +static int bmp380_int_config(struct bmp280_data *data) > > +{ > > + int ret, int_cfg = FIELD_PREP(BMP380_INT_CTRL_OPEN_DRAIN, > > + data->trig_open_drain) | > > + FIELD_PREP(BMP380_INT_CTRL_LEVEL, > > + data->trig_active_high); > > Split these two variables and make the indentation better for int_cfg. > True, makes sense. > > + ret = regmap_update_bits(data->regmap, BMP380_REG_INT_CONTROL, > > + BMP380_INT_CTRL_SETTINGS_MASK, int_cfg); > > + if (ret) { > > + dev_err(data->dev, "Could not set interrupt settings\n"); > > > + return ret; > > + } > > + > > + return 0; > > return ret; > > ? Yes, you are right. > > > +} > > ... > > > +static int bmp580_data_rdy_trigger_set_state(struct iio_trigger *trig, > > + bool state) > > +{ > > + struct bmp280_data *data = iio_trigger_get_drvdata(trig); > > + int ret; > > + > > + guard(mutex)(&data->lock); > > + > > + ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG, > > + BMP580_INT_CONFIG_INT_EN, > > > + FIELD_PREP(BMP580_INT_CONFIG_INT_EN, > > + state ? 1 : 0)); > > !!state ? > ACK. > > + if (ret) { > > + dev_err(data->dev, "Could not enable/disable interrupt\n"); > > + return ret; > > + } > > + > > + return 0; > > return ret; > > ? > > > +} > > ... > > > +static int bmp580_int_config(struct bmp280_data *data) > > Same comments as per above. > > ... > > > + if (irq > 0) { > > + if (chip_id == BMP180_CHIP_ID) { > > + ret = bmp085_fetch_eoc_irq(dev, name, irq, data); > > + if (ret) > > + return ret; > > + } > > + if (data->chip_info->trigger_probe) { > > + ret = data->chip_info->trigger_probe(indio_dev); > > + if (ret) > > + return ret; > > + } > > } > > Can be > > if (irq > 0) { > if (chip_id == BMP180_CHIP_ID) > ret = bmp085_fetch_eoc_irq(dev, name, irq, data); > if (data->chip_info->trigger_probe) > ret = data->chip_info->trigger_probe(indio_dev); > if (ret) > return ret; > } > > -- > With Best Regards, > Andy Shevchenko > > Well, it looks much more beautiful indeed. Thanks again for the feedback Andy, I really appreciate it a lot! Cheers, Vasilis
