Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH 1/4] clk/soc: mediatek: mt8183: Bind clock driver from platform device
From: Matthias Brugger @ 2020-05-18 15:57 UTC (permalink / raw)
  To: Chun-Kuang Hu, matthias.bgg
  Cc: Rob Herring, Michael Turquette, Stephen Boyd, Kate Stewart,
	devicetree, Greg Kroah-Hartman, linux-kernel,
	moderated list:ARM/Mediatek SoC support, Linux ARM, mtk01761,
	Thomas Gleixner, linux-clk, Allison Randal,
	Enric Balletbo i Serra
In-Reply-To: <CAAOTY_9-_fy4oGTBLPsXkWJ2ihL7k2hzfwiTorit+YkNi_SeMw@mail.gmail.com>



On 18/05/2020 17:30, Chun-Kuang Hu wrote:
> Hi, Matthias:
> 
> <matthias.bgg@kernel.org> 於 2020年5月18日 週一 下午7:32寫道:
>>
>> From: Matthias Brugger <matthias.bgg@gmail.com>
>>
>> The mmsys driver is now the top level entry point for the multimedia
>> system (mmsys), we bind the clock driver by creating a platform device.
>> We also bind the MediaTek DRM driver which is not yet implement and
>> therefor will errror out for now.
> 
> You may need to let CONFIG_MTK_MMSYS depends on
> CONFIG_COMMON_CLK_MT8173_MMSYS || CONFIG_COMMON_CLK_MT8183_MMSYS
> 

That will be fixed by:
https://patchwork.kernel.org/patch/11469521/

Otherwise this won't scale as we would need to add the SoCs using the mtk-mmsys
driver and this will be a lot if not all.

Regards,
Matthias

> Regards,
> Chun-Kuang.
> 
>>
>> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
>> ---
>>
>>  drivers/clk/mediatek/clk-mt8183-mm.c | 9 ++-------
>>  drivers/soc/mediatek/mtk-mmsys.c     | 8 ++++++++
>>  2 files changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8183-mm.c b/drivers/clk/mediatek/clk-mt8183-mm.c
>> index 720c696b506d..9d60e09619c1 100644
>> --- a/drivers/clk/mediatek/clk-mt8183-mm.c
>> +++ b/drivers/clk/mediatek/clk-mt8183-mm.c
>> @@ -84,8 +84,9 @@ static const struct mtk_gate mm_clks[] = {
>>
>>  static int clk_mt8183_mm_probe(struct platform_device *pdev)
>>  {
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *node = dev->parent->of_node;
>>         struct clk_onecell_data *clk_data;
>> -       struct device_node *node = pdev->dev.of_node;
>>
>>         clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
>>
>> @@ -95,16 +96,10 @@ static int clk_mt8183_mm_probe(struct platform_device *pdev)
>>         return of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
>>  }
>>
>> -static const struct of_device_id of_match_clk_mt8183_mm[] = {
>> -       { .compatible = "mediatek,mt8183-mmsys", },
>> -       {}
>> -};
>> -
>>  static struct platform_driver clk_mt8183_mm_drv = {
>>         .probe = clk_mt8183_mm_probe,
>>         .driver = {
>>                 .name = "clk-mt8183-mm",
>> -               .of_match_table = of_match_clk_mt8183_mm,
>>         },
>>  };
>>
>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
>> index cacafe23c823..783c3dd008b2 100644
>> --- a/drivers/soc/mediatek/mtk-mmsys.c
>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
>> @@ -92,6 +92,10 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
>>         .clk_driver = "clk-mt8173-mm",
>>  };
>>
>> +static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
>> +       .clk_driver = "clk-mt8183-mm",
>> +};
>> +
>>  static unsigned int mtk_mmsys_ddp_mout_en(enum mtk_ddp_comp_id cur,
>>                                           enum mtk_ddp_comp_id next,
>>                                           unsigned int *addr)
>> @@ -339,6 +343,10 @@ static const struct of_device_id of_match_mtk_mmsys[] = {
>>                 .compatible = "mediatek,mt8173-mmsys",
>>                 .data = &mt8173_mmsys_driver_data,
>>         },
>> +       {
>> +               .compatible = "mediatek,mt8183-mmsys",
>> +               .data = &mt8183_mmsys_driver_data,
>> +       },
>>         { }
>>  };
>>
>> --
>> 2.26.2
>>
>>
>> _______________________________________________
>> Linux-mediatek mailing list
>> Linux-mediatek@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply

* Re: u-boot DT configuration node
From: Rob Herring @ 2020-05-18 15:55 UTC (permalink / raw)
  To: Michal Simek
  Cc: devicetree@vger.kernel.org, u-boot@lists.denx.de, Tom Rini,
	Loic Poulain
In-Reply-To: <CAHTX3d+c5kEQdoxc0vZ4h_eSpPrnxE0wtgEfOt8eaX2Gt7Oq1g@mail.gmail.com>

On Thu, May 14, 2020 at 12:47 PM Michal Simek <monstr@monstr.eu> wrote:
>
> čt 14. 5. 2020 v 20:07 odesílatel Rob Herring <robh@kernel.org> napsal:
> >
> > On Thu, Apr 30, 2020 at 6:13 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > >
> > > On 29. 04. 20 16:55, Rob Herring wrote:
> > > > On Tue, Apr 28, 2020 at 8:51 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > > >>
> > > >> On 28. 04. 20 15:23, Rob Herring wrote:
> > > >>> On Wed, Apr 1, 2020 at 4:23 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > > >>>>
> > > >>>> Hi Rob and others,
> > > >>>>
> > > >>>> for couple of years already u-boot is using config node in root DT for
> > > >>>> u-boot configuration.
> > > >>>>
> > > >>>> Here is one example in u-boot source code.
> > > >>>> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/exynos5250-spring.dts#L47
> > > >>>>
> > > >>>> And here is dt binding description
> > > >>>> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/doc/device-tree-bindings/config.txt
> > > >>>>
> > > >>>> I was checking dt binding specification and there no such a thing
> > > >>>> described there. It means I expect this is more adhoc u-boot solution.
> > > >>>> We have reached the point where could be beneficial to put some u-boot
> > > >>>> specific configurations to DT.
> > > >>>>
> > > >>>> Actually I have done similar thing some time ago too by using chosen
> > > >>>> node and add xilinx specific property there to point to eeprom.
> > > >>>> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/zynqmp-zcu102-revA.dts#L39
> > > >>>
> > > >>> In this case, I think an alias should be used as it's more of just a
> > > >>> shortcut to finding a specific node.
> > > >>
> > > >> What alias name do you suggest to use?
> > > >> We have systems where one i2c eeprom described based board and another
> > > >> i2c eeprom describe bootable module. And I need to have shotcuts to both
> > > >> of them.
> > > >>
> > > >> dt specification doesn't list any keywords for aliases but there is
> > > >> generic name recommendation.
> > > >
> > > > I do want make aliases a registered list of names.
> > > >
> > > >> Based on keywords it should look like this.
> > > >> eeprom0 = ...;
> > > >> eeprom1 = ...;
> > > >
> > > > That was my initial thought, but maybe "nvmemX" to be a bit more generic.
> > >
> > > I am fine with that. It means that multiple eeproms and order will be
> > > direct by alias number.
> > > In past I wanted to use list but aliases number is also fine.
> > >
> > > >
> > > >
> > > >>>> I think it is a time to discuss it and do it properly.
> > > >>>>
> > > >>>> First of all my question is where we could list SW prefixes to make sure
> > > >>>> that they are listed and everybody is aware about it. We have
> > > >>>> vendor-prefixes and we should have a way to record also prefixes for sw
> > > >>>> projects. U-Boot is using u-boot. Xen has file in the kernel with using
> > > >>>> xen prefix. At least these two should be listed.
> > > >>>
> > > >>> Documentation/devicetree/bindings/vendor-prefixes.yaml.
> > > >>
> > > >> thx
> > >
> > > Sent a patch for it. Please review.
> > > https://lore.kernel.org/linux-devicetree/85b8dc9e6288270bbfdf55f1c156dba160293f01.1588239081.git.michal.simek@xilinx.com/
> > >
> > >
> > > >>>> Next my question is what is the recommended way to pass sw specific
> > > >>>> parameters via DT? I think using chosen node is more appropriate then
> > > >>>> adhoc config node. Or is there a better way how this should be done?
> > > >>>
> > > >>> /chosen
> > > >>>
> > > >>> For vendor specific things though I would be cautious. If they are
> > > >>> settings for a specific device, then they probably belong in the
> > > >>> device's node. Second, are they really vendor specific? What we don't
> > > >>> want is each vendor doing the same thing in slightly different ways.
> > > >>
> > > >> For u-boot specific setting like - offsets it should be generic for
> > > >> everybody. I was already talking to Loic that for saving u-boot
> > > >> variables to QSPI we should be using MTD partition map and put there
> > > >> maybe a flag to say that this is the location for storing them.
> > > >
> > > > I'd standardize on the partition name.
> > >
> > > ok. Documentation/devicetree/bindings/mtd/partition.txt?
> > >
> > > I have grep u-boot repo and I see these label names
> > >
> > > "NAND.u-boot";
> > > "NAND.u-boot-env";
> > > "NAND.u-boot-env.backup1";
> > > "NAND.u-boot-spl-os";
> > > "QSPI.u-boot";
> > > "QSPI.u-boot-env";
> > > "QSPI.u-boot-env.backup1";
> > > "qspi-u-boot-img";
> > > "qspi-u-boot-spl";
> > > "QSPI.u-boot-spl-os";
> > > "u-boot
> > > "u-boot";
> > > "u-boot-2";
> > > "u-boot-2.backup1";
> > > "u-boot.backup1";
> > > "u-boot-env";
> > > "u-boot-env.backup1";
> > > "u-boot-spl";
> > >
> > > kernel is kind of similar
> > > "alt-u-boot";
> > > "alt-u-boot-env";
> > > "NAND.u-boot";
> > > "NAND.u-boot-env";
> > > "NAND.u-boot-env.backup1";
> > > "NAND.u-boot-spl-os";
> > > "QSPI.u-boot";
> > > "QSPI.u-boot-env";
> > > "QSPI.u-boot-env.backup1";
> > > "QSPI.u-boot-spl-os";
> > > "u-boot
> > > "u-boot";
> > > "u-boot.backup1";
> > > "u-boot-env";
> > > "u-boot-env2";
> > > "u-boot-env.backup1";
> > > "u-boot-environment";
> > > "u-boot-factory";
> > > "u-boot-nand";
> > > "u-boot-nor";
> > > "u-boot-spi";
> > > "u-boot-spl";
> > >
> > > It means it is mix of names. I think SPI cases are the most complicated
> > > one because you can have multiple spi devices in the system and you
> > > can't use the same name for registration.
> > >
> > > That's why I think that make sense to use an optional prefix as people
> > > are using QSPI/NAND already. But not quite sure that using QSPI is
> > > generic enough because you can have multiple QSPIs. Using alias name is
> > > also not ideal because one simple change in aliases would require
> > > changes in partition name/label.
> > > Any better suggestion?
> >
> > Okay, that's a mess of names. I guess perhaps properties in /chosen
> > pointing to data would work. Then you just have to update that
> > property if you're switching partitions (using SPI vs. MMC or  for A/B
> > style partition switching). We should point to partitions rather than
> > raw offsets though.
>
> That means that when you deploy images this property doesn't need to be there
> and then your firmware (in our case u-boot) can fill this property
> based on your logic.
> I definitely want to avoid cases where we would need to maintain
> different DTs for
> different mode which would bring more overhead.

Not sure I follow. How would u-boot fill in properties it needs for
itself? The properties could be populated whenever the partitions are.
Just as stdout-path is populated when the uart node is.

> We should document these u-boot properties in the u-boot project for sure.
> But there could also be the reason to do it in Linux because likely
> these properties
> will get to Linux dtses. Would be good to get some feedback on this.

No problem with that as long as the properties are documented.

> And if we should
> document it in Linux, path and name suggestions would be welcome.

/chosen already has a schema in dt-schema[1]. Add it there or add a
chosen-u-boot.yaml.

Rob

[1] https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml

^ permalink raw reply

* Re: [PATCH 10/12] iio: imu: inv_icm42600: add accurate timestamping
From: Jean-Baptiste Maneyrol @ 2020-05-18 15:48 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: jic23@kernel.org, robh+dt@kernel.org, robh@kernel.org,
	mchehab+huawei@kernel.org, davem@davemloft.net,
	gregkh@linuxfoundation.org, linux-iio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20200508154247.00007599@Huawei.com>

Hi Jonathan,

reading timestamp register is here mainly because we can do it, and this is the only way to have the full 20 bits resolution.
But it is not very useful indeed. I will delete it.

I am not using timestamp stored inside the FIFO, because it is only present when having both accel and gyro on. There is no timestamp data for a single sensor, which is really too bad. That would have been helpful.

I will add more documentation about it. It is not that tricky, but need more deep explanations.

New series V2 is in preparation. And again my apologizes for ruining the mail text formatting.

Thanks,
JB


From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>

Sent: Friday, May 8, 2020 16:42

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: jic23@kernel.org <jic23@kernel.org>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>;
 linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>

Subject: Re: [PATCH 10/12] iio: imu: inv_icm42600: add accurate timestamping

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



On Thu, 7 May 2020 16:42:20 +0200

Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:



> Add a timestamp channel with processed value that returns full

> precision 20 bits timestamp.

> 

> Add a timestamping mechanism for buffer that provides accurate

> event timestamps when using watermark. This mechanism estimates

> device internal clock by comparing FIFO interrupts delta time and

> corresponding device elapsed time computed by parsing FIFO data.



Yikes. That is complex.  Hmm. I always get lost in the maths in these

timestamp patches so my review may be a little superficial.



However a bit more in the way of docs would be good here.



The sysfs read of timestamp is unusual and I'm not really sure what it is for.

The delays in a sysfs read of that value are likely to be enough that it's

that useful for anything.



Also, do we make use of the timestamps within the fifo records?



J





> 

> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

> ---

>  drivers/iio/imu/inv_icm42600/Makefile         |   1 +

>  drivers/iio/imu/inv_icm42600/inv_icm42600.h   |  10 +-

>  .../iio/imu/inv_icm42600/inv_icm42600_accel.c |  32 ++-

>  .../imu/inv_icm42600/inv_icm42600_buffer.c    |  28 +-

>  .../iio/imu/inv_icm42600/inv_icm42600_core.c  |   6 +

>  .../iio/imu/inv_icm42600/inv_icm42600_gyro.c  |  32 ++-

>  .../imu/inv_icm42600/inv_icm42600_timestamp.c | 246 ++++++++++++++++++

>  .../imu/inv_icm42600/inv_icm42600_timestamp.h |  82 ++++++

>  8 files changed, 421 insertions(+), 16 deletions(-)

>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c

>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h

> 

> diff --git a/drivers/iio/imu/inv_icm42600/Makefile b/drivers/iio/imu/inv_icm42600/Makefile

> index d6732118010c..1197b545a682 100644

> --- a/drivers/iio/imu/inv_icm42600/Makefile

> +++ b/drivers/iio/imu/inv_icm42600/Makefile

> @@ -7,6 +7,7 @@ inv-icm42600-y += inv_icm42600_accel.o

>  inv-icm42600-y += inv_icm42600_temp.o

>  inv-icm42600-y += inv_icm42600_trigger.o

>  inv-icm42600-y += inv_icm42600_buffer.o

> +inv-icm42600-y += inv_icm42600_timestamp.o


>  obj-$(CONFIG_INV_ICM42600_I2C) += inv-icm42600-i2c.o

>  inv-icm42600-i2c-y += inv_icm42600_i2c.o

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> index 947ca4dd245b..e15eddafe009 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> @@ -16,6 +16,7 @@

>  #include <linux/iio/trigger.h>


>  #include "inv_icm42600_buffer.h"

> +#include "inv_icm42600_timestamp.h"


>  enum inv_icm42600_chip {

>        INV_CHIP_ICM42600,

> @@ -127,6 +128,7 @@ struct inv_icm42600_suspended {

>   *  @indio_accel:    accelerometer IIO device.

>   *  @trigger:                device internal interrupt trigger

>   *  @fifo:           FIFO management structure.

> + *  @timestamp:              timestamp management structure.

>   */

>  struct inv_icm42600_state {

>        struct mutex lock;

> @@ -142,6 +144,10 @@ struct inv_icm42600_state {

>        struct iio_dev *indio_accel;

>        struct iio_trigger *trigger;

>        struct inv_icm42600_fifo fifo;

> +     struct {

> +             struct inv_icm42600_timestamp gyro;

> +             struct inv_icm42600_timestamp accel;

> +     } timestamp;

>  };


>  /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */

> @@ -382,11 +388,11 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,


>  int inv_icm42600_gyro_init(struct inv_icm42600_state *st);


> -int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev, int64_t ts);

> +int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev);


>  int inv_icm42600_accel_init(struct inv_icm42600_state *st);


> -int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev, int64_t ts);

> +int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev);


>  int inv_icm42600_trigger_init(struct inv_icm42600_state *st, int irq,

>                              int irq_type);

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c

> index 4206be54d057..ac140c824c03 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c

> @@ -17,6 +17,7 @@

>  #include "inv_icm42600.h"

>  #include "inv_icm42600_temp.h"

>  #include "inv_icm42600_buffer.h"

> +#include "inv_icm42600_timestamp.h"


>  #define INV_ICM42600_ACCEL_CHAN(_modifier, _index, _ext_info)                \

>        {                                                               \

> @@ -66,7 +67,7 @@ static const struct iio_chan_spec inv_icm42600_accel_channels[] = {

>        INV_ICM42600_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42600_ACCEL_SCAN_Z,

>                                inv_icm42600_accel_ext_infos),

>        INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),

> -     IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),

> +     INV_ICM42600_TIMESTAMP_CHAN(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),

>  };


>  /* IIO buffer data */

> @@ -94,14 +95,20 @@ static irqreturn_t inv_icm42600_accel_handler(int irq, void *_data)

>        struct iio_poll_func *pf = _data;

>        struct iio_dev *indio_dev = pf->indio_dev;

>        struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     struct inv_icm42600_timestamp *ts = &st->timestamp.accel;

>        const size_t fifo_nb = st->fifo.nb.total;

> +     const size_t accel_nb = st->fifo.nb.accel;

> +     const uint32_t fifo_period = st->fifo.period;

>        int ret;


>        /* exit if no sample */

>        if (fifo_nb == 0)

>                goto out;


> -     ret = inv_icm42600_accel_parse_fifo(indio_dev, pf->timestamp);

> +     inv_icm42600_timestamp_interrupt(ts, fifo_period, fifo_nb, accel_nb,

> +                                      pf->timestamp);

> +

> +     ret = inv_icm42600_accel_parse_fifo(indio_dev);

>        if (ret)

>                dev_err(regmap_get_device(st->map), "accel fifo error %d\n",

>                        ret);

> @@ -143,6 +150,7 @@ static int inv_icm42600_accel_update_scan_mode(struct iio_dev *indio_dev,

>        }


>        /* update data FIFO write and FIFO watermark */

> +     inv_icm42600_timestamp_apply_odr(&st->timestamp.accel, 0, 0, 0);

>        ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);

>        if (ret)

>                goto out_unlock;

> @@ -347,6 +355,7 @@ static int inv_icm42600_accel_write_odr(struct inv_icm42600_state *st,

>        mutex_lock(&st->lock);

>        conf.odr = inv_icm42600_accel_odr_conv[idx / 2];

>        ret = inv_icm42600_set_accel_conf(st, &conf, NULL);

> +     inv_icm42600_timestamp_update_odr(&st->timestamp.accel, conf.odr);

>        inv_icm42600_buffer_update_fifo_period(st);

>        inv_icm42600_buffer_update_watermark(st);

>        mutex_unlock(&st->lock);

> @@ -502,6 +511,9 @@ static int inv_icm42600_accel_read_raw(struct iio_dev *indio_dev,

>        case IIO_TEMP:

>                return inv_icm42600_temp_read_raw(indio_dev, chan, val, val2,

>                                                  mask);

> +     case IIO_TIMESTAMP:

> +             return inv_icm42600_timestamp_read_raw(indio_dev, chan, val,

> +                                                    val2, mask);

>        default:

>                return -EINVAL;

>        }

> @@ -694,13 +706,18 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st)

>        return devm_iio_device_register(dev, st->indio_accel);

>  }


> -int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev, int64_t ts)

> +int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev)

>  {

>        struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     struct inv_icm42600_timestamp *ts = &st->timestamp.accel;

> +     const size_t fifo_nb = st->fifo.nb.total;

>        const size_t accel_nb = st->fifo.nb.accel;

> +     const uint32_t fifo_period = st->fifo.period;

>        ssize_t i, size;

> +     unsigned int no;

>        const void *accel, *gyro, *temp, *timestamp;

>        unsigned int odr;

> +     int64_t ts_val;

>        struct inv_icm42600_accel_buffer buffer;


>        /* exit if no accel sample */

> @@ -708,7 +725,7 @@ int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev, int64_t ts)

>                return 0;


>        /* parse all fifo packets */

> -     for (i = 0; i < st->fifo.count; i += size) {

> +     for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {

>                size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],

>                                &accel, &gyro, &temp, &timestamp, &odr);

>                dev_dbg(regmap_get_device(st->map), "accel packet size = %zd\n",

> @@ -721,9 +738,14 @@ int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev, int64_t ts)

>                        dev_dbg(regmap_get_device(st->map), "skip accel data\n");

>                        continue;

>                }

> +             /* update odr */

> +             if (odr & INV_ICM42600_SENSOR_ACCEL)

> +                     inv_icm42600_timestamp_apply_odr(ts, fifo_period,

> +                                                      fifo_nb, no);

>                memcpy(&buffer.accel, accel, sizeof(buffer.accel));

>                memcpy(&buffer.temp, temp, sizeof(buffer.temp));

> -             iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts);

> +             ts_val = inv_icm42600_timestamp_get(ts);

> +             iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val);

>        }


>        return 0;

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c

> index b428abdc92ee..bea4c9810da7 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c

> @@ -15,6 +15,7 @@

>  #include <linux/iio/trigger_consumer.h>


>  #include "inv_icm42600.h"

> +#include "inv_icm42600_timestamp.h"

>  #include "inv_icm42600_buffer.h"


>  void inv_icm42600_buffer_update_fifo_period(struct inv_icm42600_state *st)

> @@ -194,14 +195,17 @@ static int inv_icm42600_buffer_postdisable(struct iio_dev *indio_dev)

>        unsigned int *watermark;

>        struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;

>        unsigned int sleep = 0;

> +     struct inv_icm42600_timestamp *ts;

>        int ret;


>        if (indio_dev == st->indio_gyro) {

>                sensor = INV_ICM42600_SENSOR_GYRO;

>                watermark = &st->fifo.watermark.gyro;

> +             ts = &st->timestamp.gyro;

>        } else if (indio_dev == st->indio_accel) {

>                sensor = INV_ICM42600_SENSOR_ACCEL;

>                watermark = &st->fifo.watermark.accel;

> +             ts = &st->timestamp.accel;

>        } else {

>                return -EINVAL;

>        }

> @@ -223,6 +227,8 @@ static int inv_icm42600_buffer_postdisable(struct iio_dev *indio_dev)

>        else

>                ret = inv_icm42600_set_accel_conf(st, &conf, &sleep);


> +     inv_icm42600_timestamp_reset(ts);

> +

>  out_unlock:

>        mutex_unlock(&st->lock);

>        if (sleep)

> @@ -316,11 +322,25 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,

>        if (st->fifo.nb.total == 0)

>                return 0;


> -     ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro, ts_gyro);

> -     if (ret)

> -             return ret;

> +     if (st->fifo.nb.gyro > 0) {

> +             inv_icm42600_timestamp_interrupt(&st->timestamp.gyro,

> +                                          st->fifo.period, st->fifo.nb.total,

> +                                          st->fifo.nb.gyro, ts_gyro);

> +             ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);

> +             if (ret)

> +                     return ret;

> +     }


> -     return inv_icm42600_accel_parse_fifo(st->indio_accel, ts_accel);

> +     if (st->fifo.nb.accel > 0) {

> +             inv_icm42600_timestamp_interrupt(&st->timestamp.accel,

> +                                          st->fifo.period, st->fifo.nb.total,

> +                                          st->fifo.nb.accel, ts_accel);

> +             ret = inv_icm42600_accel_parse_fifo(st->indio_accel);

> +             if (ret)

> +                     return ret;

> +     }

> +

> +     return ret;

>  }


>  int inv_icm42600_buffer_init(struct inv_icm42600_state *st)

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> index 689089065ff9..563c4348de01 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> @@ -15,6 +15,7 @@


>  #include "inv_icm42600.h"

>  #include "inv_icm42600_buffer.h"

> +#include "inv_icm42600_timestamp.h"


