| Message ID | 20240823181714.64545-7-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pressure: bmp280: Minor cleanup and interrupt support | expand |
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 ... > + fwnode = dev_fwnode(data->dev); > + if (!fwnode) > + return -ENODEV; Why do you need this? The below will fail anyway. > + irq = fwnode_irq_get(fwnode, 0); > + if (!irq) Are you sure this is correct check? > + return dev_err_probe(data->dev, -ENODEV, Shadowed error code. > + "No interrupt found.\n"); > + desc = irq_get_irq_data(irq); > + if (!desc) > + return -EINVAL; When may this fail? > + 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"); ... > +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) > + 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; ? > +} ... > +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. > + 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; ? > +} ... > +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 ? > + 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; }
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
On Sat, 24 Aug 2024 14:02:22 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > 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? irq_handler_t is a typdef for the full function signature. It will still return irqreturn_t > > > ... > > > > > + 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? It checks for NULL which is all it can actually be and returns a suitable error code if it is. > > > > > > + 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 Indeed, so if (irq < 0) return dev_err_probe(data->dev, irq, ...) carry on as valid irq. your error check if returning only if irq == 0 which never happens (due to catch for that in the code you link). Negative values are true, so !-EINVAL == false for example. >
On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote: > 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: ... > > > +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? The type of the last parameter is irq_handler_t, no need to open code it. ... > > > + 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? Yes, the function validates fwnode before use. So, please drop unneeded (or even duplicate) check. ... > > > + 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. Where is 0 mentioned in this? > 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? -ENODEV is not what fwnode_irq_get() returns on error. > > > + "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. Wouldn't fwnode_irq_get() fail already? ... > > if (ret) > > dev_err(data->dev, "Could not enable/disable interrupt\n"); Btw you may use str_enable_disable() here. > > 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? When a comment is given, it's assumed that the _full_ patch (or patch series) should be revisited for it. Or should I add to every comment something like this: "Please, check the entire code for the same or similar case and amend accordingly." ?
On Mon, Aug 26, 2024 at 11:01:50AM +0100, Jonathan Cameron wrote: > On Sat, 24 Aug 2024 14:02:22 +0200 > Vasileios Amoiridis <vassilisamir@gmail.com> wrote: > > 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: ... > > > > + 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? > It checks for NULL which is all it can actually be and returns a suitable > error code if it is. Actually not. It may be NULL, error pointer, or valid. So, for a bare minimum this check is not full (and again, fwnode APIs should validate fwnode before accessing them where it makes sense; if fwnode_irq_get() does not do that or misses the case(s), it has to be improved).
On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote: > On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote: > > 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: > > ... > > > > > +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? > > The type of the last parameter is irq_handler_t, no need to open code it. > > ... > > > > > + 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? > > Yes, the function validates fwnode before use. So, please drop unneeded (or > even duplicate) check. > > ... > > > > > + 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. > > Where is 0 mentioned in this? > > > 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? > > -ENODEV is not what fwnode_irq_get() returns on error. > > > > > + "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. > > Wouldn't fwnode_irq_get() fail already? > Hi Andy, By looking at it again, I didn't reply correct here. This function internally calls the irq_to_desc() which basically returns the irq desctiptor for this irq. This function can return NULL in case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ) or in case the interrupt number is bigger than the NR_IRQs which the irq controller can handle (!CONFIG_SPARSE_IRQ). So in my opinion, it makes sense to keep this check. Cheers, Vasilis https://elixir.bootlin.com/linux/v6.10.6/source/kernel/irq/chip.c#L155 > ... > > > > if (ret) > > > dev_err(data->dev, "Could not enable/disable interrupt\n"); > > Btw you may use str_enable_disable() here. > > > > 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? > > When a comment is given, it's assumed that the _full_ patch (or patch series) > should be revisited for it. Or should I add to every comment something like > this: > > "Please, check the entire code for the same or similar case and amend > accordingly." > > ? > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, Aug 28, 2024 at 04:01:19PM +0200, Vasileios Amoiridis wrote: > On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote: > > On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote: > > > 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: ... > > > > > + 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. > > > > Where is 0 mentioned in this? > > > > > 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? > > > > -ENODEV is not what fwnode_irq_get() returns on error. > > > > > > > + "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. > > > > Wouldn't fwnode_irq_get() fail already? > > By looking at it again, I didn't reply correct here. This function > internally calls the irq_to_desc() which basically returns the > irq desctiptor for this irq. This function can return NULL in > case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ) > or in case the interrupt number is bigger than the NR_IRQs which > the irq controller can handle (!CONFIG_SPARSE_IRQ). > > So in my opinion, it makes sense to keep this check. So, you mean that if fwnode_irq_get() succeeded there is a chance that returned Linux IRQ number is invalid?! If it's so, it's something new to me. I would like to see the details, please!
On Wed, Aug 28, 2024 at 05:17:40PM +0300, Andy Shevchenko wrote: > On Wed, Aug 28, 2024 at 04:01:19PM +0200, Vasileios Amoiridis wrote: > > On Mon, Aug 26, 2024 at 01:23:56PM +0300, Andy Shevchenko wrote: > > > On Sat, Aug 24, 2024 at 02:02:22PM +0200, Vasileios Amoiridis wrote: > > > > 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: > > ... > > > > > > > + 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. > > > > > > Where is 0 mentioned in this? > > > > > > > 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? > > > > > > -ENODEV is not what fwnode_irq_get() returns on error. > > > > > > > > > + "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. > > > > > > Wouldn't fwnode_irq_get() fail already? > > > > By looking at it again, I didn't reply correct here. This function > > internally calls the irq_to_desc() which basically returns the > > irq desctiptor for this irq. This function can return NULL in > > case the interrupt is not found in the maple tree (CONFIG_SPARSE_IRQ) > > or in case the interrupt number is bigger than the NR_IRQs which > > the irq controller can handle (!CONFIG_SPARSE_IRQ). > > > > So in my opinion, it makes sense to keep this check. > > So, you mean that if fwnode_irq_get() succeeded there is a chance that returned > Linux IRQ number is invalid?! If it's so, it's something new to me. I would like > to see the details, please! > > -- > With Best Regards, > Andy Shevchenko > > Hi Andy, I did some more digging, and from my understanding, fwnode_irq_get() directly returns the Linux IRQ which means that there should already exist the irq_desc structure that is returned by the irq_to_desc(). So as you already said, I cannot see how this function can fail, if the fwnode_irq_get() has succeeded. Thanks for asking the right questions, I am getting to know things better. Cheers, Vasilis
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index e1336aeceec0..f5268a13bfdb 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -37,12 +37,14 @@ #include <linux/module.h> #include <linux/nvmem-provider.h> #include <linux/pm_runtime.h> +#include <linux/property.h> #include <linux/random.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/iio/buffer.h> #include <linux/iio/iio.h> +#include <linux/iio/trigger.h> #include <linux/iio/trigger_consumer.h> #include <linux/iio/triggered_buffer.h> @@ -1271,6 +1273,74 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p) return IRQ_HANDLED; } +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)) +{ + struct bmp280_data *data = iio_priv(indio_dev); + struct fwnode_handle *fwnode; + int ret, irq, irq_type; + struct irq_data *desc; + + fwnode = dev_fwnode(data->dev); + if (!fwnode) + return -ENODEV; + + irq = fwnode_irq_get(fwnode, 0); + if (!irq) + return dev_err_probe(data->dev, -ENODEV, + "No interrupt found.\n"); + + desc = irq_get_irq_data(irq); + if (!desc) + return -EINVAL; + + 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"); + + ret = int_config(data); + if (ret) + return ret; + + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d", + indio_dev->name, + iio_device_id(indio_dev)); + if (!data->trig) + return -ENOMEM; + + data->trig->ops = trigger_ops; + iio_trigger_set_drvdata(data->trig, data); + + ret = devm_request_threaded_irq(data->dev, irq, NULL, + irq_thread_handler, IRQF_ONESHOT, + indio_dev->name, indio_dev); + if (ret) + return dev_err_probe(data->dev, ret, "request irq failed.\n"); + + ret = devm_iio_trigger_register(data->dev, data->trig); + if (ret) + return dev_err_probe(data->dev, ret, + "iio trigger register failed.\n"); + + indio_dev->trig = iio_trigger_get(data->trig); + + return 0; +} + static const u8 bme280_chip_ids[] = { BME280_CHIP_ID }; static const int bme280_humid_coeffs[] = { 1000, 1024 }; @@ -1769,6 +1839,85 @@ static int bmp380_chip_config(struct bmp280_data *data) return 0; } +static void bmp380_trigger_reenable(struct iio_trigger *trig) +{ + struct bmp280_data *data = iio_trigger_get_drvdata(trig); + unsigned int tmp; + int ret; + + ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &tmp); + if (ret) + dev_err(data->dev, "Failed to reset interrupt\n"); +} + +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)); + if (ret) { + dev_err(data->dev, "Could not enable/disable interrupt\n"); + return ret; + } + + return 0; +} + +static const struct iio_trigger_ops bmp380_trigger_ops = { + .set_trigger_state = &bmp380_data_rdy_trigger_set_state, + .reenable = &bmp380_trigger_reenable, +}; + +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); + + 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; +} + +static irqreturn_t bmp380_irq_thread_handler(int irq, void *p) +{ + struct iio_dev *indio_dev = p; + struct bmp280_data *data = iio_priv(indio_dev); + unsigned int int_ctrl; + int ret; + + scoped_guard(mutex, &data->lock) { + ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &int_ctrl); + if (ret) + return IRQ_NONE; + } + + if (FIELD_GET(BMP380_INT_STATUS_DRDY, int_ctrl)) + iio_trigger_poll_nested(data->trig); + + return IRQ_HANDLED; +} + +static int bmp380_trigger_probe(struct iio_dev *indio_dev) +{ + return __bmp280_trigger_probe(indio_dev, &bmp380_trigger_ops, + bmp380_int_config, + bmp380_irq_thread_handler); +} + static irqreturn_t bmp380_trigger_handler(int irq, void *p) { struct iio_poll_func *pf = p; @@ -1863,6 +2012,7 @@ const struct bmp280_chip_info bmp380_chip_info = { .wait_conv = bmp380_wait_conv, .preinit = bmp380_preinit, + .trigger_probe = bmp380_trigger_probe, .trigger_handler = bmp380_trigger_handler, }; EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280); @@ -2401,6 +2551,92 @@ static int bmp580_chip_config(struct bmp280_data *data) return 0; } +static void bmp580_trigger_reenable(struct iio_trigger *trig) +{ + struct bmp280_data *data = iio_trigger_get_drvdata(trig); + unsigned int tmp; + int ret; + + ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &tmp); + if (ret) + dev_err(data->dev, "Failed to reset interrupt\n"); +} + +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)); + if (ret) { + dev_err(data->dev, "Could not enable/disable interrupt\n"); + return ret; + } + + return 0; +} + +static const struct iio_trigger_ops bmp580_trigger_ops = { + .set_trigger_state = &bmp580_data_rdy_trigger_set_state, + .reenable = &bmp580_trigger_reenable, +}; + +static int bmp580_int_config(struct bmp280_data *data) +{ + int ret, int_cfg = FIELD_PREP(BMP580_INT_CONFIG_OPEN_DRAIN, + data->trig_open_drain) | + FIELD_PREP(BMP580_INT_CONFIG_LEVEL, + data->trig_active_high); + + ret = regmap_update_bits(data->regmap, BMP580_REG_INT_CONFIG, + BMP580_INT_CONFIG_MASK, int_cfg); + if (ret) { + dev_err(data->dev, "Could not set interrupt settings\n"); + return ret; + } + + ret = regmap_set_bits(data->regmap, BMP580_REG_INT_SOURCE, + BMP580_INT_SOURCE_DRDY); + if (ret) { + dev_err(data->dev, "Could not set interrupt source\n"); + return ret; + } + + return 0; +} + +static irqreturn_t bmp580_irq_thread_handler(int irq, void *p) +{ + struct iio_dev *indio_dev = p; + struct bmp280_data *data = iio_priv(indio_dev); + unsigned int int_ctrl; + int ret; + + scoped_guard(mutex, &data->lock) { + ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &int_ctrl); + if (ret) + return IRQ_NONE; + } + + if (FIELD_GET(BMP580_INT_STATUS_DRDY_MASK, int_ctrl)) + iio_trigger_poll_nested(data->trig); + + return IRQ_HANDLED; +} + +static int bmp580_trigger_probe(struct iio_dev *indio_dev) +{ + return __bmp280_trigger_probe(indio_dev, &bmp580_trigger_ops, + bmp580_int_config, + bmp580_irq_thread_handler); +} + static irqreturn_t bmp580_trigger_handler(int irq, void *p) { struct iio_poll_func *pf = p; @@ -2477,6 +2713,7 @@ const struct bmp280_chip_info bmp580_chip_info = { .wait_conv = bmp580_wait_conv, .preinit = bmp580_preinit, + .trigger_probe = bmp580_trigger_probe, .trigger_handler = bmp580_trigger_handler, }; EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280); @@ -3024,10 +3261,17 @@ int bmp280_common_probe(struct device *dev, * however as it happens, the BMP085 shares the chip ID of BMP180 * so we look for an IRQ if we have that. */ - if (irq > 0 && (chip_id == BMP180_CHIP_ID)) { - ret = bmp085_fetch_eoc_irq(dev, name, irq, data); - if (ret) - return ret; + 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; + } } ret = data->chip_info->set_mode(data, BMP280_SLEEP); diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h index c9840b8d58b0..0c32b6430677 100644 --- a/drivers/iio/pressure/bmp280.h +++ b/drivers/iio/pressure/bmp280.h @@ -55,8 +55,17 @@ #define BMP580_CMD_NVM_WRITE_SEQ_1 0xA0 #define BMP580_CMD_SOFT_RESET 0xB6 +#define BMP580_INT_STATUS_DRDY_MASK BIT(0) #define BMP580_INT_STATUS_POR_MASK BIT(4) +#define BMP580_INT_SOURCE_DRDY BIT(0) + +#define BMP580_INT_CONFIG_MASK GENMASK(3, 0) +#define BMP580_INT_CONFIG_LATCH BIT(0) +#define BMP580_INT_CONFIG_LEVEL BIT(1) +#define BMP580_INT_CONFIG_OPEN_DRAIN BIT(2) +#define BMP580_INT_CONFIG_INT_EN BIT(3) + #define BMP580_STATUS_CORE_RDY_MASK BIT(0) #define BMP580_STATUS_NVM_RDY_MASK BIT(1) #define BMP580_STATUS_NVM_ERR_MASK BIT(2) @@ -175,6 +184,14 @@ #define BMP380_TEMP_MEAS_OFFSET 163 #define BMP380_PRESS_MEAS_OFFSET 392 +#define BMP380_INT_STATUS_DRDY BIT(3) + +#define BMP380_INT_CTRL_SETTINGS_MASK GENMASK(2, 0) +#define BMP380_INT_CTRL_OPEN_DRAIN BIT(0) +#define BMP380_INT_CTRL_LEVEL BIT(1) +#define BMP380_INT_CTRL_LATCH BIT(2) +#define BMP380_INT_CTRL_DRDY_EN BIT(6) + #define BMP380_MIN_TEMP -4000 #define BMP380_MAX_TEMP 8500 #define BMP380_MIN_PRES 3000000 @@ -406,6 +423,9 @@ struct bmp280_data { struct regmap *regmap; struct completion done; bool use_eoc; + bool trig_open_drain; + bool trig_active_high; + struct iio_trigger *trig; const struct bmp280_chip_info *chip_info; union { struct bmp180_calib bmp180; @@ -510,6 +530,7 @@ struct bmp280_chip_info { int (*set_mode)(struct bmp280_data *data, enum bmp280_op_mode mode); int (*wait_conv)(struct bmp280_data *data); + int (*trigger_probe)(struct iio_dev *indio_dev); irqreturn_t (*trigger_handler)(int irq, void *p); };
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. Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com> --- drivers/iio/pressure/bmp280-core.c | 252 ++++++++++++++++++++++++++++- drivers/iio/pressure/bmp280.h | 21 +++ 2 files changed, 269 insertions(+), 4 deletions(-)