Re: [PATCH v3 4/4] mfd: cros_ec: add RTC as mfd subdevice
From: Lee Jones
Date: Wed Mar 15 2017 - 08:12:12 EST
On Wed, 15 Mar 2017, Enric Balletbo i Serra wrote:
> Hi Lee,
>
> On 15/03/17 11:24, Lee Jones wrote:
> > 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@xxxxxxxxxxxx>
> >>>>
> >>>> If the EC supports RTC host commands, expose an RTC device.
> >>>>
> >>>> Signed-off-by: Stephen Barber <smbarber@xxxxxxxxxxxx>
> >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> >>>> Acked-by: Benson Leung <bleung@xxxxxxxxxxxx>
> >>>> ---
> >>>> 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?
> >
>
> So just to be clear before I start to work on it. What you want is I get rid of
> the MFD stuff from drivers/platform/chrome/cros_ec_dev and move to
> drivers/mfd/cros_ec.c. The platform/chrome driver should publish how to check
> the features and leave the mfd/cros_ec driver add the MFD subdevices.
>
> For this patch series this means get rid of:
>
> drivers/platform/chrome/cros_ec_dev.c:412:static const struct mfd_cell
> cros_ec_rtc_devs[] ...
>
> drivers/platform/chrome/cros_ec_dev.c:404: ret = mfd_add_devices(ec->dev,
> 0,cros_ec_rtc_devs ...
>
> As I said the sensors subdevice (which is already upstream) uses the same
> approach, so I think you will also want do the same for the sensors subdevice?
> Sounds good if first we try to land the RTC part and the I'll prepare a patch
> series to fix the sensors hub part?
You got it. Sounds good.
> > 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.
> >
>
> Yeah, I did not try to use the SSP IIO driver as a good example to justify
> myself, I just wasted let you know that the approach we used is also used in
> other cases, thus if the approach is wrong we should have in mind that we should
> also fix the SSP IIO driver. Just this.
>
> Thanks,
> Enric
>
> >>>> 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