>  static const struct regmap_range_cfg inv_icm42600_regmap_ranges[] = {

>        {

> @@ -516,6 +517,11 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,

>        if (ret)

>                return ret;


> +     /* initialize timestamping */

> +     ret = inv_icm42600_timestamp_init(st);

> +     if (ret)

> +             return ret;

> +

>        /* setup FIFO buffer */

>        ret = inv_icm42600_buffer_init(st);

>        if (ret)

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> index dafb104abc77..1245588170bd 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> @@ -17,6 +17,7 @@

>  #include "inv_icm42600.h"

>  #include "inv_icm42600_temp.h"

>  #include "inv_icm42600_buffer.h"

> +#include "inv_icm42600_timestamp.h"


>  #define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info)         \

>        {                                                               \

> @@ -66,7 +67,7 @@ static const struct iio_chan_spec inv_icm42600_gyro_channels[] = {

>        INV_ICM42600_GYRO_CHAN(IIO_MOD_Z, INV_ICM42600_GYRO_SCAN_Z,

>                               inv_icm42600_gyro_ext_infos),

>        INV_ICM42600_TEMP_CHAN(INV_ICM42600_GYRO_SCAN_TEMP),

> -     IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_GYRO_SCAN_TIMESTAMP),



Interrupt timestamp was pretty much garbage, so I'd just not register that in the

first place.  Introduce the timestamp for the first time in this patch.



> +     INV_ICM42600_TIMESTAMP_CHAN(INV_ICM42600_GYRO_SCAN_TIMESTAMP),

>  };


>  /* IIO buffer data */

> @@ -94,14 +95,20 @@ static irqreturn_t inv_icm42600_gyro_handler(int irq, void *_data)

>        struct iio_poll_func *pf = _data;

>        struct iio_dev *indio_dev = pf->indio_dev;

>        struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     struct inv_icm42600_timestamp *ts = &st->timestamp.gyro;

>        const size_t fifo_nb = st->fifo.nb.total;

> +     const size_t gyro_nb = st->fifo.nb.gyro;

> +     const uint32_t fifo_period = st->fifo.period;

>        int ret;


>        /* exit if no sample */

>        if (fifo_nb == 0)

>                goto out;


> -     ret = inv_icm42600_gyro_parse_fifo(indio_dev, pf->timestamp);

> +     inv_icm42600_timestamp_interrupt(ts, fifo_period, fifo_nb, gyro_nb,

> +                                      pf->timestamp);

> +

> +     ret = inv_icm42600_gyro_parse_fifo(indio_dev);

>        if (ret)

>                dev_err(regmap_get_device(st->map), "gyro fifo error %d\n",

>                        ret);

> @@ -143,6 +150,7 @@ static int inv_icm42600_gyro_update_scan_mode(struct iio_dev *indio_dev,

>        }


>        /* update data FIFO write and FIFO watermark */

> +     inv_icm42600_timestamp_apply_odr(&st->timestamp.gyro, 0, 0, 0);

>        ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);

>        if (ret)

>                goto out_unlock;

> @@ -359,6 +367,7 @@ static int inv_icm42600_gyro_write_odr(struct inv_icm42600_state *st,

>        mutex_lock(&st->lock);

>        conf.odr = inv_icm42600_gyro_odr_conv[idx / 2];

>        ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);

> +     inv_icm42600_timestamp_update_odr(&st->timestamp.gyro, conf.odr);

>        inv_icm42600_buffer_update_fifo_period(st);

>        inv_icm42600_buffer_update_watermark(st);

>        mutex_unlock(&st->lock);

> @@ -514,6 +523,9 @@ static int inv_icm42600_gyro_read_raw(struct iio_dev *indio_dev,

>        case IIO_TEMP:

>                return inv_icm42600_temp_read_raw(indio_dev, chan, val, val2,

>                                                  mask);

> +     case IIO_TIMESTAMP:

> +             return inv_icm42600_timestamp_read_raw(indio_dev, chan, val,

> +                                                    val2, mask);

>        default:

>                return -EINVAL;

>        }

> @@ -706,13 +718,18 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st)

>        return devm_iio_device_register(dev, st->indio_gyro);

>  }


> -int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev, int64_t ts)

> +int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev)

>  {

>        struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     struct inv_icm42600_timestamp *ts = &st->timestamp.gyro;

> +     const size_t fifo_nb = st->fifo.nb.total;

>        const size_t gyro_nb = st->fifo.nb.gyro;

> +     const uint32_t fifo_period = st->fifo.period;

>        ssize_t i, size;

> +     unsigned int no;

>        const void *accel, *gyro, *temp, *timestamp;

>        unsigned int odr;

> +     int64_t ts_val;

>        struct inv_icm42600_gyro_buffer buffer;


>        /* exit if no gyro sample */

> @@ -720,7 +737,7 @@ int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev, int64_t ts)

>                return 0;


>        /* parse all fifo packets */

> -     for (i = 0; i < st->fifo.count; i += size) {

> +     for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {

>                size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],

>                                &accel, &gyro, &temp, &timestamp, &odr);

>                dev_dbg(regmap_get_device(st->map), "gyro packet size = %zd\n",

> @@ -733,9 +750,14 @@ int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev, int64_t ts)

>                        dev_dbg(regmap_get_device(st->map), "skip gyro data\n");

>                        continue;

>                }

> +             /* update odr */

> +             if (odr & INV_ICM42600_SENSOR_GYRO)

> +                     inv_icm42600_timestamp_apply_odr(ts, fifo_period,

> +                                                      fifo_nb, no);

>                memcpy(&buffer.gyro, gyro, sizeof(buffer.gyro));

>                memcpy(&buffer.temp, temp, sizeof(buffer.temp));

> -             iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts);

> +             ts_val = inv_icm42600_timestamp_get(ts);

> +             iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val);

>        }


>        return 0;

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c

> new file mode 100644

> index 000000000000..79cf777e2e84

> --- /dev/null

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c

> @@ -0,0 +1,246 @@

> +// SPDX-License-Identifier: GPL-2.0-or-later

> +/*

> + * Copyright (C) 2020 Invensense, Inc.

> + */

> +

> +#include <linux/device.h>

> +#include <linux/mutex.h>

> +#include <linux/pm_runtime.h>

> +#include <linux/regmap.h>

> +#include <linux/math64.h>

> +#include <linux/iio/iio.h>

> +

> +#include "inv_icm42600.h"

> +#include "inv_icm42600_timestamp.h"

> +

> +/* internal chip period is 32kHz, 31250ns */

> +#define INV_ICM42600_TIMESTAMP_PERIOD                31250

> +/* allow a jitter of +/- 2% */

> +#define INV_ICM42600_TIMESTAMP_JITTER                2

> +/* compute min and max periods accepted */

> +#define INV_ICM42600_TIMESTAMP_MIN_PERIOD(_p)                \

> +     (((_p) * (100 - INV_ICM42600_TIMESTAMP_JITTER)) / 100)

> +#define INV_ICM42600_TIMESTAMP_MAX_PERIOD(_p)                \

> +     (((_p) * (100 + INV_ICM42600_TIMESTAMP_JITTER)) / 100)

> +

> +static void inv_update_acc(struct inv_icm42600_timestamp_acc *acc, uint32_t val)

> +{

> +     uint64_t sum = 0;

> +     size_t i;

> +

> +     acc->values[acc->idx++] = val;

> +     if (acc->idx >= ARRAY_SIZE(acc->values))

> +             acc->idx = 0;

> +

> +     for (i = 0; i < ARRAY_SIZE(acc->values); ++i) {

> +             if (acc->values[i] == 0)

> +                     break;

> +             sum += acc->values[i];

> +     }

> +

> +     acc->val = div_u64(sum, i);

> +}

> +

> +static int inv_icm42600_timestamp_read(struct inv_icm42600_state *st,

> +                                    uint32_t *ts)

> +{

> +     struct device *dev = regmap_get_device(st->map);

> +     __le32 raw;

> +     int ret;

> +

> +     pm_runtime_get_sync(dev);

> +     mutex_lock(&st->lock);

> +

> +     ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,

> +                        INV_ICM42600_SIGNAL_PATH_RESET_TMST_STROBE);

> +     if (ret)

> +             goto exit;

> +

> +     /* Read timestamp value 3 registers little-endian */

> +     raw = 0;

> +     ret = regmap_bulk_read(st->map, INV_ICM42600_REG_TMSTVAL, &raw, 3);

> +     if (ret)

> +             goto exit;

> +

> +     *ts = (uint32_t)le32_to_cpu(raw);

> +exit:

> +     mutex_unlock(&st->lock);

> +     pm_runtime_mark_last_busy(dev);

> +     pm_runtime_put_autosuspend(dev);

> +     return ret;

> +}

> +

> +int inv_icm42600_timestamp_read_raw(struct iio_dev *indio_dev,

> +                                 struct iio_chan_spec const *chan,

> +                                 int *val, int *val2, long mask)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     uint32_t ts;

> +     int ret;

> +

> +     if (chan->type != IIO_TIMESTAMP)

> +             return -EINVAL;

> +

> +     switch (mask) {

> +     case IIO_CHAN_INFO_PROCESSED:

> +             ret = iio_device_claim_direct_mode(indio_dev);

> +             if (ret)

> +                     return ret;

> +             ret = inv_icm42600_timestamp_read(st, &ts);

> +             iio_device_release_direct_mode(indio_dev);



Unusual to expose it as a readable channel.  Why would you want to do this?



> +             if (ret)

> +                     return ret;

> +             *val = ts * 1000;

> +             return IIO_VAL_INT;

> +     default:

> +             return -EINVAL;

> +     }

> +}

> +

> +static void inv_icm42600_init_ts(struct inv_icm42600_timestamp *ts,

> +                              struct device *dev, uint32_t period)

> +{

> +     /* initial odr for sensor is 1kHz */

> +     const uint32_t default_period = 1000000;

> +

> +     ts->dev = dev;

> +     ts->mult = default_period / INV_ICM42600_TIMESTAMP_PERIOD;

> +     ts->new_mult = period / INV_ICM42600_TIMESTAMP_PERIOD;

> +     ts->period = default_period;

> +

> +     /* use theoretical value for chip period */

> +     inv_update_acc(&ts->chip_period, INV_ICM42600_TIMESTAMP_PERIOD);

> +}

> +

> +int inv_icm42600_timestamp_init(struct inv_icm42600_state *st)

> +{

> +     unsigned int val;

> +

> +     inv_icm42600_init_ts(&st->timestamp.gyro, regmap_get_device(st->map),

> +                          inv_icm42600_odr_to_period(st->conf.gyro.odr));

> +     inv_icm42600_init_ts(&st->timestamp.accel, regmap_get_device(st->map),

> +                          inv_icm42600_odr_to_period(st->conf.accel.odr));

> +

> +     /* enable timestamp register */

> +     val = INV_ICM42600_TMST_CONFIG_TMST_TO_REGS_EN |

> +           INV_ICM42600_TMST_CONFIG_TMST_EN;

> +     return regmap_update_bits(st->map, INV_ICM42600_REG_TMST_CONFIG,

> +                               INV_ICM42600_TMST_CONFIG_MASK, val);

> +}

> +

> +void inv_icm42600_timestamp_update_odr(struct inv_icm42600_timestamp *ts,

> +                                    int odr)

> +{

> +     uint32_t period;

> +

> +     period = inv_icm42600_odr_to_period(odr);

> +     ts->new_mult = period / INV_ICM42600_TIMESTAMP_PERIOD;

> +     dev_dbg(ts->dev, "ts new mult: %u\n", ts->new_mult);

> +}

> +

> +static bool inv_validate_period(uint32_t period, uint32_t mult)

> +{

> +     const uint32_t chip_period = INV_ICM42600_TIMESTAMP_PERIOD;

> +     uint32_t period_min, period_max;

> +

> +     /* check that time interval between interrupts is acceptable */

> +     period_min = INV_ICM42600_TIMESTAMP_MIN_PERIOD(chip_period) * mult;

> +     period_max = INV_ICM42600_TIMESTAMP_MAX_PERIOD(chip_period) * mult;

> +     if (period > period_min && period < period_max)

> +             return true;

> +     else

> +             return false;

> +}

> +

> +static bool inv_compute_chip_period(struct inv_icm42600_timestamp *ts,

> +                                 uint32_t mult, uint32_t period)

> +{

> +     uint32_t new_chip_period;

> +

> +     if (!inv_validate_period(period, mult))

> +             return false;

> +

> +     /* update chip internal period estimation */

> +     new_chip_period = period / mult;

> +     inv_update_acc(&ts->chip_period, new_chip_period);

> +

> +     dev_dbg(ts->dev, "ts chip period: %u - val %u\n", new_chip_period,

> +             ts->chip_period.val);

> +

> +     return true;

> +}

> +

> +void inv_icm42600_timestamp_interrupt(struct inv_icm42600_timestamp *ts,

> +                                   uint32_t fifo_period, size_t fifo_nb,

> +                                   size_t sensor_nb, int64_t timestamp)

> +{

> +     struct inv_icm42600_timestamp_interval *it;

> +     int64_t delta, interval;

> +     const uint32_t fifo_mult = fifo_period / INV_ICM42600_TIMESTAMP_PERIOD;

> +     uint32_t period = ts->period;

> +     int32_t m;

> +     bool valid = false;

> +

> +     if (fifo_nb == 0)

> +             return;

> +

> +     /* update interrupt timestamp and compute chip and sensor periods */

> +     it = &ts->it;

> +     it->lo = it->up;

> +     it->up = timestamp;

> +     delta = it->up - it->lo;

> +     dev_dbg(ts->dev, "ts it: %lld - %lld - %lld\n", it->lo, it->up, delta);

> +     if (it->lo != 0) {

> +             period = div_s64(delta, fifo_nb);

> +             valid = inv_compute_chip_period(ts, fifo_mult, period);

> +             if (valid)

> +                     ts->period = ts->mult * ts->chip_period.val;

> +     }

> +

> +     /* no previous data, compute theoritical value from interrupt */

> +     if (ts->timestamp == 0) {

> +             interval = (int64_t)ts->period * (int64_t)sensor_nb;

> +             ts->timestamp = it->up - interval;

> +             dev_dbg(ts->dev, "ts start: %lld\n", ts->timestamp);

> +             return;

> +     }

> +

> +     /* if interrupt interval is valid, sync with interrupt timestamp */

> +     if (valid) {

> +             /* compute real fifo_period */

> +             fifo_period = fifo_mult * ts->chip_period.val;

> +             delta = it->lo - ts->timestamp;

> +             while (delta >= (fifo_period * 3 / 2))

> +                     delta -= fifo_period;

> +             /* compute maximal adjustment value */

> +             m = INV_ICM42600_TIMESTAMP_MAX_PERIOD(ts->period) - ts->period;

> +             if (delta > m)

> +                     delta = m;

> +             else if (delta < -m)

> +                     delta = -m;

> +             dev_dbg(ts->dev, "ts adj: %lld\n", delta);

> +             ts->timestamp += delta;

> +     }

> +}

> +

> +void inv_icm42600_timestamp_apply_odr(struct inv_icm42600_timestamp *ts,

> +                                   uint32_t fifo_period, size_t fifo_nb,

> +                                   unsigned int fifo_no)

> +{

> +     int64_t interval;

> +     uint32_t fifo_mult;

> +

> +     ts->mult = ts->new_mult;

> +     ts->period = ts->mult * ts->chip_period.val;

> +     dev_dbg(ts->dev, "ts mult: %u\n", ts->mult);

> +

> +     if (ts->timestamp != 0) {

> +             /* compute real fifo period */

> +             fifo_mult = fifo_period / INV_ICM42600_TIMESTAMP_PERIOD;

> +             fifo_period = fifo_mult * ts->chip_period.val;

> +             /* compute timestamp from current interrupt after ODR change */

> +             interval = (int64_t)(fifo_nb - fifo_no) * (int64_t)fifo_period;

> +             ts->timestamp = ts->it.up - interval;

> +             dev_dbg(ts->dev, "ts new: %lld\n", ts->timestamp);

> +     }

> +}

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h b/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h

> new file mode 100644

> index 000000000000..c865e1a9a7c8

> --- /dev/null

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h

> @@ -0,0 +1,82 @@

> +/* SPDX-License-Identifier: GPL-2.0-or-later */

> +/*

> + * Copyright (C) 2020 Invensense, Inc.

> + */

> +

> +#ifndef INV_ICM42600_TIMESTAMP_H_

> +#define INV_ICM42600_TIMESTAMP_H_

> +

> +#include <linux/device.h>

> +#include <linux/iio/iio.h>

> +

> +struct inv_icm42600_state;

> +

> +struct inv_icm42600_timestamp_interval {

> +     int64_t lo;

> +     int64_t up;

> +};

> +

> +struct inv_icm42600_timestamp_acc {

> +     uint32_t val;

> +     size_t idx;

> +     uint32_t values[32];

> +};

> +

> +struct inv_icm42600_timestamp {

> +     struct device *dev;

> +     struct inv_icm42600_timestamp_interval it;

> +     int64_t timestamp;

> +     uint32_t mult;

> +     uint32_t new_mult;

> +     uint32_t period;

> +     struct inv_icm42600_timestamp_acc chip_period;

> +};

> +

> +#define INV_ICM42600_TIMESTAMP_CHAN(_index)                          \

> +     {                                                               \

> +             .type = IIO_TIMESTAMP,                                  \

> +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),     \

> +             .scan_index = _index,                                   \

> +             .scan_type = {                                          \

> +                     .sign = 's',                                    \

> +                     .realbits = 64,                                 \

> +                     .storagebits = 64,                              \

> +             },                                                      \

> +     }

> +

> +int inv_icm42600_timestamp_read_raw(struct iio_dev *indio_dev,

> +                                 struct iio_chan_spec const *chan,

> +                                 int *val, int *val2, long mask);

> +

> +int inv_icm42600_timestamp_init(struct inv_icm42600_state *st);

> +

> +void inv_icm42600_timestamp_update_odr(struct inv_icm42600_timestamp *ts,

> +                                    int odr);

> +

> +void inv_icm42600_timestamp_interrupt(struct inv_icm42600_timestamp *ts,

> +                                   uint32_t fifo_period, size_t fifo_nb,

> +                                   size_t sensor_nb, int64_t timestamp);

> +

> +static inline int64_t

> +inv_icm42600_timestamp_get(struct inv_icm42600_timestamp *ts)



Perhaps timestamp_pop?  Kind of indicates that you are destructively

retrieving a timetamp.



> +{

> +     ts->timestamp += ts->period;

> +     dev_dbg(ts->dev, "ts: %lld\n", ts->timestamp);

> +

> +     return ts->timestamp;

> +}

> +

> +void inv_icm42600_timestamp_apply_odr(struct inv_icm42600_timestamp *ts,

> +                                   uint32_t fifo_period, size_t fifo_nb,

> +                                   unsigned int fifo_no);

> +

> +static inline void

> +inv_icm42600_timestamp_reset(struct inv_icm42600_timestamp *ts)

> +{

> +     const struct inv_icm42600_timestamp_interval interval_init = {0LL, 0LL};

> +

> +     ts->it = interval_init;

> +     ts->timestamp = 0;

> +}

> +

> +#endif






^ permalink raw reply

* Re: [PATCH 17/17] ARM: dts: r8a7742: Add RWDT node
From: Geert Uytterhoeven @ 2020-05-18 15:45 UTC (permalink / raw)
  To: Lad, Prabhakar
  Cc: Sergei Shtylyov, Lad Prabhakar, Jens Axboe, Rob Herring,
	Wolfram Sang, Ulf Hansson, David S. Miller, Wim Van Sebroeck,
	Guenter Roeck, linux-ide,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux Kernel Mailing List, Linux I2C, Linux MMC List, netdev,
	Linux-Renesas, Linux Watchdog Mailing List
In-Reply-To: <CA+V-a8uuP9d6dNeRpn3O0_aOc15CqWoh0bbAfYze1_hn0dCh8g@mail.gmail.com>

On Mon, May 18, 2020 at 3:23 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Mon, May 18, 2020 at 2:17 PM Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
> > On 18.05.2020 15:27, Lad, Prabhakar wrote:
> > >>> Add a device node for the Watchdog Timer (RWDT) controller on the Renesas
> > >>> RZ/G1H (r8a7742) SoC.
> > >>>
> > >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >>> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
> > >>
> > >> Thanks for your patch!
> > >>
> > >>> --- a/arch/arm/boot/dts/r8a7742.dtsi
> > >>> +++ b/arch/arm/boot/dts/r8a7742.dtsi
> > >>> @@ -201,6 +201,16 @@
> > >>>                  #size-cells = <2>;
> > >>>                  ranges;
> > >>>
> > >>> +               rwdt: watchdog@e6020000 {
> > >>> +                       compatible = "renesas,r8a7742-wdt",
> > >>> +                                    "renesas,rcar-gen2-wdt";
> > >>> +                       reg = <0 0xe6020000 0 0x0c>;
> > >>> +                       clocks = <&cpg CPG_MOD 402>;
> > >>> +                       power-domains = <&sysc R8A7742_PD_ALWAYS_ON>;
> > >>> +                       resets = <&cpg 402>;
> > >>> +                       status = "disabled";
> > >>
> > >> Missing "interrupts" property.
> > >>
> > > "interrupts" property isn't used by rwdt driver  and can be dropped
> > > from bindings file.
> >
> >     DT describes the hardware, not its driver's abilities.

Thanks for chiming in, Sergei!

> Agreed will add, I had followed it on similar lines of r8a7743/44.

Yeah. I know it's missing for a few other SoCs, too.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
From: Geert Uytterhoeven @ 2020-05-18 15:44 UTC (permalink / raw)
  To: Qii Wang
  Cc: Wolfram Sang,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	srv_heupstream, leilk.liu, Linux Kernel Mailing List,
	linux-mediatek, Linux I2C, Linux ARM
In-Reply-To: <1589461844-15614-3-git-send-email-qii.wang@mediatek.com>

On Thu, May 14, 2020 at 3:13 PM Qii Wang <qii.wang@mediatek.com> wrote:
> This patch adds a algorithm to calculate some ac-timing parameters
> which can fully meet I2C Spec.
>
> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 277 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index 0ca6c38a..7020618 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c

> +/*
> + * Check and Calculate i2c ac-timing
> + *
> + * Hardware design:
> + * sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src
> + * xxx_cnt_div =  spec->min_xxx_ns / sample_ns
> + *
> + * Sample_ns is rounded down for xxx_cnt_div would be greater
> + * than the smallest spec.
> + * The sda_timing is chosen as the middle value between
> + * the largest and smallest.
> + */
> +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> +                                  unsigned int clk_src,
> +                                  unsigned int check_speed,
> +                                  unsigned int step_cnt,
> +                                  unsigned int sample_cnt)
> +{
> +       const struct i2c_spec_values *spec;
> +       unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> +       unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> +       long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;

So sample_ns is a 64-bit value. Is that really needed?

> +       if (!i2c->dev_comp->timing_adjust)
> +               return 0;
> +
> +       if (i2c->dev_comp->ltiming_adjust)
> +               max_sta_cnt = 0x100;
> +
> +       spec = mtk_i2c_get_spec(check_speed);
> +
> +       if (i2c->dev_comp->ltiming_adjust)
> +               clk_ns = 1000000000 / clk_src;
> +       else
> +               clk_ns = sample_ns / 2;
> +
> +       su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> +       if (su_sta_cnt > max_sta_cnt)
> +               return -1;
> +
> +       low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);

So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)

noreply@ellerman.id.au reports:

    ERROR: modpost: "__udivdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
    ERROR: modpost: "__divdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!

for 32-bit builds.

> +       max_step_cnt = mtk_i2c_max_step_cnt(check_speed);
> +       if ((2 * step_cnt) > low_cnt && low_cnt < max_step_cnt) {
> +               if (low_cnt > step_cnt) {
> +                       high_cnt = 2 * step_cnt - low_cnt;
> +               } else {
> +                       high_cnt = step_cnt;
> +                       low_cnt = step_cnt;
> +               }
> +       } else {
> +               return -2;
> +       }
> +
> +       sda_max = spec->max_hd_dat_ns / sample_ns;
> +       if (sda_max > low_cnt)
> +               sda_max = 0;
> +
> +       sda_min = DIV_ROUND_UP(spec->min_su_dat_ns, sample_ns);

One more.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH 08/12] iio: imu: inv_icm42600: add device interrupt trigger
From: Jean-Baptiste Maneyrol @ 2020-05-18 15:41 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: jic23@kernel.org, robh+dt@kernel.org, robh@kernel.org,
	mchehab+huawei@kernel.org, davem@davemloft.net,
	gregkh@linuxfoundation.org, linux-iio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20200508152229.00005de2@Huawei.com>

Hi Jonathan,

you're right, making a trigger is not very useful.

I will move this code to the buffer file or rename the file, and replace the triggered buffer setup in iio devices.

Thanks,
JB


From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>

Sent: Friday, May 8, 2020 16:22

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: jic23@kernel.org <jic23@kernel.org>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>;
 linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>

Subject: Re: [PATCH 08/12] iio: imu: inv_icm42600: add device interrupt trigger

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



On Thu, 7 May 2020 16:42:18 +0200

Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:



> Add INT1 interrupt support and use it as an iio trigger.

> Support interrupt edge and level, active high or low.

> Push-pull configuration only.

> 

> Trigger enables FIFO and will be useful with buffer support.

> 

> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>



So it's an odd trigger... But you check its only used for the

device itself which stops that being visible to anyone.



The open question in my mind is why we need a trigger at all?

If there is no advantage we don't need to expose that.  Can just

directly call the buffer functions from the interrupt handler.



It's perfectly acceptable to not have a trigger exposed if:

* It would only be useful for the device providing it.

* You don't need to use it to select between various options.



For some of the other fifo devices IIRC we do support other

triggers but if you don't provide one the local fifo is used.



Here I don't think we actually support other triggers though we

don't prevent them being used? So even simpler.

With the complex interleaved fifo I suspect it would be hard

to support other triggers anyway except in a trivial poll like

mode so probably not of interest to anyone...



J 



> ---

>  drivers/iio/imu/inv_icm42600/Kconfig          |   1 +

>  drivers/iio/imu/inv_icm42600/Makefile         |   1 +

>  drivers/iio/imu/inv_icm42600/inv_icm42600.h   |   8 +-

>  .../iio/imu/inv_icm42600/inv_icm42600_core.c  |  19 +-

>  .../iio/imu/inv_icm42600/inv_icm42600_i2c.c   |   2 +-

>  .../iio/imu/inv_icm42600/inv_icm42600_spi.c   |   2 +-

>  .../imu/inv_icm42600/inv_icm42600_trigger.c   | 177 ++++++++++++++++++

>  7 files changed, 206 insertions(+), 4 deletions(-)

>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_trigger.c

> 

> diff --git a/drivers/iio/imu/inv_icm42600/Kconfig b/drivers/iio/imu/inv_icm42600/Kconfig

> index 22390a72f0a3..7b3eaeb2aa4a 100644

> --- a/drivers/iio/imu/inv_icm42600/Kconfig

> +++ b/drivers/iio/imu/inv_icm42600/Kconfig

> @@ -2,6 +2,7 @@


>  config INV_ICM42600

>        tristate

> +     select IIO_TRIGGER


>  config INV_ICM42600_I2C

>        tristate "InvenSense ICM-426xx I2C driver"

> diff --git a/drivers/iio/imu/inv_icm42600/Makefile b/drivers/iio/imu/inv_icm42600/Makefile

> index 48965824f00c..e1f2aacbe888 100644

> --- a/drivers/iio/imu/inv_icm42600/Makefile

> +++ b/drivers/iio/imu/inv_icm42600/Makefile

> @@ -5,6 +5,7 @@ inv-icm42600-y += inv_icm42600_core.o

>  inv-icm42600-y += inv_icm42600_gyro.o

>  inv-icm42600-y += inv_icm42600_accel.o

