| Message ID | 20210428082203.3587022-2-sean@geanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/4] dt-bindings: iio: accel: fxls8962af: add interrupt options | expand |
On Wed, 28 Apr 2021 10:22:01 +0200 Sean Nyekjaer <sean@geanix.com> wrote: > Preparation commit for the next that adds hw buffered sampling > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> Entirely trivial comment inline. Otherwise looks good. > --- > This series depends on "iio: accel: add support for > FXLS8962AF/FXLS8964AF accelerometers" > > drivers/iio/accel/fxls8962af-core.c | 116 ++++++++++++++++++++++++++++ > 1 file changed, 116 insertions(+) > > diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c > index b47d81bebf43..848f3d68f5d4 100644 > --- a/drivers/iio/accel/fxls8962af-core.c > +++ b/drivers/iio/accel/fxls8962af-core.c > @@ -15,6 +15,7 @@ > #include <linux/bits.h> > #include <linux/bitfield.h> > #include <linux/module.h> > +#include <linux/of_irq.h> > #include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > #include <linux/regmap.h> > @@ -54,6 +55,10 @@ > #define FXLS8962AF_SC3_WAKE_ODR_PREP(x) FIELD_PREP(FXLS8962AF_SC3_WAKE_ODR_MASK, x) > #define FXLS8962AF_SC3_WAKE_ODR_GET(x) FIELD_GET(FXLS8962AF_SC3_WAKE_ODR_MASK, x) > #define FXLS8962AF_SENS_CONFIG4 0x18 > +#define FXLS8962AF_SC4_INT_PP_OD_MASK BIT(1) > +#define FXLS8962AF_SC4_INT_PP_OD_PREP(x) FIELD_PREP(FXLS8962AF_SC4_INT_PP_OD_MASK, x) > +#define FXLS8962AF_SC4_INT_POL_MASK BIT(0) > +#define FXLS8962AF_SC4_INT_POL_PREP(x) FIELD_PREP(FXLS8962AF_SC4_INT_POL_MASK, x) > #define FXLS8962AF_SENS_CONFIG5 0x19 > > #define FXLS8962AF_WAKE_IDLE_LSB 0x1b > @@ -62,6 +67,9 @@ > > #define FXLS8962AF_INT_EN 0x20 > #define FXLS8962AF_INT_PIN_SEL 0x21 > +#define FXLS8962AF_INT_PIN_SEL_MASK GENMASK(7, 0) > +#define FXLS8962AF_INT_PIN_SEL_INT1 0x00 > +#define FXLS8962AF_INT_PIN_SEL_INT2 GENMASK(7, 0) > > #define FXLS8962AF_OFF_X 0x22 > #define FXLS8962AF_OFF_Y 0x23 > @@ -142,6 +150,11 @@ enum { > fxls8962af_idx_ts, > }; > > +enum fxls8962af_int_pin { > + FXLS8962AF_PIN_INT1, > + FXLS8962AF_PIN_INT2, > +}; > + > static int fxls8962af_drdy(struct fxls8962af_data *data) > { > struct device *dev = regmap_get_device(data->regmap); > @@ -559,6 +572,20 @@ static int fxls8962af_reset(struct fxls8962af_data *data) > return ret; > } > > +static irqreturn_t fxls8962af_interrupt(int irq, void *p) > +{ > + struct iio_dev *indio_dev = p; > + struct fxls8962af_data *data = iio_priv(indio_dev); > + unsigned int reg; > + int ret; > + > + ret = regmap_read(data->regmap, FXLS8962AF_INT_STATUS, ®); > + if (ret < 0) > + return IRQ_NONE; > + > + return IRQ_NONE; > +} > + > static void fxls8962af_regulator_disable(void *data_ptr) > { > struct fxls8962af_data *data = data_ptr; > @@ -578,6 +605,89 @@ static void fxls8962af_pm_disable(void *dev_ptr) > fxls8962af_standby(iio_priv(indio_dev)); > } > > +static void fxls8962af_get_irq(struct device_node *of_node, enum fxls8962af_int_pin *pin) > +{ > + int irq; > + > + irq = of_irq_get_byname(of_node, "INT2"); > + if (irq > 0) { > + *pin = FXLS8962AF_PIN_INT2; > + return; > + } > + > + *pin = FXLS8962AF_PIN_INT1; > +} > + > +static int fxls8962af_irq_setup(struct iio_dev *indio_dev, int irq) > +{ > + struct fxls8962af_data *data = iio_priv(indio_dev); > + struct device *dev = regmap_get_device(data->regmap); > + unsigned long irq_type; > + bool irq_active_high; > + enum fxls8962af_int_pin int_pin; > + u8 int_pin_sel; > + int ret; > + > + fxls8962af_get_irq(dev->of_node, &int_pin); > + switch (int_pin) { > + case FXLS8962AF_PIN_INT1: > + int_pin_sel = FXLS8962AF_INT_PIN_SEL_INT1; > + break; > + case FXLS8962AF_PIN_INT2: > + int_pin_sel = FXLS8962AF_INT_PIN_SEL_INT2; > + break; > + default: > + dev_err(dev, "unsupported int pin selected\n"); > + return -EINVAL; > + } > + > + ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_PIN_SEL, > + FXLS8962AF_INT_PIN_SEL_MASK, > + int_pin_sel); > + if (ret) > + return ret; > + > + irq_type = irqd_get_trigger_type(irq_get_irq_data(irq)); > + > + switch (irq_type) { > + case IRQF_TRIGGER_HIGH: > + case IRQF_TRIGGER_RISING: > + irq_active_high = true; > + break; > + case IRQF_TRIGGER_LOW: > + case IRQF_TRIGGER_FALLING: > + irq_active_high = false; > + break; > + default: > + dev_info(dev, "mode %lx unsupported\n", irq_type); > + return -EINVAL; > + } > + > + ret = regmap_update_bits(data->regmap, FXLS8962AF_SENS_CONFIG4, > + FXLS8962AF_SC4_INT_POL_MASK, > + FXLS8962AF_SC4_INT_POL_PREP(irq_active_high)); > + if (ret < 0) > + return ret; > + > + if (device_property_read_bool(dev, "drive-open-drain")) { > + ret = regmap_update_bits(data->regmap, FXLS8962AF_SENS_CONFIG4, > + FXLS8962AF_SC4_INT_PP_OD_MASK, > + FXLS8962AF_SC4_INT_PP_OD_PREP(1)); > + if (ret < 0) > + return ret; > + > + irq_type |= IRQF_SHARED; > + } > + > + ret = devm_request_threaded_irq(dev, > + irq, > + NULL, fxls8962af_interrupt, > + irq_type | IRQF_ONESHOT, > + indio_dev->name, indio_dev); > + > + return ret; Combine these last two lines into return devm_request_threaded_irq(...) > +} > + > int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq) > { > struct fxls8962af_data *data; > @@ -637,6 +747,12 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq) > if (ret < 0) > return ret; > > + if (irq) { > + ret = fxls8962af_irq_setup(indio_dev, irq); > + if (ret < 0) > + return ret; > + } > + > ret = pm_runtime_set_active(dev); > if (ret < 0) > return ret;
On 4/28/21 10:22 AM, Sean Nyekjaer wrote: > Preparation commit for the next that adds hw buffered sampling > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- > [...] > > +static void fxls8962af_get_irq(struct device_node *of_node, enum fxls8962af_int_pin *pin) > +{ > + int irq; > + > + irq = of_irq_get_byname(of_node, "INT2"); For this I'd use device_property_match_string(dev, "interrupt-names", "INT2"). Means it won't try to map the interrupt again, and also this is the only place where the driver directly depends on OF, everything else already uses the device_ API. > + if (irq > 0) { > + *pin = FXLS8962AF_PIN_INT2; > + return; > + } > + > + *pin = FXLS8962AF_PIN_INT1; > +}
On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > On 4/28/21 10:22 AM, Sean Nyekjaer wrote: > > Preparation commit for the next that adds hw buffered sampling ... > > + irq = of_irq_get_byname(of_node, "INT2"); > > For this I'd use device_property_match_string(dev, "interrupt-names", > "INT2"). Means it won't try to map the interrupt again, and also this is > the only place where the driver directly depends on OF, everything else > already uses the device_ API. Why not platform_get_irq_byname_optional() ? > > + if (irq > 0) { > > + *pin = FXLS8962AF_PIN_INT2; > > + return; > > + }
On 4/29/21 11:35 AM, Andy Shevchenko wrote: > On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote: >> On 4/28/21 10:22 AM, Sean Nyekjaer wrote: >>> Preparation commit for the next that adds hw buffered sampling > ... > >>> + irq = of_irq_get_byname(of_node, "INT2"); >> For this I'd use device_property_match_string(dev, "interrupt-names", >> "INT2"). Means it won't try to map the interrupt again, and also this is >> the only place where the driver directly depends on OF, everything else >> already uses the device_ API. > Why not platform_get_irq_byname_optional() ? Because it is not a platform device :)
On Thu, Apr 29, 2021 at 12:37 PM Lars-Peter Clausen <lars@metafoo.de> wrote: > > On 4/29/21 11:35 AM, Andy Shevchenko wrote: > > On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > >> On 4/28/21 10:22 AM, Sean Nyekjaer wrote: > >>> Preparation commit for the next that adds hw buffered sampling > > ... > > > >>> + irq = of_irq_get_byname(of_node, "INT2"); > >> For this I'd use device_property_match_string(dev, "interrupt-names", > >> "INT2"). Means it won't try to map the interrupt again, and also this is > >> the only place where the driver directly depends on OF, everything else > >> already uses the device_ API. > > Why not platform_get_irq_byname_optional() ? > Because it is not a platform device :) Then device_property reading like this isn't really needed. What is missed is fwnode_irq_get_by_name() API which will do the right things on any resource provider.
On 4/29/21 1:35 PM, Andy Shevchenko wrote: > On Thu, Apr 29, 2021 at 12:37 PM Lars-Peter Clausen <lars@metafoo.de> wrote: >> On 4/29/21 11:35 AM, Andy Shevchenko wrote: >>> On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote: >>>> On 4/28/21 10:22 AM, Sean Nyekjaer wrote: >>>>> Preparation commit for the next that adds hw buffered sampling >>> ... >>> >>>>> + irq = of_irq_get_byname(of_node, "INT2"); >>>> For this I'd use device_property_match_string(dev, "interrupt-names", >>>> "INT2"). Means it won't try to map the interrupt again, and also this is >>>> the only place where the driver directly depends on OF, everything else >>>> already uses the device_ API. >>> Why not platform_get_irq_byname_optional() ? >> Because it is not a platform device :) > Then device_property reading like this isn't really needed. Why?
On Thu, Apr 29, 2021 at 10:19 PM Lars-Peter Clausen <lars@metafoo.de> wrote: > On 4/29/21 1:35 PM, Andy Shevchenko wrote: > > On Thu, Apr 29, 2021 at 12:37 PM Lars-Peter Clausen <lars@metafoo.de> wrote: > >> On 4/29/21 11:35 AM, Andy Shevchenko wrote: > >>> On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > >>>> On 4/28/21 10:22 AM, Sean Nyekjaer wrote: > >>>>> Preparation commit for the next that adds hw buffered sampling > >>> ... > >>> > >>>>> + irq = of_irq_get_byname(of_node, "INT2"); > >>>> For this I'd use device_property_match_string(dev, "interrupt-names", > >>>> "INT2"). Means it won't try to map the interrupt again, and also this is > >>>> the only place where the driver directly depends on OF, everything else > >>>> already uses the device_ API. > >>> Why not platform_get_irq_byname_optional() ? > >> Because it is not a platform device :) > > Then device_property reading like this isn't really needed. > Why? Because it doesn't bring any value in this case and actually makes readers confused. ACPI doesn't have properties started with # (they are special for OF and hiding it behind device property API is not correct). So, either leave it OF, or introduce the above API (or use existing fwnode_get_irq() by index).
diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c index b47d81bebf43..848f3d68f5d4 100644 --- a/drivers/iio/accel/fxls8962af-core.c +++ b/drivers/iio/accel/fxls8962af-core.c @@ -15,6 +15,7 @@ #include <linux/bits.h> #include <linux/bitfield.h> #include <linux/module.h> +#include <linux/of_irq.h> #include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> #include <linux/regmap.h> @@ -54,6 +55,10 @@ #define FXLS8962AF_SC3_WAKE_ODR_PREP(x) FIELD_PREP(FXLS8962AF_SC3_WAKE_ODR_MASK, x) #define FXLS8962AF_SC3_WAKE_ODR_GET(x) FIELD_GET(FXLS8962AF_SC3_WAKE_ODR_MASK, x) #define FXLS8962AF_SENS_CONFIG4 0x18 +#define FXLS8962AF_SC4_INT_PP_OD_MASK BIT(1) +#define FXLS8962AF_SC4_INT_PP_OD_PREP(x) FIELD_PREP(FXLS8962AF_SC4_INT_PP_OD_MASK, x) +#define FXLS8962AF_SC4_INT_POL_MASK BIT(0) +#define FXLS8962AF_SC4_INT_POL_PREP(x) FIELD_PREP(FXLS8962AF_SC4_INT_POL_MASK, x) #define FXLS8962AF_SENS_CONFIG5 0x19 #define FXLS8962AF_WAKE_IDLE_LSB 0x1b @@ -62,6 +67,9 @@ #define FXLS8962AF_INT_EN 0x20 #define FXLS8962AF_INT_PIN_SEL 0x21 +#define FXLS8962AF_INT_PIN_SEL_MASK GENMASK(7, 0) +#define FXLS8962AF_INT_PIN_SEL_INT1 0x00 +#define FXLS8962AF_INT_PIN_SEL_INT2 GENMASK(7, 0) #define FXLS8962AF_OFF_X 0x22 #define FXLS8962AF_OFF_Y 0x23 @@ -142,6 +150,11 @@ enum { fxls8962af_idx_ts, }; +enum fxls8962af_int_pin { + FXLS8962AF_PIN_INT1, + FXLS8962AF_PIN_INT2, +}; + static int fxls8962af_drdy(struct fxls8962af_data *data) { struct device *dev = regmap_get_device(data->regmap); @@ -559,6 +572,20 @@ static int fxls8962af_reset(struct fxls8962af_data *data) return ret; } +static irqreturn_t fxls8962af_interrupt(int irq, void *p) +{ + struct iio_dev *indio_dev = p; + struct fxls8962af_data *data = iio_priv(indio_dev); + unsigned int reg; + int ret; + + ret = regmap_read(data->regmap, FXLS8962AF_INT_STATUS, ®); + if (ret < 0) + return IRQ_NONE; + + return IRQ_NONE; +} + static void fxls8962af_regulator_disable(void *data_ptr) { struct fxls8962af_data *data = data_ptr; @@ -578,6 +605,89 @@ static void fxls8962af_pm_disable(void *dev_ptr) fxls8962af_standby(iio_priv(indio_dev)); } +static void fxls8962af_get_irq(struct device_node *of_node, enum fxls8962af_int_pin *pin) +{ + int irq; + + irq = of_irq_get_byname(of_node, "INT2"); + if (irq > 0) { + *pin = FXLS8962AF_PIN_INT2; + return; + } + + *pin = FXLS8962AF_PIN_INT1; +} + +static int fxls8962af_irq_setup(struct iio_dev *indio_dev, int irq) +{ + struct fxls8962af_data *data = iio_priv(indio_dev); + struct device *dev = regmap_get_device(data->regmap); + unsigned long irq_type; + bool irq_active_high; + enum fxls8962af_int_pin int_pin; + u8 int_pin_sel; + int ret; + + fxls8962af_get_irq(dev->of_node, &int_pin); + switch (int_pin) { + case FXLS8962AF_PIN_INT1: + int_pin_sel = FXLS8962AF_INT_PIN_SEL_INT1; + break; + case FXLS8962AF_PIN_INT2: + int_pin_sel = FXLS8962AF_INT_PIN_SEL_INT2; + break; + default: + dev_err(dev, "unsupported int pin selected\n"); + return -EINVAL; + } + + ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_PIN_SEL, + FXLS8962AF_INT_PIN_SEL_MASK, + int_pin_sel); + if (ret) + return ret; + + irq_type = irqd_get_trigger_type(irq_get_irq_data(irq)); + + switch (irq_type) { + case IRQF_TRIGGER_HIGH: + case IRQF_TRIGGER_RISING: + irq_active_high = true; + break; + case IRQF_TRIGGER_LOW: + case IRQF_TRIGGER_FALLING: + irq_active_high = false; + break; + default: + dev_info(dev, "mode %lx unsupported\n", irq_type); + return -EINVAL; + } + + ret = regmap_update_bits(data->regmap, FXLS8962AF_SENS_CONFIG4, + FXLS8962AF_SC4_INT_POL_MASK, + FXLS8962AF_SC4_INT_POL_PREP(irq_active_high)); + if (ret < 0) + return ret; + + if (device_property_read_bool(dev, "drive-open-drain")) { + ret = regmap_update_bits(data->regmap, FXLS8962AF_SENS_CONFIG4, + FXLS8962AF_SC4_INT_PP_OD_MASK, + FXLS8962AF_SC4_INT_PP_OD_PREP(1)); + if (ret < 0) + return ret; + + irq_type |= IRQF_SHARED; + } + + ret = devm_request_threaded_irq(dev, + irq, + NULL, fxls8962af_interrupt, + irq_type | IRQF_ONESHOT, + indio_dev->name, indio_dev); + + return ret; +} + int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq) { struct fxls8962af_data *data; @@ -637,6 +747,12 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq) if (ret < 0) return ret; + if (irq) { + ret = fxls8962af_irq_setup(indio_dev, irq); + if (ret < 0) + return ret; + } + ret = pm_runtime_set_active(dev); if (ret < 0) return ret;
Preparation commit for the next that adds hw buffered sampling Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- This series depends on "iio: accel: add support for FXLS8962AF/FXLS8964AF accelerometers" drivers/iio/accel/fxls8962af-core.c | 116 ++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+)