Re: [PATCH v7] s5k5baf: add camera sensor driver
On Mon, Sep 02, 2013 at 05:21:58PM +0100, Sylwester Nawrocki wrote: > Hi Mark, Hi Sylwester, > > On 08/27/2013 11:14 AM, Mark Rutland wrote: > >> +endpoint node > >> +- > >> + > >> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in > >> + video-interfaces.txt. This property can be only used to specify number > >> + of data lanes, i.e. the array's content is unused, only its length is > >> + meaningful. When this property is not specified default value of 1 lane > >> + will be used. > > > > Apologies for having not replied to the last posting, but having looked > > at the documentation I was provided last time [1], I don't think the > > values in the data-lanes property should be described as unused. That > > may be the way the Linux driver functions at present, but it's not how > > the generic video-interfaces binding documentation describes the > > property. > > > > If the CSI transmitter hardware doesn't support logical remapping of > > lanes, then the only valid values for data-lanes would be a contiguous > > list of lane IDs starting at 1, ending at 4 at most. Valid values for > > the property would be one of: > > > > data-lanes = <1>; > > data-lanes = <1>, <2>; > > data-lanes = <1>, <2>, <3>; > > data-lanes = <1>, <2>, <3>, <4>; > > > > We can mention the fact the hardware doesn't support remapping of lanes, > > and therefore the list must start with lane 1 and end with (at most) > > lane 4. That way a dts will match the generic binding and actually > > describe the hardware, and it's possible for Linux (or any other OS) to > > factor out the parsing of data-lanes later as desired. > > > > I don't think we should offer freedom to encode garbage in the dt when > > we can just as easily encourage more standard use of bindings that will > > make our lives easier in the long-term. > > I entirely agree, that's a very accurate analysis. > > Presumably the data-lanes property's descriptions could be improved so > it is said explicitly that array elements 0...N - 1, where N = 4, > correspond to logical data lanes 1...N. That makes sense to me. > > Then considering the values in the data-lanes property, I didn't make > the description terribly specific about the fact that pool of indexes > 0...4 is used for the clock lane and 4 data lanes. The values could well > be H/W specific, but it seems more sensible to enforce common range. > It may not match exactly with documentation of various hardware. E.g. > OMAP, see page 1661, register CSI2_COMPLEXIO_CFG [1], uses indexes > 1..5 to indicate position of a data lane and 0 is used to mark a lane > as unused. Ah, I see. I guess this boils down to what the "physical indexes" referred to by the video-interfaces binding actually are. If an interface may only ever have 5 lanes, then using a logical index sounds fine. But if we ever have the case where a CSI transmitter has more than 5 lanes (so it could communicate with multiple receviers), then the value has to become hardware-specific. At that point, we'd possibly need to define remapping of the clocks too. I'm not that familiar with CSI so I'm not sure if such a device is possible. > > I think we should have similarly defined value 0 to indicate an unused > lane. None of drivers in mainline uses this line remapping feature, so > changing meaning of the array values wouldn't presumably have any bad > side effects. I'm not sure if it's OK to make a change like this now. > IIUC the MIPI CSI-2 standard requires consecutive data lane indexes, > so valid set of data lanes could be only: <1>, <1, 2>, <1, 2, 3>, > <1, 2, 3, 4>. So there seems to be no issue for MIPI CSI-2. But for > future protocols the current convention might not have been flexible > enough. Given that the video-interfaces binding refers to the clock being on lane 0, I'm not sure it makes sense to reserve this as an unused id -- we'd be saying the lane is the clock to say it's unused, but maybe that doesn't matter. Thanks, Mark. > > > [1] http://www.mipi.org/specifications/camera-interface#CSI2 > > [2] > http://www.ti.com/general/docs/lit/getliterature.tsp?literatureNumber=swpu231ao&fileType=pdf > -- > Regards, > Sylwester > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] s5k5baf: add camera sensor driver
Hi Mark, On 08/27/2013 11:14 AM, Mark Rutland wrote: >> +endpoint node >> +- >> + >> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in >> + video-interfaces.txt. This property can be only used to specify number >> + of data lanes, i.e. the array's content is unused, only its length is >> + meaningful. When this property is not specified default value of 1 lane >> + will be used. > > Apologies for having not replied to the last posting, but having looked > at the documentation I was provided last time [1], I don't think the > values in the data-lanes property should be described as unused. That > may be the way the Linux driver functions at present, but it's not how > the generic video-interfaces binding documentation describes the > property. > > If the CSI transmitter hardware doesn't support logical remapping of > lanes, then the only valid values for data-lanes would be a contiguous > list of lane IDs starting at 1, ending at 4 at most. Valid values for > the property would be one of: > > data-lanes = <1>; > data-lanes = <1>, <2>; > data-lanes = <1>, <2>, <3>; > data-lanes = <1>, <2>, <3>, <4>; > > We can mention the fact the hardware doesn't support remapping of lanes, > and therefore the list must start with lane 1 and end with (at most) > lane 4. That way a dts will match the generic binding and actually > describe the hardware, and it's possible for Linux (or any other OS) to > factor out the parsing of data-lanes later as desired. > > I don't think we should offer freedom to encode garbage in the dt when > we can just as easily encourage more standard use of bindings that will > make our lives easier in the long-term. I entirely agree, that's a very accurate analysis. Presumably the data-lanes property's descriptions could be improved so it is said explicitly that array elements 0...N - 1, where N = 4, correspond to logical data lanes 1...N. Then considering the values in the data-lanes property, I didn't make the description terribly specific about the fact that pool of indexes 0...4 is used for the clock lane and 4 data lanes. The values could well be H/W specific, but it seems more sensible to enforce common range. It may not match exactly with documentation of various hardware. E.g. OMAP, see page 1661, register CSI2_COMPLEXIO_CFG [1], uses indexes 1..5 to indicate position of a data lane and 0 is used to mark a lane as unused. I think we should have similarly defined value 0 to indicate an unused lane. None of drivers in mainline uses this line remapping feature, so changing meaning of the array values wouldn't presumably have any bad side effects. I'm not sure if it's OK to make a change like this now. IIUC the MIPI CSI-2 standard requires consecutive data lane indexes, so valid set of data lanes could be only: <1>, <1, 2>, <1, 2, 3>, <1, 2, 3, 4>. So there seems to be no issue for MIPI CSI-2. But for future protocols the current convention might not have been flexible enough. > [1] http://www.mipi.org/specifications/camera-interface#CSI2 [2] http://www.ti.com/general/docs/lit/getliterature.tsp?literatureNumber=swpu231ao&fileType=pdf -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] s5k5baf: add camera sensor driver
Hi Andrejz,
On Monday 26 August 2013 14:34:21 Andrzej Hajda wrote:
> On 08/23/2013 02:53 PM, Laurent Pinchart wrote:
> > On Wednesday 21 August 2013 16:41:31 Andrzej Hajda wrote:
> >> Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor
> >> with embedded SoC ISP.
> >> The driver exposes the sensor as two V4L2 subdevices:
> >> - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format,
> >> no controls.
> >>
> >> - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200,
> >> pre/post ISP cropping, downscaling via selection API, controls.
> >>
> >> Signed-off-by: Sylwester Nawrocki
> >> Signed-off-by: Andrzej Hajda
> >> Signed-off-by: Kyungmin Park
> >> ---
> >> Hi,
> >>
> >> This patch incorporates Stephen's suggestions, thanks.
> >>
> >> Regards
> >> Andrzej
> >>
> >> v7
> >> - changed description of 'clock-frequency' DT property
> >>
> >> v6
> >> - endpoint node presence is now optional,
> >> - added asynchronous subdev registration support and clock
> >>
> >> handling,
> >>
> >> - use named gpios in DT bindings
> >>
> >> v5
> >> - removed hflip/vflip device tree properties
> >>
> >> v4
> >> - GPL changed to GPLv2,
> >> - bitfields replaced by u8,
> >> - cosmetic changes,
> >> - corrected s_stream flow,
> >> - gpio pins are no longer exported,
> >> - added I2C addresses to subdev names,
> >> - CIS subdev registration postponed after
> >>
> >> succesfull HW initialization,
> >>
> >> - added enums for pads,
> >> - selections are initialized only during probe,
> >> - default resolution changed to 1600x1200,
> >> - state->error pattern removed from few other functions,
> >> - entity link creation moved to registered callback.
> >>
> >> v3:
> >> - narrowed state->error usage to i2c and power errors,
> >> - private gain controls replaced by red/blue balance user controls,
> >> - added checks to devicetree gpio node parsing
> >>
> >> v2:
> >> - lower-cased driver name,
> >> - removed underscore from regulator names,
> >> - removed platform data code,
> >> - v4l controls grouped in anonymous structs,
> >> - added s5k5baf_clear_error function,
> >> - private controls definitions moved to uapi header file,
> >> - added v4l2-controls.h reservation for private controls,
> >> - corrected subdev registered/unregistered code,
> >> - .log_status sudbev op set to v4l2 helper,
> >> - moved entity link creation to probe routines,
> >> - added cleanup on error to probe function.
> >> ---
> >>
> >> .../devicetree/bindings/media/samsung-s5k5baf.txt | 59 +
> >> MAINTAINERS|7 +
> >> drivers/media/i2c/Kconfig |7 +
> >> drivers/media/i2c/Makefile |1 +
> >> drivers/media/i2c/s5k5baf.c| 2045 ++
> >> 5 files changed, 2119 insertions(+)
> >> create mode 100644
> >>
> >> Documentation/devicetree/bindings/media/samsung-s5k5baf.txt create mode
> >> 100644 drivers/media/i2c/s5k5baf.c
> >
> > [snip]
> >
> >> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
> >> new file mode 100644
> >> index 000..f21d9f1
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/s5k5baf.c
[snip]
> >> +static void s5k5baf_write_arr_seq(struct s5k5baf *state, u16 addr,
> >> +u16 count, const u16 *seq)
> >> +{
> >> + struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
> >> + u16 buf[count + 1];
> >> + int ret, n;
> >> +
> >> + s5k5baf_i2c_write(state, REG_CMDWR_ADDR, addr);
> >> + if (state->error)
> >> + return;
> >
> > I would have a preference for returning an error directly from the write
> > function instead of storing it in state->error, that would be more
> > explicit. The same is true for all read/write functions.
>
> I have introduced state->error to avoid code bloat. With this 'pattern'
> error is checked in about 10 places in the code, of course without
> scarifying code correctness.
> Replacing this pattern with classic 'return error directly from function'
> would result with adding error checks after all calls to i2c i/o functions
> and after calls to many functions which those i2c i/o calls contains.
> According to my rough estimates it is about 70 places.
>
> Similar pattern is used already in v4l2_ctrl_handler::error.
>
> >> + buf[0] = __constant_htons(REG_CMD_BUF);
> >> + for (n = 1; n <= count; ++n)
> >> + buf[n] = htons(*seq++);
> >
> > cpu_to_be16()/be16_to_cpu() here as well ?
>
> OK
>
> >> +
> >> + n *= 2;
> >> + ret = i2c_master_send(c, (char *)buf, n);
> >> + v4l2_dbg(3, debug, c, "i2c_write_seq(count=%d): %*ph\n", count,
> >> + min(2 * count, 64), seq - count);
> >> +
> >> + if (ret != n) {
> >> + v4l2_err(c, "i2c_write_seq: error during transfer (%d)\n", ret);
> >> + state->error = ret;
> >> + }
> >> +}
[snip]
> >> +static int s5k5baf_hw_set_video_bus(struct s5k5baf *state)
> >> +{
> >> + u16 en_packets;
> >> +
> >> + switch
Re: [PATCH v7] s5k5baf: add camera sensor driver
Hi, [trimming down to relevant context] > +endpoint node > +- > + > +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in > + video-interfaces.txt. This property can be only used to specify number > + of data lanes, i.e. the array's content is unused, only its length is > + meaningful. When this property is not specified default value of 1 lane > + will be used. Apologies for having not replied to the last posting, but having looked at the documentation I was provided last time [1], I don't think the values in the data-lanes property should be described as unused. That may be the way the Linux driver functions at present, but it's not how the generic video-interfaces binding documentation describes the property. If the CSI transmitter hardware doesn't support logical remapping of lanes, then the only valid values for data-lanes would be a contiguous list of lane IDs starting at 1, ending at 4 at most. Valid values for the property would be one of: data-lanes = <1>; data-lanes = <1>, <2>; data-lanes = <1>, <2>, <3>; data-lanes = <1>, <2>, <3>, <4>; We can mention the fact the hardware doesn't support remapping of lanes, and therefore the list must start with lane 1 and end with (at most) lane 4. That way a dts will match the generic binding and actually describe the hardware, and it's possible for Linux (or any other OS) to factor out the parsing of data-lanes later as desired. I don't think we should offer freedom to encode garbage in the dt when we can just as easily encourage more standard use of bindings that will make our lives easier in the long-term. Thanks, Mark. [1] http://www.mipi.org/specifications/camera-interface#CSI2 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] s5k5baf: add camera sensor driver
Hi Laurent,
Thank you for the review.
On 08/23/2013 02:53 PM, Laurent Pinchart wrote:
> Hi Andrzej,
>
> Thank you for the patch.
>
> On Wednesday 21 August 2013 16:41:31 Andrzej Hajda wrote:
>> Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor
>> with embedded SoC ISP.
>> The driver exposes the sensor as two V4L2 subdevices:
>> - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format,
>> no controls.
>> - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200,
>> pre/post ISP cropping, downscaling via selection API, controls.
>>
>> Signed-off-by: Sylwester Nawrocki
>> Signed-off-by: Andrzej Hajda
>> Signed-off-by: Kyungmin Park
>> ---
>> Hi,
>>
>> This patch incorporates Stephen's suggestions, thanks.
>>
>> Regards
>> Andrzej
>>
>> v7
>> - changed description of 'clock-frequency' DT property
>>
>> v6
>> - endpoint node presence is now optional,
>> - added asynchronous subdev registration support and clock
>> handling,
>> - use named gpios in DT bindings
>>
>> v5
>> - removed hflip/vflip device tree properties
>>
>> v4
>> - GPL changed to GPLv2,
>> - bitfields replaced by u8,
>> - cosmetic changes,
>> - corrected s_stream flow,
>> - gpio pins are no longer exported,
>> - added I2C addresses to subdev names,
>> - CIS subdev registration postponed after
>> succesfull HW initialization,
>> - added enums for pads,
>> - selections are initialized only during probe,
>> - default resolution changed to 1600x1200,
>> - state->error pattern removed from few other functions,
>> - entity link creation moved to registered callback.
>>
>> v3:
>> - narrowed state->error usage to i2c and power errors,
>> - private gain controls replaced by red/blue balance user controls,
>> - added checks to devicetree gpio node parsing
>>
>> v2:
>> - lower-cased driver name,
>> - removed underscore from regulator names,
>> - removed platform data code,
>> - v4l controls grouped in anonymous structs,
>> - added s5k5baf_clear_error function,
>> - private controls definitions moved to uapi header file,
>> - added v4l2-controls.h reservation for private controls,
>> - corrected subdev registered/unregistered code,
>> - .log_status sudbev op set to v4l2 helper,
>> - moved entity link creation to probe routines,
>> - added cleanup on error to probe function.
>> ---
>> .../devicetree/bindings/media/samsung-s5k5baf.txt | 59 +
>> MAINTAINERS|7 +
>> drivers/media/i2c/Kconfig |7 +
>> drivers/media/i2c/Makefile |1 +
>> drivers/media/i2c/s5k5baf.c| 2045 +
>> 5 files changed, 2119 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/media/samsung-s5k5baf.txt create mode
>> 100644 drivers/media/i2c/s5k5baf.c
> [snip]
>
>> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
>> new file mode 100644
>> index 000..f21d9f1
>> --- /dev/null
>> +++ b/drivers/media/i2c/s5k5baf.c
> [snip]
>
>> +enum s5k5baf_pads_id {
>> +PAD_CIS,
>> +PAD_OUT,
>> +CIS_PAD_NUM = 1,
>> +ISP_PAD_NUM = 2
>> +};
> You can just use #define's here, the enum doesn't bring any additional value
> and isn't very explicit.
OK
>
>> +static struct v4l2_rect s5k5baf_cis_rect = { 0, 0, S5K5BAF_CIS_WIDTH,
>> + S5K5BAF_CIS_HEIGHT };
> Shouldn't this be const ?
OK
>
>> +static u16 s5k5baf_i2c_read(struct s5k5baf *state, u16 addr)
>> +{
>> +struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
>> +u16 w, r;
> You should declare these variables as __be16.
OK
>
>> +struct i2c_msg msg[] = {
>> +{ .addr = c->addr, .flags = 0,
>> + .len = 2, .buf = (u8 *)&w },
>> +{ .addr = c->addr, .flags = I2C_M_RD,
>> + .len = 2, .buf = (u8 *)&r },
>> +};
>> +int ret;
>> +
>> +if (state->error)
>> +return 0;
>> +
>> +w = htons(addr);
> Wouldln't cpu_to_be16() be more appropriate ?
OK
>
>> +ret = i2c_transfer(c->adapter, msg, 2);
>> +r = ntohs(r);
> And be16_to_cpu() here.
OK
>
>> +
>> +v4l2_dbg(3, debug, c, "i2c_read: 0x%04x : 0x%04x\n", addr, r);
>> +
>> +if (ret != 2) {
>> +v4l2_err(c, "i2c_read: error during transfer (%d)\n", ret);
>> +state->error = ret;
>> +}
>> +return r;
>> +}
> [snip]
>
>> +static void s5k5baf_write_arr_seq(struct s5k5baf *state, u16 addr,
>> + u16 count, const u16 *seq)
>> +{
>> +struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
>> +u16 buf[count + 1];
>> +int ret, n;
>> +
>> +s5k5baf_i2c_write(state, REG_CMDWR_ADDR, addr);
>> +if (state->error)
>> +return;
> I would have a preference for returning an error directly from the write
> function instead of storing it in state->error, that would be more explicit.
> The same is true for all read/write functions.
I have introduced state->error to avoid code bloat. With thi
Re: [PATCH v7] s5k5baf: add camera sensor driver
Hi Andrzej,
Thank you for the patch.
On Wednesday 21 August 2013 16:41:31 Andrzej Hajda wrote:
> Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor
> with embedded SoC ISP.
> The driver exposes the sensor as two V4L2 subdevices:
> - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format,
> no controls.
> - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200,
> pre/post ISP cropping, downscaling via selection API, controls.
>
> Signed-off-by: Sylwester Nawrocki
> Signed-off-by: Andrzej Hajda
> Signed-off-by: Kyungmin Park
> ---
> Hi,
>
> This patch incorporates Stephen's suggestions, thanks.
>
> Regards
> Andrzej
>
> v7
> - changed description of 'clock-frequency' DT property
>
> v6
> - endpoint node presence is now optional,
> - added asynchronous subdev registration support and clock
> handling,
> - use named gpios in DT bindings
>
> v5
> - removed hflip/vflip device tree properties
>
> v4
> - GPL changed to GPLv2,
> - bitfields replaced by u8,
> - cosmetic changes,
> - corrected s_stream flow,
> - gpio pins are no longer exported,
> - added I2C addresses to subdev names,
> - CIS subdev registration postponed after
> succesfull HW initialization,
> - added enums for pads,
> - selections are initialized only during probe,
> - default resolution changed to 1600x1200,
> - state->error pattern removed from few other functions,
> - entity link creation moved to registered callback.
>
> v3:
> - narrowed state->error usage to i2c and power errors,
> - private gain controls replaced by red/blue balance user controls,
> - added checks to devicetree gpio node parsing
>
> v2:
> - lower-cased driver name,
> - removed underscore from regulator names,
> - removed platform data code,
> - v4l controls grouped in anonymous structs,
> - added s5k5baf_clear_error function,
> - private controls definitions moved to uapi header file,
> - added v4l2-controls.h reservation for private controls,
> - corrected subdev registered/unregistered code,
> - .log_status sudbev op set to v4l2 helper,
> - moved entity link creation to probe routines,
> - added cleanup on error to probe function.
> ---
> .../devicetree/bindings/media/samsung-s5k5baf.txt | 59 +
> MAINTAINERS|7 +
> drivers/media/i2c/Kconfig |7 +
> drivers/media/i2c/Makefile |1 +
> drivers/media/i2c/s5k5baf.c| 2045 +
> 5 files changed, 2119 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/media/samsung-s5k5baf.txt create mode
> 100644 drivers/media/i2c/s5k5baf.c
[snip]
> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
> new file mode 100644
> index 000..f21d9f1
> --- /dev/null
> +++ b/drivers/media/i2c/s5k5baf.c
[snip]
> +enum s5k5baf_pads_id {
> + PAD_CIS,
> + PAD_OUT,
> + CIS_PAD_NUM = 1,
> + ISP_PAD_NUM = 2
> +};
You can just use #define's here, the enum doesn't bring any additional value
and isn't very explicit.
> +static struct v4l2_rect s5k5baf_cis_rect = { 0, 0, S5K5BAF_CIS_WIDTH,
> + S5K5BAF_CIS_HEIGHT };
Shouldn't this be const ?
> +static u16 s5k5baf_i2c_read(struct s5k5baf *state, u16 addr)
> +{
> + struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
> + u16 w, r;
You should declare these variables as __be16.
> + struct i2c_msg msg[] = {
> + { .addr = c->addr, .flags = 0,
> + .len = 2, .buf = (u8 *)&w },
> + { .addr = c->addr, .flags = I2C_M_RD,
> + .len = 2, .buf = (u8 *)&r },
> + };
> + int ret;
> +
> + if (state->error)
> + return 0;
> +
> + w = htons(addr);
Wouldln't cpu_to_be16() be more appropriate ?
> + ret = i2c_transfer(c->adapter, msg, 2);
> + r = ntohs(r);
And be16_to_cpu() here.
> +
> + v4l2_dbg(3, debug, c, "i2c_read: 0x%04x : 0x%04x\n", addr, r);
> +
> + if (ret != 2) {
> + v4l2_err(c, "i2c_read: error during transfer (%d)\n", ret);
> + state->error = ret;
> + }
> + return r;
> +}
[snip]
> +static void s5k5baf_write_arr_seq(struct s5k5baf *state, u16 addr,
> + u16 count, const u16 *seq)
> +{
> + struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
> + u16 buf[count + 1];
> + int ret, n;
> +
> + s5k5baf_i2c_write(state, REG_CMDWR_ADDR, addr);
> + if (state->error)
> + return;
I would have a preference for returning an error directly from the write
function instead of storing it in state->error, that would be more explicit.
The same is true for all read/write functions.
> + buf[0] = __constant_htons(REG_CMD_BUF);
> + for (n = 1; n <= count; ++n)
> + buf[n] = htons(*seq++);
cpu_to_be16()/be16_to_cpu() here as well ?
> +
> + n *= 2;
> + ret = i2c_master_send(c, (char *)buf, n);
> + v4l2_dbg(3, debug, c, "i2c_
Re: [PATCH v7] s5k5baf: add camera sensor driver
On Wed, 2013-08-21 at 15:41 +0100, Andrzej Hajda wrote:
> diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> new file mode 100644
> index 000..d680d99
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt
> @@ -0,0 +1,59 @@
> +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP
> +
> +
> +Required properties:
> +
> +- compatible : "samsung,s5k5baf";
> +- reg: I2C slave address of the sensor;
> +- vdda-supply: analog power supply 2.8V (2.6V to 3.0V);
> +- vddreg-supply : regulator input power supply 1.8V (1.7V to 1.9V)
> + or 2.8V (2.6V to 3.0);
> +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V)
> + or 2.8V (2.5V to 3.1V);
> +- stbyn-gpios: GPIO connected to STDBYN pin;
> +- rstn-gpios : GPIO connected to RSTN pin;
> +- clocks : the sensor's master clock specifier (from the common
> + clock bindings);
> +- clock-names: must be "mclk";
> +
> +Optional properties:
> +
> +- clock-frequency : the frequency at which the "mclk" clock should be
> + configured to operate, in Hz; if this property is not
> + specified default 24 MHz value will be used.
> +
> +The device node should contain one 'port' child node with one child
> 'endpoint'
> +node, according to the bindings defined in Documentation/devicetree/bindings/
> +media/video-interfaces.txt. The following are properties specific to those
> +nodes.
> +
> +endpoint node
> +-
> +
> +- data-lanes : (optional) specifies MIPI CSI-2 data lanes as covered in
> + video-interfaces.txt. This property can be only used to specify number
> + of data lanes, i.e. the array's content is unused, only its length is
> + meaningful. When this property is not specified default value of 1 lane
> + will be used.
> +
> +Example:
> +
> +s5k5bafx@2d {
> + compatible = "samsung,s5k5baf";
> + reg = <0x2d>;
> + vdda-supply = <&cam_io_en_reg>;
> + vddreg-supply = <&vt_core_15v_reg>;
> + vddio-supply = <&vtcam_reg>;
> + stbyn-gpios = <&gpl2 0 1>;
> + rstn-gpios = <&gpl2 1 1>;
> + clock-names = "mclk";
> + clocks = <&clock_cam 0>;
> + clock-frequency = <2400>;
> +
> + port {
> + s5k5bafx_ep: endpoint {
> + remote-endpoint = <&csis1_ep>;
> + data-lanes = <1>;
> + };
> + };
> +};
For the binding:
Acked-by: Pawel Moll
As to the discussion about GPIO naming, I'll stand by the "call it what
it is called in the documentation" stanza...
Thanks!
Pawel
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] s5k5baf: add camera sensor driver
On 08/23/2013 11:23 AM, Sylwester Nawrocki wrote: +- stbyn-gpios : GPIO connected to STDBYN pin; >> > +- rstn-gpios : GPIO connected to RSTN pin; >> > >> > Both GPIOs above have names suggesting that they are active low. I wonder >> > how the GPIO flags cell is interpreted here, namely the polarity bit. To be more clear, the polarity bit specifies GPIO state at the GPIO controller (SoC) that corresponds to active STANDBY or RESET signal state at the sensor. So it is supposed to cover any inverter in between the sensor and an SoC. > That's a good point. The GPIO flags are be used to specify active state > of the GPIO. Some sensors happen to use different active state for those > signals. It's not the case for this sensor though AFAICT. > > Anyway IMO it would be better to name those gpios: "stby-gpios", > "rst-gpios" in case there appear revisions that have their pin named STDBY > or RST rather than STDBYN, RSTN. That seems rather unlikely though, but > since there are devices to which that could apply I think for consistency > it might be better to remove indication of polarity from the GPIO names. -- Sylwester Nawrocki Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] s5k5baf: add camera sensor driver
On 08/23/2013 12:39 AM, Tomasz Figa wrote: >> Documentation/devicetree/bindings/media/samsung-s5k5baf.txt create mode >> > 100644 drivers/media/i2c/s5k5baf.c >> > >> > diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt >> > b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt new file >> > mode 100644 >> > index 000..d680d99 >> > --- /dev/null >> > +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt >> > @@ -0,0 +1,59 @@ >> > +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP >> > + >> > + >> > +Required properties: >> > + >> > +- compatible: "samsung,s5k5baf"; >> > +- reg : I2C slave address of the sensor; > > Can this sensor have an aribitrary slave address or only a set of well > known possible addresses (e.g. listed in documentation)? According to the datasheet it can have one of two I2C addresses (0x2D, 0x3C), selectable by a dedicated pin. Also they may be revisions of the device that use different addresses. I believe what addresses are possible is out of scope of this binding document. We can handle whatever is used. >> > +- vdda-supply : analog power supply 2.8V (2.6V to 3.0V); >> > +- vddreg-supply : regulator input power supply 1.8V (1.7V to > 1.9V) >> > + or 2.8V (2.6V to 3.0); >> > +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) >> > + or 2.8V (2.5V to 3.1V); >> > +- stbyn-gpios : GPIO connected to STDBYN pin; >> > +- rstn-gpios: GPIO connected to RSTN pin; > > Both GPIOs above have names suggesting that they are active low. I wonder > how the GPIO flags cell is interpreted here, namely the polarity bit. That's a good point. The GPIO flags are be used to specify active state of the GPIO. Some sensors happen to use different active state for those signals. It's not the case for this sensor though AFAICT. Anyway IMO it would be better to name those gpios: "stby-gpios", "rst-gpios" in case there appear revisions that have their pin named STDBY or RST rather than STDBYN, RSTN. That seems rather unlikely though, but since there are devices to which that could apply I think for consistency it might be better to remove indication of polarity from the GPIO names. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] s5k5baf: add camera sensor driver
Hi Andrzej, Please see some minor comments inline. On Wednesday 21 of August 2013 16:41:31 Andrzej Hajda wrote: > Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor > with embedded SoC ISP. > The driver exposes the sensor as two V4L2 subdevices: > - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format, > no controls. > - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200, > pre/post ISP cropping, downscaling via selection API, controls. > > Signed-off-by: Sylwester Nawrocki > Signed-off-by: Andrzej Hajda > Signed-off-by: Kyungmin Park > --- > Hi, > > This patch incorporates Stephen's suggestions, thanks. > > Regards > Andrzej > > v7 > - changed description of 'clock-frequency' DT property > > v6 > - endpoint node presence is now optional, > - added asynchronous subdev registration support and clock > handling, > - use named gpios in DT bindings > > v5 > - removed hflip/vflip device tree properties > > v4 > - GPL changed to GPLv2, > - bitfields replaced by u8, > - cosmetic changes, > - corrected s_stream flow, > - gpio pins are no longer exported, > - added I2C addresses to subdev names, > - CIS subdev registration postponed after > succesfull HW initialization, > - added enums for pads, > - selections are initialized only during probe, > - default resolution changed to 1600x1200, > - state->error pattern removed from few other functions, > - entity link creation moved to registered callback. > > v3: > - narrowed state->error usage to i2c and power errors, > - private gain controls replaced by red/blue balance user controls, > - added checks to devicetree gpio node parsing > > v2: > - lower-cased driver name, > - removed underscore from regulator names, > - removed platform data code, > - v4l controls grouped in anonymous structs, > - added s5k5baf_clear_error function, > - private controls definitions moved to uapi header file, > - added v4l2-controls.h reservation for private controls, > - corrected subdev registered/unregistered code, > - .log_status sudbev op set to v4l2 helper, > - moved entity link creation to probe routines, > - added cleanup on error to probe function. > --- > .../devicetree/bindings/media/samsung-s5k5baf.txt | 59 + > MAINTAINERS|7 + > drivers/media/i2c/Kconfig |7 + > drivers/media/i2c/Makefile |1 + > drivers/media/i2c/s5k5baf.c| 2045 > 5 files changed, 2119 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/media/samsung-s5k5baf.txt create mode > 100644 drivers/media/i2c/s5k5baf.c > > diff --git a/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt > b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt new file > mode 100644 > index 000..d680d99 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/samsung-s5k5baf.txt > @@ -0,0 +1,59 @@ > +Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor with embedded SoC ISP > + > + > +Required properties: > + > +- compatible : "samsung,s5k5baf"; > +- reg : I2C slave address of the sensor; Can this sensor have an aribitrary slave address or only a set of well known possible addresses (e.g. listed in documentation)? > +- vdda-supply : analog power supply 2.8V (2.6V to 3.0V); > +- vddreg-supply: regulator input power supply 1.8V (1.7V to 1.9V) > + or 2.8V (2.6V to 3.0); > +- vddio-supply : I/O power supply 1.8V (1.65V to 1.95V) > + or 2.8V (2.5V to 3.1V); > +- stbyn-gpios : GPIO connected to STDBYN pin; > +- rstn-gpios : GPIO connected to RSTN pin; Both GPIOs above have names suggesting that they are active low. I wonder how the GPIO flags cell is interpreted here, namely the polarity bit. Otherwise the binding looks good. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] s5k5baf: add camera sensor driver
On 08/21/2013 08:41 AM, Andrzej Hajda wrote: > Driver for Samsung S5K5BAF UXGA 1/5" 2M CMOS Image Sensor > with embedded SoC ISP. > The driver exposes the sensor as two V4L2 subdevices: > - S5K5BAF-CIS - pure CMOS Image Sensor, fixed 1600x1200 format, > no controls. > - S5K5BAF-ISP - Image Signal Processor, formats up to 1600x1200, > pre/post ISP cropping, downscaling via selection API, controls. The binding, Acked-by: Stephen Warren (although it would be great if another DT binding maintainer gave it a quick look-over to make sure I didn't miss anything!) -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