>  inv-icm42600-y += inv_icm42600_temp.o

> +inv-icm42600-y += inv_icm42600_trigger.o


>  obj-$(CONFIG_INV_ICM42600_I2C) += inv-icm42600-i2c.o

>  inv-icm42600-i2c-y += inv_icm42600_i2c.o

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> index bc963b3d1800..175c1f67faee 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> @@ -13,6 +13,7 @@

>  #include <linux/regulator/consumer.h>

>  #include <linux/pm.h>

>  #include <linux/iio/iio.h>

> +#include <linux/iio/trigger.h>


>  enum inv_icm42600_chip {

>        INV_CHIP_ICM42600,

> @@ -122,6 +123,7 @@ struct inv_icm42600_suspended {

>   *  @suspended:              suspended sensors configuration.

>   *  @indio_gyro:     gyroscope IIO device.

>   *  @indio_accel:    accelerometer IIO device.

> + *  @trigger:                device internal interrupt trigger

>   */

>  struct inv_icm42600_state {

>        struct mutex lock;

> @@ -135,6 +137,7 @@ struct inv_icm42600_state {

>        struct inv_icm42600_suspended suspended;

>        struct iio_dev *indio_gyro;

>        struct iio_dev *indio_accel;

> +     struct iio_trigger *trigger;

>  };


>  /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */

> @@ -370,11 +373,14 @@ int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,

>  int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,

>                             unsigned int writeval, unsigned int *readval);


> -int inv_icm42600_core_probe(struct regmap *regmap, int chip,

> +int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,

>                            inv_icm42600_bus_setup bus_setup);


>  int inv_icm42600_gyro_init(struct inv_icm42600_state *st);


>  int inv_icm42600_accel_init(struct inv_icm42600_state *st);


> +int inv_icm42600_trigger_init(struct inv_icm42600_state *st, int irq,

> +                           int irq_type);

> +

>  #endif

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> index 4e33f263d3ea..1102c54396e3 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> @@ -447,11 +447,13 @@ static void inv_icm42600_disable_pm(void *_data)

>        pm_runtime_disable(dev);

>  }


> -int inv_icm42600_core_probe(struct regmap *regmap, int chip,

> +int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,

>                            inv_icm42600_bus_setup bus_setup)

>  {

>        struct device *dev = regmap_get_device(regmap);

>        struct inv_icm42600_state *st;

> +     struct irq_data *irq_desc;

> +     int irq_type;

>        int ret;


>        BUILD_BUG_ON(ARRAY_SIZE(inv_icm42600_hw) != INV_CHIP_NB);

> @@ -460,6 +462,16 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,

>                return -ENODEV;

>        }


> +     /* get irq data, set trigger falling by default */

> +     irq_desc = irq_get_irq_data(irq);

> +     if (!irq_desc) {

> +             dev_err(dev, "could not find IRQ %d\n", irq);

> +             return -EINVAL;

> +     }

> +     irq_type = irqd_get_trigger_type(irq_desc);

> +     if (!irq_type)

> +             irq_type = IRQF_TRIGGER_FALLING;

> +

>        st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);

>        if (!st)

>                return -ENOMEM;

> @@ -503,6 +515,11 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,

>        if (ret)

>                return ret;


> +     /* setup interrupt trigger */

> +     ret = inv_icm42600_trigger_init(st, irq, irq_type);

> +     if (ret)

> +             return ret;

> +

>        /* create and init gyroscope iio device */

>        ret = inv_icm42600_gyro_init(st);

>        if (ret)

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c

> index b61f993beacf..b1478ece43f6 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c

> @@ -70,7 +70,7 @@ static int inv_icm42600_probe(struct i2c_client *client,

>        if (IS_ERR(regmap))

>                return PTR_ERR(regmap);


> -     return inv_icm42600_core_probe(regmap, chip,

> +     return inv_icm42600_core_probe(regmap, chip, client->irq,

>                                       inv_icm42600_i2c_bus_setup);

>  }


> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c

> index 835ced68a3a3..ec784f9e3c2c 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c

> @@ -70,7 +70,7 @@ static int inv_icm42600_probe(struct spi_device *spi)

>        if (IS_ERR(regmap))

>                return PTR_ERR(regmap);


> -     return inv_icm42600_core_probe(regmap, chip,

> +     return inv_icm42600_core_probe(regmap, chip, spi->irq,

>                                       inv_icm42600_spi_bus_setup);

>  }


> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_trigger.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_trigger.c

> new file mode 100644

> index 000000000000..7a5e76305f0b

> --- /dev/null

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_trigger.c

> @@ -0,0 +1,177 @@

> +// SPDX-License-Identifier: GPL-2.0-or-later

> +/*

> + * Copyright (C) 2020 Invensense, Inc.

> + */

> +

> +#include <linux/device.h>

> +#include <linux/mutex.h>

> +#include <linux/interrupt.h>

> +#include <linux/pm_runtime.h>

> +#include <linux/regmap.h>

> +#include <linux/iio/iio.h>

> +#include <linux/iio/trigger.h>

> +#include <linux/iio/trigger_consumer.h>

> +

> +#include "inv_icm42600.h"

> +

> +static irqreturn_t inv_icm42600_trigger_timestamp(int irq, void *_data)

> +{

> +     struct inv_icm42600_state *st = _data;

> +

> +     if (st->indio_gyro)

> +             iio_pollfunc_store_time(irq, st->indio_gyro->pollfunc);

> +     if (st->indio_accel)

> +             iio_pollfunc_store_time(irq, st->indio_accel->pollfunc);

> +

> +     return IRQ_WAKE_THREAD;

> +}

> +

> +static irqreturn_t inv_icm42600_trigger_int_handler(int irq, void *_data)

> +{

> +     struct inv_icm42600_state *st = _data;

> +     struct device *dev = regmap_get_device(st->map);

> +     unsigned int status;

> +     int ret;

> +

> +     mutex_lock(&st->lock);

> +

> +     ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS, &status);

> +     if (ret)

> +             goto out_unlock;

> +     dev_dbg(dev, "int_status = %#02x\n", status);

> +

> +     /* FIFO full */

> +     if (status & INV_ICM42600_INT_STATUS_FIFO_FULL)

> +             dev_warn(dev, "FIFO full data lost!\n");

> +

> +     /* FIFO threshold reached */

> +     if (status & INV_ICM42600_INT_STATUS_FIFO_THS)

> +             iio_trigger_poll_chained(st->trigger);

> +

> +out_unlock:

> +     mutex_unlock(&st->lock);

> +     return IRQ_HANDLED;

> +}

> +

> +static int inv_icm42600_trigger_set_state(struct iio_trigger *trig, bool state)

> +{

> +     struct inv_icm42600_state *st = iio_trigger_get_drvdata(trig);

> +     unsigned int val;

> +     uint16_t dummy;

> +     int ret;

> +

> +     mutex_lock(&st->lock);

> +

> +     /*

> +      * IIO buffers preenable and postdisable are managing power on/off.

> +      * update_scan_mode is setting data FIFO enabled.

> +      */

> +     if (state) {

> +             /* set FIFO threshold interrupt */

> +             val = INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN;

> +             ret = regmap_update_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,

> +                                      val, val);

> +             if (ret)

> +                     goto out_unlock;



blank line after each block.  Makes it easier for my tired eyes to parse :)



> +             /* flush FIFO data */

> +             ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,

> +                                INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH);

> +             if (ret)

> +                     goto out_unlock;



> +             /* set FIFO in streaming mode */

> +             ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,

> +                                INV_ICM42600_FIFO_CONFIG_STREAM);

> +             if (ret)

> +                     goto out_unlock;



> +             /* workaround: dummy read of FIFO count */



Work around for what?



> +             ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,

> +                                    &dummy, sizeof(dummy));

> +     } else {

> +             /* set FIFO in bypass mode */

> +             ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,

> +                                INV_ICM42600_FIFO_CONFIG_BYPASS);

> +             if (ret)

> +                     goto out_unlock;



> +             /* flush FIFO data */

> +             ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,

> +                                INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH);

> +             if (ret)

> +                     goto out_unlock;



> +             /* disable FIFO threshold interrupt */

> +             val = INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN;

> +             ret = regmap_update_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,

> +                                      val, 0);

> +     }

> +

> +out_unlock:

> +     mutex_unlock(&st->lock);

> +     return ret;

> +}

> +

> +static int inv_icm42600_trigger_validate(struct iio_trigger *trig,

> +                                      struct iio_dev *indio_dev)

> +{

> +     struct inv_icm42600_state *st = iio_trigger_get_drvdata(trig);

> +

> +     if (iio_device_get_drvdata(indio_dev) != st)

> +             return -ENODEV;

> +

> +     return 0;

> +}

> +

> +static const struct iio_trigger_ops inv_icm42600_trigger_ops = {

> +     .set_trigger_state = &inv_icm42600_trigger_set_state,

> +     .validate_device = &inv_icm42600_trigger_validate,

> +};

> +

> +int inv_icm42600_trigger_init(struct inv_icm42600_state *st, int irq,

> +                           int irq_type)

> +{

> +     struct device *dev = regmap_get_device(st->map);

> +     unsigned int val;

> +     int ret;

> +

> +     st->trigger = devm_iio_trigger_alloc(dev, "%s-dev", st->name);

> +     if (!st->trigger)

> +             return -ENOMEM;

> +

> +     st->trigger->dev.parent = dev;

> +     st->trigger->ops = &inv_icm42600_trigger_ops;

> +     iio_trigger_set_drvdata(st->trigger, st);

> +

> +     /* configure INT1 with correct mode */

> +     /* falling or both-edge */

> +     if (irq_type & IRQF_TRIGGER_FALLING) {

> +             val = INV_ICM42600_INT_CONFIG_INT1_ACTIVE_LOW;

> +     } else if (irq_type == IRQF_TRIGGER_RISING) {

> +             val = INV_ICM42600_INT_CONFIG_INT1_ACTIVE_HIGH;

> +     } else if (irq_type == IRQF_TRIGGER_LOW) {

> +             val = INV_ICM42600_INT_CONFIG_INT1_ACTIVE_LOW |

> +                             INV_ICM42600_INT_CONFIG_INT1_LATCHED;

> +     } else if (irq_type == IRQF_TRIGGER_HIGH) {

> +             val = INV_ICM42600_INT_CONFIG_INT1_ACTIVE_LOW |

> +                             INV_ICM42600_INT_CONFIG_INT1_LATCHED;

> +     } else {

> +             dev_err(dev, "invalid interrupt type %#x\n", irq_type);

> +             return -EINVAL;

> +     }

> +     val |= INV_ICM42600_INT_CONFIG_INT1_PUSH_PULL;

> +     ret = regmap_write(st->map, INV_ICM42600_REG_INT_CONFIG, val);

> +     if (ret)

> +             return ret;

> +

> +     /* Deassert async reset for proper INT pin operation (cf datasheet) */

> +     ret = regmap_update_bits(st->map, INV_ICM42600_REG_INT_CONFIG1,

> +                              INV_ICM42600_INT_CONFIG1_ASYNC_RESET, 0);

> +     if (ret)

> +             return ret;

> +

> +     ret = devm_request_threaded_irq(dev, irq,

> +                                     inv_icm42600_trigger_timestamp,

> +                                     inv_icm42600_trigger_int_handler,

> +                                     irq_type, "inv_icm42600", st);

> +     if (ret)

> +             return ret;

> +

> +     return devm_iio_trigger_register(dev, st->trigger);

> +}






^ permalink raw reply

* Re: [PATCH 04/12] iio: imu: inv_icm42600: add gyroscope IIO device
From: Jean-Baptiste Maneyrol @ 2020-05-18 15:33 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: jic23@kernel.org, robh+dt@kernel.org, robh@kernel.org,
	mchehab+huawei@kernel.org, davem@davemloft.net,
	gregkh@linuxfoundation.org, linux-iio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <MN2PR12MB4422B32CB3C4BFD0AF5FFF3CC4B80@MN2PR12MB4422.namprd12.prod.outlook.com>

Hi Jonathan,

I agree with all comments.

For regmap_bulk_read, by looking at source code it doesn't seem to requires specific alignment, except if bus read callback is expecting that. But I can see numerous drivers calling regmap_bulk_read with a data buffer on the stack and not particularly aligned.

And we definitely can read calibration offset registers while running, the lock is indeed not needed.

Thanks,
JB



From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>

Sent: Friday, May 8, 2020 16:01

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: jic23@kernel.org <jic23@kernel.org>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>;
 linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>

Subject: Re: [PATCH 04/12] iio: imu: inv_icm42600: add gyroscope IIO device

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



On Thu, 7 May 2020 16:42:14 +0200

Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:



> Add IIO device for gyroscope sensor with data polling interface.

> Attributes: raw, scale, sampling_frequency, calibbias.

> 

> Gyroscope in low noise mode.

> 

> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

Few trivial things and questions inline.



J



> ---

>  drivers/iio/imu/inv_icm42600/inv_icm42600.h   |   4 +

>  .../iio/imu/inv_icm42600/inv_icm42600_core.c  |   5 +

>  .../iio/imu/inv_icm42600/inv_icm42600_gyro.c  | 549 ++++++++++++++++++

>  3 files changed, 558 insertions(+)

>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> 

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> index 8da4c8249aed..ca41a9d6404a 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> @@ -120,6 +120,7 @@ struct inv_icm42600_suspended {

>   *  @orientation:    sensor chip orientation relative to main hardware.

>   *  @conf:           chip sensors configurations.

>   *  @suspended:              suspended sensors configuration.

> + *  @indio_gyro:     gyroscope IIO device.

>   */

>  struct inv_icm42600_state {

>        struct mutex lock;

> @@ -131,6 +132,7 @@ struct inv_icm42600_state {

>        struct iio_mount_matrix orientation;

>        struct inv_icm42600_conf conf;

>        struct inv_icm42600_suspended suspended;

> +     struct iio_dev *indio_gyro;

>  };


>  /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */

> @@ -369,4 +371,6 @@ int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,

>  int inv_icm42600_core_probe(struct regmap *regmap, int chip,

>                            inv_icm42600_bus_setup bus_setup);


> +int inv_icm42600_gyro_init(struct inv_icm42600_state *st);

> +

>  #endif

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> index 35bdf4f9d31e..151257652ce6 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> @@ -503,6 +503,11 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,

>        if (ret)

>                return ret;


> +     /* create and init gyroscope iio device */



'Kind' of obvious from function name?   Maybe drop the comment?



> +     ret = inv_icm42600_gyro_init(st);

> +     if (ret)

> +             return ret;

> +

>        /* setup runtime power management */

>        ret = pm_runtime_set_active(dev);

>        if (ret)

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> new file mode 100644

> index 000000000000..74aa2b5fa611

> --- /dev/null

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> @@ -0,0 +1,549 @@

> +// SPDX-License-Identifier: GPL-2.0-or-later

> +/*

> + * Copyright (C) 2020 Invensense, Inc.

> + */

> +

> +#include <linux/device.h>

> +#include <linux/mutex.h>

> +#include <linux/interrupt.h>

> +#include <linux/pm_runtime.h>

> +#include <linux/regmap.h>

> +#include <linux/delay.h>

> +#include <linux/iio/iio.h>

> +

> +#include "inv_icm42600.h"

> +

> +#define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info)         \

> +     {                                                               \

> +             .type = IIO_ANGL_VEL,                                   \

> +             .modified = 1,                                          \

> +             .channel2 = _modifier,                                  \

> +             .info_mask_separate =                                   \

> +                     BIT(IIO_CHAN_INFO_RAW) |                        \

> +                     BIT(IIO_CHAN_INFO_CALIBBIAS),                   \

> +             .info_mask_shared_by_type =                             \

> +                     BIT(IIO_CHAN_INFO_SCALE),                       \

> +             .info_mask_shared_by_type_available =                   \

> +                     BIT(IIO_CHAN_INFO_SCALE),                       \

> +             .info_mask_shared_by_all =                              \

> +                     BIT(IIO_CHAN_INFO_SAMP_FREQ),                   \

> +             .info_mask_shared_by_all_available =                    \

> +                     BIT(IIO_CHAN_INFO_SAMP_FREQ),                   \

> +             .scan_index = _index,                                   \

> +             .scan_type = {                                          \

> +                     .sign = 's',                                    \

> +                     .realbits = 16,                                 \

> +                     .storagebits = 16,                              \

> +                     .shift = 0,                                     \



Shift has the 'obviously' default of 0, so normally we don't bother explicitly

setting it to 0 like this.



> +                     .endianness = IIO_BE,                           \

> +             },                                                      \

> +             .ext_info = _ext_info,                                  \

> +     }

> +

> +enum inv_icm42600_gyro_scan {

> +     INV_ICM42600_GYRO_SCAN_X,

> +     INV_ICM42600_GYRO_SCAN_Y,

> +     INV_ICM42600_GYRO_SCAN_Z,

> +};

> +

> +static const struct iio_chan_spec_ext_info inv_icm42600_gyro_ext_infos[] = {

> +     IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, inv_icm42600_get_mount_matrix),

> +     {},

> +};

> +

> +static const struct iio_chan_spec inv_icm42600_gyro_channels[] = {

> +     INV_ICM42600_GYRO_CHAN(IIO_MOD_X, INV_ICM42600_GYRO_SCAN_X,

> +                            inv_icm42600_gyro_ext_infos),

> +     INV_ICM42600_GYRO_CHAN(IIO_MOD_Y, INV_ICM42600_GYRO_SCAN_Y,

> +                            inv_icm42600_gyro_ext_infos),

> +     INV_ICM42600_GYRO_CHAN(IIO_MOD_Z, INV_ICM42600_GYRO_SCAN_Z,

> +                            inv_icm42600_gyro_ext_infos),

> +};

> +

> +static int inv_icm42600_gyro_read_sensor(struct inv_icm42600_state *st,

> +                                      struct iio_chan_spec const *chan,

> +                                      int16_t *val)

> +{

> +     struct device *dev = regmap_get_device(st->map);

> +     struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;

> +     unsigned int reg;

> +     __be16 data;

> +     int ret;

> +

> +     if (chan->type != IIO_ANGL_VEL)

> +             return -EINVAL;

> +

> +     switch (chan->channel2) {

> +     case IIO_MOD_X:

> +             reg = INV_ICM42600_REG_GYRO_DATA_X;

> +             break;

> +     case IIO_MOD_Y:

> +             reg = INV_ICM42600_REG_GYRO_DATA_Y;

> +             break;

> +     case IIO_MOD_Z:

> +             reg = INV_ICM42600_REG_GYRO_DATA_Z;

> +             break;

> +     default:

> +             return -EINVAL;

> +     }

> +

> +     pm_runtime_get_sync(dev);

> +     mutex_lock(&st->lock);

> +

> +     /* enable gyro sensor */

> +     conf.mode = INV_ICM42600_SENSOR_MODE_LOW_NOISE;

> +     ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);

> +     if (ret)

> +             goto exit;

> +

> +     /* read gyro register data */

> +     ret = regmap_bulk_read(st->map, reg, &data, sizeof(data));



IIRC bulk reads need to be to dma safe buffers.  So typically on the stack and

in appropriately aligned location in any containing structure.



> +     if (ret)

> +             goto exit;

> +

> +     *val = (int16_t)be16_to_cpu(data);

> +     if (*val == INV_ICM42600_DATA_INVALID)

> +             ret = -EINVAL;

> +exit:

> +     mutex_unlock(&st->lock);

> +     pm_runtime_mark_last_busy(dev);

> +     pm_runtime_put_autosuspend(dev);

> +     return ret;

> +}

> +

> +/* IIO format int + nano */

> +static const int inv_icm42600_gyro_scale[] = {

> +     /* +/- 2000dps => 0.001065264 rad/s */

> +     [2 * INV_ICM42600_GYRO_FS_2000DPS] = 0,

> +     [2 * INV_ICM42600_GYRO_FS_2000DPS + 1] = 1065264,

> +     /* +/- 1000dps => 0.000532632 rad/s */

> +     [2 * INV_ICM42600_GYRO_FS_1000DPS] = 0,

> +     [2 * INV_ICM42600_GYRO_FS_1000DPS + 1] = 532632,

> +     /* +/- 500dps => 0.000266316 rad/s */

> +     [2 * INV_ICM42600_GYRO_FS_500DPS] = 0,

> +     [2 * INV_ICM42600_GYRO_FS_500DPS + 1] = 266316,

> +     /* +/- 250dps => 0.000133158 rad/s */

> +     [2 * INV_ICM42600_GYRO_FS_250DPS] = 0,

> +     [2 * INV_ICM42600_GYRO_FS_250DPS + 1] = 133158,

> +     /* +/- 125dps => 0.000066579 rad/s */

> +     [2 * INV_ICM42600_GYRO_FS_125DPS] = 0,

> +     [2 * INV_ICM42600_GYRO_FS_125DPS + 1] = 66579,

> +     /* +/- 62.5dps => 0.000033290 rad/s */

> +     [2 * INV_ICM42600_GYRO_FS_62_5DPS] = 0,

> +     [2 * INV_ICM42600_GYRO_FS_62_5DPS + 1] = 33290,

> +     /* +/- 31.25dps => 0.000016645 rad/s */

> +     [2 * INV_ICM42600_GYRO_FS_31_25DPS] = 0,

> +     [2 * INV_ICM42600_GYRO_FS_31_25DPS + 1] = 16645,

> +     /* +/- 15.625dps => 0.000008322 rad/s */

> +     [2 * INV_ICM42600_GYRO_FS_15_625DPS] = 0,

> +     [2 * INV_ICM42600_GYRO_FS_15_625DPS + 1] = 8322,

> +};

> +

> +static int inv_icm42600_gyro_read_scale(struct inv_icm42600_state *st,

> +                                     int *val, int *val2)

> +{

> +     unsigned int idx;

> +

> +     mutex_lock(&st->lock);

> +     idx = st->conf.gyro.fs;



Seems like we shouldn't need the lock to retrieve a single value.

Is there some odd intermediate state somewhere I'm missing?



> +     mutex_unlock(&st->lock);

> +

> +     *val = inv_icm42600_gyro_scale[2 * idx];

> +     *val2 = inv_icm42600_gyro_scale[2 * idx + 1];

> +     return IIO_VAL_INT_PLUS_NANO;

> +}

> +

> +static int inv_icm42600_gyro_write_scale(struct inv_icm42600_state *st,

> +                                      int val, int val2)

> +{

> +     struct device *dev = regmap_get_device(st->map);

> +     unsigned int idx;

> +     struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;

> +     int ret;

> +

> +     for (idx = 0; idx < ARRAY_SIZE(inv_icm42600_gyro_scale); idx += 2) {

> +             if (val == inv_icm42600_gyro_scale[idx] &&

> +                             val2 == inv_icm42600_gyro_scale[idx + 1])



Alignment of code seems odd.



> +                     break;

> +     }

> +     if (idx >= ARRAY_SIZE(inv_icm42600_gyro_scale))

> +             return -EINVAL;

> +

> +     /* update gyro fs */

> +     pm_runtime_get_sync(dev);

> +

> +     mutex_lock(&st->lock);

> +     conf.fs = idx / 2;

> +     ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);

> +     mutex_unlock(&st->lock);

> +

> +     pm_runtime_mark_last_busy(dev);

> +     pm_runtime_put_autosuspend(dev);

> +

> +     return ret;

> +}

> +

> +/* IIO format int + micro */

> +static const int inv_icm42600_gyro_odr[] = {

> +     /* 12.5Hz */

> +     12, 500000,

> +     /* 25Hz */

> +     25, 0,

> +     /* 50Hz */

> +     50, 0,

> +     /* 100Hz */

> +     100, 0,

> +     /* 200Hz */

> +     200, 0,

> +     /* 1kHz */

> +     1000, 0,

> +     /* 2kHz */

> +     2000, 0,

> +     /* 4kHz */

> +     4000, 0,

> +};

> +

> +static const int inv_icm42600_gyro_odr_conv[] = {

> +     INV_ICM42600_ODR_12_5HZ,

> +     INV_ICM42600_ODR_25HZ,

> +     INV_ICM42600_ODR_50HZ,

> +     INV_ICM42600_ODR_100HZ,

> +     INV_ICM42600_ODR_200HZ,

> +     INV_ICM42600_ODR_1KHZ_LN,

> +     INV_ICM42600_ODR_2KHZ_LN,

> +     INV_ICM42600_ODR_4KHZ_LN,

> +};

> +

> +static int inv_icm42600_gyro_read_odr(struct inv_icm42600_state *st,

> +                                   int *val, int *val2)

> +{

> +     unsigned int odr;

> +     unsigned int i;

> +

> +     mutex_lock(&st->lock);

> +     odr = st->conf.gyro.odr;

> +     mutex_unlock(&st->lock);

> +

> +     for (i = 0; i < ARRAY_SIZE(inv_icm42600_gyro_odr_conv); ++i) {

> +             if (inv_icm42600_gyro_odr_conv[i] == odr)

> +                     break;

> +     }

> +     if (i >= ARRAY_SIZE(inv_icm42600_gyro_odr_conv))

> +             return -EINVAL;

> +

> +     *val = inv_icm42600_gyro_odr[2 * i];

> +     *val2 = inv_icm42600_gyro_odr[2 * i + 1];

> +

> +     return IIO_VAL_INT_PLUS_MICRO;

> +}

> +

> +static int inv_icm42600_gyro_write_odr(struct inv_icm42600_state *st,

> +                                    int val, int val2)

> +{

> +     struct device *dev = regmap_get_device(st->map);

> +     unsigned int idx;

> +     struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;

> +     int ret;

> +

> +     for (idx = 0; idx < ARRAY_SIZE(inv_icm42600_gyro_odr); idx += 2) {

> +             if (val == inv_icm42600_gyro_odr[idx] &&

> +                             val2 == inv_icm42600_gyro_odr[idx + 1])

> +                     break;

> +     }

> +     if (idx >= ARRAY_SIZE(inv_icm42600_gyro_odr))

> +             return -EINVAL;

> +

> +     /* update gyro odr */

> +     pm_runtime_get_sync(dev);

> +

> +     mutex_lock(&st->lock);

> +     conf.odr = inv_icm42600_gyro_odr_conv[idx / 2];

> +     ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);

> +     mutex_unlock(&st->lock);

> +

> +     pm_runtime_mark_last_busy(dev);

> +     pm_runtime_put_autosuspend(dev);

> +

> +     return ret;

> +}

> +

