by Jose Abreu

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver

HI Sylwester,

On 19-06-2017 23:10, Sylwester Nawrocki wrote:
> On 06/19/2017 11:33 AM, Jose Abreu wrote:
>> On 18-06-2017 19:04, Sylwester Nawrocki wrote:
>>> On 06/16/2017 06:38 PM, Jose Abreu wrote:
>>>> This is an initial submission for the Synopsys Designware HDMI RX
>>>> Controller Driver. This driver interacts with a phy driver so that
>>>> a communication between them is created and a video pipeline is
>>>> configured.
>>>>
>>>> The controller + phy pipeline can then be integrated into a fully
>>>> featured system that can be able to receive video up to 4k@60Hz
>>>> with deep color 48bit RGB, depending on the platform. Although,
>>>> this initial version does not yet handle deep color modes.
>>>> Signed-off-by: Jose Abreu <[email protected]>
>>>>
>>>> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev)
>>>> +{
>>>> + request_module(pdevinfo.name);
>>>> +
>>>> + dw_dev->phy_pdev = platform_device_register_full(&pdevinfo);
>>>> + if (IS_ERR(dw_dev->phy_pdev)) {
>>>> + dev_err(dw_dev->dev, "failed to register phy device\n");
>>>> + return PTR_ERR(dw_dev->phy_pdev);
>>>> + }
>>>> +
>>>> + if (!dw_dev->phy_pdev->dev.driver) {
>>>> + dev_err(dw_dev->dev, "failed to initialize phy driver\n");
>>>> + goto err;
>>>> + }
>>> I think this is not safe because there is nothing preventing unbinding
>>> or unloading the driver at this point.
>>>
>>>> + if (!try_module_get(dw_dev->phy_pdev->dev.driver->owner)) {
>>> So dw_dev->phy_pdev->dev.driver may be already NULL here.
>> How can I make sure it wont be NULL? Because I've seen other
>> media drivers do this and I don't think they do any kind of
>> locking, but they do this mainly for I2C subdevs.
> You could do device_lock(dev)/device_unlock(dev) to avoid possible races.
> And setting 'suppress_bind_attrs' field in the sub-device drivers would
> disable sysfs unbind attributes, so sub-device driver wouldn't get unbound
> unexpectedly trough sysfs.

Hmm, ok. I changed this, now I'm using driver_find() +
driver_for_each_device(). Its working but I'm starting to think
about whether this should go into v4l2 core because I've seen
other drivers also do this.

>
>>>> + dev_err(dw_dev->dev, "failed to get phy module\n");
>>>> + goto err;
>>>> + }
>>>> +
>>>> + dw_dev->phy_sd = dev_get_drvdata(&dw_dev->phy_pdev->dev);
>>>> + if (!dw_dev->phy_sd) {
>>>> + dev_err(dw_dev->dev, "failed to get phy subdev\n");
>>>> + goto err_put;
>>>> + }
>>>> +
>>>> + if (v4l2_device_register_subdev(&dw_dev->v4l2_dev, dw_dev->phy_sd)) {
>>>> + dev_err(dw_dev->dev, "failed to register phy subdev\n");
>>>> + goto err_put;
>>>> + }
>>> I'd suggest usign v4l2-async API, so we use a common pattern for sub-device
>>> registration. And with recent change [1] you could handle this PHY subdev
>>> in a standard way. That might be more complicated than it is now but should
>>> make any future platform integration easier.
>> So I will instantiate phy driver and then wait for phy driver to
>> register into v4l2 core?
> Yes, for instance the RX controller driver registers a notifier, instantiates
> the child PHY device and then waits until the PHY driver completes initialization.
>
>>>> + module_put(dw_dev->phy_pdev->dev.driver->owner);
>>>> + return 0;
>>>> +
>>>> +err_put:
>>>> + module_put(dw_dev->phy_pdev->dev.driver->owner);
>>>> +err:
>>>> + platform_device_unregister(dw_dev->phy_pdev);
>>>> + return -EINVAL;
>>>> +}
>>>> +static int dw_hdmi_power_on(struct dw_hdmi_dev *dw_dev, u32 input)
>>>> +{
>>>> + struct dw_hdmi_work_data *data;
>>>> + unsigned long flags;
>>>> +
>>>> + data = devm_kzalloc(dw_dev->dev, sizeof(*data), GFP_KERNEL);
>>> Why use devm_{kzalloc, kfree} when dw_hdmi_power_on() is not only called
>>> in the device's probe() callback, but in other places, including interrupt
>>> handler? devm_* API is normally used when life time of a resource is more
>>> or less equal to life time of struct device or its matched driver. Were
>>> there any specific reasons to not just use kzalloc()/kfree() ?
>> No specific reason, I just thought it would be safer because if I
>> cancel a work before it started then memory will remain
>> allocated. But I will change to kzalloc().
> OK, I overlooked such situation. Since you allow one job queued maybe
> just embed struct work_struct in struct dw_hdmi_dev and retrieve it with
> container_of() macro in the work handler and use additional field in
> struct dw_hdmi_dev protected with dw_dev->lock for passing the input
> index?

Yes, seems ok. As I'm already locking before queuing work and
also checking if theres is a pending work I can just overwrite
configured_input earlier.

Best regards,
Jose Miguel Abreu

>
>>>> + if (!data)
>>>> + return -ENOMEM;
>>>> +
>>>> + INIT_WORK(&data->work, dw_hdmi_work_handler);
>>>> + data->dw_dev = dw_dev;
>>>> + data->input = input;
>>>> +
>>>> + spin_lock_irqsave(&dw_dev->lock, flags);
>>>> + if (dw_dev->pending_config) {
>>>> + devm_kfree(dw_dev->dev, data);
>>>> + spin_unlock_irqrestore(&dw_dev->lock, flags);
>>>> + return 0;
>>>> + }
>>>> +
>>>> + queue_work(dw_dev->wq, &data->work);
>>>> + dw_dev->pending_config = true;
>>>> + spin_unlock_irqrestore(&dw_dev->lock, flags);
>>>> + return 0;
>>>> +}
>>>> +struct dw_hdmi_rx_pdata {
>>>> + /* Controller configuration */
>>>> + struct dw_hdmi_hdcp14_key hdcp14_keys;
>>>> + /* 5V sense interface */
>>>> + bool (*dw_5v_status)(void __iomem *regs, int input);
>>>> + void (*dw_5v_clear)(void __iomem *regs);
>>>> + void __iomem *dw_5v_arg;> + /* Zcal interface */
>>>> + void (*dw_zcal_reset)(void __iomem *regs);
>>>> + bool (*dw_zcal_done)(void __iomem *regs);
>>>> + void __iomem *dw_zcal_arg;
>>> I'm just wondering if these operations could be modeled with the regmap,
>>> so we could avoid callbacks in the platform data structure.
>> Hmm, I don't think that is safe because registers may not be
>> adjacent to each other. And maybe I was a little generous in
>> passing a __iomem argument, maybe it should be just void instead
>> because this can be not a regmap at all.
> I meant two separate regmaps, but it's not that good anyway, since
> register address and register bit fields not specific to the HDMI RX
> block would need to be handled in this driver.
>
> --
> Regards,
> Sylwester