On Tue, May 20, 2014 at 03:56:15PM +0200, Benjamin Gaignard wrote:
> Add driver to support analog TV ouput.

Unfortunate that this has the same abbreviation as High-Definition
Audio...

> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
[...]
> +/* Video DACs control */
> +#define VIDEO_DACS_CONTROL_MASK         0x0FFF
> +#define VIDEO_DACS_CONTROL_SYSCFG2535   0x085C /* for stih416 */
> +#define DAC_CFG_HD_OFF_SHIFT            5
> +#define DAC_CFG_HD_OFF_MASK             (0x7 << DAC_CFG_HD_OFF_SHIFT)
> +#define VIDEO_DACS_CONTROL_SYSCFG5072   0x0120 /* for stih407 */

syscfg is starting to look more and more like it could be a syscon
driver or some other specialized, platform-specific driver.

> +/* Index (in supported modes table) of the preferred video mode */
> +#define HDA_PREF_MODE_IDX               0

I don't think this symbolic name is particularly useful.

> +/* Reference to the hda device */
> +struct device *hda_dev;

This shouldn't be needed.

> +static void sti_hda_configure_awg(struct sti_hda *hda, u32 *awg_instr, int 
> nb)
> +{
> +     int i;

unsigned

> +/*
> + * Get modes
> + *
> + * @drm_connector: pointer on the drm connector
> + *
> + * Return number of modes
> + */
> +static int sti_hda_get_modes(struct drm_connector *drm_connector)
> +{
> +     struct drm_device *dev = drm_connector->dev;
> +     struct drm_display_mode *mode;
> +     int i, count;

unsigned for i. count should probably stay signed since DRM uses that
throughout.

> +
> +     DRM_DEBUG_DRIVER("\n");
> +
> +     for (i = 0, count = 0; i < ARRAY_SIZE(hda_supported_modes); i++) {

You can initialize count when it's declared to make the loop more
idiomatic.

> +             mode = drm_mode_duplicate(dev, &hda_supported_modes[i].mode);
> +             if (!mode)
> +                     continue;
> +             mode->vrefresh = drm_mode_vrefresh(mode);
> +
> +             /* Set the preferred mode */
> +             if (i == HDA_PREF_MODE_IDX)
> +                     mode->type |= DRM_MODE_TYPE_PREFERRED;
> +
> +             drm_mode_probed_add(drm_connector, mode);
> +             count++;
> +     }
> +
> +     drm_mode_sort(&drm_connector->modes);
> +
> +     return count;
> +}
> +
> +

Gratuituous newline.

> +static int sti_hda_probe(struct platform_device *pdev)
> +{
[...]
> +}
> +
> +static int sti_hda_remove(struct platform_device *pdev)
> +{
[...]
> +}
> +
> +static struct of_device_id hda_match_types[] = {
> +     {
> +      .compatible = "st,stih416-hda",
> +      },
> +     {
> +      .compatible = "st,stih407-hda",
> +      },
> +     { /* end node */ }
> +};
> +MODULE_DEVICE_TABLE(of, hda_match_types);
> +
> +struct platform_driver sti_hda_driver = {
> +     .driver = {
> +                .name = "sti-hda",
> +                .owner = THIS_MODULE,
> +                .of_match_table = hda_match_types,
> +                },
> +     .probe = sti_hda_probe,
> +     .remove = sti_hda_remove,
> +};
> +
> +module_platform_driver(sti_hda_driver);
> +
> +MODULE_LICENSE("GPL");

Same comments here as for all previous patches.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140521/9aabf2da/attachment.sig>

Reply via email to