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

[Enric Balletbo i Serra <enric.balletbo@collabora.com>]

Hi Lee,

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.

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:
>