Re: [PATCH 2/3] fsi: occ: Add support for P10

[Guenter Roeck]

On 7/19/20 9:47 PM, Joel Stanley wrote:
> On Sun, 19 Jul 2020 at 22:13, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>
>> On Fri, May 01, 2020 at 10:08:32AM -0500, Eddie James wrote:
>>> The P10 OCC has a different SRAM address for the command and response
>>> buffers. In addition, the SBE commands to access the SRAM have changed
>>> format. Add versioning to the driver to handle these differences.
>>>
>>> Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>
>>
>> I don't recall seeing a maintainer Ack for this patch, nor any response
>> at all. I'd be happy to apply the patch through hwmon, but I would need
>> an Ack from a maintainer.
> 
> That would be great. I had one question before it goes in, but once
> Eddie has sorted that out it can go through your tree.
> 
>>
>> Thanks,
>> Guenter
>>
>>> ---
>>>  drivers/fsi/fsi-occ.c | 126 ++++++++++++++++++++++++++++++------------
>>>  1 file changed, 92 insertions(+), 34 deletions(-)
> 
>>> @@ -508,6 +557,7 @@ static int occ_probe(struct platform_device *pdev)
>>>       struct occ *occ;
>>>       struct platform_device *hwmon_dev;
>>>       struct device *dev = &pdev->dev;
>>> +     const void *md =  of_device_get_match_data(dev);
>>>       struct platform_device_info hwmon_dev_info = {
>>>               .parent = dev,
>>>               .name = "occ-hwmon",
>>> @@ -517,6 +567,7 @@ static int occ_probe(struct platform_device *pdev)
>>>       if (!occ)
>>>               return -ENOMEM;
>>>
>>> +     occ->version = (enum versions)md;
> 
> The 0day bot warns about this when bulding for 64 bit architectures.
> 
> How about you drop the match data and instead match on the compatible
> string to know which version to expect?
> 

That seems to be less desirable and defeat the purpose of of_device_get_match_data().
I have seen better solutions. Some options:

	version = (uintptr_t)of_device_get_match_data(dev);
	version = (unsigned long)of_device_get_match_data(dev);
	version = (enum versions)(uintptr_t)of_device_get_match_data(dev);

You don't otherwise use the "md" variable, so you might as well drop it.

Guenter

>>>       occ->dev = dev;
>>>       occ->sbefifo = dev->parent;
>>>       mutex_init(&occ->occ_lock);
>>> @@ -575,7 +626,14 @@ static int occ_remove(struct platform_device *pdev)
>>>  }
>>>
>>>  static const struct of_device_id occ_match[] = {
>>> -     { .compatible = "ibm,p9-occ" },
>>> +     {
>>> +             .compatible = "ibm,p9-occ",
>>> +             .data = (void *)occ_p9
>>> +     },
>>> +     {
>>> +             .compatible = "ibm,p10-occ",
>>> +             .data = (void *)occ_p10
>>> +     },
>>>       { },
>>>  };
>>>