From: Lee Jones <lee.jones@linaro.org> To: Enric Balletbo i Serra <enric.balletbo@collabora.com> Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>, linux-kernel@vger.kernel.org, rtc-linux@googlegroups.com, Olof Johansson <olof@lixom.net>, Stephen Barber <smbarber@chromium.org>, Jonathan Cameron <jic23@cam.ac.uk> Subject: Re: [PATCH v3 4/4] mfd: cros_ec: add RTC as mfd subdevice Date: Wed, 15 Mar 2017 10:24:49 +0000 [thread overview] Message-ID: <20170315102449.bqr3hezlrdovjyrq@dell> (raw) In-Reply-To: <f934a89e-8f0b-5a74-1c6f-8b9f1d85ba72@collabora.com> On Tue, 14 Mar 2017, Enric Balletbo i Serra wrote: > On 14/03/17 14:59, Lee Jones wrote: > > On Tue, 14 Feb 2017, Enric Balletbo i Serra wrote: > > > >> From: Stephen Barber <smbarber@chromium.org> > >> > >> If the EC supports RTC host commands, expose an RTC device. > >> > >> Signed-off-by: Stephen Barber <smbarber@chromium.org> > >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > >> Acked-by: Benson Leung <bleung@chromium.org> > >> --- > >> Changes since v2: > >> - Acked by Benson Leung > >> Changes since v1: > >> - none > >> > >> drivers/platform/chrome/cros_ec_dev.c | 22 ++++++++++++++++++++++ > >> 1 file changed, 22 insertions(+) > >> > >> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c > >> index 47268ec..ebe029d 100644 > >> --- a/drivers/platform/chrome/cros_ec_dev.c > >> +++ b/drivers/platform/chrome/cros_ec_dev.c > >> @@ -383,6 +383,24 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec) > >> kfree(msg); > >> } > >> > >> +static const struct mfd_cell cros_ec_rtc_devs[] = { > >> + { > >> + .name = "cros-ec-rtc", > >> + .id = -1, > >> + }, > >> +}; > >> + > >> +static void cros_ec_rtc_register(struct cros_ec_dev *ec) > >> +{ > >> + int ret; > >> + > >> + ret = mfd_add_devices(ec->dev, 0, cros_ec_rtc_devs, > >> + ARRAY_SIZE(cros_ec_rtc_devs), > >> + NULL, 0, NULL); > >> + if (ret) > >> + dev_err(ec->dev, "failed to add cros-ec-rtc device: %d\n", ret); > >> +} > > > > Holey poop! Why are you using the MFD API outside of MFD? > > > > Why can't you register this from the MFD driver? > > > > Actually the MFD doesn't know how to check if this feature is available or not, > instead is the platform driver cros_ec_dev who knows how to check this and if it > exists adds the rtc device. > > if (cros_ec_check_features(ec, EC_FEATURE_RTC)) > cros_ec_rtc_register(ec); /* add the mfd device */ > > Same approach was used in the same file for the Sensors Hub (already upstream). See: > > drivers/platform/chrome/cros_ec_dev.c:462 > drivers/platform/chrome/cros_ec_dev.c:372 > > I didn't know that the MFD API was restricted outside MFD. In such case what I > need to do is let know the MFD driver about the cros_ec_check_features > (implemented in platform driver cros_ec_dev), this doesn't seems good to me but > I might be wrong. So please, let me know which option do you prefer and if it's > the case we will need to change I'll try to do it. > > Note that I think that a similar use case is used in > drivers/iio/common/ssp_sensors/ssp_dev.c:535, where the iio driver registers the > sensors to the mfd. It would be advantageous to avoid a web of inter-subsystem calls to register devices. I think I could bear calls to mfd_add_* from drivers/platform, as the two subsystems are fairly interchangeable, and it does have the added benefit of saving duplication of the device registering code. Calling mfd_add_* from IIO seems plain wrong though. A better solution however and one that we've utilised in the past is to have the MFD drivers call into the platform (i.e. drivers/platform) drivers to see if certain devices are available. Is this possible in your use-case? NB: I've just had a look at the SSP IIO driver and I have a number of problems with it. You should not be using that as a good example of why mfd_add_* should be used outside of MFD. > >> static int ec_device_probe(struct platform_device *pdev) > >> { > >> int retval = -ENOMEM; > >> @@ -441,6 +459,10 @@ static int ec_device_probe(struct platform_device *pdev) > >> if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE)) > >> cros_ec_sensors_register(ec); > >> > >> + /* check whether this EC instance has RTC host command support */ > >> + if (cros_ec_check_features(ec, EC_FEATURE_RTC)) > >> + cros_ec_rtc_register(ec); > >> + > >> return 0; > >> > >> dev_reg_failed: > > -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
WARNING: multiple messages have this Message-ID (diff)
From: Lee Jones <lee.jones@linaro.org> To: Enric Balletbo i Serra <enric.balletbo@collabora.com> Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>, linux-kernel@vger.kernel.org, rtc-linux@googlegroups.com, Olof Johansson <olof@lixom.net>, Stephen Barber <smbarber@chromium.org>, Jonathan Cameron <jic23@cam.ac.uk> Subject: [rtc-linux] Re: [PATCH v3 4/4] mfd: cros_ec: add RTC as mfd subdevice Date: Wed, 15 Mar 2017 10:24:49 +0000 [thread overview] Message-ID: <20170315102449.bqr3hezlrdovjyrq@dell> (raw) In-Reply-To: <f934a89e-8f0b-5a74-1c6f-8b9f1d85ba72@collabora.com> On Tue, 14 Mar 2017, Enric Balletbo i Serra wrote: > On 14/03/17 14:59, Lee Jones wrote: > > On Tue, 14 Feb 2017, Enric Balletbo i Serra wrote: > >=20 > >> From: Stephen Barber <smbarber@chromium.org> > >> > >> If the EC supports RTC host commands, expose an RTC device. > >> > >> Signed-off-by: Stephen Barber <smbarber@chromium.org> > >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > >> Acked-by: Benson Leung <bleung@chromium.org> > >> --- > >> Changes since v2: > >> - Acked by Benson Leung > >> Changes since v1: > >> - none > >> > >> drivers/platform/chrome/cros_ec_dev.c | 22 ++++++++++++++++++++++ > >> 1 file changed, 22 insertions(+) > >> > >> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/= chrome/cros_ec_dev.c > >> index 47268ec..ebe029d 100644 > >> --- a/drivers/platform/chrome/cros_ec_dev.c > >> +++ b/drivers/platform/chrome/cros_ec_dev.c > >> @@ -383,6 +383,24 @@ static void cros_ec_sensors_register(struct cros_= ec_dev *ec) > >> kfree(msg); > >> } > >> =20 > >> +static const struct mfd_cell cros_ec_rtc_devs[] =3D { > >> + { > >> + .name =3D "cros-ec-rtc", > >> + .id =3D -1, > >> + }, > >> +}; > >> + > >> +static void cros_ec_rtc_register(struct cros_ec_dev *ec) > >> +{ > >> + int ret; > >> + > >> + ret =3D mfd_add_devices(ec->dev, 0, cros_ec_rtc_devs, > >> + ARRAY_SIZE(cros_ec_rtc_devs), > >> + NULL, 0, NULL); > >> + if (ret) > >> + dev_err(ec->dev, "failed to add cros-ec-rtc device: %d\n", ret); > >> +} > >=20 > > Holey poop! Why are you using the MFD API outside of MFD? > >=20 > > Why can't you register this from the MFD driver? > >=20 >=20 > Actually the MFD doesn't know how to check if this feature is available o= r not, > instead is the platform driver cros_ec_dev who knows how to check this an= d if it > exists adds the rtc device. >=20 > if (cros_ec_check_features(ec, EC_FEATURE_RTC)) > cros_ec_rtc_register(ec); /* add the mfd device */ >=20 > Same approach was used in the same file for the Sensors Hub (already upst= ream). See: >=20 > drivers/platform/chrome/cros_ec_dev.c:462 > drivers/platform/chrome/cros_ec_dev.c:372 >=20 > I didn't know that the MFD API was restricted outside MFD. In such case w= hat I > need to do is let know the MFD driver about the cros_ec_check_features > (implemented in platform driver cros_ec_dev), this doesn't seems good to = me but > I might be wrong. So please, let me know which option do you prefer and i= f it's > the case we will need to change I'll try to do it. >=20 > Note that I think that a similar use case is used in > drivers/iio/common/ssp_sensors/ssp_dev.c:535, where the iio driver regist= ers the > sensors to the mfd. It would be advantageous to avoid a web of inter-subsystem calls to register devices. I think I could bear calls to mfd_add_* from drivers/platform, as the two subsystems are fairly interchangeable, and it does have the added benefit of saving duplication of the device registering code. Calling mfd_add_* from IIO seems plain wrong though. A better solution however and one that we've utilised in the past is to have the MFD drivers call into the platform (i.e. drivers/platform) drivers to see if certain devices are available. Is this possible in your use-case? NB: I've just had a look at the SSP IIO driver and I have a number of problems with it. You should not be using that as a good example of why mfd_add_* should be used outside of MFD. > >> static int ec_device_probe(struct platform_device *pdev) > >> { > >> int retval =3D -ENOMEM; > >> @@ -441,6 +459,10 @@ static int ec_device_probe(struct platform_device= *pdev) > >> if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE)) > >> cros_ec_sensors_register(ec); > >> =20 > >> + /* check whether this EC instance has RTC host command support */ > >> + if (cros_ec_check_features(ec, EC_FEATURE_RTC)) > >> + cros_ec_rtc_register(ec); > >> + > >> return 0; > >> =20 > >> dev_reg_failed: > >=20 --=20 Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org =E2=94=82 Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog --=20 You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. ---=20 You received this message because you are subscribed to the Google Groups "= rtc-linux" group. To unsubscribe from this group and stop receiving emails from it, send an e= mail to rtc-linux+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
next prev parent reply other threads:[~2017-03-15 10:24 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-02-14 22:15 [PATCH v3 1/4] mfd: cros_ec: Add helper for event notifier Enric Balletbo i Serra 2017-02-14 22:15 ` [rtc-linux] " Enric Balletbo i Serra 2017-02-14 22:15 ` [PATCH v3 2/4] mfd: cros_ec: Introduce RTC commands and events definitions Enric Balletbo i Serra 2017-02-14 22:15 ` [rtc-linux] " Enric Balletbo i Serra 2017-02-14 22:15 ` [PATCH v3 3/4] rtc: cros-ec: add cros-ec-rtc driver Enric Balletbo i Serra 2017-02-14 22:15 ` [rtc-linux] " Enric Balletbo i Serra 2017-02-14 22:15 ` [PATCH v3 4/4] mfd: cros_ec: add RTC as mfd subdevice Enric Balletbo i Serra 2017-02-14 22:15 ` [rtc-linux] " Enric Balletbo i Serra 2017-03-14 13:59 ` Lee Jones 2017-03-14 13:59 ` [rtc-linux] " Lee Jones 2017-03-14 14:44 ` Enric Balletbo i Serra 2017-03-14 14:44 ` [rtc-linux] " Enric Balletbo i Serra 2017-03-15 10:24 ` Lee Jones [this message] 2017-03-15 10:24 ` Lee Jones 2017-03-15 11:22 ` Enric Balletbo i Serra 2017-03-15 11:22 ` [rtc-linux] " Enric Balletbo i Serra 2017-03-15 12:10 ` Lee Jones 2017-03-15 12:10 ` [rtc-linux] " Lee Jones 2017-03-21 18:04 ` Tracy Smith 2017-07-12 10:13 [PATCH v3 0/4] mfd: cros-ec: Some fixes and improvements Enric Balletbo i Serra 2017-07-12 10:13 ` [PATCH v3 4/4] mfd: cros_ec: add RTC as mfd subdevice Enric Balletbo i Serra 2017-07-20 7:15 ` Lee Jones
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=20170315102449.bqr3hezlrdovjyrq@dell \ --to=lee.jones@linaro.org \ --cc=alexandre.belloni@free-electrons.com \ --cc=enric.balletbo@collabora.com \ --cc=jic23@cam.ac.uk \ --cc=linux-kernel@vger.kernel.org \ --cc=olof@lixom.net \ --cc=rtc-linux@googlegroups.com \ --cc=smbarber@chromium.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.