> +static int inv_icm42600_gyro_read_offset(struct inv_icm42600_state *st,

> +                                      struct iio_chan_spec const *chan,

> +                                      int16_t *val)

> +{

> +     struct device *dev = regmap_get_device(st->map);

> +     unsigned int reg;

> +     uint8_t data[2];

> +     int ret;

> +

> +     if (chan->type != IIO_ANGL_VEL)

> +             return -EINVAL;

> +

> +     switch (chan->channel2) {

> +     case IIO_MOD_X:

> +             reg = INV_ICM42600_REG_OFFSET_USER0;

> +             break;

> +     case IIO_MOD_Y:

> +             reg = INV_ICM42600_REG_OFFSET_USER1;

> +             break;

> +     case IIO_MOD_Z:

> +             reg = INV_ICM42600_REG_OFFSET_USER3;

> +             break;

> +     default:

> +             return -EINVAL;

> +     }

> +

> +     pm_runtime_get_sync(dev);

> +

> +     /* read gyro offset data */

> +     mutex_lock(&st->lock);

> +     ret = regmap_bulk_read(st->map, reg, &data, sizeof(data));

> +     mutex_unlock(&st->lock);

> +     if (ret)

> +             goto exit;

> +

> +     switch (chan->channel2) {

> +     case IIO_MOD_X:

> +             *val = (int16_t)(((data[1] & 0x0F) << 8) | data[0]);



This doesn't look right for negative values.  You would be better

off with a sign extend of the 12 bit value.



> +             break;

> +     case IIO_MOD_Y:

> +             *val = (int16_t)(((data[0] & 0xF0) << 4) | data[1]);

> +             break;

> +     case IIO_MOD_Z:

> +             *val = (int16_t)(((data[1] & 0x0F) << 8) | data[0]);

> +             break;

> +     default:

> +             ret = -EINVAL;

> +             break;

> +     }

> +

> +exit:

> +     pm_runtime_mark_last_busy(dev);

> +     pm_runtime_put_autosuspend(dev);

> +     return ret;

> +}

> +

> +static int inv_icm42600_gyro_write_offset(struct inv_icm42600_state *st,

> +                                       struct iio_chan_spec const *chan,

> +                                       int val)

> +{

> +     struct device *dev = regmap_get_device(st->map);

> +     unsigned int reg, regval;

> +     uint8_t data[2];

> +     int ret;

> +

> +     if (chan->type != IIO_ANGL_VEL)

> +             return -EINVAL;

> +

> +     switch (chan->channel2) {

> +     case IIO_MOD_X:

> +             reg = INV_ICM42600_REG_OFFSET_USER0;

> +             break;

> +     case IIO_MOD_Y:

> +             reg = INV_ICM42600_REG_OFFSET_USER1;

> +             break;

> +     case IIO_MOD_Z:

> +             reg = INV_ICM42600_REG_OFFSET_USER3;

> +             break;

> +     default:

> +             return -EINVAL;

> +     }

> +

> +     /* value is limited to 12 bits signed */

> +     if (val < -2048 || val > 2047)

> +             return -EINVAL;



Perhaps worth an available callback to give the range?



> +

> +     pm_runtime_get_sync(dev);

> +     mutex_lock(&st->lock);

> +

> +     switch (chan->channel2) {

> +     case IIO_MOD_X:

> +             /* OFFSET_USER1 register is shared */

> +             ret = regmap_read(st->map, INV_ICM42600_REG_OFFSET_USER1,

> +                               &regval);

> +             if (ret)

> +                     goto out_unlock;

> +             data[0] = val & 0xFF;

> +             data[1] = (regval & 0xF0) | ((val & 0xF00) >> 8);

> +             break;

> +     case IIO_MOD_Y:

> +             /* OFFSET_USER1 register is shared */

> +             ret = regmap_read(st->map, INV_ICM42600_REG_OFFSET_USER1,

> +                               &regval);

> +             if (ret)

> +                     goto out_unlock;

> +             data[0] = ((val & 0xF00) >> 4) | (regval & 0x0F);

> +             data[1] = val & 0xFF;

> +             break;

> +     case IIO_MOD_Z:

> +             /* OFFSET_USER4 register is shared */

> +             ret = regmap_read(st->map, INV_ICM42600_REG_OFFSET_USER4,

> +                               &regval);

> +             if (ret)

> +                     goto out_unlock;

> +             data[0] = val & 0xFF;

> +             data[1] = (regval & 0xF0) | ((val & 0xF00) >> 8);

> +             break;

> +     default:

> +             ret = -EINVAL;

> +             goto out_unlock;

> +     }

> +

> +     ret = regmap_bulk_write(st->map, reg, data, sizeof(data));

> +

> +out_unlock:

> +     mutex_unlock(&st->lock);

> +     pm_runtime_mark_last_busy(dev);

> +     pm_runtime_put_autosuspend(dev);

> +     return ret;

> +}

> +

> +static int inv_icm42600_gyro_read_raw(struct iio_dev *indio_dev,

> +                                   struct iio_chan_spec const *chan,

> +                                   int *val, int *val2, long mask)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     int16_t data;

> +     int ret;

> +

> +     if (chan->type != IIO_ANGL_VEL)

> +             return -EINVAL;

> +

> +     switch (mask) {

> +     case IIO_CHAN_INFO_RAW:

> +             ret = iio_device_claim_direct_mode(indio_dev);

> +             if (ret)

> +                     return ret;

> +             ret = inv_icm42600_gyro_read_sensor(st, chan, &data);

> +             iio_device_release_direct_mode(indio_dev);

> +             if (ret)

> +                     return ret;

> +             *val = data;

> +             return IIO_VAL_INT;

> +     case IIO_CHAN_INFO_SCALE:

> +             return inv_icm42600_gyro_read_scale(st, val, val2);

> +     case IIO_CHAN_INFO_SAMP_FREQ:

> +             return inv_icm42600_gyro_read_odr(st, val, val2);

> +     case IIO_CHAN_INFO_CALIBBIAS:

> +             ret = iio_device_claim_direct_mode(indio_dev);

> +             if (ret)

> +                     return ret;



I'm curious.  Why can't we read back a calibration offset whilst doing

buffered capture?



> +             ret = inv_icm42600_gyro_read_offset(st, chan, &data);

> +             iio_device_release_direct_mode(indio_dev);

> +             if (ret)

> +                     return ret;

> +             *val = data;

> +             return IIO_VAL_INT;

> +     default:

> +             return -EINVAL;

> +     }

> +}

> +

> +static int inv_icm42600_gyro_read_avail(struct iio_dev *indio_dev,

> +                                     struct iio_chan_spec const *chan,

> +                                     const int **vals,

> +                                     int *type, int *length, long mask)

> +{

> +     if (chan->type != IIO_ANGL_VEL)

> +             return -EINVAL;

> +

> +     switch (mask) {

> +     case IIO_CHAN_INFO_SCALE:

> +             *vals = inv_icm42600_gyro_scale;

> +             *type = IIO_VAL_INT_PLUS_NANO;

> +             *length = ARRAY_SIZE(inv_icm42600_gyro_scale);

> +             return IIO_AVAIL_LIST;

> +     case IIO_CHAN_INFO_SAMP_FREQ:

> +             *vals = inv_icm42600_gyro_odr;

> +             *type = IIO_VAL_INT_PLUS_MICRO;

> +             *length = ARRAY_SIZE(inv_icm42600_gyro_odr);

> +             return IIO_AVAIL_LIST;

> +     default:

> +             return -EINVAL;

> +     }

> +}

> +

> +static int inv_icm42600_gyro_write_raw(struct iio_dev *indio_dev,

> +                                    struct iio_chan_spec const *chan,

> +                                    int val, int val2, long mask)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     int ret;

> +

> +     if (chan->type != IIO_ANGL_VEL)

> +             return -EINVAL;

> +

> +     switch (mask) {

> +     case IIO_CHAN_INFO_SCALE:

> +             ret = iio_device_claim_direct_mode(indio_dev);

> +             if (ret)

> +                     return ret;

> +             ret = inv_icm42600_gyro_write_scale(st, val, val2);

> +             iio_device_release_direct_mode(indio_dev);

> +             return ret;

> +     case IIO_CHAN_INFO_SAMP_FREQ:

> +             return inv_icm42600_gyro_write_odr(st, val, val2);

> +     case IIO_CHAN_INFO_CALIBBIAS:

> +             ret = iio_device_claim_direct_mode(indio_dev);

> +             if (ret)

> +                     return ret;

> +             ret = inv_icm42600_gyro_write_offset(st, chan, val);

> +             iio_device_release_direct_mode(indio_dev);

> +             return ret;

> +     default:

> +             return -EINVAL;

> +     }

> +}

> +

> +static int inv_icm42600_gyro_write_raw_get_fmt(struct iio_dev *indio_dev,

> +                                            struct iio_chan_spec const *chan,

> +                                            long mask)

> +{

> +     if (chan->type != IIO_ANGL_VEL)

> +             return -EINVAL;

> +

> +     switch (mask) {

> +     case IIO_CHAN_INFO_SCALE:

> +             return IIO_VAL_INT_PLUS_NANO;

> +     case IIO_CHAN_INFO_SAMP_FREQ:

> +             return IIO_VAL_INT_PLUS_MICRO;

> +     case IIO_CHAN_INFO_CALIBBIAS:

> +             return IIO_VAL_INT;

> +     default:

> +             return -EINVAL;

> +     }

> +}

> +

> +static const struct iio_info inv_icm42600_gyro_info = {

> +     .read_raw = inv_icm42600_gyro_read_raw,

> +     .read_avail = inv_icm42600_gyro_read_avail,

> +     .write_raw = inv_icm42600_gyro_write_raw,

> +     .write_raw_get_fmt = inv_icm42600_gyro_write_raw_get_fmt,

> +     .debugfs_reg_access = inv_icm42600_debugfs_reg,

> +};

> +

> +int inv_icm42600_gyro_init(struct inv_icm42600_state *st)

> +{

> +     struct device *dev = regmap_get_device(st->map);

> +     const char *name;

> +     struct iio_dev *indio_dev;

> +

> +     name = devm_kasprintf(dev, GFP_KERNEL, "%s-gyro", st->name);

> +     if (!name)

> +             return -ENOMEM;

> +

> +     indio_dev = devm_iio_device_alloc(dev, 0);

> +     if (!indio_dev)

> +             return -ENOMEM;

> +

> +     iio_device_set_drvdata(indio_dev, st);

> +     indio_dev->dev.parent = dev;

> +     indio_dev->name = name;

> +     indio_dev->info = &inv_icm42600_gyro_info;

> +     indio_dev->modes = INDIO_DIRECT_MODE;

> +     indio_dev->channels = inv_icm42600_gyro_channels;

> +     indio_dev->num_channels = ARRAY_SIZE(inv_icm42600_gyro_channels);

> +

> +     st->indio_gyro = indio_dev;

> +     return devm_iio_device_register(dev, st->indio_gyro);

> +}






^ permalink raw reply

* Re: [PATCH 09/12] iio: imu: inv_icm42600: add buffer support in iio devices
From: Jean-Baptiste Maneyrol @ 2020-05-18 15:32 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: jic23@kernel.org, robh+dt@kernel.org, robh@kernel.org,
	mchehab+huawei@kernel.org, davem@davemloft.net,
	gregkh@linuxfoundation.org, linux-iio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20200508151933.00007d56@Huawei.com>

Hi Jonathan,

no problem with the comments.

fifo_decode_packet will move to source code, 

For the following sleep comment:
> +     if (sleep_gyro > sleep_temp)
> +             sleep = sleep_gyro;
> +     else
> +             sleep = sleep_temp;
> +     if (sleep)
> +             msleep(sleep);

        if (sleep_gyro > sleep_temp)
                msleep(sleep_gyro);
        else
                msleep(sleep_temp);

I am using an intermediate local variable to avoid a call to msleep(0) which is in fact sleeping for real for 1 jiffies as far as I understand. So is it OK to keep it as is?

Thanks,
JB



From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>

Sent: Friday, May 8, 2020 16:19

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: jic23@kernel.org <jic23@kernel.org>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>;
 linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>

Subject: Re: [PATCH 09/12] iio: imu: inv_icm42600: add buffer support in iio devices

 


 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.



On Thu, 7 May 2020 16:42:19 +0200

Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:



> Use triggered buffer by parsing FIFO data read in device trigger.

> Support hwfifo watermark by multiplexing gyro and accel settings.

> Support hwfifo flush.

> 

> Simply use interrupt timestamp first.

> 

> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

> ---

>  drivers/iio/imu/inv_icm42600/Kconfig          |   3 +-

>  drivers/iio/imu/inv_icm42600/Makefile         |   1 +

>  drivers/iio/imu/inv_icm42600/inv_icm42600.h   |   8 +

>  .../iio/imu/inv_icm42600/inv_icm42600_accel.c | 183 +++++++++

>  .../imu/inv_icm42600/inv_icm42600_buffer.c    | 353 ++++++++++++++++++

>  .../imu/inv_icm42600/inv_icm42600_buffer.h    | 162 ++++++++

>  .../iio/imu/inv_icm42600/inv_icm42600_core.c  |  23 ++

>  .../iio/imu/inv_icm42600/inv_icm42600_gyro.c  | 183 +++++++++

>  .../imu/inv_icm42600/inv_icm42600_trigger.c   |  15 +-

>  9 files changed, 928 insertions(+), 3 deletions(-)

>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c

>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h

> 

> diff --git a/drivers/iio/imu/inv_icm42600/Kconfig b/drivers/iio/imu/inv_icm42600/Kconfig

> index 7b3eaeb2aa4a..8c0969319c49 100644

> --- a/drivers/iio/imu/inv_icm42600/Kconfig

> +++ b/drivers/iio/imu/inv_icm42600/Kconfig

> @@ -2,7 +2,8 @@


>  config INV_ICM42600

>        tristate

> -     select IIO_TRIGGER

> +     select IIO_BUFFER

> +     select IIO_TRIGGERED_BUFFER


>  config INV_ICM42600_I2C

>        tristate "InvenSense ICM-426xx I2C driver"

> diff --git a/drivers/iio/imu/inv_icm42600/Makefile b/drivers/iio/imu/inv_icm42600/Makefile

> index e1f2aacbe888..d6732118010c 100644

> --- a/drivers/iio/imu/inv_icm42600/Makefile

> +++ b/drivers/iio/imu/inv_icm42600/Makefile

> @@ -6,6 +6,7 @@ inv-icm42600-y += inv_icm42600_gyro.o

>  inv-icm42600-y += inv_icm42600_accel.o

>  inv-icm42600-y += inv_icm42600_temp.o

>  inv-icm42600-y += inv_icm42600_trigger.o

> +inv-icm42600-y += inv_icm42600_buffer.o


>  obj-$(CONFIG_INV_ICM42600_I2C) += inv-icm42600-i2c.o

>  inv-icm42600-i2c-y += inv_icm42600_i2c.o

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> index 175c1f67faee..947ca4dd245b 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h

> @@ -15,6 +15,8 @@

>  #include <linux/iio/iio.h>

>  #include <linux/iio/trigger.h>


> +#include "inv_icm42600_buffer.h"

> +

>  enum inv_icm42600_chip {

>        INV_CHIP_ICM42600,

>        INV_CHIP_ICM42602,

> @@ -124,6 +126,7 @@ struct inv_icm42600_suspended {

>   *  @indio_gyro:     gyroscope IIO device.

>   *  @indio_accel:    accelerometer IIO device.

>   *  @trigger:                device internal interrupt trigger

> + *  @fifo:           FIFO management structure.

>   */

>  struct inv_icm42600_state {

>        struct mutex lock;

> @@ -138,6 +141,7 @@ struct inv_icm42600_state {

>        struct iio_dev *indio_gyro;

>        struct iio_dev *indio_accel;

>        struct iio_trigger *trigger;

> +     struct inv_icm42600_fifo fifo;

>  };


>  /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */

> @@ -378,8 +382,12 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,


>  int inv_icm42600_gyro_init(struct inv_icm42600_state *st);


> +int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev, int64_t ts);

> +

>  int inv_icm42600_accel_init(struct inv_icm42600_state *st);


> +int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev, int64_t ts);

> +

>  int inv_icm42600_trigger_init(struct inv_icm42600_state *st, int irq,

>                              int irq_type);


> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c

> index 74dac5f283d4..4206be54d057 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c

> @@ -10,9 +10,13 @@

>  #include <linux/regmap.h>

>  #include <linux/delay.h>

>  #include <linux/iio/iio.h>

> +#include <linux/iio/buffer.h>

> +#include <linux/iio/triggered_buffer.h>

> +#include <linux/iio/trigger_consumer.h>


>  #include "inv_icm42600.h"

>  #include "inv_icm42600_temp.h"

> +#include "inv_icm42600_buffer.h"


>  #define INV_ICM42600_ACCEL_CHAN(_modifier, _index, _ext_info)                \

>        {                                                               \

> @@ -46,6 +50,7 @@ enum inv_icm42600_accel_scan {

>        INV_ICM42600_ACCEL_SCAN_Y,

>        INV_ICM42600_ACCEL_SCAN_Z,

>        INV_ICM42600_ACCEL_SCAN_TEMP,

> +     INV_ICM42600_ACCEL_SCAN_TIMESTAMP,

>  };


>  static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {

> @@ -61,8 +66,100 @@ static const struct iio_chan_spec inv_icm42600_accel_channels[] = {

>        INV_ICM42600_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42600_ACCEL_SCAN_Z,

>                                inv_icm42600_accel_ext_infos),

>        INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),

> +     IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),

>  };


> +/* IIO buffer data */

> +struct inv_icm42600_accel_buffer {

> +     struct inv_icm42600_fifo_sensor_data accel;

> +     int8_t temp;

> +     int64_t timestamp;

> +};

> +

> +#define INV_ICM42600_SCAN_MASK_ACCEL_3AXIS                           \

> +     (BIT(INV_ICM42600_ACCEL_SCAN_X) |                               \

> +     BIT(INV_ICM42600_ACCEL_SCAN_Y) |                                \

> +     BIT(INV_ICM42600_ACCEL_SCAN_Z))

> +

> +#define INV_ICM42600_SCAN_MASK_TEMP  BIT(INV_ICM42600_ACCEL_SCAN_TEMP)

> +

> +static const unsigned long inv_icm42600_accel_scan_masks[] = {

> +     /* 3-axis accel + temperature */

> +     INV_ICM42600_SCAN_MASK_ACCEL_3AXIS | INV_ICM42600_SCAN_MASK_TEMP,

> +     0,

> +};

> +

> +static irqreturn_t inv_icm42600_accel_handler(int irq, void *_data)

> +{

> +     struct iio_poll_func *pf = _data;

> +     struct iio_dev *indio_dev = pf->indio_dev;

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     const size_t fifo_nb = st->fifo.nb.total;

> +     int ret;

> +

> +     /* exit if no sample */

> +     if (fifo_nb == 0)

> +             goto out;

> +

> +     ret = inv_icm42600_accel_parse_fifo(indio_dev, pf->timestamp);

> +     if (ret)

> +             dev_err(regmap_get_device(st->map), "accel fifo error %d\n",

> +                     ret);

> +

> +out:

> +     iio_trigger_notify_done(indio_dev->trig);

> +     return IRQ_HANDLED;

> +}

> +

> +/* enable accelerometer sensor and FIFO write */

> +static int inv_icm42600_accel_update_scan_mode(struct iio_dev *indio_dev,

> +                                            const unsigned long *scan_mask)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;

> +     unsigned int fifo_en = 0;

> +     unsigned int sleep_temp = 0;

> +     unsigned int sleep_accel = 0;

> +     unsigned int sleep;

> +     int ret;

> +

> +     mutex_lock(&st->lock);

> +

> +     if (*scan_mask & INV_ICM42600_SCAN_MASK_TEMP) {

> +             /* enable temp sensor */

> +             ret = inv_icm42600_set_temp_conf(st, true, &sleep_temp);

> +             if (ret)

> +                     goto out_unlock;

> +             fifo_en |= INV_ICM42600_SENSOR_TEMP;

> +     }

> +

> +     if (*scan_mask & INV_ICM42600_SCAN_MASK_ACCEL_3AXIS) {

> +             /* enable accel sensor */

> +             conf.mode = INV_ICM42600_SENSOR_MODE_LOW_NOISE;

> +             ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_accel);

> +             if (ret)

> +                     goto out_unlock;

> +             fifo_en |= INV_ICM42600_SENSOR_ACCEL;

> +     }

> +

> +     /* update data FIFO write and FIFO watermark */

> +     ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);

> +     if (ret)

> +             goto out_unlock;

> +     ret = inv_icm42600_buffer_update_watermark(st);

> +

> +out_unlock:

> +     mutex_unlock(&st->lock);

> +     /* sleep maximum required time */

> +     if (sleep_accel > sleep_temp)

> +             sleep = sleep_accel;

> +     else

> +             sleep = sleep_temp;

> +     if (sleep)

> +             msleep(sleep);

> +     return ret;

> +}

> +

>  static int inv_icm42600_accel_read_sensor(struct inv_icm42600_state *st,

>                                          struct iio_chan_spec const *chan,

>                                          int16_t *val)

> @@ -250,6 +347,8 @@ static int inv_icm42600_accel_write_odr(struct inv_icm42600_state *st,

>        mutex_lock(&st->lock);

>        conf.odr = inv_icm42600_accel_odr_conv[idx / 2];

>        ret = inv_icm42600_set_accel_conf(st, &conf, NULL);

> +     inv_icm42600_buffer_update_fifo_period(st);

> +     inv_icm42600_buffer_update_watermark(st);

>        mutex_unlock(&st->lock);


>        pm_runtime_mark_last_busy(dev);

> @@ -512,12 +611,51 @@ static int inv_icm42600_accel_write_raw_get_fmt(struct iio_dev *indio_dev,

>        }

>  }


> +static int inv_icm42600_accel_hwfifo_set_watermark(struct iio_dev *indio_dev,

> +                                                unsigned int val)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     int ret;

> +

> +     mutex_lock(&st->lock);

> +

> +     st->fifo.watermark.accel = val;

> +     ret = inv_icm42600_buffer_update_watermark(st);

> +

> +     mutex_unlock(&st->lock);

> +

> +     return ret;

> +}

> +

> +static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,

> +                                        unsigned int count)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     int ret;

> +

> +     if (count == 0)

> +             return 0;

> +

> +     mutex_lock(&st->lock);

> +

> +     ret = inv_icm42600_buffer_hwfifo_flush(st, count);

> +     if (!ret)

> +             ret = st->fifo.nb.accel;

> +

> +     mutex_unlock(&st->lock);

> +

> +     return ret;

> +}

> +

>  static const struct iio_info inv_icm42600_accel_info = {

>        .read_raw = inv_icm42600_accel_read_raw,

>        .read_avail = inv_icm42600_accel_read_avail,

>        .write_raw = inv_icm42600_accel_write_raw,

>        .write_raw_get_fmt = inv_icm42600_accel_write_raw_get_fmt,

>        .debugfs_reg_access = inv_icm42600_debugfs_reg,

> +     .update_scan_mode = inv_icm42600_accel_update_scan_mode,

> +     .hwfifo_set_watermark = inv_icm42600_accel_hwfifo_set_watermark,

> +     .hwfifo_flush_to_buffer = inv_icm42600_accel_hwfifo_flush,

>  };


>  int inv_icm42600_accel_init(struct inv_icm42600_state *st)

> @@ -525,6 +663,7 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st)

>        struct device *dev = regmap_get_device(st->map);

>        const char *name;

>        struct iio_dev *indio_dev;

> +     int ret;


>        name = devm_kasprintf(dev, GFP_KERNEL, "%s-accel", st->name);

>        if (!name)

> @@ -541,7 +680,51 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st)

>        indio_dev->modes = INDIO_DIRECT_MODE;

>        indio_dev->channels = inv_icm42600_accel_channels;

>        indio_dev->num_channels = ARRAY_SIZE(inv_icm42600_accel_channels);

> +     indio_dev->available_scan_masks = inv_icm42600_accel_scan_masks;

> +

> +     ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,

> +                                           inv_icm42600_accel_handler,

> +                                           &inv_icm42600_buffer_ops);

> +     if (ret)

> +             return ret;

> +

> +     indio_dev->trig = iio_trigger_get(st->trigger);


>        st->indio_accel = indio_dev;

>        return devm_iio_device_register(dev, st->indio_accel);

>  }

> +

> +int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev, int64_t ts)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     const size_t accel_nb = st->fifo.nb.accel;

> +     ssize_t i, size;

> +     const void *accel, *gyro, *temp, *timestamp;

> +     unsigned int odr;

> +     struct inv_icm42600_accel_buffer buffer;

> +

> +     /* exit if no accel sample */

> +     if (accel_nb == 0)

> +             return 0;

> +

> +     /* parse all fifo packets */

> +     for (i = 0; i < st->fifo.count; i += size) {

> +             size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],

> +                             &accel, &gyro, &temp, &timestamp, &odr);

> +             dev_dbg(regmap_get_device(st->map), "accel packet size = %zd\n",

> +                     size);

> +             /* quit if error or FIFO is empty */

> +             if (size <= 0)

> +                     return size;

> +             /* skip packet if no accel data or data is invalid */

> +             if (accel == NULL || !inv_icm42600_fifo_is_data_valid(accel)) {

> +                     dev_dbg(regmap_get_device(st->map), "skip accel data\n");

> +                     continue;

> +             }

> +             memcpy(&buffer.accel, accel, sizeof(buffer.accel));

> +             memcpy(&buffer.temp, temp, sizeof(buffer.temp));

> +             iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts);

> +     }

> +

> +     return 0;

> +}

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c

> new file mode 100644

> index 000000000000..b428abdc92ee

> --- /dev/null

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c

> @@ -0,0 +1,353 @@

> +// SPDX-License-Identifier: GPL-2.0-or-later

> +/*

> + * Copyright (C) 2020 Invensense, Inc.

> + */

> +

> +#include <linux/device.h>

> +#include <linux/mutex.h>

> +#include <linux/pm_runtime.h>

> +#include <linux/regmap.h>

> +#include <linux/delay.h>

> +#include <linux/math64.h>

> +#include <linux/iio/iio.h>

> +#include <linux/iio/buffer.h>

> +#include <linux/iio/triggered_buffer.h>

> +#include <linux/iio/trigger_consumer.h>

> +

> +#include "inv_icm42600.h"

