Hi Laurent, Thanks for the review. On Thu, May 2, 2013 at 4:32 PM, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > Hi Prabhakar, > > Thank you for the patch. > > On Thursday 02 May 2013 12:34:25 Prabhakar Lad wrote: >> On Thu, May 2, 2013 at 12:25 PM, Sascha Hauer wrote: >> > On Thu, May 02, 2013 at 11:52:34AM +0530, Prabhakar Lad wrote: >> >> From: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> >> >> >> >> add OF support for the mt9p031 sensor driver. >> >> Alongside this patch sorts the header inclusion alphabetically. >> >> [Snip] >> >> >> +#if defined(CONFIG_OF) >> >> +static struct mt9p031_platform_data * >> >> + mt9p031_get_pdata(struct i2c_client *client) > > Please align this line on the left. > OK >> >> +{ >> >> + if (client->dev.of_node) { >> > >> > By inverting the logic here and returning immediately you can safe an >> > indention level for the bulk of this function. >> >> OK >> >> >> + struct device_node *np; >> >> + struct mt9p031_platform_data *pdata; >> >> + >> >> + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL); >> >> + if (!np) >> >> + return NULL; >> >> + >> >> + pdata = devm_kzalloc(&client->dev, >> >> + sizeof(struct mt9p031_platform_data), >> >> + GFP_KERNEL); >> >> + if (!pdata) { >> >> + dev_err(&client->dev, >> >> + "mt9p031 failed allocate memeory\n"); >> >> + return NULL; >> > >> > s/memeory/memory/ >> > >> > Better drop this message completely. If you are really out of memory >> > you'll notice it quite fast anyway. >> >> alright I'll drop the message. >> >> >> + } >> >> + pdata->reset = of_get_named_gpio(client->dev.of_node, >> >> + "gpio-reset", 0); >> >> + if (!gpio_is_valid(pdata->reset)) >> >> + pdata->reset = -1; > > I've just sent a patch that converts the manual -1 checks in the driver to > gpio_is_valid(). You can thus drop these two lines. > OK I'll drop this check. >> >> + if (of_property_read_u32(np, "input-clock-frequency", >> >> + &pdata->ext_freq)) >> >> + return NULL; >> >> + >> >> + if (of_property_read_u32(np, "pixel-clock-frequency", >> >> + &pdata->target_freq)) >> >> + return NULL; >> > >> > returning NULL here means that when these properties are missing the >> > driver bails out with the message "No platform data\n" which is not >> > very helpful for users. How about just ignoring this here and return >> > pdata? The driver will probably print a more useful message later when >> > it notices that the clock params are invalid. >> >> Yes would be good idea of not returning NULL. >> >> >> + >> >> + return pdata; >> >> + } >> >> + >> >> + return client->dev.platform_data; >> >> +} >> >> +#else >> >> +static struct mt9p031_platform_data * >> >> + mt9p031_get_pdata(struct i2c_client *client) >> >> +{ >> >> + return client->dev.platform_data; >> >> +} >> >> +#endif >> >> + >> >> >> >> static int mt9p031_probe(struct i2c_client *client, >> >> const struct i2c_device_id *did) >> >> { >> >> - struct mt9p031_platform_data *pdata = client->dev.platform_data; >> >> + struct mt9p031_platform_data *pdata = mt9p031_get_pdata(client); >> >> >> >> struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent); >> >> struct mt9p031 *mt9p031; >> >> unsigned int i; >> >> @@ -1070,8 +1120,16 @@ static const struct i2c_device_id mt9p031_id[] = { >> >> >> >> }; >> >> MODULE_DEVICE_TABLE(i2c, mt9p031_id); >> >> >> >> +static const struct of_device_id mt9p031_of_match[] = { >> >> + { .compatible = "aptina,mt9p031", }, >> >> + { .compatible = "aptina,mt9p031m", }, >> >> + {}, >> >> +}; >> > >> > I would have expected something like: >> > >> > static const struct of_device_id mt9p031_of_match[] = { >> > { >> > .compatible = "aptina,mt9p031-sensor", >> > .data = (void *)MT9P031_MODEL_COLOR, >> > }, { >> > .compatible = "aptina,mt9p031m-sensor", >> > .data = (void *)MT9P031_MODEL_MONOCHROME, >> > }, { >> > /* sentinel */ >> > }, >> > }; >> > >> > of_id = of_match_device(mt9p031_of_match, &client->dev); >> > if (of_id) >> > mt9p031->model = (enum mt9p031_model)of_id->data; >> > >> > To handle monochrome sensors. >> >> OK will do the same. > > And please guard the table with #ifdef CONFIG_OF. > But guarding the table #ifdef CONFIG_OF would cause compilation failure for below code when CONFIG_OF is undefined in probe of_id = of_match_device(of_match_ptr(mt9p031_of_match), &client->dev); if (of_id) mt9p031->model = (enum mt9p031_model)of_id->data; and also in mt9p031_i2c_driver structure, of_match_table = of_match_ptr(mt9p031_of_match), which force me to define mt9p031_of_match to NULL when CONFIG_OF is undefined Regards, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html