mbox series

[v3,0/7] OV9281 support

Message ID 20220722131947.2456988-1-alexander.stein@ew.tq-group.com
Headers show
Series OV9281 support | expand

Message

Alexander Stein July 22, 2022, 1:19 p.m. UTC
Hi,

this is the 3rd series adding support for OV9281 which is quite similar to OV9282.
This includes:
* a small cleanup (Patch 1)
* adding a new compatible (Patch 2 & 3)
* adding support for regulators (Patch 4 & 5)
* Fixing v4l2 subdev name depending on actual model name (Patch 6)
* Add regmap support (Patch 7)

Thanks for anyone doing review and giving a feedback.

Here are the changes in v3:
* Removed struct field documentation as well (Patch 1)
* Dropped v2 Patch 6 (wrong approach)
* Added new Patch 6 to set subdev name according to model parsed form compatible
* Added new Patch 7 adding regmap support
  This is a preparation to solve the defunct auto-increment using regmap's
  'use_single_read' (still WIP)

Here are the changes in v2:
* Added Krzysztof's a-b for Patch 2 & 4
* Added Daniele's a-b for Patch 1 & 3
* Removed additional error message in ov9282_power_off
* Renamed function from ov9282_configure_regulators to ov9282_get_regulators
* Cleaned-up reading ID registers

The regulator support is based on the driver from Raspberry Pi downstream kernel
[1], the ID register read fix as well. Please refer to [2] why this fix is
required. I can confirm this is necessary by checking with a Logic analyzer on
the i2c bus.

Best regards,
Alexander

[1] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/ov9281.c
[2] https://github.com/raspberrypi/linux/commit/58deee7c917e1c3c5e37987c3a89ad19d791f58a


Alexander Stein (7):
  media: i2c: ov9282: remove unused and unset i2c_client member
  media: dt-bindings: media: Add compatible for ov9281
  media: i2c: ov9282: Add ov9281 compatible
  media: dt-bindings: media: ov9282: Add power supply properties
  media: i2c: ov9282: Add regulator support
  media: i2c: ov9282: Set v4l2 subdev name according to sensor model
  media: i2c: ov9282: Add regmap support

 .../bindings/media/i2c/ovti,ov9282.yaml       |  14 ++-
 drivers/media/i2c/ov9282.c                    | 102 ++++++++++++------
 2 files changed, 84 insertions(+), 32 deletions(-)

Comments

Alexander Stein July 25, 2022, 6:38 a.m. UTC | #1
Hi Sakari,

Am Freitag, 22. Juli 2022, 15:50:30 CEST schrieb Sakari Ailus:
> Hi Alexander,
> 
> On Fri, Jul 22, 2022 at 03:19:46PM +0200, Alexander Stein wrote:
> > To distinguish ov9281 & ov9282 the name has to be explicitly set.
> > i2c_client already has the name parsed from the compatible.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 
> >  drivers/media/i2c/ov9282.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 352dbe21a902..dbc0a4cd060f 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -1047,6 +1047,7 @@ static int ov9282_probe(struct i2c_client *client)
> > 
> >  	/* Initialize subdev */
> >  	v4l2_i2c_subdev_init(&ov9282->sd, client, &ov9282_subdev_ops);
> > 
> > +	v4l2_i2c_subdev_set_name(&ov9282->sd, client, client->name, NULL);
> 
> Could you instead do this based on the compatible string in the driver,
> using device_get_match_data()? The approach works on non-OF systems, too.

I actually don't like doing the same as of_modalias_node() is doing.
Until non-OF support is added (if ever), I don't see any benefit in doing so 
right now.

Best regards,
Alexander

> >  	ret = ov9282_parse_hw_config(ov9282);
> >  	if (ret) {
Sakari Ailus July 28, 2022, 11:47 a.m. UTC | #2
Hi Alexander,

On Mon, Jul 25, 2022 at 08:38:57AM +0200, Alexander Stein wrote:
> Hi Sakari,
> 
> Am Freitag, 22. Juli 2022, 15:50:30 CEST schrieb Sakari Ailus:
> > Hi Alexander,
> > 
> > On Fri, Jul 22, 2022 at 03:19:46PM +0200, Alexander Stein wrote:
> > > To distinguish ov9281 & ov9282 the name has to be explicitly set.
> > > i2c_client already has the name parsed from the compatible.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > 
> > >  drivers/media/i2c/ov9282.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index 352dbe21a902..dbc0a4cd060f 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -1047,6 +1047,7 @@ static int ov9282_probe(struct i2c_client *client)
> > > 
> > >  	/* Initialize subdev */
> > >  	v4l2_i2c_subdev_init(&ov9282->sd, client, &ov9282_subdev_ops);
> > > 
> > > +	v4l2_i2c_subdev_set_name(&ov9282->sd, client, client->name, NULL);
> > 
> > Could you instead do this based on the compatible string in the driver,
> > using device_get_match_data()? The approach works on non-OF systems, too.
> 
> I actually don't like doing the same as of_modalias_node() is doing.
> Until non-OF support is added (if ever), I don't see any benefit in doing so 
> right now.

client->name will be wrong on un-OF; putting this string to device match
data is a better option. It'll be about the same number of lines, too.