From: Vitor Soares <Vitor.Soares@synopsys.com>
To: Lorenzo Bianconi <lorenzo@kernel.org>,
Vitor Soares <Vitor.Soares@synopsys.com>
Cc: "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
"linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"broonie@kernel.org" <broonie@kernel.org>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"rafael@kernel.org" <rafael@kernel.org>,
"bbrezillon@knernel.org" <bbrezillon@knernel.org>,
"Joao.Pinto@synopsys.com" <Joao.Pinto@synopsys.com>
Subject: RE: [PATCH v3 3/3] iio: imu: st_lsm6dsx: add i3c basic support for LSM6DSO and LSM6DSR
Date: Thu, 11 Jul 2019 10:12:34 +0000 [thread overview]
Message-ID: <SN6PR12MB2655AC6DF4A959A1705FC8A4AEF30@SN6PR12MB2655.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20190710194405.GA10520@lore-desk-wlan.lan>
Hi Lorenzo,
From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Wed, Jul 10, 2019 at 20:44:05
> > For today the st_lsm6dsx driver support LSM6DSO and LSM6DSR sensor only in
> > spi and i2c mode.
> >
> > The LSM6DSO and LSM6DSR are also i3c capable so lets give i3c support to
> > them.
>
> Hi Vitor,
>
> just few comments inline.
>
> Regards,
> Lorenzo
>
> >
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> > Changes in v3:
> > Remove unnecessary st_lsm6dsx_i3c_data table used to hold device name
> > Use st_lsm6dsx_probe new form
> >
> > Changes in v2:
> > Add support for LSM6DSR
> > Set pm_ops to st_lsm6dsx_pm_ops
> >
> > drivers/iio/imu/st_lsm6dsx/Kconfig | 8 +++-
> > drivers/iio/imu/st_lsm6dsx/Makefile | 1 +
> > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c | 61 +++++++++++++++++++++++++++++
> > 3 files changed, 69 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> >
>
> [...]
>
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > new file mode 100644
> > index 0000000..f683754
> > --- /dev/null
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i3c.c
> > @@ -0,0 +1,61 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates.
> > + *
> > + * Author: Vitor Soares <vitor.soares@synopsys.com>
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/i3c/device.h>
> > +#include <linux/i3c/master.h>
> > +#include <linux/slab.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +
> > +#include "st_lsm6dsx.h"
> > +
> > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[];
> > +
>
> why do we need this? I guess you can just move st_lsm6dsx_i3c_ids definition here
>
> > +static const struct regmap_config st_lsm6dsx_i3c_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > +};
> > +
> > +static int st_lsm6dsx_i3c_probe(struct i3c_device *i3cdev)
> > +{
> > + const struct i3c_device_id *id = i3c_device_match_id(i3cdev,
> > + st_lsm6dsx_i3c_ids);
>
> i3c_device_match_id can theoretically fail so is it better to check
> return value here? (maybe I am too paranoid :))
>
> > + struct regmap *regmap;
> > + int hw_id = (int)id->data;
>
> I guess we do not need this since we use it just in st_lsm6dsx_probe(),
> we can just do:
>
> return st_lsm6dsx_probe(&i3cdev->dev, 0, (int)id->data, regmap);
> > +
> > + regmap = devm_regmap_init_i3c(i3cdev, &st_lsm6dsx_i3c_regmap_config);
> > + if (IS_ERR(regmap)) {
> > + dev_err(&i3cdev->dev, "Failed to register i3c regmap %d\n",
> > + (int)PTR_ERR(regmap));
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + return st_lsm6dsx_probe(&i3cdev->dev, 0, hw_id, regmap);
> > +}
> > +
> > +static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > + I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
> > + I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(i3c, st_lsm6dsx_i3c_ids);
> > +
> > +static struct i3c_driver st_lsm6dsx_driver = {
> > + .driver = {
> > + .name = "st_lsm6dsx_i3c",
> > + .pm = &st_lsm6dsx_pm_ops,
> > + },
> > + .probe = st_lsm6dsx_i3c_probe,
> > + .id_table = st_lsm6dsx_i3c_ids,
> > +};
> > +module_i3c_driver(st_lsm6dsx_driver);
> > +
> > +MODULE_AUTHOR("Vitor Soares <vitor.soares@synopsys.com>");
> > +MODULE_DESCRIPTION("STMicroelectronics st_lsm6dsx i3c driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
Thanks for your comments. I will address them and send a new version.
Best regards,
Vitor Soares
next prev parent reply other threads:[~2019-07-11 10:12 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-10 14:56 [PATCH v3 0/3] Add ST lsm6dso i3c support Vitor Soares
2019-07-10 14:56 ` [PATCH v3 1/3] regmap: add i3c bus support Vitor Soares
2019-07-10 15:02 ` Mark Brown
2019-07-10 14:56 ` [PATCH v3 2/3] i3c: move i3c_device_match_id to device.c and export it Vitor Soares
2019-07-10 14:56 ` [PATCH v3 3/3] iio: imu: st_lsm6dsx: add i3c basic support for LSM6DSO and LSM6DSR Vitor Soares
2019-07-10 19:44 ` Lorenzo Bianconi
2019-07-11 10:12 ` Vitor Soares [this message]
2019-07-11 11:00 ` Vitor Soares
2019-07-11 12:44 ` Lorenzo Bianconi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN6PR12MB2655AC6DF4A959A1705FC8A4AEF30@SN6PR12MB2655.namprd12.prod.outlook.com \
--to=vitor.soares@synopsys.com \
--cc=Joao.Pinto@synopsys.com \
--cc=bbrezillon@knernel.org \
--cc=broonie@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-i3c@lists.infradead.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lorenzo@kernel.org \
--cc=rafael@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).