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

[Enric Balletbo i Serra]

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?

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