Re: [PATCH v2 4/5] mfd: cros_ec_dev: Add CEC sub-device registration

2018-05-18 Thread Enric Balletbo Serra 
2018-05-18 15:05 GMT+02:00 Neil Armstrong :
> The EC can expose a CEC bus, thus add the cros-ec-cec MFD sub-device
> when the CEC feature bit is present.
>
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/mfd/cros_ec_dev.c | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 1d6dc5c..272969e 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -383,6 +383,10 @@ static void cros_ec_sensors_register(struct cros_ec_dev 
> *ec)
> kfree(msg);
>  }
>
> +static const struct mfd_cell cros_ec_cec_cells[] = {
> +   { .name = "cros-ec-cec" }
> +};
> +
>  static const struct mfd_cell cros_ec_rtc_cells[] = {
> { .name = "cros-ec-rtc" }
>  };
> @@ -426,6 +430,18 @@ 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 CEC host command support */
> +   if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
> +   retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> +cros_ec_cec_cells,
> +ARRAY_SIZE(cros_ec_cec_cells),
> +NULL, 0, NULL);
> +   if (retval)
> +   dev_err(ec->dev,
> +   "failed to add cros-ec-cec device: %d\n",
> +   retval);
> +   }
> +
> /* Check whether this EC instance has RTC host command support */
> if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
> retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> --
> 2.7.4
>

Reviewed-by: Enric Balletbo i Serra 

Thanks,
 Enric

Re: [PATCH v2 4/5] mfd: cros_ec_dev: Add CEC sub-device registration

2018-05-16 Thread Neil Armstrong 
Hi Enric,

On 15/05/2018 18:40, Enric Balletbo Serra wrote:
> Hi Neil,
> 
> I suspect that this patch will conflict with some patches that will be
> queued for 4.18 that also introduces new devices, well, for now I
> don't see these merged in the Lee's tree.

Indeed, I found your patches, I'll rebase this one when Lee pushes them in his 
tree.

> 
> Based on some reviews I got when I send a patch to this file ...
> 
> 2018-05-15 17:29 GMT+02:00 Hans Verkuil :
>> On 05/15/2018 04:42 PM, Neil Armstrong wrote:
>>> The EC can expose a CEC bus, thus add the cros-ec-cec MFD sub-device
>>> when the CEC feature bit is present.
>>>
>>> Signed-off-by: Neil Armstrong 
>>
>> For what it is worth (not an MFD expert):
>>
>> Acked-by: Hans Verkuil 
>>
>> Thanks!
>>
>> Hans
>>
>>> ---
>>>  drivers/mfd/cros_ec_dev.c | 16 
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>>> index eafd06f..57064ec 100644
>>> --- a/drivers/mfd/cros_ec_dev.c
>>> +++ b/drivers/mfd/cros_ec_dev.c
>>> @@ -383,6 +383,18 @@ static void cros_ec_sensors_register(struct 
>>> cros_ec_dev *ec)
>>>   kfree(msg);
>>>  }
>>>
>>> +static void cros_ec_cec_register(struct cros_ec_dev *ec)
>>> +{
>>> + int ret;
>>> + struct mfd_cell cec_cell = {
>>> + .name = "cros-ec-cec",
>>> + };
>>> +
>>> + ret = mfd_add_devices(ec->dev, 0, _cell, 1, NULL, 0, NULL);
>>> + if (ret)
>>> + dev_err(ec->dev, "failed to add EC CEC\n");
>>> +}
>>> +
> 
> Do not create a single function to only call mfd_add_devices, instead
> do the following on top:
> 
> static const struct mfd_cell cros_ec_cec_cells[] = {
> { .name = "cros-ec-cec" }
> };

OK

> 
> 
>>>  static int ec_device_probe(struct platform_device *pdev)
>>>  {
>>>   int retval = -ENOMEM;
>>> @@ -422,6 +434,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 handles CEC. */
>>> + if (cros_ec_check_features(ec, EC_FEATURE_CEC))
>>> + cros_ec_cec_register(ec);
>>> +
> 
> and use PLATFORM_DEVID_AUTO and the ARRAY_SIZE macro, something like this.
> 
> /* Check whether this EC instance handles CEC */
> if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
> retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
>   cros_ec_cec_cells,
>   
> ARRAY_SIZE(cros_ec_cec_cells),
>   NULL, 0, NULL);
> if (retval)
> dev_err(ec->dev, "failed to add cros-ec-cec device: %d\n",
>  retval);
> }

Ok, like the RTC registration.

