Re: [PATCH v3 1/2] drm: bridge: Add sii902x driver

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Maxime,

On Mon, 2 May 2016 13:05:57 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:



> > +static void sii902x_reset(struct sii902x *sii902x)
> > +{
> > +	gpiod_set_value(sii902x->reset_gpio, 1);
> > +
> > +	msleep(100);  
> 
> Sleeping for 100ms seems like a lot. Is it required, or is it just a
> leftover from an early development?

I wish I had the answer, unfortunately I don't have the datasheet and
the implementation I based my work on [1] is sleeping 100ms.

> 
> > +
> > +	gpiod_set_value(sii902x->reset_gpio, 0);
> > +}
> > +
> > +static void sii902x_connector_destroy(struct drm_connector *connector)
> > +{
> > +	drm_connector_unregister(connector);
> > +	drm_connector_cleanup(connector);
> > +}
> > +
> > +static enum drm_connector_status
> > +sii902x_connector_detect(struct drm_connector *connector, bool force)
> > +{
> > +	struct sii902x *sii902x = connector_to_sii902x(connector);
> > +	unsigned int status;
> > +
> > +	regmap_read(sii902x->regmap, SI902X_INT_STATUS, &status);
> > +
> > +	return (status & SI902X_PLUGGED_STATUS) ?
> > +	       connector_status_connected : connector_status_disconnected;
> > +}
> > +
> > +static const struct drm_connector_funcs sii902x_atomic_connector_funcs = {
> > +	.dpms = drm_atomic_helper_connector_dpms,
> > +	.detect = sii902x_connector_detect,
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = sii902x_connector_destroy,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static const struct drm_connector_funcs sii902x_connector_funcs = {
> > +	.dpms = drm_atomic_helper_connector_dpms,

Should be drm_atomic_helper_dpms here.

> > +	.detect = sii902x_connector_detect,
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = sii902x_connector_destroy,
> > +};  
> 
> I'm guessing this is to support both atomic and !atomic drivers. I
> thought it was working automatically?

Hm, the dw-hdmi driver is providing both, but maybe it's not needed.
Daniel, any comments?

Thanks for the review.

Boris

[1]https://github.com/linux4sam/linux-at91/blob/linux-3.10-at91/drivers/hdmi/encoder-sii9022.c#L92

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel