Hi Sylwester,
On Sun, Feb 3, 2013 at 6:57 PM, Sylwester Nawrocki
<sylvester.nawrocki@xxxxxxxxx> wrote:
> Hi Prabhakar,
>
> On 02/03/2013 11:13 AM, Prabhakar Lad wrote:
> [...]
>
>>>> +Required Properties :
>>>> +- compatible: Must be "ti,tvp514x-decoder"
>>>
>>>
>>>
>>> There are no significant differences among TVP514* devices as listed
>>> above,
>>> you would like to handle above ?
>
>
> Sorry for the mangled sentence, I intended to write "in the driver" instead
> of the last "above".
>
>
Not a problem I got what you intended :)
>>> I'm just wondering if you don't need ,for instance, two separate
>>> compatible
>>> properties, e.g. "ti,tvp5146-decoder" and "ti,tvp5147-decoder" ?
>>>
>> There are few differences in init/power sequence tough, I would still
>> like to have
>> single compatible property "ti,tvp514x-decoder", If you feel we need
>> separate
>> property I will change it please let me know on this.
>
>
> As Sekhar already mentioned, wildcards in the compatible property should
> not be used. You could just use exact part names in the compatible
> properties and list them all in the tvp514x_of_match[] array. Even though
> this driver doesn't care about the differences between various tvp514?
> chips, there might be others that do.
>
> [...]
>
Ok, I'll have separate compatible properties.
>>>> +#if defined(CONFIG_OF)
>>>> +static const struct of_device_id tvp514x_of_match[] = {
>>>> + {.compatible = "ti,tvp514x-decoder", },
>>>> + {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, tvp514x_of_match);
>>>> +
>>>> +static struct tvp514x_platform_data
>>>> + *tvp514x_get_pdata(struct i2c_client *client)
>>>> +{
>>>> + if (!client->dev.platform_data&& client->dev.of_node) {
>>>>
>>>> + struct tvp514x_platform_data *pdata;
>>>> + struct v4l2_of_endpoint bus_cfg;
>>>> + struct device_node *endpoint;
>>>> +
>>>> + pdata = devm_kzalloc(&client->dev,
>>>> + sizeof(struct tvp514x_platform_data),
>>>> + GFP_KERNEL);
>>>> + client->dev.platform_data = pdata;
>>>
>>>
>>>
>>> Do you really need to set client->dev.platform_data this way ?
>>> What about passing struct tvp514x_decoder pointer to this function
>>> and initializing struct tvp514x_decoder::pdata instead ?
>>>
>>>
>> Yes that can work too, I'll do the same.
>
>
> Ok, thanks.
>
>
>>>> + if (!pdata)
>>>> + return NULL;
>>>> +
>>>> + endpoint = of_get_child_by_name(client->dev.of_node,
>>>> "port");
>>>> + if (endpoint)
>>>> + endpoint = of_get_child_by_name(endpoint,
>>>> "endpoint");
>>>
>>>
>>>
>>> I think you could replace these two calls above with
>>>
>>> endpoint =
>>> v4l2_of_get_next_endpoint(client->dev.of_node);
>>>
>> Ok
>>
>>> Now I see I should have modified this function to ensure it works also
>>> when
>>> 'port' nodes are grouped in a 'ports' node.
>>>
>> So V5 series of V4l OF parser doesn't have this fix ?
>
>
> No, it doesn't. I think we need something along the lines of:
>
> diff --git a/drivers/media/v4l2-core/v4l2-of.c
> b/drivers/media/v4l2-core/v4l2-of.c
> index e1f570b..8a286f1 100644
> --- a/drivers/media/v4l2-core/v4l2-of.c
> +++ b/drivers/media/v4l2-core/v4l2-of.c
> @@ -185,10 +185,15 @@ struct device_node *v4l2_of_get_next_endpoint(const
> struct device_node *parent,
> * This is the first call, we have to find a port within
> * this node.
> */
> - for_each_child_of_node(parent, port) {
> + while (port = of_get_next_child(parent, port)) {
> if (!of_node_cmp(port->name, "port"))
> break;
> - }
> + if (!of_node_cmp(port->name, "ports")) {
> + parent = port;
> + of_node_put(port);
> + port = NULL:
> + }
> + };
> if (port) {
> /* Found a port, get an endpoint. */
> endpoint = of_get_next_child(port, NULL);
>
> However this shouldn't affect you, as you don't use the 'ports' node...
> I will likely post v6 including this fix tomorrow.
>
yes I don't need it, Just asked so that I can test my driver on latest
version :)
Regards,
--Prabhakar
> --
>
> Regards,
> Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