Thanks,
Neil

> 
> Best regards,
>   Enric
> 
>>>   /* Take control of the lightbar from the EC. */
>>>   lb_manual_suspend_ctrl(ec, 1);
>>>
>>>
>>
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/5] mfd: cros_ec_dev: Add CEC sub-device registration

2018-05-15 Thread Enric Balletbo Serra 
Hi Neil,

I suspect that this patch will conflict with some patches that will be
queued for 4.18 that also introduces new devices, well, for now I
don't see these merged in the Lee's tree.

Based on some reviews I got when I send a patch to this file ...

2018-05-15 17:29 GMT+02:00 Hans Verkuil :
> On 05/15/2018 04:42 PM, Neil Armstrong wrote:
>> The EC can expose a CEC bus, thus add the cros-ec-cec MFD sub-device
>> when the CEC feature bit is present.
>>
>> Signed-off-by: Neil Armstrong 
>
> For what it is worth (not an MFD expert):
>
> Acked-by: Hans Verkuil 
>
> Thanks!
>
> Hans
>
>> ---
>>  drivers/mfd/cros_ec_dev.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>> index eafd06f..57064ec 100644
>> --- a/drivers/mfd/cros_ec_dev.c
>> +++ b/drivers/mfd/cros_ec_dev.c
>> @@ -383,6 +383,18 @@ static void cros_ec_sensors_register(struct cros_ec_dev 
>> *ec)
>>   kfree(msg);
>>  }
>>
>> +static void cros_ec_cec_register(struct cros_ec_dev *ec)
>> +{
>> + int ret;
>> + struct mfd_cell cec_cell = {
>> + .name = "cros-ec-cec",
>> + };
>> +
>> + ret = mfd_add_devices(ec->dev, 0, _cell, 1, NULL, 0, NULL);
>> + if (ret)
>> + dev_err(ec->dev, "failed to add EC CEC\n");
>> +}
>> +

Do not create a single function to only call mfd_add_devices, instead
do the following on top:

static const struct mfd_cell cros_ec_cec_cells[] = {
{ .name = "cros-ec-cec" }
};


>>  static int ec_device_probe(struct platform_device *pdev)
>>  {
>>   int retval = -ENOMEM;
>> @@ -422,6 +434,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 handles CEC. */
>> + if (cros_ec_check_features(ec, EC_FEATURE_CEC))
>> + cros_ec_cec_register(ec);
>> +

and use PLATFORM_DEVID_AUTO and the ARRAY_SIZE macro, something like this.

/* Check whether this EC instance handles CEC */
if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
  cros_ec_cec_cells,
  ARRAY_SIZE(cros_ec_cec_cells),
  NULL, 0, NULL);
if (retval)
dev_err(ec->dev, "failed to add cros-ec-cec device: %d\n",
 retval);
}

Best regards,
  Enric

>>   /* Take control of the lightbar from the EC. */
>>   lb_manual_suspend_ctrl(ec, 1);
>>
>>
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/5] mfd: cros_ec_dev: Add CEC sub-device registration

2018-05-15 Thread Hans Verkuil 
On 05/15/2018 04:42 PM, Neil Armstrong wrote:
> The EC can expose a CEC bus, thus add the cros-ec-cec MFD sub-device
> when the CEC feature bit is present.
> 
> Signed-off-by: Neil Armstrong 

For what it is worth (not an MFD expert):

Acked-by: Hans Verkuil 

Thanks!

Hans

> ---
>  drivers/mfd/cros_ec_dev.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index eafd06f..57064ec 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -383,6 +383,18 @@ static void cros_ec_sensors_register(struct cros_ec_dev 
> *ec)
>   kfree(msg);
>  }
>  
> +static void cros_ec_cec_register(struct cros_ec_dev *ec)
> +{
> + int ret;
> + struct mfd_cell cec_cell = {
> + .name = "cros-ec-cec",
> + };
> +
> + ret = mfd_add_devices(ec->dev, 0, _cell, 1, NULL, 0, NULL);
> + if (ret)
> + dev_err(ec->dev, "failed to add EC CEC\n");
> +}
> +
>  static int ec_device_probe(struct platform_device *pdev)
>  {
>   int retval = -ENOMEM;
> @@ -422,6 +434,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 handles CEC. */
> + if (cros_ec_check_features(ec, EC_FEATURE_CEC))
> + cros_ec_cec_register(ec);
> +
>   /* Take control of the lightbar from the EC. */
>   lb_manual_suspend_ctrl(ec, 1);
>  
>