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 <smbar...@chromium.org>
> >>
> >> If the EC supports RTC host commands, expose an RTC device.
> >>
> >> Signed-off-by: Stephen Barber <smbar...@chromium.org>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>
> >> Acked-by: Benson Leung <ble...@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
