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

[Lee Jones]

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