> +#include "inv_icm42600_buffer.h"

> +

> +void inv_icm42600_buffer_update_fifo_period(struct inv_icm42600_state *st)

> +{

> +     uint32_t period_gyro, period_accel, period;

> +

> +     if (st->fifo.en & INV_ICM42600_SENSOR_GYRO)

> +             period_gyro = inv_icm42600_odr_to_period(st->conf.gyro.odr);

> +     else

> +             period_gyro = U32_MAX;

> +

> +     if (st->fifo.en & INV_ICM42600_SENSOR_ACCEL)

> +             period_accel = inv_icm42600_odr_to_period(st->conf.accel.odr);

> +     else

> +             period_accel = U32_MAX;

> +

> +     if (period_gyro <= period_accel)

> +             period = period_gyro;

> +     else

> +             period = period_accel;

> +

> +     st->fifo.period = period;

> +}

> +

> +int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,

> +                                 unsigned int fifo_en)

> +{

> +     unsigned int mask, val;

> +     int ret;

> +

> +     /* update only FIFO EN bits */

> +     mask = INV_ICM42600_FIFO_CONFIG1_TMST_FSYNC_EN |

> +             INV_ICM42600_FIFO_CONFIG1_TEMP_EN |

> +             INV_ICM42600_FIFO_CONFIG1_GYRO_EN |

> +             INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;

> +

> +     val = 0;

> +     if (fifo_en & INV_ICM42600_SENSOR_GYRO)

> +             val |= INV_ICM42600_FIFO_CONFIG1_GYRO_EN;

> +     if (fifo_en & INV_ICM42600_SENSOR_ACCEL)

> +             val |= INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;

> +     if (fifo_en & INV_ICM42600_SENSOR_TEMP)

> +             val |= INV_ICM42600_FIFO_CONFIG1_TEMP_EN;

> +

> +     ret = regmap_update_bits(st->map, INV_ICM42600_REG_FIFO_CONFIG1,

> +                              mask, val);

> +     if (ret)

> +             return ret;

> +

> +     st->fifo.en = fifo_en;

> +     inv_icm42600_buffer_update_fifo_period(st);

> +

> +     return 0;

> +}

> +

> +static size_t inv_icm42600_get_packet_size(unsigned int fifo_en)

> +{

> +     size_t packet_size;

> +

> +     if ((fifo_en & INV_ICM42600_SENSOR_GYRO) &&

> +                     (fifo_en & INV_ICM42600_SENSOR_ACCEL))

> +             packet_size = INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;

> +     else

> +             packet_size = INV_ICM42600_FIFO_1SENSOR_PACKET_SIZE;

> +

> +     return packet_size;

> +}

> +

> +static unsigned int inv_icm42600_wm_truncate(unsigned int watermark,

> +                                          size_t packet_size)

> +{

> +     size_t wm_size;

> +     unsigned int wm;

> +

> +     wm_size = watermark * packet_size;

> +     if (wm_size > INV_ICM42600_FIFO_WATERMARK_MAX)

> +             wm_size = INV_ICM42600_FIFO_WATERMARK_MAX;

> +

> +     wm = wm_size / packet_size;

> +

> +     return wm;

> +}

> +

> +int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)

> +{

> +     size_t packet_size, wm_size;

> +     unsigned int wm_gyro, wm_accel, watermark;

> +     uint32_t period_gyro, period_accel, period;

> +     int64_t latency_gyro, latency_accel, latency;

> +     unsigned int mask, val;

> +     bool restore;

> +     __le16 raw_wm;

> +     int ret;

> +

> +     packet_size = inv_icm42600_get_packet_size(st->fifo.en);

> +

> +     /* get minimal latency, depending on sensor watermark and odr */

> +     wm_gyro = inv_icm42600_wm_truncate(st->fifo.watermark.gyro,

> +                                        packet_size);

> +     wm_accel = inv_icm42600_wm_truncate(st->fifo.watermark.accel,

> +                                         packet_size);

> +     period_gyro = inv_icm42600_odr_to_period(st->conf.gyro.odr);

> +     period_accel = inv_icm42600_odr_to_period(st->conf.accel.odr);

> +     latency_gyro = (int64_t)period_gyro * (int64_t)wm_gyro;

> +     latency_accel = (int64_t)period_accel * (int64_t)wm_accel;

> +     if (latency_gyro == 0) {

> +             latency = latency_accel;

> +             watermark = wm_accel;

> +     } else if (latency_accel == 0) {

> +             latency = latency_gyro;

> +             watermark = wm_gyro;

> +     } else {

> +             /* compute the smallest latency that is a multiple of both */

> +             if (latency_gyro <= latency_accel) {

> +                     latency = latency_gyro;

> +                     latency -= latency_accel % latency_gyro;

> +             } else {

> +                     latency = latency_accel;

> +                     latency -= latency_gyro % latency_accel;

> +             }

> +             /* use the shortest period */

> +             if (period_gyro <= period_accel)

> +                     period = period_gyro;

> +             else

> +                     period = period_accel;

> +             /* all this works because periods are multiple of each others */

> +             watermark = div_s64(latency, period);

> +             if (watermark < 1)

> +                     watermark = 1;

> +     }

> +     wm_size = watermark * packet_size;

> +     dev_dbg(regmap_get_device(st->map), "watermark: %u (%zu)\n",

> +             watermark, wm_size);

> +

> +     /* changing FIFO watermark requires to turn off watermark interrupt */

> +     mask = INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN;

> +     val = 0;

> +     ret = regmap_update_bits_check(st->map, INV_ICM42600_REG_INT_SOURCE0,

> +                                    mask, val, &restore);

> +     if (ret)

> +             return ret;

> +

> +     raw_wm = INV_ICM42600_FIFO_WATERMARK_VAL(wm_size);

> +     ret = regmap_bulk_write(st->map, INV_ICM42600_REG_FIFO_WATERMARK,

> +                             &raw_wm, sizeof(raw_wm));

> +     if (ret)

> +             return ret;

> +

> +     /* restore watermark interrupt */

> +     if (restore) {

> +             mask = INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN;

> +             val = INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN;

> +             ret = regmap_update_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,

> +                                      mask, val);

> +             if (ret)

> +                     return ret;

> +     }

> +

> +     return 0;

> +}

> +

> +static int inv_icm42600_buffer_preenable(struct iio_dev *indio_dev)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     struct device *dev = regmap_get_device(st->map);

> +

> +     pm_runtime_get_sync(dev);

> +

> +     return 0;

> +}

> +

> +static int inv_icm42600_buffer_postdisable(struct iio_dev *indio_dev)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     struct device *dev = regmap_get_device(st->map);

> +     unsigned int sensor;

> +     unsigned int *watermark;

> +     struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;

> +     unsigned int sleep = 0;

> +     int ret;

> +

> +     if (indio_dev == st->indio_gyro) {

> +             sensor = INV_ICM42600_SENSOR_GYRO;

> +             watermark = &st->fifo.watermark.gyro;

> +     } else if (indio_dev == st->indio_accel) {

> +             sensor = INV_ICM42600_SENSOR_ACCEL;

> +             watermark = &st->fifo.watermark.accel;

> +     } else {

> +             return -EINVAL;

> +     }

> +

> +     mutex_lock(&st->lock);

> +

> +     ret = inv_icm42600_buffer_set_fifo_en(st, st->fifo.en & ~sensor);

> +     if (ret)

> +             goto out_unlock;

> +

> +     *watermark = 0;

> +     ret = inv_icm42600_buffer_update_watermark(st);

> +     if (ret)

> +             goto out_unlock;

> +

> +     conf.mode = INV_ICM42600_SENSOR_MODE_OFF;

> +     if (sensor == INV_ICM42600_SENSOR_GYRO)

> +             ret = inv_icm42600_set_gyro_conf(st, &conf, &sleep);

> +     else

> +             ret = inv_icm42600_set_accel_conf(st, &conf, &sleep);

> +

> +out_unlock:

> +     mutex_unlock(&st->lock);

> +     if (sleep)

> +             msleep(sleep);

> +     pm_runtime_mark_last_busy(dev);

> +     pm_runtime_put_autosuspend(dev);

> +

> +     return ret;

> +}

> +

> +const struct iio_buffer_setup_ops inv_icm42600_buffer_ops = {

> +     .preenable = inv_icm42600_buffer_preenable,

> +     .postenable = iio_triggered_buffer_postenable,

> +     .predisable = iio_triggered_buffer_predisable,

> +     .postdisable = inv_icm42600_buffer_postdisable,

> +};

> +

> +int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,

> +                               unsigned int max)

> +{

> +     struct device *dev = regmap_get_device(st->map);

> +     __be16 raw_fifo_count;

> +     size_t max_count;

> +     ssize_t i, size;

> +     const void *accel, *gyro, *temp, *timestamp;

> +     unsigned int odr;

> +     int ret;

> +

> +     /* reset all samples counters */

> +     st->fifo.count = 0;

> +     st->fifo.nb.gyro = 0;

> +     st->fifo.nb.accel = 0;

> +     st->fifo.nb.total = 0;

> +

> +     /* compute maximum FIFO read size */

> +     if (max == 0)

> +             max_count = sizeof(st->fifo.data);

> +     else

> +             max_count = max * inv_icm42600_get_packet_size(st->fifo.en);

> +

> +     /* read FIFO count value */

> +     ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,

> +                            &raw_fifo_count, sizeof(raw_fifo_count));

> +     if (ret)

> +             return ret;

> +     st->fifo.count = be16_to_cpu(raw_fifo_count);

> +     dev_dbg(dev, "FIFO count = %zu\n", st->fifo.count);

> +

> +     /* check and sanitize FIFO count value */

> +     if (st->fifo.count == 0)

> +             return 0;

> +     if (st->fifo.count > max_count)

> +             st->fifo.count = max_count;

> +

> +     /* read all FIFO data in internal buffer */

> +     ret = regmap_noinc_read(st->map, INV_ICM42600_REG_FIFO_DATA,

> +                             st->fifo.data, st->fifo.count);

> +     if (ret)

> +             return ret;

> +

> +     /* compute number of samples for each sensor */

> +     for (i = 0; i < st->fifo.count; i += size) {

> +             size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],

> +                             &accel, &gyro, &temp, &timestamp, &odr);

> +             if (size <= 0)

> +                     break;

> +             if (gyro != NULL && inv_icm42600_fifo_is_data_valid(gyro))

> +                     st->fifo.nb.gyro++;

> +             if (accel != NULL && inv_icm42600_fifo_is_data_valid(accel))

> +                     st->fifo.nb.accel++;

> +             st->fifo.nb.total++;

> +     }

> +

> +     return 0;

> +}

> +

> +int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,

> +                                  unsigned int count)

> +{

> +     int64_t ts_gyro, ts_accel;

> +     int ret;

> +

> +     dev_dbg(regmap_get_device(st->map), "FIFO flush %u\n", count);

> +

> +     ts_gyro = iio_get_time_ns(st->indio_gyro);

> +     ts_accel = iio_get_time_ns(st->indio_accel);

> +     ret = inv_icm42600_buffer_fifo_read(st, count);

> +     if (ret)

> +             return ret;

> +

> +     if (st->fifo.nb.total == 0)

> +             return 0;

> +

> +     ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro, ts_gyro);

> +     if (ret)

> +             return ret;

> +

> +     return inv_icm42600_accel_parse_fifo(st->indio_accel, ts_accel);

> +}

> +

> +int inv_icm42600_buffer_init(struct inv_icm42600_state *st)

> +{

> +     unsigned int mask, val;

> +     int ret;

> +

> +     /*

> +      * Default FIFO configuration (bits 7 to 5)

> +      * - use invalid value

> +      * - FIFO count in bytes

> +      * - FIFO count in big endian

> +      */

> +     mask = GENMASK(7, 5);

> +     val = INV_ICM42600_INTF_CONFIG0_FIFO_COUNT_ENDIAN;

> +     ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG0,

> +                              mask, val);

> +     if (ret)

> +             return ret;

> +

> +     /*

> +      * Enable FIFO partial read and continuous watermark interrupt.

> +      * Disable all FIFO EN bits.

> +      */

> +     mask = GENMASK(6, 5) | GENMASK(3, 0);

> +     val = INV_ICM42600_FIFO_CONFIG1_RESUME_PARTIAL_RD |

> +           INV_ICM42600_FIFO_CONFIG1_WM_GT_TH;

> +     return regmap_update_bits(st->map, INV_ICM42600_REG_FIFO_CONFIG1,

> +                               mask, val);

> +}

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h

> new file mode 100644

> index 000000000000..74b91c0e664b

> --- /dev/null

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h

> @@ -0,0 +1,162 @@

> +/* SPDX-License-Identifier: GPL-2.0-or-later */

> +/*

> + * Copyright (C) 2020 Invensense, Inc.

> + */

> +

> +#ifndef INV_ICM42600_BUFFER_H_

> +#define INV_ICM42600_BUFFER_H_

> +

> +#include <linux/kernel.h>

> +#include <linux/bits.h>

> +

> +struct inv_icm42600_state;

> +

> +#define INV_ICM42600_SENSOR_GYRO     BIT(0)

> +#define INV_ICM42600_SENSOR_ACCEL    BIT(1)

> +#define INV_ICM42600_SENSOR_TEMP     BIT(2)

> +

> +struct inv_icm42600_fifo {

> +     unsigned int en;

> +     uint32_t period;

> +     struct {

> +             unsigned int gyro;

> +             unsigned int accel;

> +     } watermark;

> +     size_t count;

> +     struct {

> +             size_t gyro;

> +             size_t accel;

> +             size_t total;

> +     } nb;

> +     uint8_t data[2080];

> +};

> +

> +/* FIFO header: 1 byte */

> +#define INV_ICM42600_FIFO_HEADER_MSG         BIT(7)

> +#define INV_ICM42600_FIFO_HEADER_ACCEL               BIT(6)

> +#define INV_ICM42600_FIFO_HEADER_GYRO                BIT(5)

> +#define INV_ICM42600_FIFO_HEADER_TMST_FSYNC  GENMASK(3, 2)

> +#define INV_ICM42600_FIFO_HEADER_ODR_ACCEL   BIT(1)

> +#define INV_ICM42600_FIFO_HEADER_ODR_GYRO    BIT(0)

> +

> +/* FIFO data packet */

> +struct inv_icm42600_fifo_sensor_data {

> +     __be16 x;

> +     __be16 y;

> +     __be16 z;

> +} __packed;

> +#define INV_ICM42600_FIFO_DATA_INVALID               -32768

> +

> +struct inv_icm42600_fifo_1sensor_packet {

> +     uint8_t header;

> +     struct inv_icm42600_fifo_sensor_data data;

> +     int8_t temp;

> +} __packed;

> +#define INV_ICM42600_FIFO_1SENSOR_PACKET_SIZE                8

> +

> +struct inv_icm42600_fifo_2sensors_packet {

> +     uint8_t header;

> +     struct inv_icm42600_fifo_sensor_data accel;

> +     struct inv_icm42600_fifo_sensor_data gyro;

> +     int8_t temp;

> +     __be16 timestamp;

> +} __packed;

> +#define INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE               16

> +

> +static inline int16_t inv_icm42600_fifo_get_sensor_data(__be16 d)

> +{

> +     return be16_to_cpu(d);

> +}

> +

> +static inline bool

> +inv_icm42600_fifo_is_data_valid(const struct inv_icm42600_fifo_sensor_data *s)

> +{

> +     int16_t x, y, z;

> +

> +     x = inv_icm42600_fifo_get_sensor_data(s->x);

> +     y = inv_icm42600_fifo_get_sensor_data(s->y);

> +     z = inv_icm42600_fifo_get_sensor_data(s->z);

> +

> +     if (x == INV_ICM42600_FIFO_DATA_INVALID &&

> +                     y == INV_ICM42600_FIFO_DATA_INVALID &&

> +                     z == INV_ICM42600_FIFO_DATA_INVALID)

> +             return false;

> +

> +     return true;

> +}

> +

> +static inline ssize_t inv_icm42600_fifo_decode_packet(const void *packet,



Bit big to be in the header..



> +             const void **accel, const void **gyro, const void **temp,

> +             const void **timestamp, unsigned int *odr)

> +{

> +     const struct inv_icm42600_fifo_1sensor_packet *pack1 = packet;

> +     const struct inv_icm42600_fifo_2sensors_packet *pack2 = packet;

> +     uint8_t header = *((const uint8_t *)packet);

> +

> +     /* FIFO empty */

> +     if (header & INV_ICM42600_FIFO_HEADER_MSG) {

> +             *accel = NULL;

> +             *gyro = NULL;

> +             *temp = NULL;

> +             *timestamp = NULL;

> +             *odr = 0;

> +             return 0;

> +     }

> +

> +     /* handle odr flags */

> +     *odr = 0;

> +     if (header & INV_ICM42600_FIFO_HEADER_ODR_GYRO)

> +             *odr |= INV_ICM42600_SENSOR_GYRO;

> +     if (header & INV_ICM42600_FIFO_HEADER_ODR_ACCEL)

> +             *odr |= INV_ICM42600_SENSOR_ACCEL;

> +

> +     /* accel + gyro */

> +     if ((header & INV_ICM42600_FIFO_HEADER_ACCEL) &&

> +                     (header & INV_ICM42600_FIFO_HEADER_GYRO)) {

> +             *accel = &pack2->accel;

> +             *gyro = &pack2->gyro;

> +             *temp = &pack2->temp;

> +             *timestamp = &pack2->timestamp;

> +             return INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;

> +     }

> +

> +     /* accel only */

> +     if (header & INV_ICM42600_FIFO_HEADER_ACCEL) {

> +             *accel = &pack1->data;

> +             *gyro = NULL;

> +             *temp = &pack1->temp;

> +             *timestamp = NULL;

> +             return INV_ICM42600_FIFO_1SENSOR_PACKET_SIZE;

> +     }

> +

> +     /* gyro only */

> +     if (header & INV_ICM42600_FIFO_HEADER_GYRO) {

> +             *accel = NULL;

> +             *gyro = &pack1->data;

> +             *temp = &pack1->temp;

> +             *timestamp = NULL;

> +             return INV_ICM42600_FIFO_1SENSOR_PACKET_SIZE;

> +     }

> +

> +     /* invalid packet if here */

> +     return -EINVAL;

> +}

> +

> +extern const struct iio_buffer_setup_ops inv_icm42600_buffer_ops;

> +

> +int inv_icm42600_buffer_init(struct inv_icm42600_state *st);

> +

> +void inv_icm42600_buffer_update_fifo_period(struct inv_icm42600_state *st);

> +

> +int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,

> +                                 unsigned int fifo_en);

> +

> +int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st);

> +

> +int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,

> +                               unsigned int max);

> +

> +int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,

> +                                  unsigned int count);

> +

> +#endif

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> index 1102c54396e3..689089065ff9 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> @@ -14,6 +14,7 @@

>  #include <linux/iio/iio.h>


>  #include "inv_icm42600.h"

> +#include "inv_icm42600_buffer.h"


>  static const struct regmap_range_cfg inv_icm42600_regmap_ranges[] = {

>        {

> @@ -515,6 +516,11 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,

>        if (ret)

>                return ret;


> +     /* setup FIFO buffer */

> +     ret = inv_icm42600_buffer_init(st);

> +     if (ret)

> +             return ret;

> +

>        /* setup interrupt trigger */

>        ret = inv_icm42600_trigger_init(st, irq, irq_type);

>        if (ret)

> @@ -559,6 +565,16 @@ static int __maybe_unused inv_icm42600_suspend(struct device *dev)

>                goto out_unlock;

>        }


> +     /* disable FIFO data streaming */

> +     if (iio_buffer_enabled(st->indio_gyro) ||

> +                     iio_buffer_enabled(st->indio_accel)) {

> +             /* set FIFO in bypass mode */

> +             ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,

> +                                INV_ICM42600_FIFO_CONFIG_BYPASS);

> +             if (ret)

> +                     goto out_unlock;

> +     }

> +

>        ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,

>                                         INV_ICM42600_SENSOR_MODE_OFF, false,

>                                         NULL);

> @@ -594,6 +610,13 @@ static int __maybe_unused inv_icm42600_resume(struct device *dev)

>        if (ret)

>                goto out_unlock;


> +     /* restore FIFO data streaming */

> +     if (iio_buffer_enabled(st->indio_gyro) ||

> +                     iio_buffer_enabled(st->indio_accel)) {

> +             ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,

> +                                INV_ICM42600_FIFO_CONFIG_STREAM);

> +     }

> +

>  out_unlock:

>        mutex_unlock(&st->lock);

>        return ret;

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> index c0164ab2830e..dafb104abc77 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> @@ -10,9 +10,13 @@

>  #include <linux/regmap.h>

>  #include <linux/delay.h>

>  #include <linux/iio/iio.h>

> +#include <linux/iio/buffer.h>

> +#include <linux/iio/triggered_buffer.h>

> +#include <linux/iio/trigger_consumer.h>


>  #include "inv_icm42600.h"

>  #include "inv_icm42600_temp.h"

> +#include "inv_icm42600_buffer.h"


>  #define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info)         \

>        {                                                               \

> @@ -46,6 +50,7 @@ enum inv_icm42600_gyro_scan {

>        INV_ICM42600_GYRO_SCAN_Y,

>        INV_ICM42600_GYRO_SCAN_Z,

>        INV_ICM42600_GYRO_SCAN_TEMP,

> +     INV_ICM42600_GYRO_SCAN_TIMESTAMP,

>  };


>  static const struct iio_chan_spec_ext_info inv_icm42600_gyro_ext_infos[] = {

> @@ -61,8 +66,100 @@ static const struct iio_chan_spec inv_icm42600_gyro_channels[] = {

>        INV_ICM42600_GYRO_CHAN(IIO_MOD_Z, INV_ICM42600_GYRO_SCAN_Z,

>                               inv_icm42600_gyro_ext_infos),

>        INV_ICM42600_TEMP_CHAN(INV_ICM42600_GYRO_SCAN_TEMP),

> +     IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_GYRO_SCAN_TIMESTAMP),

>  };


> +/* IIO buffer data */

> +struct inv_icm42600_gyro_buffer {

> +     struct inv_icm42600_fifo_sensor_data gyro;

> +     int8_t temp;

> +     int64_t timestamp;

> +};

> +

> +#define INV_ICM42600_SCAN_MASK_GYRO_3AXIS                            \

> +     (BIT(INV_ICM42600_GYRO_SCAN_X) |                                \

> +     BIT(INV_ICM42600_GYRO_SCAN_Y) |                                 \

> +     BIT(INV_ICM42600_GYRO_SCAN_Z))

> +

> +#define INV_ICM42600_SCAN_MASK_TEMP  BIT(INV_ICM42600_GYRO_SCAN_TEMP)

> +

> +static const unsigned long inv_icm42600_gyro_scan_masks[] = {

> +     /* 3-axis gyro + temperature */

> +     INV_ICM42600_SCAN_MASK_GYRO_3AXIS | INV_ICM42600_SCAN_MASK_TEMP,

> +     0,

> +};

> +

> +static irqreturn_t inv_icm42600_gyro_handler(int irq, void *_data)

> +{

> +     struct iio_poll_func *pf = _data;

> +     struct iio_dev *indio_dev = pf->indio_dev;

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     const size_t fifo_nb = st->fifo.nb.total;

> +     int ret;

> +

> +     /* exit if no sample */

> +     if (fifo_nb == 0)

> +             goto out;

> +

> +     ret = inv_icm42600_gyro_parse_fifo(indio_dev, pf->timestamp);

> +     if (ret)

> +             dev_err(regmap_get_device(st->map), "gyro fifo error %d\n",

> +                     ret);

> +

> +out:

> +     iio_trigger_notify_done(indio_dev->trig);

> +     return IRQ_HANDLED;

> +}

> +

> +/* enable gyroscope sensor and FIFO write */

> +static int inv_icm42600_gyro_update_scan_mode(struct iio_dev *indio_dev,

> +                                           const unsigned long *scan_mask)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;

> +     unsigned int fifo_en = 0;

> +     unsigned int sleep_gyro = 0;

> +     unsigned int sleep_temp = 0;

> +     unsigned int sleep;

> +     int ret;

> +

> +     mutex_lock(&st->lock);

> +

> +     if (*scan_mask & INV_ICM42600_SCAN_MASK_TEMP) {

> +             /* enable temp sensor */

> +             ret = inv_icm42600_set_temp_conf(st, true, &sleep_temp);

> +             if (ret)

> +                     goto out_unlock;

> +             fifo_en |= INV_ICM42600_SENSOR_TEMP;

> +     }

> +

> +     if (*scan_mask & INV_ICM42600_SCAN_MASK_GYRO_3AXIS) {

> +             /* enable gyro sensor */

> +             conf.mode = INV_ICM42600_SENSOR_MODE_LOW_NOISE;

> +             ret = inv_icm42600_set_gyro_conf(st, &conf, &sleep_gyro);

> +             if (ret)

> +                     goto out_unlock;

> +             fifo_en |= INV_ICM42600_SENSOR_GYRO;

> +     }

> +

> +     /* update data FIFO write and FIFO watermark */

> +     ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);

> +     if (ret)

> +             goto out_unlock;



blank line



> +     ret = inv_icm42600_buffer_update_watermark(st);

> +

> +out_unlock:

> +     mutex_unlock(&st->lock);

> +     /* sleep maximum required time */

> +     if (sleep_gyro > sleep_temp)

> +             sleep = sleep_gyro;

> +     else

> +             sleep = sleep_temp;

> +     if (sleep)

> +             msleep(sleep);



        if (sleep_gyro > sleep_temp)

                msleep(sleep_gyro);

        else

                msleep(sleep_temp);



> +     return ret;

> +}

> +

>  static int inv_icm42600_gyro_read_sensor(struct inv_icm42600_state *st,

>                                         struct iio_chan_spec const *chan,

>                                         int16_t *val)

> @@ -262,6 +359,8 @@ static int inv_icm42600_gyro_write_odr(struct inv_icm42600_state *st,

>        mutex_lock(&st->lock);

>        conf.odr = inv_icm42600_gyro_odr_conv[idx / 2];

>        ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);

> +     inv_icm42600_buffer_update_fifo_period(st);

> +     inv_icm42600_buffer_update_watermark(st);

>        mutex_unlock(&st->lock);


>        pm_runtime_mark_last_busy(dev);

> @@ -524,12 +623,51 @@ static int inv_icm42600_gyro_write_raw_get_fmt(struct iio_dev *indio_dev,

>        }

