Laurent Pinchart wrote:
> Hi Sakari,
Heippa,
> On Tuesday 15 November 2011 16:28:06 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>>> On Monday 14 November 2011 10:34:57 Sakari Ailus wrote:
>
> [snip]
>
>>>>> + if (rval & ~AS_FAULT_INFO_INDICATOR_LED)
>>>>> + dev_dbg(&client->dev, "No faults, nice\n");
>>>>> +
>>>>> + return rval;
>>>>> +}
>>>>> +
>>>>> +static int as3645a_get_ctrl(struct v4l2_ctrl *ctrl)
>>>>> +{
>>>>> + struct as3645a *flash =
>>>>> + container_of(ctrl->handler, struct as3645a, ctrls);
>>>>> + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev);
>>>>> + int fault;
>>>>> +
>>>>> + switch (ctrl->id) {
>>>>> + case V4L2_CID_FLASH_FAULT:
>>>>> + fault = as3645a_read_fault(flash);
>>>>> + if (fault < 0)
>>>>> + return fault;
>>>>
>>>> ctrl->cur.val = 0 here?
>>>
>>> fault being negative means that reading the fault register failed. In
>>> that case I don't think we should update ctrl->cur.val.
>>
>> I thought that ctrl->cur.val should be assigned to zero before settings
>> bits in it. Does that sound better? :-)
>
> Oops, my bad, I misunderstood your comment. I'll fix that.
Thanks.
>>>>> +static int as3645a_registered(struct v4l2_subdev *sd)
>>>>> +{
>>>>> + struct as3645a *flash = to_as3645a(sd);
>>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>>> + int rval, man, model, rfu, version;
>>>>> + const char *factory;
>>>>> +
>>>>> + /* Power up the flash driver and read manufacturer ID, model ID, RFU
>>>>> + * and version.
>>>>> + */
>>>>> + as3645a_set_power(&flash->subdev, 1);
>>>>> +
>>>>> + rval = as3645a_read(flash, AS_DESIGN_INFO_REG);
>>>>> + if (rval < 0)
>>>>> + goto power_off;
>>>>> +
>>>>> + man = AS_DESIGN_INFO_FACTORY(rval);
>>>>> + model = AS_DESIGN_INFO_MODEL(rval);
>>>>> +
>>>>> + rval = as3645a_read(flash, AS_VERSION_CONTROL_REG);
>>>>> + if (rval < 0)
>>>>> + goto power_off;
>>>>> +
>>>>> + rfu = AS_VERSION_CONTROL_RFU(rval);
>>>>> + version = AS_VERSION_CONTROL_VERSION(rval);
>>>>> +
>>>>> + /* Verify the chip model and version. */
>>>>> + if (model != 0x0001 || rfu != 0x0000) {
>>>>> + dev_err(&client->dev, "AS3645A not detected "
>>>>> + "(model %d rfu %d)\n", model, rfu);
>>>>> + rval = -ENODEV;
>>>>
>>>> Is this so grave issue we should discontinue? I'd perhaps print a
>>>> warning if even that.
>>>
>>> This could mean that the chip isn't an AS3645A/LM3555 at all. Many I2C
>>> drivers perform the same check, they read the ID register and fail if it
>>> doesn't contain the expected value.
>>
>> Shouldn't we instead check the design info register has a valid id in it
>> only? The spec I have doesn't say anything about the version of the
>> chip.
>
> I agree that checking the model shoudl be enough. However, if a new chip comes
> out with an RFU value != 0, it will probably mean that the driver will need to
> be adapted anyway. Isn't it better to catch that instead of ignoring it ?
Ok. I'm fine with that.
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx
--
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
