Hi,
On 1/23/23 19:09, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 01:51:36PM +0100, Hans de Goede wrote:
>> The following drivers under drivers/media/i2c: ov08x40.c, ov13858.c,
>> ov13b10.c, ov2680.c, ov2685.c, ov2740.c, ov4689.c, ov5670.c,
>> ov5675.c, ov5695.c, ov8856.c, ov9282.c and ov9734.c,
>>
>> as well as various "atomisp" sensor drivers in drivers/staging, *all*
>> use register access helpers with the following function prototypes:
>>
>> int ovxxxx_read_reg(struct ovxxxx_dev *sensor, u16 reg,
>> unsigned int len, u32 *val);
>>
>> int ovxxxx_write_reg(struct ovxxxx_dev *sensor, u16 reg,
>> unsigned int len, u32 val);
>>
>> To read/write registers on Omnivision OVxxxx image sensors wich expect
>> a 16 bit register address in big-endian format and which have 1-3 byte
>> wide registers, in big-endian format (for the higher width registers).
>>
>> Add a new ovxxxx_16bit_addr_reg_helpers.h header file with static inline
>> versions of these register access helpers, so that this code duplication
>> can be removed.
>
> ...
>
>> +#include <asm/unaligned.h>
>
> Usually we put linux/* followed by asm/*.
Ack.
>
>> +#include <linux/dev_printk.h>
>> +#include <linux/i2c.h>
>> +
>> +static inline int ovxxxx_read_reg(struct i2c_client *client, u16 reg,
>> + unsigned int len, u32 *val)
>> +{
>> + struct i2c_msg msgs[2];
>
>> + u8 addr_buf[2] = { reg >> 8, reg & 0xff };
>
> Let's use unaligned.h or byteorder/generic.h?
>
> __be16 addr_buf = cpu_to_be16(reg);
Ack.
>
>> + u8 data_buf[4] = { 0, };
>
> 0, is not needed.
>
>> + int ret;
>> +
>> + if (len > 4)
>> + return -EINVAL;
>> +
>> + msgs[0].addr = client->addr;
>> + msgs[0].flags = 0;
>> + msgs[0].len = ARRAY_SIZE(addr_buf);
>> + msgs[0].buf = addr_buf;
>
> msgs[0].buf = &addr_buf;
>
>> + msgs[1].addr = client->addr;
>> + msgs[1].flags = I2C_M_RD;
>> + msgs[1].len = len;
>
>> + msgs[1].buf = &data_buf[4 - len];
>
> This trick I don't like. Can we have like other driver have it, i.e. switch
> case for the possible length and explicit usage of the endian conversion calls?
This new header (which is intended to eventually be used in many other
ovXXXX drivers too) is modeled after the reg access helpers
in drivers/media/i2c/ov*.c
And those do use be16 for the addr_buf in some cases, so I'm fine
with changing that. But non of them do a switch-case on len,
instead they all use similar tricks as this code (which was
copied from drivers/media/i2c/ov2680.c) does.
So I would prefer to keep this as is, so that the new
ovxxxx_16bit_addr_reg_helpers.h code is more like the code which
it intends to replace.
Regards,
Hans
>
>> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>> + if (ret != ARRAY_SIZE(msgs)) {
>> + dev_err(&client->dev, "read error: reg=0x%4x: %d\n", reg, ret);
>> + return -EIO;
>> + }
>> +
>> + *val = get_unaligned_be32(data_buf);
>> +
>> + return 0;
>> +}
>> +
>> +#define ovxxxx_read_reg8(s, r, v) ovxxxx_read_reg(s, r, 1, v)
>> +#define ovxxxx_read_reg16(s, r, v) ovxxxx_read_reg(s, r, 2, v)
>> +#define ovxxxx_read_reg24(s, r, v) ovxxxx_read_reg(s, r, 3, v)
>> +
>> +static inline int ovxxxx_write_reg(struct i2c_client *client, u16 reg,
>> + unsigned int len, u32 val)
>> +{
>> + u8 buf[6];
>> + int ret;
>> +
>> + if (len > 4)
>> + return -EINVAL;
>
>> + put_unaligned_be16(reg, buf);
>> + put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
>
> Something similar here?
>
>> + ret = i2c_master_send(client, buf, len + 2);
>> + if (ret != len + 2) {
>> + dev_err(&client->dev, "write error: reg=0x%4x: %d\n", reg, ret);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#define ovxxxx_write_reg8(s, r, v) ovxxxx_write_reg(s, r, 1, v)
>> +#define ovxxxx_write_reg16(s, r, v) ovxxxx_write_reg(s, r, 2, v)
>> +#define ovxxxx_write_reg24(s, r, v) ovxxxx_write_reg(s, r, 3, v)
>> +
>> +static inline int ovxxxx_mod_reg(struct i2c_client *client, u16 reg, u8 mask, u8 val)
>> +{
>> + u32 readval;
>> + int ret;
>> +
>> + ret = ovxxxx_read_reg8(client, reg, &readval);
>> + if (ret < 0)
>> + return ret;
>
>> + readval &= ~mask;
>> + val &= mask;
>> + val |= readval;
>
> Usual pattern:
>
> val = (readval & ~mask) | (val & mask);
>
>> + return ovxxxx_write_reg8(client, reg, val);
>> +}
>
