On Thu, Aug 29, 2024 at 03:40:34PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 28, 2024 at 10:51:26PM +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.
>
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
> > ---
> > drivers/iio/pressure/bmp280-core.c | 231 ++++++++++++++++++++++++++++-
> > drivers/iio/pressure/bmp280.h | 21 +++
> > 2 files changed, 250 insertions(+), 2 deletions(-)
>
> ...
>
> > +static int __bmp280_trigger_probe(struct iio_dev *indio_dev,
> > + const struct iio_trigger_ops *trigger_ops,
> > + int (*int_config)(struct bmp280_data *data),
> > + irq_handler_t irq_thread_handler)
> > +{
> > + struct bmp280_data *data = iio_priv(indio_dev);
>
> With
>
> struct device *dev = data->dev;
>
> you may shorten some lines below and collapse a few.
>
ACK.
> > + struct fwnode_handle *fwnode;
> > + int ret, irq, irq_type;
>
> Why irq_type is signed?
>
True, this can be made u32.
> Also try to make that returned variable is closer to the end of the definition
> block. And it might be worth to follow reversed xmas tree order (longer lines
> first).
>
> > + struct irq_data *desc;
> > +
> > + irq = fwnode_irq_get(dev_fwnode(data->dev), 0);
> > + if (irq < 0)
> > + return dev_err_probe(data->dev, irq, "No interrupt found.\n");
> > +
> > + desc = irq_get_irq_data(irq);
> > + irq_type = irqd_get_trigger_type(desc);
>
> So, altogether it may be written as
>
> irq_type = irqd_get_trigger_type(irq_get_irq_data(irq));
>
> And looking further, we have a helper for that:
> irq_get_trigger_type(). Why not use it?
>
I was not aware of that, I can definitely change it.
> > + 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 int bmp380_int_config(struct bmp280_data *data)
> > +{
> > + int pin_drive_cfg = FIELD_PREP(BMP380_INT_CTRL_OPEN_DRAIN,
> > + data->trig_open_drain);
> > + int pin_level_cfg = FIELD_PREP(BMP380_INT_CTRL_LEVEL,
> > + data->trig_active_high);
> > + int ret, int_cfg = pin_drive_cfg | pin_level_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 ret;
>
> One of them is redundant or the last one should be return 0, but why not just
> leave one of them?
>
I can just leave one of them, true!
> > +}
>
> ...
>
> > +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;
>
> So far you have three different styles in the same patch for this!
> Choose one and be consistent with it.
>
> > +}
>
> ...
>
> > + int (*trigger_probe)(struct iio_dev *indio_dev);
> > irqreturn_t (*trigger_handler)(int irq, void *p);
>
> Yeah, at some point this can be changed to
>
> irq_handler_t trigger_handler;
>
I can definitely try that in a separate patch, thanks for pointing
all these out!
Cheers,
Vasilis
> --
> With Best Regards,
> Andy Shevchenko
>
>
