[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240904102812.GB44250@vamoiridPC>
Date: Wed, 4 Sep 2024 12:28:12 +0200
From: Vasileios Amoiridis <vassilisamir@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Vasileios Amoiridis <vassilisamir@...il.com>, jic23@...nel.org,
lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, ang.iglesiasg@...il.com,
linus.walleij@...aro.org, biju.das.jz@...renesas.com,
javier.carrasco.cruz@...il.com, semen.protsenko@...aro.org,
579lpy@...il.com, ak@...klinger.de, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
christophe.jaillet@...adoo.fr
Subject: Re: [PATCH v5 6/7] iio: pressure: bmp280: Add data ready trigger
support
On Tue, Sep 03, 2024 at 05:36:13PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 02, 2024 at 08:42:21PM +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),
> > + irq_handler_t irq_thread_handler)
>
> Would it make sense (note, I do *not* know the correct answer!) to have
> something like
>
> struct foo {
> const struct iio_trigger_ops *trigger_ops;
> int (*int_config)(struct bmp280_data *data);
> irq_handler_t irq_thread_handler;
> };
>
> and pass it around?
>
>From my point of view it won't be more clear, it might complicate things,
I feel like this *looks* better.
> Also int_config sounds non-related to interrupt, however it's about interrupt
> pin, right? perhaps name it differently here?
>
> E.g.,
>
> interrupt_pin_config
>
> ?
>
This sounds good yes!
> > +{
> > + struct bmp280_data *data = iio_priv(indio_dev);
> > + struct device *dev = data->dev;
> > + struct fwnode_handle *fwnode;
> > + u32 irq_type;
> > + int ret, irq;
> > +
> > + irq = fwnode_irq_get(dev_fwnode(dev), 0);
> > + if (irq < 0)
> > + return dev_err_probe(dev, irq, "No interrupt found.\n");
> > +
> > + irq_type = irq_get_trigger_type(irq);
> > + 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(dev, -EINVAL, "Invalid interrupt type specified.\n");
> > + }
> > +
> > + data->trig_open_drain =
> > + fwnode_property_read_bool(fwnode, "int-open-drain");
>
>
> Where do you initialise fwnode?
>
Ok, I need to retest this with the device-tree.
> > + 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(dev, ret, "request irq failed.\n");
>
> IRQ
>
ACK.
> > + ret = devm_iio_trigger_register(data->dev, data->trig);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "iio trigger register failed.\n");
> > +
> > + indio_dev->trig = iio_trigger_get(data->trig);
> > +
> > + return 0;
> > +}
>
> ...
>
> > +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));
> > + if (ret)
> > + dev_err(data->dev,
> > + "Could not %s interrupt.\n", str_enable_disable(state));
> > + return ret;
>
> Somewhere above (or in another patch) you used the style with 'return 0;' at
> the end. Please, check the resulting code for the consistency and choose one
> style for all.
>
ACK.
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Powered by blists - more mailing lists