>  }


> +static int inv_icm42600_gyro_hwfifo_set_watermark(struct iio_dev *indio_dev,

> +                                               unsigned int val)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     int ret;

> +

> +     mutex_lock(&st->lock);

> +

> +     st->fifo.watermark.gyro = val;

> +     ret = inv_icm42600_buffer_update_watermark(st);

> +

> +     mutex_unlock(&st->lock);

> +

> +     return ret;

> +}

> +

> +static int inv_icm42600_gyro_hwfifo_flush(struct iio_dev *indio_dev,

> +                                       unsigned int count)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     int ret;

> +

> +     if (count == 0)

> +             return 0;

> +

> +     mutex_lock(&st->lock);

> +

> +     ret = inv_icm42600_buffer_hwfifo_flush(st, count);

> +     if (!ret)

> +             ret = st->fifo.nb.gyro;

> +

> +     mutex_unlock(&st->lock);

> +

> +     return ret;

> +}

> +

>  static const struct iio_info inv_icm42600_gyro_info = {

>        .read_raw = inv_icm42600_gyro_read_raw,

>        .read_avail = inv_icm42600_gyro_read_avail,

>        .write_raw = inv_icm42600_gyro_write_raw,

>        .write_raw_get_fmt = inv_icm42600_gyro_write_raw_get_fmt,

>        .debugfs_reg_access = inv_icm42600_debugfs_reg,

> +     .update_scan_mode = inv_icm42600_gyro_update_scan_mode,

> +     .hwfifo_set_watermark = inv_icm42600_gyro_hwfifo_set_watermark,

> +     .hwfifo_flush_to_buffer = inv_icm42600_gyro_hwfifo_flush,

>  };


>  int inv_icm42600_gyro_init(struct inv_icm42600_state *st)

> @@ -537,6 +675,7 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st)

>        struct device *dev = regmap_get_device(st->map);

>        const char *name;

>        struct iio_dev *indio_dev;

> +     int ret;


>        name = devm_kasprintf(dev, GFP_KERNEL, "%s-gyro", st->name);

>        if (!name)

> @@ -553,7 +692,51 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st)

>        indio_dev->modes = INDIO_DIRECT_MODE;

>        indio_dev->channels = inv_icm42600_gyro_channels;

>        indio_dev->num_channels = ARRAY_SIZE(inv_icm42600_gyro_channels);

> +     indio_dev->available_scan_masks = inv_icm42600_gyro_scan_masks;

> +

> +     ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,

> +                                           inv_icm42600_gyro_handler,

> +                                           &inv_icm42600_buffer_ops);

> +     if (ret)

> +             return ret;

> +

> +     indio_dev->trig = iio_trigger_get(st->trigger);


>        st->indio_gyro = indio_dev;

>        return devm_iio_device_register(dev, st->indio_gyro);

>  }

> +

> +int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev, int64_t ts)

> +{

> +     struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

> +     const size_t gyro_nb = st->fifo.nb.gyro;

> +     ssize_t i, size;

> +     const void *accel, *gyro, *temp, *timestamp;

> +     unsigned int odr;

> +     struct inv_icm42600_gyro_buffer buffer;

> +

> +     /* exit if no gyro sample */

> +     if (gyro_nb == 0)

> +             return 0;

> +

> +     /* parse all fifo packets */

> +     for (i = 0; i < st->fifo.count; i += size) {

> +             size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],

> +                             &accel, &gyro, &temp, &timestamp, &odr);

> +             dev_dbg(regmap_get_device(st->map), "gyro packet size = %zd\n",

> +                     size);

> +             /* quit if error or FIFO is empty */

> +             if (size <= 0)

> +                     return size;



blank line here.



> +             /* skip packet if no gyro data or data is invalid */

> +             if (gyro == NULL || !inv_icm42600_fifo_is_data_valid(gyro)) {

> +                     dev_dbg(regmap_get_device(st->map), "skip gyro data\n");



Very noisy logging. I'd drop it for the final version of the driver.



> +                     continue;

> +             }

> +             memcpy(&buffer.gyro, gyro, sizeof(buffer.gyro));

> +             memcpy(&buffer.temp, temp, sizeof(buffer.temp));



                buffer.temp = temp;



> +             iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts);

> +     }

> +

> +     return 0;

> +}

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_trigger.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_trigger.c

> index 7a5e76305f0b..5667e0204722 100644

> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_trigger.c

> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_trigger.c

> @@ -13,6 +13,7 @@

>  #include <linux/iio/trigger_consumer.h>


>  #include "inv_icm42600.h"

> +#include "inv_icm42600_buffer.h"


>  static irqreturn_t inv_icm42600_trigger_timestamp(int irq, void *_data)

