Re: [PATCH v3 4/4] mfd: cros_ec: add RTC as mfd subdevice

[Lee Jones]

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?

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