Re: [PATCH v2 2/2] media: i2c: add Samsung S5KJN1 image sensor device driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Vladimir,

On Tue, Oct 21, 2025 at 01:16:09PM +0300, Vladimir Zapolskiy wrote:
> Hi Sakari,
> 
> On 10/21/25 12:25, Sakari Ailus wrote:
> > Hi Vladimir,
> > 
> > On Thu, Oct 16, 2025 at 05:04:19AM +0300, Vladimir Zapolskiy wrote:
> > > +	{ S5KJN1_REG_X_ADDR_START,  0x0000 },
> > > +	{ S5KJN1_REG_Y_ADDR_START,  0x0000 },
> > > +	{ S5KJN1_REG_X_ADDR_END,    0x1fff },
> > > +	{ S5KJN1_REG_Y_ADDR_END,    0x181f },
> > > +	{ S5KJN1_REG_X_OUTPUT_SIZE, 0x0ff0 },
> > > +	{ S5KJN1_REG_Y_OUTPUT_SIZE, 0x0c00 },
> > > +	{ CCI_REG16(0x0350), 0x0008 },
> > > +	{ CCI_REG16(0x0352), 0x0008 },
> > > +	{ CCI_REG16(0x0900), 0x0122 },
> > > +	{ CCI_REG16(0x0380), 0x0002 },
> > > +	{ CCI_REG16(0x0382), 0x0002 },
> > > +	{ CCI_REG16(0x0384), 0x0002 },
> > > +	{ CCI_REG16(0x0386), 0x0002 },
> > > +	{ CCI_REG16(0x0110), 0x1002 },
> > > +	{ CCI_REG16(0x0114), 0x0301 },
> > > +	{ CCI_REG16(0x0116), 0x3000 },
> > > +
> > > +	/* Clock settings */
> > > +	{ CCI_REG16(0x0136), 0x1800 },
> > > +	{ CCI_REG16(0x013e), 0x0000 },
> > > +	{ CCI_REG16(0x0300), 0x0006 },
> > > +	{ CCI_REG16(0x0302), 0x0001 },
> > > +	{ CCI_REG16(0x0304), 0x0004 },
> > > +	{ CCI_REG16(0x0306), 0x008c },
> > > +	{ CCI_REG16(0x0308), 0x0008 },
> > > +	{ CCI_REG16(0x030a), 0x0001 },
> > > +	{ CCI_REG16(0x030c), 0x0000 },
> > > +	{ CCI_REG16(0x030e), 0x0004 },
> > > +	{ CCI_REG16(0x0310), 0x0092 },
> > > +	{ CCI_REG16(0x0312), 0x0000 },
> > > +
> > > +	{ CCI_REG16(0x080e), 0x0000 },
> > > +	{ S5KJN1_REG_VTS,    0x10c0 },
> > > +	{ S5KJN1_REG_HTS,    0x1100 },
> > > +	{ CCI_REG16(0x0702), 0x0000 },
> > > +	{ S5KJN1_REG_EXPOSURE, 0x0100 },
> > > +	{ CCI_REG16(0x0200), 0x0100 },
> > > +	{ CCI_REG16(0x0d00), 0x0101 },
> > > +	{ CCI_REG16(0x0d02), 0x0101 },
> > > +	{ CCI_REG16(0x0d04), 0x0102 },
> > > +	{ CCI_REG16(0x6226), 0x0000 },
> > > +	{ CCI_REG16(0x0816), 0x1c00 },
> > 
> > This looks interestingly CCS compliant. It might be worth taking the MSRs
> > and trying with the CCS driver.
> 
> The register map is similar to CCS (and it's explicitly mentioned in a comment
> withing the driver), but it is not compatible due to a known number of
> registers, for instance 0x0310 register is not a CCS_R_PLL_MODE, but a PLL
> setting etc.
> 
> The same reasoning is applicable to the second sensor driver Samsung S5K3M5,
> moreover even these two sensors have different interfaces to registers,
> e.g. it's not possible to separately configure HLIP/VFLIP settings for the
> latter one, while it's working nicely, and even a "stream on" control bit
> in 0x100 (CCS_R_MODE_SELECT) register are different...
> 
> So, I believe it would be more tedious and unclean to add a number of
> exceptions to the CSS driver rather than to add a sensor specific driver.

CCS has a mechanism (via CCS static data) to cover cases such as this
albeit it wasn't intended for standard registers. Perhaps it should be
extended. Either way, that's for another time.

I'll review the rest of the driver soon.

> 
> > Where is this sensor found?
> > 
> 
> For a while I work on Qualcomm ISP support, and these Samsung sensors are
> found on SM8x50-HDK and SM8x50-QRD boards:
> 
> https://lore.kernel.org/linux-arm-msm/20251013235500.1883847-1-vladimir.zapolskiy@linaro.org/
> 
> If you ask about the downstream code, there is a multitude of downstream
> Android drivers of these Samsung sensors found on github.com, the init
> sequence for modes is taken from these drivers and the driver is tested
> on the boards in my access.

Ack.

-- 
Kind regards,

Sakari Ailus