Re: [PATCH v3 32/33] smiapp: Add driver.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

On Thursday 01 March 2012 16:01:39 Sakari Ailus wrote:
> Laurent Pinchart wrote:

[snip]

> >>>> +		return sensor->pixel_array->ctrl_handler.error;
> >>>> +	}
> >>>> +
> >>>> +	sensor->pixel_array->sd.ctrl_handler =
> >>>> +		&sensor->pixel_array->ctrl_handler;
> >>>> +
> >>>> +	v4l2_ctrl_cluster(2, &sensor->hflip);
> >>> 
> >>> Shouldn't you move this before the control handler check ?
> >> 
> >> Why? It can't fail.
> > 
> > I thought it could fail. You could then leave it here, but it would be
> > easier from a maintenance point of view to check the error code after
> > completing all control-related initialization, as it would avoid
> > introducing a bug if for some reason the v4l2_ctrl_cluster() function
> > needs to return an error later.
> Then every other driver must also take that into account. And as
> Sylwester said, there are things to check before that as well.
> 
> So I could also re-check the control handler error status after the
> function but currently it doesn't look like it would make sense.

Sylwester made a very good point. Let's leave the code as-is.

[snip]

> >> The lvalues are module parameters whereas the rvalues are sensor
> >> parameters.>> 
> >>>> +	if (!minfo->manufacturer_id && !minfo->model_id) {
> >>>> +		minfo->manufacturer_id = minfo->sensor_manufacturer_id;
> >>>> +		minfo->model_id = minfo->sensor_model_id;
> >>>> +		minfo->revision_number_major = minfo->sensor_revision_number;
> >>>> +	}
> >>>> +
> >>>> +	for (i = 0; i < ARRAY_SIZE(smiapp_module_idents); i++) {
> >>>> +		if (smiapp_module_idents[i].manufacturer_id
> >>>> +		    != minfo->manufacturer_id)
> >>>> +			continue;
> >>>> +		if (smiapp_module_idents[i].model_id != minfo->model_id)
> >>>> +			continue;
> >>>> +		if (smiapp_module_idents[i].flags
> >>>> +		    & SMIAPP_MODULE_IDENT_FLAG_REV_LE) {
> >>>> +			if (smiapp_module_idents[i].revision_number_major
> >>>> +			    < minfo->revision_number_major)
> >>>> +				continue;
> >>>> +		} else {
> >>>> +			if (smiapp_module_idents[i].revision_number_major
> >>>> +			    != minfo->revision_number_major)
> >>>> +				continue;
> >>>> +		}
> >>>> +
> >>>> +		minfo->name = smiapp_module_idents[i].name;
> >>>> +		minfo->quirk = smiapp_module_idents[i].quirk;
> >>>> +		break;
> >>>> +	}
> >>>> +
> >>>> +	if (i >= ARRAY_SIZE(smiapp_module_idents))
> >>>> +		dev_warn(&client->dev, "no quirks for this module\n");
> >>> 
> >>> Maybe a message such as "unknown SMIA++ module - trying generic support"
> >>> would be better ? Many of the known modules have no quirks.
> >> 
> >> I'd like to think it as a positive message of the conformance of the
> >> sensor
> >> --- still it may inform that the quirks are actually missing. What do you
> >> think?
> > 
> > In that case I think something similar to my message is better :-) I agree
> > about the meaning the message should convey.
> 
> I understand from your message that the sensor should have quirks and
> the fact they're missing is a fall-back solution. :-)

Just use any message you want that says that the sensor model isn't known to 
the driver, but should still work as it's supposed to be standard-compliant 
:-)

[snip]

> >>>> +	}
> >>>> +
> >>>> +	if (ssd != ssd->sensor->pixel_array) {
> >>>> +		sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >>>> +		sel.pad = SMIAPP_PAD_SINK;
> >>>> +		sel.target = V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE;
> >>>> +		__smiapp_get_selection(sd, fh, &sel);
> >>>> +		try_sel = v4l2_subdev_get_try_compose(fh, SMIAPP_PAD_SINK);
> >>>> +		*try_sel = sel.r;
> >>>> +	}
> >>>> +
> >>>> +	rval = smiapp_set_power(sd, 1);
> >>>> +
> >>>> +	mutex_unlock(&ssd->sensor->mutex);
> >>>> +
> >>>> +	if (rval < 0)
> >>>> +		goto out;
> >>>> +
> >>>> +	/* Was the sensor already powered on? */
> >>>> +	if (ssd->sensor->power_count > 1)
> >>> 
> >>> power_count is accessed in smiapp_set_power without taking the
> >>> power_mutex lock. Are two locks really needed ?
> >> 
> >> Well, now that you mention it, control handler setup function that
> >> wouldn't take the locks would resolve the issue, I think. Should I create
> >> one?
> > 
> > I'd ask Hans about that.
> > 
> > [snip]
> 
> I agree. I think I'll postpone the change so we can have time for
> discussion. Would you be ok with that?

OK.

-- 
Regards,

Laurent Pinchart