>  {

> @@ -45,8 +46,18 @@ static irqreturn_t inv_icm42600_trigger_int_handler(int irq, void *_data)

>                dev_warn(dev, "FIFO full data lost!\n");


>        /* FIFO threshold reached */

> -     if (status & INV_ICM42600_INT_STATUS_FIFO_THS)

> -             iio_trigger_poll_chained(st->trigger);

> +     if (status & INV_ICM42600_INT_STATUS_FIFO_THS) {

> +             ret = inv_icm42600_buffer_fifo_read(st, 0);

> +             if (ret)

> +                     dev_err(dev, "FIFO read error %d\n", ret);

> +     } else {

> +             st->fifo.count = 0;

> +             st->fifo.nb.gyro = 0;

> +             st->fifo.nb.accel = 0;

> +             st->fifo.nb.total = 0;

> +     }

> +

> +     iio_trigger_poll_chained(st->trigger);


>  out_unlock:

>        mutex_unlock(&st->lock);






^ permalink raw reply

* Re: [PATCH 1/2] drivers: thermal: tsens: Add 0C (zeorC) interrupt support
From: Amit Kucheria @ 2020-05-18 15:33 UTC (permalink / raw)
  To: manafm
  Cc: Andy Gross, Bjorn Andersson, Zhang Rui, Daniel Lezcano,
	Rob Herring, linux-arm-msm, Linux PM list, DTML,
	Linux Kernel Mailing List
In-Reply-To: <bbc9eef884f3b83191b582a80d3dd5d3@codeaurora.org>

On Sun, May 17, 2020 at 3:58 PM <manafm@codeaurora.org> wrote:
>
> On 2020-05-05 17:39, Amit Kucheria wrote:
> > Hi Manaf,
> >
> > Typo: fix zeorC in subject line.
> Done
> >
> > Please rebase this patch[1] on top of my patch merging tsens-common.c
> > and tsens.c.
> >
> > [1]
> > https://lore.kernel.org/linux-arm-msm/e30e2ba6fa5c007983afd4d7d4e0311c0b57917a.1588183879.git.amit.kucheria@linaro.org/
> Done
> >
> > On Tue, May 5, 2020 at 4:42 PM Manaf Meethalavalappu Pallikunhi
> > <manafm@codeaurora.org> wrote:
> >>
> >> TSENS IP v2.6+ adds 0C interrupt support. It triggers set
> >> interrupt when aggregated minimum temperature of all TSENS falls
> >> below 0C preset threshold and triggers reset interrupt when aggregate
> >> minimum temperature of all TSENS crosses above reset threshold.
> >> Add support for this interrupt in the driver.
> >>
> >> It adds another sensor to the of-thermal along with all individual
> >> TSENS. It enables to add any mitigation for 0C interrupt.
> >>
> >> Signed-off-by: Manaf Meethalavalappu Pallikunhi
> >> <manafm@codeaurora.org>
> >> ---
> >>  drivers/thermal/qcom/tsens-common.c | 72
> >> ++++++++++++++++++++++++++++-
> >>  drivers/thermal/qcom/tsens-v2.c     |  7 +++
> >>  drivers/thermal/qcom/tsens.c        | 51 ++++++++++++++++++--
> >>  drivers/thermal/qcom/tsens.h        | 11 +++++
> >>  4 files changed, 135 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/thermal/qcom/tsens-common.c
> >> b/drivers/thermal/qcom/tsens-common.c
> >> index 172545366636..44e7edeb9a90 100644
> >> --- a/drivers/thermal/qcom/tsens-common.c
> >> +++ b/drivers/thermal/qcom/tsens-common.c
> >> @@ -198,7 +198,8 @@ static void tsens_set_interrupt_v1(struct
> >> tsens_priv *priv, u32 hw_id,
> >>                 index = LOW_INT_CLEAR_0 + hw_id;
> >>                 break;
> >>         case CRITICAL:
> >> -               /* No critical interrupts before v2 */
> >> +       case ZEROC:
> >> +               /* No critical and 0c interrupts before v2 */
> >>                 return;
> >>         }
> >>         regmap_field_write(priv->rf[index], enable ? 0 : 1);
> >> @@ -229,6 +230,9 @@ static void tsens_set_interrupt_v2(struct
> >> tsens_priv *priv, u32 hw_id,
> >>                 index_mask  = CRIT_INT_MASK_0 + hw_id;
> >>                 index_clear = CRIT_INT_CLEAR_0 + hw_id;
> >>                 break;
> >> +       case ZEROC:
> >> +               /* Nothing to handle for 0c interrupt */
> >> +               return;
> >>         }
> >>
> >>         if (enable) {
> >> @@ -360,6 +364,34 @@ static inline u32 masked_irq(u32 hw_id, u32 mask,
> >> enum tsens_ver ver)
> >>         return 0;
> >>  }
> >>
> >> +/**
> >> + * tsens_0c_irq_thread - Threaded interrupt handler for 0c interrupt
> >
> > Let's use zeroc instead of 0c in the function and variable names and
> > comments everywhere. Easier to grep and better consistency too.
> Done
> >
> >> + * @irq: irq number
> >> + * @data: tsens controller private data
> >> + *
> >> + * Whenever interrupt triggers notify thermal framework using
> >> + * thermal_zone_device_update() to update cold temperature
> >> mitigation.
> >
> > How is this mitigation updated?
> Updated comment section
> >> + *
> >> + * Return: IRQ_HANDLED
> >> + */
> >> +irqreturn_t tsens_0c_irq_thread(int irq, void *data)
> >> +{
> >> +       struct tsens_priv *priv = data;
> >> +       struct tsens_sensor *s = &priv->sensor[priv->num_sensors];
> >> +       int temp, ret;
> >> +
> >> +       ret = regmap_field_read(priv->rf[TSENS_0C_STATUS], &temp);


> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       dev_dbg(priv->dev, "[%u] %s: 0c interrupt is %s\n",
> >> +               s->hw_id, __func__, temp ? "triggered" : "cleared");
> >
> > So triggered is printed for non-zero (including negative) values?
> This zeroc hardware generates each interrupt when any of the TSENS that
> it monitors goes below 5C or above 10c. These thresholds are not
> configurable. Hence we don't expect this to be changed from kernel side.
> So this sensor (status register) will read 0 or 1.  1 means soc
> temperature is in cold condition and 0 means it is in normal
> temperature.

All this information belongs in the function description and the part
about the status register returning 0 (for temp > 10) and 1 (for temp
<=5) belongs in the patch description too. Please add it to v3.

What happens at 7 degrees? Will the HW continue returning 1 due to
some hysteresis?

I'll review v2 in a bit.

> >
> >> +
> >> +       thermal_zone_device_update(s->tzd, THERMAL_EVENT_UNSPECIFIED);
> >> +
> >> +       return IRQ_HANDLED;
> >> +}
> >> +
> >>  /**
> >>   * tsens_critical_irq_thread() - Threaded handler for critical
> >> interrupts
> >>   * @irq: irq number
> >> @@ -566,6 +598,20 @@ void tsens_disable_irq(struct tsens_priv *priv)
> >>         regmap_field_write(priv->rf[INT_EN], 0);
> >>  }
> >>
> >> +int tsens_get_0c_int_status(const struct tsens_sensor *s, int *temp)
> >> +{
> >> +       struct tsens_priv *priv = s->priv;
> >> +       int last_temp = 0, ret;
> >> +
> >> +       ret = regmap_field_read(priv->rf[TSENS_0C_STATUS],
> >> &last_temp);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       *temp = last_temp;
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>  int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp)
> >>  {
> >>         struct tsens_priv *priv = s->priv;
> >> @@ -833,6 +879,30 @@ int __init init_common(struct tsens_priv *priv)
> >>                 regmap_field_write(priv->rf[CC_MON_MASK], 1);
> >>         }
> >>
> >> +       if (tsens_version(priv) > VER_1_X &&  ver_minor > 5) {
> >> +               /* 0C interrupt is present only on v2.6+ */
> >> +               priv->rf[TSENS_0C_INT_EN] =
> >> devm_regmap_field_alloc(dev,
> >> +                                               priv->srot_map,
> >> +
> >> priv->fields[TSENS_0C_INT_EN]);
> >> +               if (IS_ERR(priv->rf[TSENS_0C_INT_EN])) {
> >> +                       ret = PTR_ERR(priv->rf[TSENS_0C_INT_EN]);
> >> +                       goto err_put_device;
> >> +               }
> >> +
> >> +               /* Check whether 0C interrupt is enabled or not */
> >> +               regmap_field_read(priv->rf[TSENS_0C_INT_EN],
> >> &enabled);
> >> +               if (enabled) {
> >> +                       priv->feat->zero_c_int = 1;
> >
> > This should be done at the beginning of the block where you check our
> > version is > 2.6 since the flag only says whether the feature is
> > present.
> Done
> >
> >> +                       priv->rf[TSENS_0C_STATUS] =
> >> devm_regmap_field_alloc(dev,
> >> +                                               priv->tm_map,
> >> +
> >> priv->fields[TSENS_0C_STATUS]);
> >> +                       if (IS_ERR(priv->rf[TSENS_0C_STATUS])) {
> >> +                               ret =
> >> PTR_ERR(priv->rf[TSENS_0C_STATUS]);
> >> +                               goto err_put_device;
> >> +                       }
> >> +               }
> >> +       }
> >> +
> >>         spin_lock_init(&priv->ul_lock);
> >>         tsens_enable_irq(priv);
> >>         tsens_debug_init(op);
> >> diff --git a/drivers/thermal/qcom/tsens-v2.c
> >> b/drivers/thermal/qcom/tsens-v2.c
> >> index b293ed32174b..ce80d82c7255 100644
> >> --- a/drivers/thermal/qcom/tsens-v2.c
> >> +++ b/drivers/thermal/qcom/tsens-v2.c
> >> @@ -11,6 +11,7 @@
> >>  /* ----- SROT ------ */
> >>  #define SROT_HW_VER_OFF        0x0000
> >>  #define SROT_CTRL_OFF          0x0004
> >> +#define SROT_OC_CTRL_OFF       0x0018
> >>
> >>  /* ----- TM ------ */
> >>  #define TM_INT_EN_OFF                  0x0004
> >> @@ -23,6 +24,7 @@
> >>  #define TM_Sn_UPPER_LOWER_THRESHOLD_OFF 0x0020
> >>  #define TM_Sn_CRITICAL_THRESHOLD_OFF   0x0060
> >>  #define TM_Sn_STATUS_OFF               0x00a0
> >> +#define TM_0C_INT_STATUS_OFF           0x00e0
> >>  #define TM_TRDY_OFF                    0x00e4
> >>  #define TM_WDOG_LOG_OFF                0x013c
> >>
> >> @@ -45,6 +47,7 @@ static const struct reg_field
> >> tsens_v2_regfields[MAX_REGFIELDS] = {
> >>         /* CTRL_OFF */
> >>         [TSENS_EN]     = REG_FIELD(SROT_CTRL_OFF,    0,  0),
> >>         [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF,    1,  1),
> >> +       [TSENS_0C_INT_EN] = REG_FIELD(SROT_OC_CTRL_OFF, 0,  0),
> >>
> >>         /* ----- TM ------ */
> >>         /* INTERRUPT ENABLE */
> >> @@ -86,6 +89,9 @@ static const struct reg_field
> >> tsens_v2_regfields[MAX_REGFIELDS] = {
> >>         REG_FIELD_FOR_EACH_SENSOR16(CRITICAL_STATUS, TM_Sn_STATUS_OFF,
> >> 19,  19),
> >>         REG_FIELD_FOR_EACH_SENSOR16(MAX_STATUS,      TM_Sn_STATUS_OFF,
> >> 20,  20),
> >>
> >> +       /* 0C INETRRUPT STATUS */
> >
> > Typo: Interrupt
> >
> >> +       [TSENS_0C_STATUS] = REG_FIELD(TM_0C_INT_STATUS_OFF, 0, 0),
> >> +
> >>         /* TRDY: 1=ready, 0=in progress */
> >>         [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
> >>  };
> >> @@ -93,6 +99,7 @@ static const struct reg_field
> >> tsens_v2_regfields[MAX_REGFIELDS] = {
> >>  static const struct tsens_ops ops_generic_v2 = {
> >>         .init           = init_common,
> >>         .get_temp       = get_temp_tsens_valid,
> >> +       .get_0c_status  = tsens_get_0c_int_status,
> >>  };
> >>
> >>  struct tsens_plat_data data_tsens_v2 = {
> >> diff --git a/drivers/thermal/qcom/tsens.c
> >> b/drivers/thermal/qcom/tsens.c
> >> index 2f77d235cf73..e60870c53383 100644
> >> --- a/drivers/thermal/qcom/tsens.c
> >> +++ b/drivers/thermal/qcom/tsens.c
> >> @@ -14,6 +14,17 @@
> >>  #include <linux/thermal.h>
> >>  #include "tsens.h"
> >>
> >> +static int tsens_0c_get_temp(void *data, int *temp)
> >> +{
> >> +       struct tsens_sensor *s = data;
> >> +       struct tsens_priv *priv = s->priv;
> >> +
> >> +       if (priv->ops->get_0c_status)
> >> +               return priv->ops->get_0c_status(s, temp);
> >> +
> >> +       return -ENOTSUPP;
> >> +}
> >> +
> >>  static int tsens_get_temp(void *data, int *temp)
> >>  {
> >>         struct tsens_sensor *s = data;
> >> @@ -85,6 +96,10 @@ static const struct thermal_zone_of_device_ops
> >> tsens_of_ops = {
> >>         .set_trips = tsens_set_trips,
> >>  };
> >>
> >> +static const struct thermal_zone_of_device_ops tsens_0c_of_ops = {
> >> +       .get_temp = tsens_0c_get_temp,
> >> +};
> >> +
> >>  static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
> >>                               irq_handler_t thread_fn)
> >>  {
> >> @@ -142,6 +157,21 @@ static int tsens_register(struct tsens_priv
> >> *priv)
> >>                 ret = tsens_register_irq(priv, "critical",
> >>                                          tsens_critical_irq_thread);
> >>
> >> +       if (priv->feat->zero_c_int) {
> >> +               priv->sensor[priv->num_sensors].priv = priv;
> >> +               tzd = devm_thermal_zone_of_sensor_register(priv->dev,
> >> +
> >> priv->sensor[priv->num_sensors].hw_id,
> >> +
> >> &priv->sensor[priv->num_sensors],
> >> +                                       &tsens_0c_of_ops);
> >> +               if (IS_ERR(tzd)) {
> >> +                       ret = 0;
> >> +                       return ret;
> >> +               }
> >> +
> >> +               priv->sensor[priv->num_sensors].tzd = tzd;
> >
> > Why can't this happen in the previous loop, but increase the loop to
> > <= num_sensors? It is duplicated code.
> I think if i change  default loop logic to <= num_sensors, it will break
> other legacy targets, right ?
> My idea is to guard all changes related to zeroc under zeroc related
> feature flag.
> Again, since we cannot configure any threshold from kernel side, there
> is no set_trip ops for this sensor, so we need to call register function
> differently in compared to regular sensor
> >
> >> +               ret = tsens_register_irq(priv, "zeroc",
> >> tsens_0c_irq_thread);
> >> +       }
> >> +
> >>         return ret;
> >>  }
> >>
> >> @@ -178,11 +208,22 @@ static int tsens_probe(struct platform_device
> >> *pdev)
> >>                 return -EINVAL;
> >>         }
> >>
> >> -       priv = devm_kzalloc(dev,
> >> -                            struct_size(priv, sensor, num_sensors),
> >> -                            GFP_KERNEL);
> >> -       if (!priv)
> >> -               return -ENOMEM;
> >> +       /* Check for 0c interrupt is enabled or not */
> >> +       if (platform_get_irq_byname(pdev, "zeroc") > 0) {
> >> +               priv = devm_kzalloc(dev,
> >> +                               struct_size(priv, sensor, num_sensors
> >> + 1),
> >> +                               GFP_KERNEL);
> >
> > Instead of doing this, simply do the following,
> >
> > if (platform_get_irq_byname(pdev, "zeroc") > 0) {
> >         num_sensors++;
> >
> > The kzalloc will just work then, no?
> I just changed logic in v2.  Basically this zeroc feature is an
> optional. There is a chance that we don't need to enable in software
> even though hardware support is present.
> So I used another variable to check whether feature is enabled or not by
> checking DT interrupt configuration.

I've looked briefly at v2 and I don't like the way we play around with
num_sensors by special casing zeroc_en to allocate extra memory and
then revert it. It feels very convoluted. I'll address the rest of the
comments on v2.

> >
> >> +               if (!priv)
> >> +                       return -ENOMEM;
> >> +               /* Use Max sensor index as 0c sensor hw_id */
> >> +               priv->sensor[num_sensors].hw_id =
> >> data->feat->max_sensors;
> >> +       } else {
> >> +               priv = devm_kzalloc(dev,
> >> +                               struct_size(priv, sensor,
> >> num_sensors),
> >> +                               GFP_KERNEL);
> >> +               if (!priv)
> >> +                       return -ENOMEM;
> >> +       }
> >>
> >>         priv->dev = dev;
> >>         priv->num_sensors = num_sensors;
> >> diff --git a/drivers/thermal/qcom/tsens.h
> >> b/drivers/thermal/qcom/tsens.h
> >> index 502acf0e6828..5b53a0352b4d 100644
> >> --- a/drivers/thermal/qcom/tsens.h
> >> +++ b/drivers/thermal/qcom/tsens.h
> >> @@ -34,6 +34,7 @@ enum tsens_irq_type {
> >>         LOWER,
> >>         UPPER,
> >>         CRITICAL,
> >> +       ZEROC,
> >>  };
> >>
> >>  /**
> >> @@ -64,6 +65,7 @@ struct tsens_sensor {
> >>   * @suspend: Function to suspend the tsens device
> >>   * @resume: Function to resume the tsens device
> >>   * @get_trend: Function to get the thermal/temp trend
> >> + * @get_0c_status: Function to get the 0c interrupt status
> >>   */
> >>  struct tsens_ops {
> >>         /* mandatory callbacks */
> >> @@ -76,6 +78,7 @@ struct tsens_ops {
> >>         int (*suspend)(struct tsens_priv *priv);
> >>         int (*resume)(struct tsens_priv *priv);
> >>         int (*get_trend)(struct tsens_sensor *s, enum thermal_trend
> >> *trend);
> >> +       int (*get_0c_status)(const struct tsens_sensor *s, int *temp);
> >>  };
> >>
> >>  #define REG_FIELD_FOR_EACH_SENSOR11(_name, _offset, _startbit,
> >> _stopbit) \
> >> @@ -161,6 +164,8 @@ enum regfield_ids {
> >>         TSENS_SW_RST,
> >>         SENSOR_EN,
> >>         CODE_OR_TEMP,
> >> +       /* 0C CTRL OFFSET */
> >> +       TSENS_0C_INT_EN,
> >>
> >>         /* ----- TM ------ */
> >>         /* TRDY */
> >> @@ -485,6 +490,8 @@ enum regfield_ids {
> >>         MAX_STATUS_14,
> >>         MAX_STATUS_15,
> >>
> >> +       TSENS_0C_STATUS,        /* 0C INTERRUPT status */
> >> +
> >>         /* Keep last */
> >>         MAX_REGFIELDS
> >>  };
> >> @@ -497,6 +504,7 @@ enum regfield_ids {
> >>   * @srot_split: does the IP neatly splits the register space into
> >> SROT and TM,
> >>   *              with SROT only being available to secure boot
> >> firmware?
> >>   * @has_watchdog: does this IP support watchdog functionality?
> >> + * @zero_c_int: does this IP support 0C interrupt ?
> >>   * @max_sensors: maximum sensors supported by this version of the IP
> >>   */
> >>  struct tsens_features {
> >> @@ -505,6 +513,7 @@ struct tsens_features {
> >>         unsigned int adc:1;
> >>         unsigned int srot_split:1;
> >>         unsigned int has_watchdog:1;
> >> +       unsigned int zero_c_int:1;
> >
> > zeroc_interrupt
> Done
> >
> >>         unsigned int max_sensors;
> >>  };
> >>
> >> @@ -580,11 +589,13 @@ void compute_intercept_slope(struct tsens_priv
> >> *priv, u32 *pt1, u32 *pt2, u32 mo
> >>  int init_common(struct tsens_priv *priv);
> >>  int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp);
> >>  int get_temp_common(const struct tsens_sensor *s, int *temp);
> >> +int tsens_get_0c_int_status(const struct tsens_sensor *s, int *temp);
> >>  int tsens_enable_irq(struct tsens_priv *priv);
> >>  void tsens_disable_irq(struct tsens_priv *priv);
> >>  int tsens_set_trips(void *_sensor, int low, int high);
> >>  irqreturn_t tsens_irq_thread(int irq, void *data);
> >>  irqreturn_t tsens_critical_irq_thread(int irq, void *data);
> >> +irqreturn_t tsens_0c_irq_thread(int irq, void *data);
> >>
> >>  /* TSENS target */
> >>  extern struct tsens_plat_data data_8960;
> >> --
> >> 2.26.2

^ permalink raw reply

* Re: [PATCH 1/4] clk/soc: mediatek: mt8183: Bind clock driver from platform device
From: Chun-Kuang Hu @ 2020-05-18 15:30 UTC (permalink / raw)
  To: matthias.bgg
  Cc: Rob Herring, Matthias Brugger, Michael Turquette, Stephen Boyd,
	Kate Stewart, devicetree, Greg Kroah-Hartman, linux-kernel,
	moderated list:ARM/Mediatek SoC support, Linux ARM, mtk01761,
	Thomas Gleixner, linux-clk, Allison Randal
In-Reply-To: <20200518113156.25009-1-matthias.bgg@kernel.org>

Hi, Matthias:

<matthias.bgg@kernel.org> 於 2020年5月18日 週一 下午7:32寫道:
>
> From: Matthias Brugger <matthias.bgg@gmail.com>
>
> The mmsys driver is now the top level entry point for the multimedia
> system (mmsys), we bind the clock driver by creating a platform device.
> We also bind the MediaTek DRM driver which is not yet implement and
> therefor will errror out for now.

You may need to let CONFIG_MTK_MMSYS depends on
CONFIG_COMMON_CLK_MT8173_MMSYS || CONFIG_COMMON_CLK_MT8183_MMSYS

Regards,
Chun-Kuang.

>
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> ---
>
>  drivers/clk/mediatek/clk-mt8183-mm.c | 9 ++-------
>  drivers/soc/mediatek/mtk-mmsys.c     | 8 ++++++++
>  2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8183-mm.c b/drivers/clk/mediatek/clk-mt8183-mm.c
> index 720c696b506d..9d60e09619c1 100644
> --- a/drivers/clk/mediatek/clk-mt8183-mm.c
> +++ b/drivers/clk/mediatek/clk-mt8183-mm.c
> @@ -84,8 +84,9 @@ static const struct mtk_gate mm_clks[] = {
>
>  static int clk_mt8183_mm_probe(struct platform_device *pdev)
>  {
> +       struct device *dev = &pdev->dev;
> +       struct device_node *node = dev->parent->of_node;
>         struct clk_onecell_data *clk_data;
> -       struct device_node *node = pdev->dev.of_node;
>
>         clk_data = mtk_alloc_clk_data(CLK_MM_NR_CLK);
>
> @@ -95,16 +96,10 @@ static int clk_mt8183_mm_probe(struct platform_device *pdev)
>         return of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
>  }
>
> -static const struct of_device_id of_match_clk_mt8183_mm[] = {
> -       { .compatible = "mediatek,mt8183-mmsys", },
> -       {}
> -};
> -
>  static struct platform_driver clk_mt8183_mm_drv = {
>         .probe = clk_mt8183_mm_probe,
>         .driver = {
>                 .name = "clk-mt8183-mm",
> -               .of_match_table = of_match_clk_mt8183_mm,
>         },
>  };
>
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index cacafe23c823..783c3dd008b2 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.c
> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> @@ -92,6 +92,10 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
>         .clk_driver = "clk-mt8173-mm",
>  };
>
> +static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
> +       .clk_driver = "clk-mt8183-mm",
> +};
> +
>  static unsigned int mtk_mmsys_ddp_mout_en(enum mtk_ddp_comp_id cur,
>                                           enum mtk_ddp_comp_id next,
>                                           unsigned int *addr)
> @@ -339,6 +343,10 @@ static const struct of_device_id of_match_mtk_mmsys[] = {
>                 .compatible = "mediatek,mt8173-mmsys",
>                 .data = &mt8173_mmsys_driver_data,
>         },
> +       {
> +               .compatible = "mediatek,mt8183-mmsys",
> +               .data = &mt8183_mmsys_driver_data,
> +       },
>         { }
>  };
>
> --
> 2.26.2
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply

* Re: [PATCH 1/2] dt-bindings: spi: Add Baikal-T1 System Boot SPI Controller binding
From: Rob Herring @ 2020-05-18 15:26 UTC (permalink / raw)
  To: Serge Semin
  Cc: Mark Brown, Serge Semin, Ramil Zaripov, Alexey Malahov,
	Thomas Bogendoerfer, Paul Burton, Ralf Baechle, John Garry,
	Chuanhong Guo, Tomer Maimon, Lee Jones, Miquel Raynal,
	Arnd Bergmann, linux-mips, linux-spi, devicetree, linux-kernel
In-Reply-To: <20200508093621.31619-2-Sergey.Semin@baikalelectronics.ru>

On Fri, May 08, 2020 at 12:36:20PM +0300, Serge Semin wrote:
> Baikal-T1 Boot SPI is a part of the SoC System Controller and is
> responsible for the system bootup from an external SPI flash. It's a DW
> APB SSI-based SPI-controller with no interrupts, no DMA, with just one
> native chip-select available and a single reference clock. Since Baikal-T1
> SoC is normally booted up from an external SPI flash this SPI controller
> in most of the cases is supposed to be connected to a single SPI-nor
> flash. Additionally in order to provide a transparent from CPU point of
> view initial code execution procedure the system designers created an IP
> block which physically maps the SPI flash found at CS0 to a memory region.
> 
> Co-developed-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Chuanhong Guo <gch981213@gmail.com>
> Cc: Tomer Maimon <tmaimon77@gmail.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-mips@vger.kernel.org
> Cc: linux-spi@vger.kernel.org
> ---
>  .../bindings/spi/baikal,bt1-sys-ssi.yaml      | 100 ++++++++++++++++++
>  1 file changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/baikal,bt1-sys-ssi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/baikal,bt1-sys-ssi.yaml b/Documentation/devicetree/bindings/spi/baikal,bt1-sys-ssi.yaml
> new file mode 100644
> index 000000000000..d9d3257d78f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/baikal,bt1-sys-ssi.yaml
> @@ -0,0 +1,100 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/baikal,bt1-sys-ssi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Baikal-T1 System Boot SSI Controller
> +
> +description: |
> +  Baikal-T1 System Controller includes a Boot SPI Controller, which is
> +  responsible for loading chip bootup code from an external SPI flash. In order
> +  to do this transparently from CPU point of view there is a dedicated IP block
> +  mapping the 16MB flash to a dedicated MMIO range. The controller is based on
> +  the DW APB SSI IP-core but equipped with very limited resources: no IRQ,
> +  no DMA, a single native CS being necessarily connected to a 16MB SPI flash
> +  (otherwise the system won't bootup from the flash), internal Tx/Rx FIFO of
> +  just 8 bytes depth. Access to DW APB SSI controller registers is mutually
> +  exclusive from normal MMIO interface and from physically mapped SPI Flash
> +  memory. So either one or another way of using the controller functionality
> +  can be enabled at a time.
> +
> +maintainers:
> +  - Serge Semin <fancer.lancer@gmail.com>
> +
> +allOf:
> +  - $ref: spi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: baikal,bt1-sys-ssi
> +
> +  reg:
> +    items:
> +      - description: Baikal-T1 Boot Controller configuration registers
> +      - description: Physically mapped SPI flash ROM found at CS0
> +
> +  reg-names:
> +    items:
> +      - const: config
> +      - const: map
> +
> +  clocks:
> +    description: SPI Controller reference clock source

Can drop this. 

> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: ssi_clk
> +
> +  num-cs:
> +    const: 1
> +
> +patternProperties:
> +  "^.*@[0-9a-f]+":
> +    type: object
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 0
> +
> +      spi-rx-bus-width:
> +        const: 1
> +
> +      spi-tx-bus-width:
> +        const: 1

What's the point of these 2 properties if they aren't required?

> +
> +unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names

> +  - "#address-cells"
> +  - "#size-cells"

These 2 are required by spi-controller.yaml, so you can drop here.

> +  - clocks
> +
> +examples:
> +  - |
> +    spi@1f040000 {
> +      compatible = "baikal,bt1-sys-ssi";
> +      reg = <0x1f040000 0x1000>,
> +            <0x1c000000 0x1000000>;
> +      reg-names = "config", "map";
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      clocks = <&ccu_sys>;
> +      clock-names = "ssi_clk";
> +
> +      boot_flash: flash@0 {
> +        compatible = "jedec,spi-nor";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        reg = <0>;
> +
> +        spi-max-frequency = <25000000>;
> +      };
> +    };
> +...
> -- 
> 2.25.1
> 

^ permalink raw reply

* Re: [PATCH 28/28] dt-bindings: usb: Convert ehci-mv to json-schema
From: Rob Herring @ 2020-05-18 15:20 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Linus Walleij, Bartosz Golaszewski, Thomas Gleixner, Jason Cooper,
	Marc Zyngier, Mauro Carvalho Chehab, Ulf Hansson,
	Kishon Vijay Abraham I, Alessandro Zummo, Alexandre Belloni,
	Greg Kroah-Hartman, Mark Brown, Daniel Lezcano, Andrew Lunn,
	Gregory Clement, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
	devicetree, linux-kernel@vger.kernel.org,
	open list:GPIO SUBSYSTEM, Linux I2C, Linux Media Mailing List,
	linux-mmc, open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
	open list:SERIAL DRIVERS, linux-spi, Linux USB List
In-Reply-To: <20200517193451.GG1695525@furthur.local>

On Sun, May 17, 2020 at 1:34 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> On Fri, Mar 27, 2020 at 01:55:20PM -0600, Rob Herring wrote:
> > On Tue, Mar 17, 2020 at 10:39:22AM +0100, Lubomir Rintel wrote:
> > > A straightforward conversion of the ehci-mv binding to DT schema format
> > > using json-schema.
> > >
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > ---
> > >  .../devicetree/bindings/usb/ehci-mv.txt       | 23 -------
> > >  .../bindings/usb/marvell,pxau2o-ehci.yaml     | 60 +++++++++++++++++++
> > >  2 files changed, 60 insertions(+), 23 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/usb/ehci-mv.txt
> > >  create mode 100644 Documentation/devicetree/bindings/usb/marvell,pxau2o-ehci.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/ehci-mv.txt b/Documentation/devicetree/bindings/usb/ehci-mv.txt
> > > deleted file mode 100644
> > > index 335589895763e..0000000000000
> > > --- a/Documentation/devicetree/bindings/usb/ehci-mv.txt
> > > +++ /dev/null
> > > @@ -1,23 +0,0 @@
> > > -* Marvell PXA/MMP EHCI controller.
> > > -
> > > -Required properties:
> > > -
> > > -- compatible: must be "marvell,pxau2o-ehci"
> > > -- reg: physical base addresses of the controller and length of memory mapped region
> > > -- interrupts: one EHCI controller interrupt should be described here
> > > -- clocks: phandle list of usb clocks
> > > -- clock-names: should be "USBCLK"
> > > -- phys: phandle for the PHY device
> > > -- phy-names: should be "usb"
> > > -
> > > -Example:
> > > -
> > > -   ehci0: usb-ehci@d4208000 {
> > > -           compatible = "marvell,pxau2o-ehci";
> > > -           reg = <0xd4208000 0x200>;
> > > -           interrupts = <44>;
> > > -           clocks = <&soc_clocks MMP2_CLK_USB>;
> > > -           clock-names = "USBCLK";
> > > -           phys = <&usb_otg_phy>;
> > > -           phy-names = "usb";
> > > -   };
> > > diff --git a/Documentation/devicetree/bindings/usb/marvell,pxau2o-ehci.yaml b/Documentation/devicetree/bindings/usb/marvell,pxau2o-ehci.yaml
> > > new file mode 100644
> > > index 0000000000000..189025ef1e92e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/marvell,pxau2o-ehci.yaml
> > > @@ -0,0 +1,60 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> >
> > Same license comment.
>
> I wrote that binding document and chose that license.

Okay, but please make it GPL-2.0-only instead of or-later. If everyone
was attentive to licensing picking their own variations would be fine,
but they aren't and just copy-n-paste. So there's 2 choices
GPL-2.0-only (for converted bindings) or (GPL-2.0-only OR
BSD-2-Clause) for new or re-licensed bindings.

Rob

^ permalink raw reply

* Re: [PATCH 2/2] spi: Add Baikal-T1 System Boot SPI Controller driver
From: Mark Brown @ 2020-05-18 15:19 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Ramil Zaripov, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, Lee Jones, Miquel Raynal,
	Arnd Bergmann, Rob Herring, linux-mips, devicetree, John Garry,
	Chuanhong Guo, Andy Shevchenko, Eddie James, Geert Uytterhoeven,
	Chris Packham, Tomer Maimon, Masahisa Kojima, Krzysztof Kozlowski,
	Florian Fainelli, Jassi Brar, linux-kernel, linux-spi
In-Reply-To: <20200518000542.ohtpem3lo2pbixbu@mobilestation>

[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]

On Mon, May 18, 2020 at 03:05:42AM +0300, Serge Semin wrote:
> On Mon, May 11, 2020 at 10:25:06PM +0100, Mark Brown wrote:

> > Yes, some flags should work here - the issue was that at least some
> > controllers may end up trying to do multiple SPI operations for one
> > spi-mem thing which will break if the chip select doesn't get changed to
> > correspond with what's going on.

> Ok. New SPI flag it is then. It will be something like this:
> + #define SPI_CONTROLLER_FLASH_SS		BIT(6)

I'd rather use CS than SS (it's more common in the code).

> So, what do you think?

Should be fine, controllers that have an issue implementing just
shouldn't set the flag.

> > > > It's not clear to me that this hardware actually supports spi_mem in
> > > > hardware?

> > > SPI-mem operations are implemented by means of the EEPROM-read and Tx-only
> > > modes of the controller.

> > Sure, but those seem like normal SPI-level things rather than cases
> > where the hardware understands that it has a flash attached and is doing
> > flash specific things.

> No, hardware can't detect whether the flash is attached. This must be defined by
> the platform, like based on the DT sub-nodes.

This isn't about autodetection, it's about the abstraction level the
hardware is operating on - some hardware is able to generate flash
operations by itself (possibly with some help programming the opcodes
that are needed by a given flash), some hardware just works at the
bytestream level.

> > A very common case for this stuff is that
> > controllers have acceleration blocks for read and fall back on normal
> > SPI for writes and erases, that sounds like what's going on here.
> 
> Well, yeah, they do provide some acceleration. EEPROM-read provides automatic
> write-cmd-dummy-data-then-read operations. But in this case the only thing we
> have to push into the SPI Tx FIFO is command and dummy bytes. The read operation

So it's a write then read but you have to program the write each time?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* Re: [PATCH 25/28] dt-bindings: rtc: Convert sa1100-rtc to json-schema
From: Rob Herring @ 2020-05-18 15:12 UTC (permalink / raw)
  To: Lubomir Rintel
  Cc: Linus Walleij, Bartosz Golaszewski, Thomas Gleixner, Jason Cooper,
	Marc Zyngier, Mauro Carvalho Chehab, Ulf Hansson,
	Kishon Vijay Abraham I, Alessandro Zummo, Alexandre Belloni,
	Greg Kroah-Hartman, Mark Brown, Daniel Lezcano, Andrew Lunn,
	Gregory Clement, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
	devicetree, linux-kernel@vger.kernel.org,
	open list:GPIO SUBSYSTEM, Linux I2C, Linux Media Mailing List,
	linux-mmc, open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
	open list:SERIAL DRIVERS, linux-spi, Linux USB List
In-Reply-To: <20200517192557.GE1695525@furthur.local>

On Sun, May 17, 2020 at 1:26 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> On Fri, Mar 27, 2020 at 01:49:02PM -0600, Rob Herring wrote:
> > On Tue, Mar 17, 2020 at 10:39:19AM +0100, Lubomir Rintel wrote:
> > > Convert the sa1100-rtc binding to DT schema format using json-schema.
> > >
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > ---
> > >  .../devicetree/bindings/rtc/sa1100-rtc.txt    | 17 ------
> > >  .../devicetree/bindings/rtc/sa1100-rtc.yaml   | 55 +++++++++++++++++++
> > >  2 files changed, 55 insertions(+), 17 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/rtc/sa1100-rtc.txt
> > >  create mode 100644 Documentation/devicetree/bindings/rtc/sa1100-rtc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/rtc/sa1100-rtc.txt b/Documentation/devicetree/bindings/rtc/sa1100-rtc.txt
> > > deleted file mode 100644
> > > index 968ac820254bb..0000000000000
> > > --- a/Documentation/devicetree/bindings/rtc/sa1100-rtc.txt
> > > +++ /dev/null
> > > @@ -1,17 +0,0 @@
> > > -* Marvell Real Time Clock controller
> > > -
> > > -Required properties:
> > > -- compatible: should be "mrvl,sa1100-rtc"
> > > -- reg: physical base address of the controller and length of memory mapped
> > > -  region.
> > > -- interrupts: Should be two. The first interrupt number is the rtc alarm
> > > -  interrupt and the second interrupt number is the rtc hz interrupt.
> > > -- interrupt-names: Assign name of irq resource.
> > > -
> > > -Example:
> > > -   rtc: rtc@d4010000 {
> > > -           compatible = "mrvl,mmp-rtc";
> > > -           reg = <0xd4010000 0x1000>;
> > > -           interrupts = <5>, <6>;
> > > -           interrupt-names = "rtc 1Hz", "rtc alarm";
> > > -   };
> > > diff --git a/Documentation/devicetree/bindings/rtc/sa1100-rtc.yaml b/Documentation/devicetree/bindings/rtc/sa1100-rtc.yaml
> > > new file mode 100644
> > > index 0000000000000..53a8b72df9f34
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/sa1100-rtc.yaml
> > > @@ -0,0 +1,55 @@
> >
> > License
>
> The original file lacked one. Should I just go with GPL-2.0?

Yes, that is what files in the kernel with no license default to. Or
figure out the authors and get permission to dual-license.

Rob

^ permalink raw reply

* Re: [PATCH v2 18/19] spi: dw: Use regset32 DebugFS method to create regdump file
From: Andy Shevchenko @ 2020-05-18 15:06 UTC (permalink / raw)
  To: Serge Semin
  Cc: Serge Semin, Mark Brown, Georgy Vlasov, Ramil Zaripov,
	Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
	Arnd Bergmann, Allison Randal, Gareth Williams, Rob Herring,
	linux-mips, devicetree, Wan Ahmad Zainie, Charles Keepax,
	Clement Leger, wuxu.wu, Phil Edworthy, Thomas Gleixner,
	Jarkko Nikula, linux-spi, linux-kernel
In-Reply-To: <20200518140825.jnkfpybxnwq7fx2m@mobilestation>

On Mon, May 18, 2020 at 05:08:25PM +0300, Serge Semin wrote:
> On Mon, May 18, 2020 at 02:18:22PM +0300, Andy Shevchenko wrote:
> > On Sat, May 16, 2020 at 11:46:34PM +0300, Serge Semin wrote:
> > > On Fri, May 15, 2020 at 06:10:56PM +0300, Andy Shevchenko wrote:
> > > > On Fri, May 15, 2020 at 01:47:57PM +0300, Serge Semin wrote:
> > > > > DebugFS kernel interface provides a dedicated method to create the
> > > > > registers dump file. Use it instead of creating a generic DebugFS
> > > > > file with manually written read callback function.

> > > > With below nit addressed,
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > > > > +#define DW_SPI_DBGFS_REG(_name, _off)	\
> > > > > +{					\
> > > > > +	.name = _name,			\
> > > > 
> > > > > +	.offset = _off			\
> > > > 
> > > > As previously discussed (did I miss your answer?) the comma at the end leaves
> > > > better pattern for maintenance prospective.
> > > 
> > > Ah, sorry. Missed that. This comma is hardly needed seeing the structure
> > > consists of just two elements. So I'd rather leave it as is.
> > 
> > While it's a really small thing, I consider that it's not good to make
> > someone's else problem what can be done here. So, please, consider to add a
> > comma. Look at the other drivers and code in the kernel. This is at least
> > defacto preferred style.
> 
> Andy, you never give up, don't you? =)

First of all, I really appreciate work you have done so far (I mean it).
Now to the point.

You see, I always try to have a rationale behind any proposed comment. I agree,
that some of them can be considered as a bikeshedding for a certain developer,
but on big picture with this scale of project even small change (being made or
being rejected to be made) can provoke additional churn with a good magnitude,
if we consider all possible cases where somebody, e.g., can take into account
existing code to copy'n'paste from). So, I would easy give up on something if
there will be a stronger (than mine) argument why the proposed thing is not
good to be done (at least as a part of the discussed patch set). I also want
to and will learn from the developers as a reviewer.

Hope that above will clarify my reviewer's point of view to the code.

> Agreed then. I'll add comma to the
> initializer and also after the last member here:
>         DW_SPI_DBGFS_REG("ISR", DW_SPI_ISR),
>         DW_SPI_DBGFS_REG("DMACR", DW_SPI_DMACR),
>         DW_SPI_DBGFS_REG("DMATDLR", DW_SPI_DMATDLR),
> -       DW_SPI_DBGFS_REG("DMARDLR", DW_SPI_DMARDLR)
> +       DW_SPI_DBGFS_REG("DMARDLR", DW_SPI_DMARDLR),

Good catch, thanks for taking it into consideration as well.

>  };
>  
>  static int dw_spi_debugfs_init(struct dw_spi *dws)

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply

* [PATCH V2] dt-bindings: clock: Convert i.MX7D clock to json-schema
From: Anson Huang @ 2020-05-18 14:52 UTC (permalink / raw)
  To: mturquette, sboyd, robh+dt, shawnguo, s.hauer, kernel, festevam,
	Frank.Li, linux-clk, devicetree, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

Convert the i.MX7D clock binding to DT schema format using json-schema.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
Changes since V1:
	- Update maintainer's e-mail address.
---
 .../devicetree/bindings/clock/imx7d-clock.txt      | 13 -----
 .../devicetree/bindings/clock/imx7d-clock.yaml     | 64 ++++++++++++++++++++++
 2 files changed, 64 insertions(+), 13 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/imx7d-clock.txt
 create mode 100644 Documentation/devicetree/bindings/clock/imx7d-clock.yaml

diff --git a/Documentation/devicetree/bindings/clock/imx7d-clock.txt b/Documentation/devicetree/bindings/clock/imx7d-clock.txt
deleted file mode 100644
index 9d3026d..0000000
--- a/Documentation/devicetree/bindings/clock/imx7d-clock.txt
+++ /dev/null
@@ -1,13 +0,0 @@
-* Clock bindings for Freescale i.MX7 Dual
-
-Required properties:
-- compatible: Should be "fsl,imx7d-ccm"
-- reg: Address and length of the register set
-- #clock-cells: Should be <1>
-- clocks: list of clock specifiers, must contain an entry for each required
-  entry in clock-names
-- clock-names: should include entries "ckil", "osc"
-
-The clock consumer should specify the desired clock by having the clock
-ID in its "clocks" phandle cell.  See include/dt-bindings/clock/imx7d-clock.h
-for the full list of i.MX7 Dual clock IDs.
diff --git a/Documentation/devicetree/bindings/clock/imx7d-clock.yaml b/Documentation/devicetree/bindings/clock/imx7d-clock.yaml
new file mode 100644
index 0000000..8cd0573
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/imx7d-clock.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/imx7d-clock.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Clock bindings for Freescale i.MX7 Dual
+
+maintainers:
+  - Frank Li <Frank.Li@nxp.com>
+  - Anson Huang <Anson.Huang@nxp.com>
+
+description: |
+  The clock consumer should specify the desired clock by having the clock
+  ID in its "clocks" phandle cell. See include/dt-bindings/clock/imx7d-clock.h
+  for the full list of i.MX7 Dual clock IDs.
+
+properties:
+  compatible:
+    const: fsl,imx7d-ccm
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: CCM interrupt request 1
+      - description: CCM interrupt request 2
+    maxItems: 2
+
+  '#clock-cells':
+    const: 1
+
+  clocks:
+    items:
+      - description: 32k osc
+      - description: 24m osc
+
+  clock-names:
+    items:
+      - const: ckil
+      - const: osc
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    clock-controller@30380000 {
+        compatible = "fsl,imx7d-ccm";
+        reg = <0x30380000 0x10000>;
+        interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
+        #clock-cells = <1>;
+        clocks = <&ckil>, <&osc>;
+        clock-names = "ckil", "osc";
+    };
-- 
2.7.4


^ permalink raw reply related

* Re: [PATCH v2 10/19] spi: dw: Use DMA max burst to set the request thresholds
From: Serge Semin @ 2020-05-18 14:59 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Serge Semin, Mark Brown, Alexey Malahov, Thomas Bogendoerfer,
	Paul Burton, Ralf Baechle, Arnd Bergmann, Allison Randal,
	Gareth Williams, Rob Herring, linux-mips, devicetree,
	Georgy Vlasov, Ramil Zaripov, Jarkko Nikula, Thomas Gleixner,
	Wan Ahmad Zainie, Linus Walleij, Clement Leger, linux-spi,
	Linux Kernel Mailing List
In-Reply-To: <20200518144834.GD1634618@smile.fi.intel.com>

On Mon, May 18, 2020 at 05:48:34PM +0300, Andy Shevchenko wrote:
> On Mon, May 18, 2020 at 04:43:06PM +0300, Serge Semin wrote:
> > On Mon, May 18, 2020 at 04:25:20PM +0300, Andy Shevchenko wrote:
> > > On Mon, May 18, 2020 at 3:53 PM Serge Semin
> > > <Sergey.Semin@baikalelectronics.ru> wrote:
> > > > On Mon, May 18, 2020 at 02:03:43PM +0300, Andy Shevchenko wrote:
> > > > > On Sat, May 16, 2020 at 11:01:33PM +0300, Serge Semin wrote:
> > > > > > On Fri, May 15, 2020 at 05:38:42PM +0300, Andy Shevchenko wrote:
> > > > > > > On Fri, May 15, 2020 at 01:47:49PM +0300, Serge Semin wrote:
> 
> ...
> 
> > > > > > It's not like anyone cared about padding in this structure in the first place)
> > > > >
> > > > > I think I have been caring (to some extend).
> > > >
> > > > Well, If you have then instead of asking to rearrange just two members (which
> > > > by the way finely grouped by the Tx-Rx affiliation) why not sending a
> > > > patch, which would refactor the whole structure so to be optimal for the x64
> > > > platforms? I don't really see why this gets very important for you seeing
> > > > Mark is Ok with this. My current commit follows the common driver design
> > > > including the DW SSI data members grouping. On the second thought I'll leave
> > > > it as is then.
> > > 
> > > Again same issue here. What is really easy to do for you here, will
> > > become a burden and additional churn to anybody else.
> > > So, why not to minimize it in the first place? Same with comma in
> > > another patch. Sorry, I really don't get it.
> > 
> > If comma is more or less understandable (though adding it is absolutely
> > redundant there and doesn't worth even a bit of time spending for the
> > discussion), here you consider the patch from padding point of view.
> > The driver developer didn't care about it, but did care about grouping the
> > members in a corresponding way. The padding burden will be there anyway and
> > should be fixed for the whole structure in an additional patch. Until then
> > the way of grouping should be preserved.
> 
> Like you said, we spent already much more time than that simple change can be
> satisfied. And like you said, "deleloper ... did care about groupping members
> in a corresponding way". So, if we look at this in the original code, my
> suggestion, besides padding benefit, is consistent with existing pattern in
> that data structure.

What pattern do you mean? As I see it, my implementation is consistent with
current structure structure, while yours is not.

-Sergey

> 
> Note, I agree on extern keyword change can be postponed (it was in the original
> code), but here you introduce a new code...
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

^ permalink raw reply

* RE: [PATCH] dt-bindings: clock: Convert i.MX7D clock to json-schema
From: Anson Huang @ 2020-05-18 14:58 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Michael Turquette, Stephen Boyd, Rob Herring, Shawn Guo,
	Sascha Hauer, Sascha Hauer, Frank Li, linux-clk,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel, dl-linux-imx
In-Reply-To: <CAOMZO5BeBcvmAwEJTUhHSn=_chx7OVkkxj2oB+50hGH-7H7SfA@mail.gmail.com>

Hi, Fabio


> Subject: Re: [PATCH] dt-bindings: clock: Convert i.MX7D clock to json-schema
> 
> On Mon, May 18, 2020 at 11:16 AM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> 
> > +maintainers:
> > +  - Frank Li <Frank.Li@freescale.com>
> 
> The freescale.com e-mail domain is no longer valid for quite some time :-)

Thanks for reminder, I will use Frank's nxp e-mail address instead.

Anson

^ permalink raw reply

* Re: [PATCH v3 0/4] PCI: cadence: Deprecate inbound/outbound specific bindings
From: Lorenzo Pieralisi @ 2020-05-18 14:57 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Bjorn Helgaas, Rob Herring, Robin Murphy, Tom Joseph, linux-pci,
	devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20200508130646.23939-1-kishon@ti.com>

On Fri, May 08, 2020 at 06:36:42PM +0530, Kishon Vijay Abraham I wrote:
> This series is a result of comments given by Rob Herring @ [1].
> Patch series changes the DT bindings and makes the corresponding driver
> changes.
> 
> Changes from v2:
> 1) Changed the order of patches (no solid reason. Just save some
> rebasing effort for me)
> 2) Added Acked-by Tom and Rob except for the dma-ranges patch
> 3) Re-worked dma-ranges patch for it do decode multiple dma-ranges
>    and configure BAR0, BAR1 and NO_BAR instead of just NO_BAR [2].
> 
> Changes from v1:
> 1) Added Reviewed-by: Rob Herring <robh@kernel.org> for dt-binding patch
> 2) Fixed nitpick comments from Bjorn Helgaas
> 3) Added a patch to read 32-bit Vendor ID/Device ID property from DT
> 
> [1] -> http://lore.kernel.org/r/20200219202700.GA21908@bogus
> [2] -> http://lore.kernel.org/r/eb1ffcb3-264f-5174-1f25-b5b2d3269840@ti.com
> 
> Kishon Vijay Abraham I (4):
>   dt-bindings: PCI: cadence: Deprecate inbound/outbound specific
>     bindings
>   PCI: cadence: Remove "cdns,max-outbound-regions" DT property
>   PCI: cadence: Fix to read 32-bit Vendor ID/Device ID property from DT
>   PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits"
>     property
> 
>  .../bindings/pci/cdns,cdns-pcie-ep.yaml       |   2 +-
>  .../bindings/pci/cdns,cdns-pcie-host.yaml     |   3 +-
>  .../devicetree/bindings/pci/cdns-pcie-ep.yaml |  25 +++
>  .../bindings/pci/cdns-pcie-host.yaml          |  10 ++
>  .../devicetree/bindings/pci/cdns-pcie.yaml    |   8 -
>  .../controller/cadence/pcie-cadence-host.c    | 151 +++++++++++++++---
>  drivers/pci/controller/cadence/pcie-cadence.h |  23 ++-
>  7 files changed, 182 insertions(+), 40 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml

I have applied patches 1-3 to pci/cadence (that I think are
self-contained), waiting for the dma-ranges discussion to wrap up on
patch 4.

Lorenzo

^ permalink raw reply

* [PATCH 2/3] dt-bindings: timer: Convert i.MX TPM to json-schema
From: Anson Huang @ 2020-05-18 14:47 UTC (permalink / raw)
  To: daniel.lezcano, tglx, robh+dt, shawnguo, s.hauer, kernel,
	festevam, ping.bai, aisheng.dong, linux-kernel, devicetree,
	linux-arm-kernel
  Cc: Linux-imx
In-Reply-To: <1589813260-20036-1-git-send-email-Anson.Huang@nxp.com>

Convert the i.MX TPM binding to DT schema format using json-schema.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 .../devicetree/bindings/timer/nxp,tpm-timer.txt    | 28 ----------
 .../devicetree/bindings/timer/nxp,tpm-timer.yaml   | 63 ++++++++++++++++++++++
 2 files changed, 63 insertions(+), 28 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/timer/nxp,tpm-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/nxp,tpm-timer.yaml

diff --git a/Documentation/devicetree/bindings/timer/nxp,tpm-timer.txt b/Documentation/devicetree/bindings/timer/nxp,tpm-timer.txt
deleted file mode 100644
index f82087b..0000000
--- a/Documentation/devicetree/bindings/timer/nxp,tpm-timer.txt
+++ /dev/null
@@ -1,28 +0,0 @@
-NXP Low Power Timer/Pulse Width Modulation Module (TPM)
-
-The Timer/PWM Module (TPM) supports input capture, output compare,
-and the generation of PWM signals to control electric motor and power
-management applications. The counter, compare and capture registers
-are clocked by an asynchronous clock that can remain enabled in low
-power modes. TPM can support global counter bus where one TPM drives
-the counter bus for the others, provided bit width is the same.
-
-Required properties:
-
-- compatible :	should be "fsl,imx7ulp-tpm"
-- reg :		Specifies base physical address and size of the register sets
-		for the clock event device and clock source device.
-- interrupts :	Should be the clock event device interrupt.
-- clocks :	The clocks provided by the SoC to drive the timer, must contain
-		an entry for each entry in clock-names.
-- clock-names : Must include the following entries: "ipg" and "per".
-
-Example:
-tpm5: tpm@40260000 {
-	compatible = "fsl,imx7ulp-tpm";
-	reg = <0x40260000 0x1000>;
-	interrupts = <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>;
-	clocks = <&clks IMX7ULP_CLK_NIC1_BUS_DIV>,
-		 <&clks IMX7ULP_CLK_LPTPM5>;
-	clock-names = "ipg", "per";
-};
diff --git a/Documentation/devicetree/bindings/timer/nxp,tpm-timer.yaml b/Documentation/devicetree/bindings/timer/nxp,tpm-timer.yaml
new file mode 100644
index 0000000..0d34610
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/nxp,tpm-timer.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/nxp,tpm-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP Low Power Timer/Pulse Width Modulation Module (TPM)
+
+maintainers:
+  - Dong Aisheng <aisheng.dong@nxp.com>
+
+description: |
+  The Timer/PWM Module (TPM) supports input capture, output compare,
+  and the generation of PWM signals to control electric motor and power
+  management applications. The counter, compare and capture registers
+  are clocked by an asynchronous clock that can remain enabled in low
+  power modes. TPM can support global counter bus where one TPM drives
+  the counter bus for the others, provided bit width is the same.
+
+properties:
+  compatible:
+    const: fsl,imx7ulp-tpm
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: SoC TPM ipg clock
+      - description: SoC TPM per clock
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: ipg
+      - const: per
+    maxItems: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx7ulp-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    timer@40260000 {
+        compatible = "fsl,imx7ulp-tpm";
+        reg = <0x40260000 0x1000>;
+        interrupts = <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&scg1 IMX7ULP_CLK_NIC1_BUS_DIV>,
+                 <&pcc2 IMX7ULP_CLK_LPTPM5>;
+        clock-names = "ipg", "per";
+    };
-- 
2.7.4


^ permalink raw reply related

* [PATCH 1/3] dt-bindings: timer: Convert i.MX GPT to json-schema
From: Anson Huang @ 2020-05-18 14:47 UTC (permalink / raw)
  To: daniel.lezcano, tglx, robh+dt, shawnguo, s.hauer, kernel,
	festevam, ping.bai, aisheng.dong, linux-kernel, devicetree,
	linux-arm-kernel
  Cc: Linux-imx
In-Reply-To: <1589813260-20036-1-git-send-email-Anson.Huang@nxp.com>

Convert the i.MX GPT binding to DT schema format using json-schema.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 .../devicetree/bindings/timer/fsl,imxgpt.txt       |  45 ---------
 .../devicetree/bindings/timer/fsl,imxgpt.yaml      | 109 +++++++++++++++++++++
 2 files changed, 109 insertions(+), 45 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/timer/fsl,imxgpt.txt
 create mode 100644 Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml

diff --git a/Documentation/devicetree/bindings/timer/fsl,imxgpt.txt b/Documentation/devicetree/bindings/timer/fsl,imxgpt.txt
deleted file mode 100644
index 5d8fd5b..0000000
--- a/Documentation/devicetree/bindings/timer/fsl,imxgpt.txt
+++ /dev/null
@@ -1,45 +0,0 @@
-Freescale i.MX General Purpose Timer (GPT)
-
-Required properties:
-
-- compatible : should be one of following:
-  for i.MX1:
-  - "fsl,imx1-gpt";
-  for i.MX21:
-  - "fsl,imx21-gpt";
-  for i.MX27:
-  - "fsl,imx27-gpt", "fsl,imx21-gpt";
-  for i.MX31:
-  - "fsl,imx31-gpt";
-  for i.MX25:
-  - "fsl,imx25-gpt", "fsl,imx31-gpt";
-  for i.MX50:
-  - "fsl,imx50-gpt", "fsl,imx31-gpt";
-  for i.MX51:
-  - "fsl,imx51-gpt", "fsl,imx31-gpt";
-  for i.MX53:
-  - "fsl,imx53-gpt", "fsl,imx31-gpt";
-  for i.MX6Q:
-  - "fsl,imx6q-gpt", "fsl,imx31-gpt";
-  for i.MX6DL:
-  - "fsl,imx6dl-gpt";
-  for i.MX6SL:
-  - "fsl,imx6sl-gpt", "fsl,imx6dl-gpt";
-  for i.MX6SX:
-  - "fsl,imx6sx-gpt", "fsl,imx6dl-gpt";
-- reg : specifies base physical address and size of the registers.
-- interrupts : should be the gpt interrupt.
-- clocks : the clocks provided by the SoC to drive the timer, must contain
-           an entry for each entry in clock-names.
-- clock-names : must include "ipg" entry first, then "per" entry.
-
-Example:
-
-gpt1: timer@10003000 {
-	compatible = "fsl,imx27-gpt", "fsl,imx21-gpt";
-	reg = <0x10003000 0x1000>;
-	interrupts = <26>;
-	clocks = <&clks IMX27_CLK_GPT1_IPG_GATE>,
-		 <&clks IMX27_CLK_PER1_GATE>;
-	clock-names = "ipg", "per";
-};
diff --git a/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml b/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
new file mode 100644
index 0000000..5c7186b
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/fsl,imxgpt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX General Purpose Timer (GPT)
+
+maintainers:
+  - Sascha Hauer <s.hauer@pengutronix.de>
+
+properties:
+  compatible:
+    oneOf:
+      - description: on i.MX1 the following compatible must be specified
+        items:
+          - const: "fsl,imx1-gpt"
+
+      - description: on i.MX21 the following compatible must be specified
+        items:
+          - const: "fsl,imx21-gpt"
+
+      - description: on i.MX27 the following compatibles must be specified
+        items:
+          - const: "fsl,imx27-gpt"
+          - const: "fsl,imx21-gpt"
+
+      - description: on i.MX31 the following compatible must be specified
+        items:
+          - const: "fsl,imx31-gpt"
+
+      - description: on i.MX25 the following compatibles must be specified
+        items:
+          - const: "fsl,imx25-gpt"
+          - const: "fsl,imx31-gpt"
+
+      - description: on i.MX50 the following compatibles must be specified
+        items:
+          - const: "fsl,imx50-gpt"
+          - const: "fsl,imx31-gpt"
+
+      - description: on i.MX51 the following compatibles must be specified
+        items:
+          - const: "fsl,imx51-gpt"
+          - const: "fsl,imx31-gpt"
+
+      - description: on i.MX53 the following compatibles must be specified
+        items:
+          - const: "fsl,imx53-gpt"
+          - const: "fsl,imx31-gpt"
+
+      - description: on i.MX6Q the following compatibles must be specified
+        items:
+          - const: "fsl,imx6q-gpt"
+          - const: "fsl,imx31-gpt"
+
+      - description: on i.MX6DL the following compatible must be specified
+        items:
+          - const: "fsl,imx6dl-gpt"
+
+      - description: on i.MX6SL the following compatibles must be specified
+        items:
+          - const: "fsl,imx6sl-gpt"
+          - const: "fsl,imx6dl-gpt"
+
+      - description: on i.MX6SX the following compatibles must be specified
+        items:
+          - const: "fsl,imx6sx-gpt"
+          - const: "fsl,imx6dl-gpt"
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: SoC GPT ipg clock
+      - description: SoC GPT per clock
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: ipg
+      - const: per
+    maxItems: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx27-clock.h>
+
+    timer@10003000 {
+        compatible = "fsl,imx27-gpt", "fsl,imx21-gpt";
+        reg = <0x10003000 0x1000>;
+        interrupts = <26>;
+        clocks = <&clks IMX27_CLK_GPT1_IPG_GATE>,
+                 <&clks IMX27_CLK_PER1_GATE>;
+        clock-names = "ipg", "per";
+    };
-- 
2.7.4


^ permalink raw reply related

* [PATCH 3/3] dt-bindings: timer: Convert i.MX SYSCTR to json-schema
From: Anson Huang @ 2020-05-18 14:47 UTC (permalink / raw)
  To: daniel.lezcano, tglx, robh+dt, shawnguo, s.hauer, kernel,
	festevam, ping.bai, aisheng.dong, linux-kernel, devicetree,
	linux-arm-kernel
  Cc: Linux-imx
In-Reply-To: <1589813260-20036-1-git-send-email-Anson.Huang@nxp.com>

Convert the i.MX SYSCTR binding to DT schema format using json-schema.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 .../devicetree/bindings/timer/nxp,sysctr-timer.txt | 25 ----------
 .../bindings/timer/nxp,sysctr-timer.yaml           | 54 ++++++++++++++++++++++
 2 files changed, 54 insertions(+), 25 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/nxp,sysctr-timer.yaml

diff --git a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt
deleted file mode 100644
index d576599..0000000
--- a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt
+++ /dev/null
@@ -1,25 +0,0 @@
-NXP System Counter Module(sys_ctr)
-
-The system counter(sys_ctr) is a programmable system counter which provides
-a shared time base to Cortex A15, A7, A53, A73, etc. it is intended for use in
-applications where the counter is always powered and support multiple,
-unrelated clocks. The compare frame inside can be used for timer purpose.
-
-Required properties:
-
-- compatible :      should be "nxp,sysctr-timer"
-- reg :             Specifies the base physical address and size of the comapre
-                    frame and the counter control, read & compare.
-- interrupts :      should be the first compare frames' interrupt
-- clocks : 	    Specifies the counter clock.
-- clock-names: 	    Specifies the clock's name of this module
-
-Example:
-
-	system_counter: timer@306a0000 {
-		compatible = "nxp,sysctr-timer";
-		reg = <0x306a0000 0x20000>;/* system-counter-rd & compare */
-		clocks = <&clk_8m>;
-		clock-names = "per";
-		interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
-	};
diff --git a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.yaml b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.yaml
new file mode 100644
index 0000000..830211c
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/nxp,sysctr-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP System Counter Module(sys_ctr)
+
+maintainers:
+  - Bai Ping <ping.bai@nxp.com>
+
+description: |
+  The system counter(sys_ctr) is a programmable system counter
+  which provides a shared time base to Cortex A15, A7, A53, A73,
+  etc. it is intended for use in applications where the counter
+  is always powered and support multiple, unrelated clocks. The
+  compare frame inside can be used for timer purpose.
+
+properties:
+  compatible:
+    const: nxp,sysctr-timer
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: per
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    timer@306a0000 {
+        compatible = "nxp,sysctr-timer";
+        reg = <0x306a0000 0x20000>;
+        clocks = <&clk_8m>;
+        clock-names = "per";
+        interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
+     };
-- 
2.7.4


^ permalink raw reply related

* [PATCH 0/3] Covert i.MX GPT/TPM/SYSCTR timer binding to json-schema
From: Anson Huang @ 2020-05-18 14:47 UTC (permalink / raw)
  To: daniel.lezcano, tglx, robh+dt, shawnguo, s.hauer, kernel,
	festevam, ping.bai, aisheng.dong, linux-kernel, devicetree,
	linux-arm-kernel
  Cc: Linux-imx

This patch series converts i.MX GPT, TPM and system counter timer binding
to json-schema, test build passed.

Anson Huang (3):
  dt-bindings: timer: Convert i.MX GPT to json-schema
  dt-bindings: timer: Convert i.MX TPM to json-schema
  dt-bindings: timer: Convert i.MX SYSCTR to json-schema

 .../devicetree/bindings/timer/fsl,imxgpt.txt       |  45 ---------
 .../devicetree/bindings/timer/fsl,imxgpt.yaml      | 109 +++++++++++++++++++++
 .../devicetree/bindings/timer/nxp,sysctr-timer.txt |  25 -----
 .../bindings/timer/nxp,sysctr-timer.yaml           |  54 ++++++++++
 .../devicetree/bindings/timer/nxp,tpm-timer.txt    |  28 ------
 .../devicetree/bindings/timer/nxp,tpm-timer.yaml   |  63 ++++++++++++
 6 files changed, 226 insertions(+), 98 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/timer/fsl,imxgpt.txt
 create mode 100644 Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
 delete mode 100644 Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/nxp,sysctr-timer.yaml
 delete mode 100644 Documentation/devicetree/bindings/timer/nxp,tpm-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/nxp,tpm-timer.yaml

-- 
2.7.4


^ permalink raw reply

* Re: [PATCH 3/3] iio: magnetometer: ak8975: Add gpio reset support
From: Andy Shevchenko @ 2020-05-18 14:55 UTC (permalink / raw)
  To: Jonathan Albrieux
  Cc: Linux Kernel Mailing List, Andy Shevchenko,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Greg Kroah-Hartman, Hartmut Knaack, Jonathan Cameron,
	Lars-Peter Clausen, Linus Walleij,
	open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler,
	Steve Winslow, Thomas Gleixner, Jonathan Cameron
In-Reply-To: <20200518133645.19127-4-jonathan.albrieux@gmail.com>

On Mon, May 18, 2020 at 4:38 PM Jonathan Albrieux
<jonathan.albrieux@gmail.com> wrote:

> +       gpiod_set_value_cansleep(data->reset_gpiod, 1);

(1)

...

> +       /*
> +        * If reset pin is provided then will be set to high on power on
> +        * and to low on power off according to AK09911 datasheet
> +        */

Wording is confusing, perhaps you have to use 'asserted / deasserted'.

Btw, in (1) it's also "high" (asserted). I barely understand how it's
supposed to work in all cases?

> +       reset_gpiod = devm_gpiod_get_optional(&client->dev,
> +                                             "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(reset_gpiod))
> +               return PTR_ERR(reset_gpiod);


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply

* Re: [PATCH v5 0/6] drm: Prepare to use a GPIO on ti-sn65dsi86 for Hot Plug Detect
From: Doug Anderson @ 2020-05-18 14:50 UTC (permalink / raw)
  To: Sam Ravnborg, LinusW, Stephen Boyd, Laurent Pinchart
  Cc: Bartosz Golaszewski, David Airlie, Daniel Vetter, Rob Herring,
	Neil Armstrong, Andrzej Hajda, Sandeep Panda, Rob Clark,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Jernej Skrabec, Jeffrey Hugo, Stephen Boyd, linux-arm-msm,
	Jonas Karlman, dri-devel, open list:GPIO SUBSYSTEM, Andy Gross,
	Thierry Reding, Krzysztof Kozlowski, Paul Walmsley,
	Bjorn Andersson, LKML
In-Reply-To: <CAD=FV=VBU7JmTdvgWjyj_ytrFmz6Gkx2OjVr1FxLh9DBG_jN6w@mail.gmail.com>

Sam,

On Sat, May 9, 2020 at 3:48 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Sat, May 9, 2020 at 1:15 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > Hi Douglas.
> >
> > On Thu, May 07, 2020 at 02:34:54PM -0700, Douglas Anderson wrote:
> > >
> > > As talked about in commit c2bfc223882d ("drm/bridge: ti-sn65dsi86:
> > > Remove the mystery delay"), the normal HPD pin on ti-sn65dsi86 is
> > > kinda useless, at least for embedded DisplayPort (eDP).  However,
> > > despite the fact that the actual HPD pin on the bridge is mostly
> > > useless for eDP, the concept of HPD for eDP still makes sense.  It
> > > allows us to optimize out a hardcoded delay that many panels need if
> > > HPD isn't hooked up.  Panel timing diagrams show HPD as one of the
> > > events to measure timing from and we have to assume the worst case if
> > > we can't actually read HPD.
> > >
> > > One way to use HPD for eDP without using the mostly useless HPD pin on
> > > ti-sn65dsi86 is to route the panel's HPD somewhere else in the system,
> > > like to a GPIO.  This works great because eDP panels aren't physically
> > > hotplugged.  That means the debouncing logic that caused us problems
> > > wasn't really needed and a raw GPIO works great.
> > >
> > > As per the above, a smart board designer would realize the value of
> > > HPD and choose to route it to a GPIO somewhere on the board to avoid
> > > the silly sn65dsi86 debouncing.  While said "smart designer" could
> > > theoretically route HPD anywhere on the board, a really smart designer
> > > would realize that there are several GPIOs on the bridge itself that
> > > are nearly useless for anything but this purpose and route HPD to one
> > > of those.
> > >
> > > This series of patches is intended to allow the scenario described
> > > above.
> > >
> > > This patch has been tested on a board that is not yet mainline.  On
> > > the hardware I have:
> > > - Panel spec says HPD could take up to 200 ms to come up, so without
> > >   HPD hooked up we need to delay 200 ms.
> > > - On my board the panel is powered by the same rail as the
> > >   touchscreen.  By chance of probe order the touchscreen comes up
> > >   first.  This means by the time we check HPD in ti_sn_bridge_enable()
> > >   it's already up.  Thus we can use the panel on 200 ms earlier.
> > > - If I measure HPD on this pane it comes up ~56 ms after the panel is
> > >   powered.  This means I can save 144 ms of delay.
> > >
> > > Side effects (though not main goals) of this series are:
> > > - ti-sn65dsi86 GPIOs are now exported in Linux.
> > > - ti-sn65dsi86 bindings are converted to yaml.
> > > - Common panel bindings now have "hpd-gpios" listed.
> > > - The simple-panel driver in Linux can delay in prepare based on
> > >   "hpd-gpios"
> > > - ti-sn65dsi86 bindings (and current user) now specifies "no-hpd"
> > >   if HPD isn't hooked up.
> > >
> > > Changes in v5:
> > > - Use of_xlate so that numbers in dts start at 1, not 0.
> > > - Squash https://lore.kernel.org/r/20200506140208.v2.2.I0a2bca02b09c1fcb6b09479b489736d600b3e57f@changeid/
> > >
> > > Changes in v4:
> > > - Don't include gpio.h
> > > - Use gpiochip_get_data() instead of container_of() to get data.
> > > - GPIOF_DIR_XXX => GPIO_LINE_DIRECTION_XXX
> > > - Use Linus W's favorite syntax to read a bit from a bitfield.
> > > - Define and use SN_GPIO_MUX_MASK.
> > > - Add a comment about why we use a bitmap for gchip_output.
> > > - Tacked on "or is otherwise unusable." to description.
> > >
> > > Changes in v3:
> > > - Becaue => Because
> > > - Add a kernel-doc to our pdata to clarify double-duty of gchip_output.
> > > - More comments about how powering off affects us (get_dir, dir_input).
> > > - Cleanup tail of ti_sn_setup_gpio_controller() to avoid one "return".
> > > - Use a bitmap rather than rolling my own.
> > > - Remind how gpio_get_optional() works in the commit message.
> > > - useful implement => useful to implement
> > >
> > > Changes in v2:
> > > - ("Export...GPIOs") is 1/2 of replacement for ("Allow...bridge GPIOs")
> > > - ("dt-bindings: display: Add hpd-gpios to panel-common...") new for v2
> > > - ("simple...hpd-gpios") is 1/2 of replacement for ("Allow...bridge GPIOs")
> > > - specification => specifier.
> > > - power up => power.
> > > - Added back missing suspend-gpios.
> > > - data-lanes and lane-polarities are are the right place now.
> > > - endpoints don't need to be patternProperties.
> > > - Specified more details for data-lanes and lane-polarities.
> > > - Added old example back in, fixing bugs in it.
> > > - Example i2c bus is just called "i2c", not "i2c1" now.
> > > - ("dt-bindings: drm/bridge: ti-sn65dsi86: Document no-hpd") new for v2.
> > > - ("arm64: dts: sdm845: Add "no-hpd" to sn65dsi86 on cheza") new for v2.
> > >
> > > Douglas Anderson (6):
> > >   drm/bridge: ti-sn65dsi86: Export bridge GPIOs to Linux
> > >   dt-bindings: display: Add hpd-gpios to panel-common bindings
> > >   drm/panel-simple: Support hpd-gpios for delaying prepare()
> > >   dt-bindings: drm/bridge: ti-sn65dsi86: Convert to yaml
> > >   dt-bindings: drm/bridge: ti-sn65dsi86: Document no-hpd
> > >   arm64: dts: sdm845: Add "no-hpd" to sn65dsi86 on cheza
> >
> > Applied:
> > >   dt-bindings: display: Add hpd-gpios to panel-common bindings
> > >   drm/panel-simple: Support hpd-gpios for delaying prepare()
> > to drm-misc-next.
> >
> > The others was missing reviews so we need to wait for feedback.
>
> Thanks!
>
> Given the previous feedback from Linus W, Stephen, and Laurent I
> expect things are good enough to land now, but it'd be good to get
> confirmation (I removed some of the previous tags just to get
> confirmation).  If we can get review tags early next week maybe it'll
> still be in time to land for 5.8?

I think all the others have reviews now.  Is there anything blocking
them from getting applied?

Thanks!

-Doug

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox