On 25/07/16 16:41, Lorenzo Pieralisi wrote:
[...]
>>> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
>>> index 308791f..2362232 100644
>>> --- a/include/linux/of_iommu.h
>>> +++ b/include/linux/of_iommu.h
>>> @@ -15,13 +15,8 @@ extern void of_iommu_init(void);
>>> extern const struct iommu_ops *of_iommu_configure(struct device *dev,
>>> struct device_node *master_np);
>>>
>>> -struct iommu_fwspec {
>>> - const struct iommu_ops *iommu_ops;
>>> - struct device_node *iommu_np;
>>> - void *iommu_priv;
>>> - unsigned int num_ids;
>>> - u32 ids[];
>>> -};
>>> +void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
>>> +const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
>>
>> Is there some reason we need to retain the existing definitions of
>> these? I was assuming we'd be able to move the entire implementation
>> over to the fwspec code and leave behind nothing more than trivial
>> wrappers, e.g.:
>>
>> #define of_iommu_get_ops(np) iommu_fwspec_get_ops(&(np)->fwnode_handle)
>
> Yep, that's exactly what I did but then I was bitten by config
> dependencies. If we implement of_iommu_get/set_ops() as wrappers,
> we have to compile iommu_fwspec_get/set_ops() on arches that may
> not have struct dev_archdata.iommu, unless we introduce yet another
> config symbol to avoid compiling that code (see eg iommu_fwspec_init(),
> we can't compile it on eg x86 even though we do need of_iommu_get_ops()
> on it - so iommu_fwspec_get_ops(), that lives in the same compilation
> unit as eg iommu_fwspec_init()).
>
> So short answer is: there is no reason apart from dev_archdata.iommu
> being arch specific, if we were able to move iommu_fwspec to generic
> code (ie struct device, somehow) I would certainly get rid of this
> stupid code duplication (or as I said I can add a config entry for
> that, more ideas are welcome).
OK, given Rob's comment as well, I guess breaking that dependency is to
everyone's benefit. Since it's quite closely related, how about if we
follow the arch_setup_dma_ops() pattern with an
arch_{get,set}_iommu_fwspec(dev) type thing?
Robin.
>
> Thanks,
> Lorenzo
>
>>
>> Robin.
>>
>>> #else
>>>
>>> @@ -39,17 +34,14 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
>>> return NULL;
>>> }
>>>
>>> -struct iommu_fwspec;
>>> -
>>> -#endif /* CONFIG_OF_IOMMU */
>>> +static inline void of_iommu_set_ops(struct device_node *np,
>>> + const struct iommu_ops *ops)
>>> +{ }
>>>
>>> -int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
>>> -void iommu_fwspec_free(struct device *dev);
>>> -int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>>> -struct iommu_fwspec *dev_iommu_fwspec(struct device *dev);
>>> +static inline const struct iommu_ops *
>>> +of_iommu_get_ops(struct device_node *np) { return NULL; }
>>>
>>> -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
>>> -const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
>>> +#endif /* CONFIG_OF_IOMMU */
>>>
>>> extern struct of_device_id __iommu_of_table;
>>>
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
